Re: [PATCH v6 1/5] nfs: make nfs_page pin-aware

On Fri, Aug 14, 2026 at 02:46:57PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] Subrequests incorrectly drop GUP pins multiple times because `req == req->wb_head` evaluates to true for all requests during teardown, causing pin count underflow and Use-After-Free.
> - [Medium] Hardcoded unpin count of 1 in `unpin_user_folio()` leaks GUP pins for large folios spanning multiple pages.
> --
> 
> --- Patch [1]: [PATCH v6 1/5] nfs: make nfs_page pin-aware ---
> commit 21b0f62730c29dca3085e0b77d441b62786919e8
> Author: Pranjal Shrivastava <[email protected]>
> 
> nfs: make nfs_page pin-aware
> 
> Modernizing the NFS Direct I/O path to use iov_iter_extract_pages()
> introduces page pinning (GUP) instead of standard page referencing.
> To handle this correctly, nfs_page must track whether it holds a
> pin or a standard reference.
> 
> > diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
> > index 7dd478ffc2fab..a562cfe2a126d 100644
> > --- a/fs/nfs/pagelist.c
> > +++ b/fs/nfs/pagelist.c
> [ ... ]
> > @@ -552,11 +566,21 @@ static void nfs_clear_request(struct nfs_page *req)
> >  	struct nfs_open_context *ctx;
> >  
> >  	if (folio != NULL) {
> > -		folio_put(folio);
> > +		if (test_and_clear_bit(PG_PINNED, &req->wb_flags)) {
> > +			if (req == req->wb_head)
> > +				unpin_user_folio(folio, 1);
> 
> [Severity: Medium]
> Will this leak GUP pins for large folios spanning multiple pages?
> 
> If a caller uses nfs_page_create_from_folio() with pinned=true for a folio
> spanning multiple pages, hardcoding 1 releases only one pin and leaks the rest.
> 
> Also, does this check for the head request evaluate to true for all
> subrequests?
> 
> During teardown, nfs_page_group_destroy() breaks the page group linkage by
> setting tmp->wb_head = tmp before freeing the request:
> 
> nfs_page_group_destroy()
>     tmp->wb_head = tmp;
>     nfs_free_request(tmp);
> 
> Because of this, when nfs_clear_request() is called, req == req->wb_head will
> evaluate to true for all subrequests, causing each subrequest to unconditionally
> invoke unpin_user_folio() and potentially resulting in a pin count underflow
> and Use-After-Free.
> 
> > +		} else {
> > +			folio_put(folio);
> > +		}
> >  		req->wb_folio = NULL;
> >  		clear_bit(PG_FOLIO, &req->wb_flags);
> >  	} else if (page != NULL) {
> > -		put_page(page);
> > +		if (test_and_clear_bit(PG_PINNED, &req->wb_flags)) {
> > +			if (req == req->wb_head)
> > +				unpin_user_page(page);
> 
> [Severity: Medium]
> Similar to the issue above, because nfs_page_group_destroy() severs the
> linkage and sets tmp->wb_head = tmp, will this unconditionally call
> unpin_user_page() for all subrequests and cause a pin count underflow?
> 

These are fixed by patch 2 with the introduction of wb_nr_pinned.

Thanks,
Praan