Re: [PATCH v2 2/2] sunrpc: add a slot to rqstp->rq_bvec for TCP record marker

[Chuck Lever <chuck.lever@oracle.com>]

On 10/9/25 8:10 PM, NeilBrown wrote:
> On Thu, 09 Oct 2025, Jeff Layton wrote:
>> On Thu, 2025-10-09 at 08:51 +1100, NeilBrown wrote:
>>> On Thu, 09 Oct 2025, Jeff Layton wrote:
>>>> We've seen some occurrences of messages like this in dmesg on some knfsd
>>>> servers:
>>>>
>>>>     xdr_buf_to_bvec: bio_vec array overflow
>>>>
>>>> Usually followed by messages like this that indicate a short send (note
>>>> that this message is from an older kernel and the amount that it reports
>>>> attempting to send is short by 4 bytes):
>>>>
>>>>     rpc-srv/tcp: nfsd: sent 1048155 when sending 1048152 bytes - shutting down socket
>>>>
>>>> svc_tcp_sendmsg() steals a slot in the rq_bvec array for the TCP record
>>>> marker. If the send is an unaligned READ call though, then there may not
>>>> be enough slots in the rq_bvec array in some cases.
>>>>
>>>> Add a slot to the rq_bvec array, and fix up the array lengths in the
>>>> callers that care.
>>>>
>>>> Fixes: e18e157bb5c8 ("SUNRPC: Send RPC message on TCP with a single sock_sendmsg() call")
>>>> Tested-by: Brandon Adams <brandona@meta.com>
>>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>>>> ---
>>>>  fs/nfsd/vfs.c        | 6 +++---
>>>>  net/sunrpc/svc.c     | 3 ++-
>>>>  net/sunrpc/svcsock.c | 4 ++--
>>>>  3 files changed, 7 insertions(+), 6 deletions(-)
>>>
>>> I can't say that I'm liking this patch.
>>>
>>> There are 11 place where (in nfsd-testing recently) where
>>> rq_maxpages is used (as opposed to declared or assigned).
>>>
>>> 3 in nfsd/vfs.c
>>> 4 in sunrpc/svc.c
>>> 1 in sunrpc/svc_xprt.c
>>> 2 in sunrpc/svcsock.c
>>> 1 in xprtrdma/svc_rdma_rc.c
>>>
>>> Your patch changes six of those to add 1.  I guess the others aren't
>>> "callers that care".  It would help to have it clearly stated why, or
>>> why not, a caller might care.
>>>
>>> But also, what does "rq_maxpages" even mean now?
>>> The comment in svc.h still says "num of entries in rq_pages"
>>> which is certainly no longer the case.
>>> But if it was the case, we should have called it "rq_numpages"
>>> or similar.
>>> But maybe it wasn't meant to be the number of pages in the array,
>>> maybe it was meant to be the maximum number of pages is a request
>>> or a reply.....
>>> No - that is sv_max_mesg, to which we add 2 and 1.
>>> So I could ask "why not just add another 1 in svc_serv_maxpages()?"
>>> Would the callers that might not care be harmed if rq_maxpages were
>>> one larger than it is?
>>>
>>> It seems to me that rq_maxpages is rather confused and the bug you have
>>> found which requires this patch is some evidence to that confusion.  We
>>> should fix the confusion, not just the bug.
>>>
>>> So simple question to cut through my waffle:
>>> Would this:
>>> -	return DIV_ROUND_UP(serv->sv_max_mesg, PAGE_SIZE) + 2 + 1;
>>> +	return DIV_ROUND_UP(serv->sv_max_mesg, PAGE_SIZE) + 2 + 1 + 1;
>>>
>>> fix the problem.  If not, why not?  If so, can we just do this?
>>> then look at renaming rq_maxpages to rq_numpages and audit all the uses
>>> (and maybe you have already audited...).
>>>
>>
>> I get the objection. I'm not crazy about all of the adjustments either.
>>
>> rq_maxpages is used to size two fields in the rqstp: rq_pages and
>> rq_bvec. It turns out that they both want rq_maxpages + 1 slots. The
>> rq_pages array needs the extra slot for a NULL terminator, and rq_bvec
>> needs it for the TCP record marker.
> 
> Somehow the above para helped a lot for me to understand what the issue
> is here - thanks.
> 
> rq_bvec is used for two quite separate purposes.
> 
> nfsd/vfs.c uses it to assemble read/write requests to send to the
> filesystem.
> sunrpc/svcsock.c uses to assemble send/recv requests to send to the
> network.
> 
> It might help me if this were documented clearly in svc.h as I seem to
> have had to discover several times now :-(
> 
> Should these even use the same rq_bvec?

Perhaps not, now that you point out that these are two quite independent
use cases.

It might make sense for svcsock.c to allocate a bvec array for each
svc_sock that is the size needed for the given socket type. (UDP doesn't
need a record market and is limited to 64KB per send, for instance).


> I guess it makes sense to share
> but we should be cautious about letting the needs of one side infect the
> code of the other side.


-- 
Chuck Lever