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

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?

> +		} else {
> +			put_page(page);
> +		}
>  		req->wb_page = NULL;
>  	}

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