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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 (1) · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The patch adds a memory leak in tcp_bpf_send_verdict()
  (net/ipv4/tcp_bpf.c).
- [Medium] bpf, sockmap: stale msg->cork_bytes re-corks data in
  tcp_bpf_send_verdict()

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch
  makes it reachable in more ways.

--

bpf, sockmap: fix cork data loss after redirect

This patch changes the cork cleanup in the __SK_REDIRECT case of
tcp_bpf_send_verdict(). The corked sk_msg is now freed only when it has
been fully consumed or the redirect failed. Otherwise it is kept so the
more_data loop can send the remaining bytes.

> 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;
> +			}

[Severity: Medium]
When the redirect keeps a partially sent cork alive, can the leftover
bytes get re-corked even though the program did not ask for it?

msg->cork_bytes is only written by sk_msg_init() and
bpf_msg_cork_bytes(). When the cork is satisfied, tcp_bpf_sendmsg()
clears psock->cork_bytes and sets psock->eval to __SK_NONE. It leaves
psock->cork->cork_bytes at the old threshold.
sk_psock_msg_verdict() does not reset it before running the program
either.

With this patch the sequence looks like this:

  - the program calls bpf_msg_cork_bytes(msg, N), and the message is
    copied into psock->cork with cork_bytes == N
  - later sends fill the cork, so msg->sg.size >= N, and the program
    returns a redirect with apply_bytes smaller than sg.size
  - tcp_bpf_sendmsg_redir() sends apply_bytes, psock->apply_bytes
    drops to 0, psock->eval becomes __SK_NONE, and msg is now kept
    alive
  - at more_data the program runs again on the leftover, and it may
    not call bpf_msg_cork_bytes() at all

msg->cork_bytes is still N, and the leftover sg.size is usually
smaller than N, so this check fires:

	if (msg->cork_bytes &&
	    msg->cork_bytes > msg->sg.size && !enospc) {
		psock->cork_bytes = msg->cork_bytes - msg->sg.size;

The leftover is parked in a new psock->cork, and nothing is sent until
the application writes another N - sg.size bytes. An application that
waits for a reply before sending more would stall. This is also the
path that orphans the detached cork, as noted above.

Before this patch the cork was always freed after the redirect, so the
stale cork_bytes was never looked at again in this case. The on-stack
msg from tcp_bpf_sendmsg() can probably hit the same stale value on
the existing more_data loop, so part of this may be pre-existing. The
patch does make the cork case reach it.

Should msg->cork_bytes be cleared when tcp_bpf_sendmsg() decides the
cork is fully accounted? Or should sk_psock_msg_verdict() reset it
before each program run? I am not sure which semantics SK_MSG programs
are expected to rely on across reruns.

[Severity: High]
Does this leak the corked struct sk_msg when data remains?

Earlier in the same __SK_REDIRECT case, the cork has already been
detached from the psock:

		if (psock->cork) {
			cork = true;
			psock->cork = NULL;
		}

Once this branch keeps msg alive, the local msg is the only reference to
the kzalloc'd cork. Neither sk_psock_cork_free() nor tcp_bpf_sendmsg() can
reach it any more.

The code then jumps back to more_data. On the next pass, the only exit
that kfree()s the struct is another __SK_REDIRECT that fully consumes the
message or fails. The other exits appear to leak it.

The first one is the re-cork path. Only bpf_msg_cork_bytes() and
sk_msg_init() write msg->cork_bytes, so it is never reset. After a
partial redirect the remaining sg.size is usually smaller than cork_bytes:

	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);
			if (!psock->cork) {
				sk_msg_free(sk, msg);
				*copied = 0;
				return -ENOMEM;
			}
		}
		memcpy(psock->cork, msg, sizeof(*msg));
		return 0;
	}

psock->cork is NULL at this point, so a new cork is allocated, the old
struct is copied into it, and the old struct is orphaned. The -ENOMEM
branch also only calls sk_msg_free().

The second one is the __SK_PASS case. It never calls kfree(msg), whether
the push succeeds or fails. This case is reached when the program returns
SK_PASS, or when the msg_parser has been detached.

The third one is the __SK_DROP/default case. It calls sk_msg_free() and
returns -EACCES without calling kfree(msg).

Before this patch, the cork branch always freed msg and set it to NULL.
So a detached heap cork never went back through the loop.

The existing test_txmsg_cork_hangs case in test_sockmap.c sets
txmsg_redir = 1, txmsg_apply = 4097 and txmsg_cork = 4097. It probably
takes the re-cork path, because the test program calls
bpf_msg_cork_bytes() on every run. The test does not check for leaks.

Could the leftover be handed back to psock->cork? Alternatively, could the
code track that msg is a detached heap cork and kfree() it on every exit?

>  			ret = 0;
>  		}
>  		break;

[Severity: High]
This is a pre-existing issue and was not introduced by this patch, but
the patch adds another way to reach it.

tcp_bpf_send_verdict() drops the socket lock around
tcp_bpf_sendmsg_redir(). It then goes back to more_data without
re-checking psock->cork, psock->eval, psock->sk_redir or
psock->apply_bytes.

The __SK_REDIRECT case also assumes that a non-NULL psock->cork is the
local msg, and never checks msg == psock->cork. The re-cork path also
memcpy()s over any existing psock->cork.

Can a second sender on the same socket race with this window?

Thread A                                Thread B
tcp_bpf_sendmsg()
  msg_tx = &tmp (psock->cork == NULL)
  tcp_bpf_send_verdict()
    __SK_REDIRECT, apply_bytes < size
    apply_bytes hits 0, eval = __SK_NONE
    release_sock(sk)
    tcp_bpf_sendmsg_redir()             tcp_bpf_sendmsg()
                                          lock_sock(sk)
                                          program sets cork_bytes
                                          psock->cork = M2 (kzalloc)
                                          memcpy(psock->cork, msg, ...)
                                          release_sock(sk)
    lock_sock(sk)
    data remains, goto more_data

Thread A can then go one of two ways.

If it takes the re-cork path, memcpy(M2, msg) overwrites thread B's
message. B's data is lost, and its sg pages and sk_mem charge leak.

If it takes __SK_REDIRECT, it sets cork = true and psock->cork = NULL
without owning M2, so M2 leaks. An example is thread A sending exactly
3 * 4097 bytes with apply = cork = 4097.

In that case msg is still the on-stack tmp from tcp_bpf_sendmsg():

		if (psock->cork) {
			msg_tx = psock->cork;
		} else {
			msg_tx = &tmp;
			sk_msg_init(msg_tx);
		}

When the redirect fully consumes the message or fails, the cork cleanup
runs sk_msg_free(sk, msg) and kfree(msg) on a stack address.

With this patch, a detached heap cork msg can now also go around the loop
after the lock is dropped. So both outcomes can now also leak or
overwrite M2 when msg is a heap cork.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261114115222.180345-1-omermetekaya0%40gmail.com