Re: [RFC PATCH] NFSD: Remove WARN_ON_ONCE in nfsd_iter_read()

[Mike Snitzer <snitzer@kernel.org>]

On Thu, Sep 11, 2025 at 04:18:58PM -0400, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> The *count parameter does not appear to be explicitly restricted
> to being smaller than rsize, so it might be possible to overrun
> the rq_bvec array.
> 
> Rather than overrunning the array (damage done!) and then WARNING
> once, let's harden the loop so that it terminates before the end of
> rq_bvec. This should result in a short read, which is OK (clients
> recover by sending additional READ requests for the remaining unread
> bytes).
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfsd/vfs.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> There might be a similar issue with rq_next_page in this loop?
> 
> Suppose that nfsd4_encode_readv() encounters a second READ operation
> in a COMPOUND, and the two READ operations together comprise more
> than "rsize" total bytes of payload. Each rq_bvec is under the page
> limit, but the total number of pages consumed from rq_pages might
> exceed rq_maxpages.

This concern would appear well-founded; but probably best to deal with
it independently.
 
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 714777c221ed..e2f0fe3f82c0 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -1120,13 +1120,13 @@ __be32 nfsd_iter_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  		bvec_set_page(&rqstp->rq_bvec[v], *(rqstp->rq_next_page++),
>  			      len, base);
>  		total -= len;
> -		++v;
>  		base = 0;
> +		if (++v >= rqstp->rq_maxpages)
> +			break;

Shouldn't this be == instead of >= ?
Not seeing how it could ever become greater without first being equal.

Other than that, this patch is a welcome obvious improvement:

Reviewed-by: Mike Snitzer <snitzer@kernel.org>

>  	}
> -	WARN_ON_ONCE(v > rqstp->rq_maxpages);
>  
> -	trace_nfsd_read_vector(rqstp, fhp, offset, *count);
> -	iov_iter_bvec(&iter, ITER_DEST, rqstp->rq_bvec, v, *count);
> +	trace_nfsd_read_vector(rqstp, fhp, offset, *count - total);
> +	iov_iter_bvec(&iter, ITER_DEST, rqstp->rq_bvec, v, *count - total);
>  	host_err = vfs_iocb_iter_read(file, &kiocb, &iter);
>  	return nfsd_finish_read(rqstp, fhp, file, offset, count, eof, host_err);
>  }
> -- 
> 2.50.0
> 
>