> On Aug 29, 2022, at 9:43 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>
> On Sun, 2022-08-28 at 14:50 -0400, Chuck Lever wrote:
>> For many years, NFSD has conserved the number of pages held by
>> each nfsd thread by combining the RPC receive and send buffers
>> into a single array of pages. The dividing line between the
>> receive and send buffer is pointed to by svc_rqst::rq_respages.
>>
>
> nit: Given that you don't look at rq_respages in the patch below, the
> previous sentence is not particularly relevant. It might be better to
> just explain that rq_res describes the part of the array that is the
> response buffer, so we want to consult it for the max length.
Good point.
>> Thus the send buffer shrinks when the received RPC record
>> containing the RPC Call is large.
>>
>> nfsd3_init_dirlist_pages() needs to account for the space in the
>> svc_rqst::rq_pages array already consumed by the RPC receive buffer.
>> Otherwise READDIR reply encoding can wander off the end of the page
>> array.
>>
>> Thanks to Aleksi Illikainen and Kari Hulkko for discovering this
>> issue.
>>
>> Reported-by: Ben Ronallo <Benjamin.Ronallo@xxxxxxxxxxxx>
>> Fixes: f5dcccd647da ("NFSD: Update the NFSv2 READDIR entry encoder to use struct xdr_stream")
>> Fixes: 7f87fc2d34d4 ("NFSD: Update NFSv3 READDIR entry encoders to use struct xdr_stream")
>> Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
>> ---
>> fs/nfsd/nfs3proc.c | 5 ++---
>> fs/nfsd/nfsproc.c | 5 ++---
>> 2 files changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
>> index a41cca619338..fab87e9e0b20 100644
>> --- a/fs/nfsd/nfs3proc.c
>> +++ b/fs/nfsd/nfs3proc.c
>> @@ -564,12 +564,11 @@ static void nfsd3_init_dirlist_pages(struct svc_rqst *rqstp,
>> struct xdr_buf *buf = &resp->dirlist;
>> struct xdr_stream *xdr = &resp->xdr;
>>
>> - count = clamp(count, (u32)(XDR_UNIT * 2), svc_max_payload(rqstp));
>> -
>> memset(buf, 0, sizeof(*buf));
>>
>> /* Reserve room for the NULL ptr & eof flag (-2 words) */
>> - buf->buflen = count - XDR_UNIT * 2;
>> + buf->buflen = clamp(count, (u32)(XDR_UNIT * 2), rqstp->rq_res.buflen);
>> + buf->buflen -= XDR_UNIT * 2;
>> buf->pages = rqstp->rq_next_page;
>> rqstp->rq_next_page += (buf->buflen + PAGE_SIZE - 1) >> PAGE_SHIFT;
>>
>> diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
>> index 7381972f1677..23c273cb68a9 100644
>> --- a/fs/nfsd/nfsproc.c
>> +++ b/fs/nfsd/nfsproc.c
>> @@ -567,12 +567,11 @@ static void nfsd_init_dirlist_pages(struct svc_rqst *rqstp,
>> struct xdr_buf *buf = &resp->dirlist;
>> struct xdr_stream *xdr = &resp->xdr;
>>
>> - count = clamp(count, (u32)(XDR_UNIT * 2), svc_max_payload(rqstp));
>> -
>> memset(buf, 0, sizeof(*buf));
>>
>> /* Reserve room for the NULL ptr & eof flag (-2 words) */
>> - buf->buflen = count - XDR_UNIT * 2;
>> + buf->buflen = clamp(count, (u32)(XDR_UNIT * 2), rqstp->rq_res.buflen);
>> + buf->buflen -= XDR_UNIT * 2;
>> buf->pages = rqstp->rq_next_page;
>> rqstp->rq_next_page++;
>>
>>
>>
>
> I wonder if a better fix would be to make svc_max_payload take the
> already-consumed arg space into account? We'd need to fix up the other
> callers of course.
svc_max_payload() is used in places where the server's maximum
payload value is desired and in other places where the request's
maximum payload value is desired (as in this case). We'd have to
sort these cases.
But, now that I'm looking at svc_max_payload() call sites, it
does appear that some of these will fall prey to the same bug.
Eg, nfsd3_proc_read().
So, let me do that audit and redrive the series.
> In any case, the patch itself looks fine:
>
> Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>
--
Chuck Lever
