Re: [PATCH bpf 03/15] bpf, sockmap: Don't let an SK_PASS verdict cover the next message

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] bpf: corrupt verdict state after tcp_bpf_push() in tcp_bpf_send_verdict()
--

commit cb1b40f55f45104a6d23bf64e86e3c0a379b2459
Author: Alexei Starovoitov <ast@kernel.org>

bpf, sockmap: Don't let an SK_PASS verdict cover the next message

This commit resets psock->eval before calling tcp_bpf_push() when
psock->apply_bytes is 0, and subtracts any sent bytes from
psock->apply_bytes if tcp_bpf_push() fails.

> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index d90bcc911b36b..072ffd8347a6f 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c
> @@ -457,8 +457,14 @@ static int tcp_bpf_send_verdict(struct sock *sk, struct sk_psock *psock,
>  
>  	switch (psock->eval) {
>  	case __SK_PASS:
> +		/* The push may sleep without the socket lock, and it may fail */
> +		if (!psock->apply_bytes)
> +			psock->eval = __SK_NONE;
> +		origsize = msg->sg.size;
>  		ret = tcp_bpf_push(sk, msg, tosend, flags, true);
>  		if (unlikely(ret)) {
> +			/* A part of it may have gone out */
> +			sk_msg_apply_bytes(psock, origsize - msg->sg.size);
>  			*copied -= sk_msg_free(sk, msg);
>  			break;
>  		}

[Severity: High]
Can this corrupt a concurrent sender's verdict state or leave psock->eval
set when psock->apply_bytes reaches 0?

In tcp_bpf_send_verdict() under case __SK_PASS, if Thread A enters with
psock->apply_bytes == 0, it resets psock->eval = __SK_NONE before calling
tcp_bpf_push(). If tcp_bpf_push() -> tcp_sendmsg_locked() releases the
socket lock in sk_stream_wait_memory(), a concurrent Thread B in
tcp_bpf_sendmsg() -> tcp_bpf_send_verdict() can see psock->eval == __SK_NONE,
run sk_psock_msg_verdict(), and install a new psock->eval (including
__SK_REDIRECT with psock->sk_redir) and psock->apply_bytes.

When Thread A returns from tcp_bpf_push(), it still calls
sk_msg_apply_bytes() on both the error and success paths, and on success runs
the post-switch cleanup:

net/ipv4/tcp_bpf.c:tcp_bpf_send_verdict() {
	...
	case __SK_PASS:
		/* The push may sleep without the socket lock, and it may fail */
		if (!psock->apply_bytes)
			psock->eval = __SK_NONE;
		origsize = msg->sg.size;
		ret = tcp_bpf_push(sk, msg, tosend, flags, true);
		if (unlikely(ret)) {
			/* A part of it may have gone out */
			sk_msg_apply_bytes(psock, origsize - msg->sg.size);
			*copied -= sk_msg_free(sk, msg);
			break;
		}
		sk_msg_apply_bytes(psock, tosend);
		break;
	...
	if (likely(!ret)) {
		if (!psock->apply_bytes) {
			psock->eval =  __SK_NONE;
			if (psock->sk_redir) {
				sock_put(psock->sk_redir);
				psock->sk_redir = NULL;
			}
		}
}

Wouldn't Thread A subtract its own sent bytes from Thread B's
psock->apply_bytes, and if psock->apply_bytes is 0 when ret == 0, reset
Thread B's psock->eval to __SK_NONE and drop Thread B's psock->sk_redir?

Also, on the error path (unlikely(ret)), if sk_msg_apply_bytes(psock,
origsize - msg->sg.size) reduces a non-zero psock->apply_bytes to 0 (for
instance if another sender consumed part of psock->apply_bytes while
tcp_bpf_push() slept), the post-switch block is skipped because ret != 0.
Wouldn't that leave psock->eval as __SK_PASS with psock->apply_bytes == 0,
causing the next message to bypass the sk_msg program?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261101192057.2296489-1-alexei.starovoitov@gmail.com?part=3