Re: [PATCH mptcp-next v4 4/5] mptcp: implement psock_update_sk_prot

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Infinite kernel loop in `sock_map_close` and `sock_map_destroy` when a listening MPTCP socket in a sockmap accepts a new connection.
- [High] Missing `recvmsg` override for MPTCP BPF sockets causes packets redirected via SK_PASS or SK_REDIRECT to be permanently leaked.
- [Medium] Attaching a `msg_parser` (`sk_msg`) program to an MPTCP socket silently succeeds but is never executed due to missing `sendmsg` override.
--

--- Patch [4]: [PATCH mptcp-next v4 4/5] mptcp: implement psock_update_sk_prot ---
commit b125c7aa25aed0ed03598733bc75a385728067e7
Author: Geliang Tang <geliang@kernel.org>
Subject: mptcp: implement psock_update_sk_prot

This patch adds basic MPTCP support for BPF sockmap by implementing the
psock_update_sk_prot callback. This enables MPTCP sockets to be added to
sockmap and supports the sk_skb stream_verdict redirect path.

Link: https://lore.kernel.org/oe-kbuild-all/202512261144.DxrvwMS3-lkp@intel.com/
Link: https://github.com/multipath-tcp/mptcp_net-next/issues/521

> diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
> index 82b0ad25f7001..df41f72603c0f 100644
> --- a/net/mptcp/bpf.c
> +++ b/net/mptcp/bpf.c

[ ... ]

> +static void mptcp_bpf_rebuild_protos(struct proto prot[MPTCP_BPF_NUM_CFGS],
> +				     struct proto *base)
> +{
> +	prot[MPTCP_BPF_BASE]			= *base;
> +	prot[MPTCP_BPF_BASE].destroy		= sock_map_destroy;
> +	prot[MPTCP_BPF_BASE].close		= sock_map_close;

[Severity: Critical]
Does accepting a connection on a listening MPTCP socket in a sockmap lead to
an infinite loop here?

When an incoming connection is accepted, the socket is cloned and the child
socket inherits mptcp_bpf_prots. Because sk_user_data (the psock) is cleared
for the child socket, sock_map_close() will find a NULL psock and attempt to
call the original sk_prot->close.

Since MPTCP lacks an equivalent to tcp_bpf_clone() to restore the original
protocol on cloned sockets, sk_prot->close resolves back to sock_map_close()
itself, triggering an infinite goto retry loop.

> +	prot[MPTCP_BPF_BASE].sock_is_readable	= sk_msg_is_readable;
> +
> +	prot[MPTCP_BPF_TX]			= prot[MPTCP_BPF_BASE];
> +	prot[MPTCP_BPF_RX]			= prot[MPTCP_BPF_BASE];
> +	prot[MPTCP_BPF_TXRX]			= prot[MPTCP_BPF_BASE];
> +}

[Severity: High]
Does this cause redirected packets to be permanently leaked?

If a stream_verdict program returns SK_PASS (the default behavior to allow
packets) or another BPF program redirects to this MPTCP socket via
SK_REDIRECT, the data is placed into the BPF ingress_msg queue.

Because this patch explicitly does not override recvmsg(), mptcp_recvmsg()
only reads from sk_receive_queue and never checks the BPF ingress queues.
The data appears to be permanently blackholed, and since mptcp_poll() checks
only MPTCP-level data, EPOLLIN is broken as well.

[ ... ]

> +int mptcp_bpf_update_proto(struct sock *sk, struct sk_psock *psock,
> +			   bool restore)
> +{
> +	int family = sk->sk_family == AF_INET6 ? MPTCP_BPF_IPV6 :
> +						 MPTCP_BPF_IPV4;
> +	int config = psock->progs.msg_parser   ? MPTCP_BPF_TX   :
> +						 MPTCP_BPF_BASE;

[Severity: Medium]
Should attaching a msg_parser program return an error like -EOPNOTSUPP
instead of silently succeeding?

If a msg_parser program is attached, the configuration is set to MPTCP_BPF_TX.
However, mptcp_bpf_rebuild_protos() maps MPTCP_BPF_TX back to MPTCP_BPF_BASE,
which does not override sendmsg(). As a result, the attachment succeeds but
outgoing packets will bypass the BPF program entirely.

> +
> +	if (psock->progs.stream_verdict || psock->progs.skb_verdict)
> +		config = (config == MPTCP_BPF_TX) ? MPTCP_BPF_TXRX :
> +						    MPTCP_BPF_RX;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1787554581.git.tanggeliang@kylinos.cn?part=4