Re: [PATCH] xfrm: espintcp: build sk_msg locally before publishing

[Sabrina Dubroca]

2026-09-22, 16:53:35 +0200, Bruno Produit wrote:
> From: Kyle Zeng <kylebot@xxxxxxxxxx>
> 
> espintcp_sendmsg() builds a new message directly in ctx->partial. If
> allocation fails, sk_stream_wait_memory() drops the socket lock while
> the shared sk_msg remains unpublished with emsg->len equal to zero. A
> concurrent sender can then reuse the same slot. If the first sender is

Could we add an ->owned flag to emsg to make the other sender
wait/abort when the flag is set (whether the emsg has been fully set
up or not)?

If not, comments below.

> interrupted, its failure path frees state now owned by the second sender
> while TCP may still be consuming it, causing a use-after-free.
> 
> Construct the message in a call-local sk_msg instead.


> After allocation
> and any lock-dropping wait, recheck that the shared partial slot is still
> free, then transfer the completed message into it. Failure cleanup
> consequently releases only state owned by the current call.

Please don't describe what the patch does. We can read the code.

> The recheck
> also covers packets submitted through the common IPv4 and IPv6
> espintcp_push_skb() path.

I have no idea what this means.


> diff --git a/net/xfrm/espintcp.c b/net/xfrm/espintcp.c
> index 674aedc..1642b34 100644
> --- a/net/xfrm/espintcp.c
> +++ b/net/xfrm/espintcp.c
> @@ -311,6 +311,7 @@ static int espintcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
>  	struct espintcp_msg *emsg = &ctx->partial;
>  	struct iov_iter pfx_iter;
>  	struct kvec pfx_iov = {};
> +	struct sk_msg *skmsg;

nit: reverse xmas tree ordering

>  	size_t msglen = size + 2;
>  	char buf[2] = {0};
>  	int err, end;
> @@ -324,6 +325,11 @@ static int espintcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
>  	if (msg->msg_controllen)
>  		return -EOPNOTSUPP;
>  
> +	skmsg = kmalloc_obj(*skmsg);
> +	if (!skmsg)
> +		return -ENOMEM;
> +	sk_msg_init(skmsg);

Why do that before trying (and possibly failing) to push the pending
message?

>  	lock_sock(sk);
>  
>  	err = espintcp_push_msgs(sk, msg->msg_flags & MSG_DONTWAIT);
> @@ -337,10 +343,9 @@ static int espintcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
>  		goto unlock;
>  	}
>  
> -	sk_msg_init(&emsg->skmsg);
>  	while (1) {
>  		/* only -ENOMEM is possible since we don't coalesce */
> -		err = sk_msg_alloc(sk, &emsg->skmsg, msglen, 0);
> +		err = sk_msg_alloc(sk, skmsg, msglen, 0);
>  		if (!err)
>  			break;
>  
> @@ -348,25 +353,30 @@ static int espintcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
>  		if (err)
>  			goto fail;
>  	}
> +	if (emsg->len) {
> +		err = -ENOBUFS;
> +		goto fail;
> +	}

Do another espintcp_push_msgs before giving up?

And there should be a comment here to explain why we need to recheck
emsg->len even though we already did at the top (something like "we
may have dropped the lock in sk_stream_wait_memory, check if someone
else used the emsg").

-- 
Sabrina