Re: [PATCH v6 2/5] nfs: track number of pinned pages in nfs_page

On Fri, Aug 14, 2026 at 02:47:13PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] Latent stack out-of-bounds read by passing a single local stack variable to unpin_user_pages() with a dynamically loaded length.
> --
> 
> --- Patch [2]: [PATCH v6 2/5] nfs: track number of pinned pages in nfs_page ---
> commit 1fad80f498aa716421accabc5c2572235b589709
> Author: Pranjal Shrivastava <[email protected]>
> 
> nfs: track number of pinned pages in nfs_page
> 
> This commit adds wb_nr_pinned to struct nfs_page to track the number
> of pinned pages. It updates request creation and cleanup helpers to
> ensure that only primary requests perform the final unpinning operation.
> 
> > diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
> > index a562cfe2a126d..b9ccf2a87e3c9 100644
> > --- a/fs/nfs/pagelist.c
> > +++ b/fs/nfs/pagelist.c
> 
> [ ... ]
> 
> > @@ -567,8 +572,8 @@ static void nfs_clear_request(struct nfs_page *req)
> >  
> >  	if (folio != NULL) {
> >  		if (test_and_clear_bit(PG_PINNED, &req->wb_flags)) {
> > -			if (req == req->wb_head)
> > -				unpin_user_folio(folio, 1);
> > +			if (req->wb_nr_pinned > 0)
> > +				unpin_user_folio(folio, req->wb_nr_pinned);
> >  		} else {
> >  			folio_put(folio);
> >  		}
> > @@ -576,8 +581,8 @@ static void nfs_clear_request(struct nfs_page *req)
> >  		clear_bit(PG_FOLIO, &req->wb_flags);
> >  	} else if (page != NULL) {
> >  		if (test_and_clear_bit(PG_PINNED, &req->wb_flags)) {
> > -			if (req == req->wb_head)
> > -				unpin_user_page(page);
> > +			if (req->wb_nr_pinned > 0)
> > +				unpin_user_pages(&page, req->wb_nr_pinned);
> 
> [Severity: Medium]
> Could this lead to an out-of-bounds stack read if wb_nr_pinned is ever
> greater than 1?
> 
> Looking at nfs_clear_request(), page is declared as a single local pointer
> on the stack:
> 
> static void nfs_clear_request(struct nfs_page *req)
> {
> 	...
> 	struct page *page = req->wb_page;
> 
> If req->wb_nr_pinned exceeds 1, unpin_user_pages() will read adjacent stack
> memory to interpret as struct page pointers. While wb_nr_pinned is currently
> hardcoded to 1 when a request is created from a page, passing a dynamically
> loaded length alongside a pointer to a single local stack variable seems
> prone to future regressions.
> 

In this (non-folio) path wb_nr_pinned would always ever be 1. Although,
I think we could use unpin_user_page (singular) here.

Thanks,
Praan

> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2