Re: [PATCH] bpf, sockmap: fix cork data loss after redirect

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] bpf, sockmap: leak of detached cork sk_msg in
  tcp_bpf_send_verdict()
- [Medium] bpf, sockmap: stale msg->cork_bytes re-corks data in
  tcp_bpf_send_verdict()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [High] bpf: memory leak in tcp_bpf_sendmsg() due to cork allocation
  failure [1]
--

commit 5eaba7adf21d338926f608cc6e21ea8e9156e647
Author: Ömer Mete Kaya <omermetekaya0@gmail.com>

bpf, sockmap: fix cork data loss after redirect

This commit keeps a corked sk_msg alive across partial __SK_REDIRECT
sends in tcp_bpf_send_verdict() so that remaining bytes can be processed
by the more_data loop.

> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index 8cf63a9e7fed1..e6a8184aad6a4 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c
> @@ -499,9 +499,13 @@ static int tcp_bpf_send_verdict(struct sock *sk, struct sk_psock *psock,
>  				*copied -= free;
>  		}
>  		if (cork) {
> -			sk_msg_free(sk, msg);
> -			kfree(msg);
> -			msg = NULL;
> +			if (ret < 0 ||
> +			    !msg->sg.data[msg->sg.start].page_link ||
> +			    !msg->sg.data[msg->sg.start].length) {
> +				sk_msg_free(sk, msg);
> +				kfree(msg);
> +				msg = NULL;
> +			}
>  			ret = 0;
>  		}
>  		break;

[Severity: High]
Does keeping msg alive across goto more_data leak the heap-allocated
struct sk_msg when a subsequent iteration does not take __SK_REDIRECT?

When a corked message enters __SK_REDIRECT, tcp_bpf_send_verdict() sets
cork = true and psock->cork = NULL before releasing the socket lock,
leaving msg as the only pointer to that allocation.  If a partial
redirect leaves unsent bytes and jumps to more_data, kfree(msg) is only
called in the __SK_REDIRECT branch.  If the next iteration instead hits
the re-cork path (which sees !psock->cork, allocates a new psock->cork
and copies msg into it), __SK_PASS, or __SK_DROP, tcp_bpf_send_verdict()
returns without freeing msg:

net/ipv4/tcp_bpf.c:tcp_bpf_send_verdict() {
	if (msg->cork_bytes &&
	    msg->cork_bytes > msg->sg.size && !enospc) {
		psock->cork_bytes = msg->cork_bytes - msg->sg.size;
		if (!psock->cork) {
			psock->cork = kzalloc_obj(*psock->cork,
						  GFP_ATOMIC | __GFP_NOWARN);
			...
		}
		memcpy(psock->cork, msg, sizeof(*msg));
		return 0;
	}
}

[Severity: Medium]
Can a stale msg->cork_bytes value also spuriously re-cork the remaining
bytes on the next more_data iteration?

When tcp_bpf_sendmsg() finishes accumulating psock->cork_bytes, it resets
psock->cork_bytes = 0 and psock->eval = __SK_NONE, but leaves
msg_tx->cork_bytes set to the old threshold on psock->cork:

net/ipv4/tcp_bpf.c:tcp_bpf_sendmsg() {
		if (psock->cork_bytes) {
			if (size > psock->cork_bytes)
				psock->cork_bytes = 0;
			else
				psock->cork_bytes -= size;
			if (psock->cork_bytes && !enospc)
				goto out_err;
			/* All cork bytes are accounted, rerun the prog. */
			psock->eval = __SK_NONE;
			psock->cork_bytes = 0;
		}
}

Once a partial send reduces msg->sg.size below that original
msg->cork_bytes value and loops back to more_data, the check
msg->cork_bytes > msg->sg.size in tcp_bpf_send_verdict() triggers again
and re-corks the residual data even if the BPF verdict program did not
request corking for the remaining bytes.

[Severity: High]
This problem wasn't introduced by this patch, but setting
psock->cork_bytes before checking whether kzalloc_obj() succeeds in
tcp_bpf_send_verdict() [1] leaves psock->cork_bytes non-zero while
psock->cork is NULL on allocation failure.  On a subsequent
tcp_bpf_sendmsg() call, msg_tx falls back to the stack-allocated tmp
message, allocates scatterlist pages via sk_msg_alloc(), and then takes
the psock->cork_bytes check to out_err without calling sk_msg_free() on
tmp, leaking the allocated pages.

[1] https://sashiko.dev/#/bug/linux-2ff2eab9-5d5b-4e91-82d9-b38be18677c9

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114115222.180345-1-omermetekaya0@gmail.com?part=1