Re: [PATCH 6.12 513/602] sunrpc: allocate a separate bvec array for socket sends

Hi Greg/Sasha/ and sunrpc maintainers,

On 30/07/26 7:45 pm, Greg Kroah-Hartman wrote:
> 6.12-stable review patch.  If anyone has any objections, please let me know.
> 
> ------------------
> 
> From: Jeff Layton <[email protected]>
> 
> [ Upstream commit 6b3b697d65d46a0f640216a3f6c72856c159c567 ]
> 
> svc_tcp_sendmsg() calls xdr_buf_to_bvec() with the second slot of
> rq_bvec as the start, but doesn't reduce the array length by one, which
> could lead to an array overrun. Also, rq_bvec is always rq_maxpages in
> length, which can be too short in some cases, since the TCP record
> marker consumes a slot.
> 
> Fix both problems by adding a separate bvec array to the svc_sock that
> is specifically for sending. For TCP, make this array one slot longer
> than rq_maxpages, to account for the record marker. For UDP, only
> allocate as large an array as we need since it's limited to 64k of
> payload.
> 
> Signed-off-by: Jeff Layton <[email protected]>
> Reviewed-by: NeilBrown <[email protected]>
> Signed-off-by: Chuck Lever <[email protected]>
> Stable-dep-of: 18c1cc698861 ("SUNRPC: Return an error from xdr_buf_to_bvec() on overflow")
> Signed-off-by: Sasha Levin <[email protected]>
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
> ---
>   include/linux/sunrpc/svcsock.h |    3 +++
>   net/sunrpc/svcsock.c           |   37 ++++++++++++++++++++++++++++---------
>   2 files changed, 31 insertions(+), 9 deletions(-)
> 
> --- a/include/linux/sunrpc/svcsock.h
> +++ b/include/linux/sunrpc/svcsock.h
> @@ -26,6 +26,9 @@ struct svc_sock {

> @@ -1423,6 +1433,13 @@ static struct svc_sock *svc_setup_socket
>   	if (!svsk)
>   		return ERR_PTR(-ENOMEM);
>   
> +	svsk->sk_bvec = kcalloc(RPCSVC_MAXPAGES + 1, sizeof(*svsk->sk_bvec),
> +				GFP_KERNEL);
> +	if (!svsk->sk_bvec) {
> +		kfree(svsk);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +


I reviewed the 6.12.y backport of 6b3b697d65d ("sunrpc: allocate a
separate bvec array for socket sends").

The main fix looks correct. In 6.12.y, the TCP send path has room for
RPCSVC_MAXPAGES payload slots plus the record-marker slot, and the UDP
send path still uses SUNRPC_MAX_UDP_SENDPAGES, so the bounds issue
addressed upstream appears fixed.

The only branch difference I noticed is in allocation policy.

Upstream uses svc_sock_sendpages() so that: (downstream 6.12.y doesn't 
have svc_sock_sendpages())

     SOCK_STREAM + SVC_SOCK_TEMPORARY -> maxpages + 1
     permanent SOCK_STREAM            -> no allocation
     SOCK_DGRAM                       -> UDP-specific allocation
     other socket types               -> -EINVAL

The 6.12.y backport instead allocates sk_bvec unconditionally in
svc_setup_socket() with kcalloc(RPCSVC_MAXPAGES + 1, ...), including
for permanent TCP listener sockets.

I do not see a correctness issue here. So I think this looks like an 
acceptable 6.12.y adaptation, even though it does not preserve the exact 
upstream allocation policy.

I don't think there is anything to fix, but would like to double check, 
thoughts ?

Thanks,
Harshit


>   	inet = sock->sk;
>   
>   	if (pmap_register) {
> @@ -1432,6 +1449,7 @@ static struct svc_sock *svc_setup_socket
>   				     inet->sk_protocol,
>   				     ntohs(inet_sk(inet)->inet_sport));
>   		if (err < 0) {
> +			kfree(svsk->sk_bvec);
>   			kfree(svsk);
>   			return ERR_PTR(err);
>   		}
> @@ -1651,5 +1669,6 @@ static void svc_sock_free(struct svc_xpr
>   	if (pfc->va)
>   		__page_frag_cache_drain(virt_to_head_page(pfc->va),
>   					pfc->pagecnt_bias);
> +	kfree(svsk->sk_bvec);
>   	kfree(svsk);
>   }
> 
> 
>