Re: [PATCH bpf v3 1/2] bpf, sockmap: settle sk_forward_alloc for strparser SK_PASS

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

New issues:
- [Low] Multi-line comments added in the patch violate the BPF subsystem comment style.

Pre-existing issues:
- [High] Missing socket lock in sk_psock_skb_ingress() for cross-socket redirects causes sk_forward_alloc corruption.
--

--- Patch [1]: [PATCH bpf v3 1/2] bpf, sockmap: settle sk_forward_alloc for strparser SK_PASS ---
commit 74acfd67ff001cb4d8a5e9b0c6aaa8bfc595f145
Author: Junseo Lim <[email protected]>

bpf, sockmap: settle sk_forward_alloc for strparser SK_PASS

This commit addresses sk_forward_alloc deficits caused by strparser SK_PASS
queuing cloned skbs back to the same socket. It settles any deficit before
assigning receive ownership to unowned self-pass skbs.

> diff --git a/net/core/skmsg.c b/net/core/skmsg.c
> index 2521b643fa05d..347cb168f7491 100644
> --- a/net/core/skmsg.c
> +++ b/net/core/skmsg.c
[ ... ]
>  static int sk_psock_skb_ingress(struct sk_psock *psock, struct sk_buff *skb,
> -				u32 off, u32 len)
> +				u32 off, u32 len, bool settle_fwd_alloc)
>  {
>  	struct sock *sk = psock->sk;
>  	struct sk_msg *msg;
>  	int err;
>  
>  	if (unlikely(skb->sk == sk))
> -		return sk_psock_skb_ingress_self(psock, skb, off, len, true);
> +		return sk_psock_skb_ingress_self_backlog(psock, skb, off,
> +							 len, true,
> +							 settle_fwd_alloc);
>  	msg = sk_psock_create_ingress_msg(sk, skb);
>  	if (!msg)
>  		return -EAGAIN;

[Severity: High]
This is a pre-existing issue, but does this cross-socket redirect path modify
sk_forward_alloc without holding the socket lock?

When called asynchronously from the backlog workqueue:
sk_psock_backlog() -> sk_psock_handle_skb() -> sk_psock_skb_ingress()

If the skb is redirected to a different socket (where skb->sk does not match
the destination sk), it bypasses the self_backlog path which properly holds
lock_sock(sk). It then proceeds to call sk_psock_create_ingress_msg() and
skb_set_owner_r() without the target socket's lock. Since sk_forward_alloc
is updated via non-atomic operations, could concurrent updates to the target
socket corrupt sk_forward_alloc and cause page_counter underflows?

> @@ -618,34 +621,100 @@ static int sk_psock_skb_ingress(struct sk_psock *psock, struct sk_buff *skb,
[ ... ]
> +/* Puts an skb on the ingress queue for psock->sk.
> + *
> + * If the skb already has receive ownership for this socket, leave socket
> + * memory accounting untouched. Otherwise, before assigning receive ownership
> + * to an unowned strparser SK_PASS skb, settle any existing sk_forward_alloc
> + * deficit from earlier clone charges.
>   */
>  static int sk_psock_skb_ingress_self(struct sk_psock *psock, struct sk_buff *skb,

[Severity: Low]
This isn't a bug, but does this comment format follow the BPF subsystem
style?

The BPF subsystem uses the modern kernel multi-line comment style, which
requires the opening /* to be on its own line.

[ ... ]
> +	/* This is used in tcp_bpf_recvmsg_parser() to determine whether the
> +	 * data originates from the socket's own protocol stack. No need to
> +	 * refcount sk because msg's lifetime is bound to sk via the ingress_msg.
> +	 */
> +	msg->sk = sk;

[Severity: Low]
This isn't a bug, but should this multi-line comment also have its opening
/* on a separate line to match the subsystem conventions?

[ ... ]
> +static int sk_psock_skb_ingress_self_backlog(struct sk_psock *psock,
[ ... ]
> +	/* This is used in tcp_bpf_recvmsg_parser() to determine whether the
> +	 * data originates from the socket's own protocol stack. No need to
> +	 * refcount sk because msg's lifetime is bound to sk via the ingress_msg.
> +	 */
> +	msg->sk = sk;

[Severity: Low]
This isn't a bug, but is the opening /* on the same line missing the BPF
comment style requirement here as well?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1