Re: [PATCH v2 mptcp-next] Squash-to: "mptcp: leverage the backlog for RX packet processing"

[Geliang Tang <geliang@kernel.org>]

Hi Paolo,

Thanks for this fix.

On Sun, 2025-11-09 at 14:53 +0100, Paolo Abeni wrote:
> If a subflow receives data before gaining the memcg while the msk
> socket lock is held at accept time, or the PM locks the msk socket
> while still unaccepted and subflows push data to it at the same time,
> the mptcp_graph_subflows() can complete with a non empty backlog.
> 
> The msk will try to borrow such memory, but (some) of the skbs there
> where not memcg charged. When the msk finally will return such
> accounted
> memory, we should hit the same splat of #597.
> [even if so far I was unable to replicate this scenario]
> 
> This patch tries to address such potential issue by:
> - preventing the subflow from queuing data into the backlog after
>   gaining the memcg. This ensure that at the end of the look all the
>   skbs in the backlog (if any) are _not_ memory accounted.
> - mem charge the backlog to msk
> - 'restart' the subflow and spool any data waiting there.
> 
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>  net/mptcp/protocol.c | 46
> ++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 44 insertions(+), 2 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 5e9325c7ea9c..d6b08e1de358 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -4082,10 +4082,12 @@ static void mptcp_graph_subflows(struct sock
> *sk)
>  {
>  	struct mptcp_subflow_context *subflow;
>  	struct mptcp_sock *msk = mptcp_sk(sk);
> +	struct sock *ssk;
> +	int old_amt, amt;
> +	bool slow;
>  
>  	mptcp_for_each_subflow(msk, subflow) {
> -		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> -		bool slow;
> +		ssk = mptcp_subflow_tcp_sock(subflow);
>  
>  		slow = lock_sock_fast(ssk);
>  
> @@ -4095,8 +4097,48 @@ static void mptcp_graph_subflows(struct sock
> *sk)
>  		if (!ssk->sk_socket)
>  			mptcp_sock_graft(ssk, sk->sk_socket);
>  
> +		if (!mem_cgroup_from_sk(sk))
> +			goto unlock;

I think it's better to use "continue" here, just like in v1, so that
other subflows also have a chance to call mptcp_sock_graft(), but we
need to call unlock_sock_fast() before "continue".

Besides, wouldn't it be more appropriate to squash these lines into
"mptcp: fix memcg accounting for passive sockets"?

> +
>  		__mptcp_inherit_cgrp_data(sk, ssk);
>  		__mptcp_inherit_memcg(sk, ssk, GFP_KERNEL);
> +
> +		/* Prevent subflows from queueing data into the
> backlog
> +		 * as soon as cg is set; note that we can't race
> +		 * with __mptcp_close_ssk setting this bit for a
> really
> +		 * closing socket, because we hold the msk socket
> lock here.
> +		 */
> +		subflow->closing = 1;
> +
> +unlock:
> +		unlock_sock_fast(ssk, slow);
> +	}
> +
> +	if (!mem_cgroup_from_sk(sk))
> +		return;
> +
> +	/* Charge the bl memory, note that __sk_charge accounted for
> +	 * fwd memory and rmem only
> +	 */
> +	mptcp_data_lock(sk);
> +	old_amt = sk_mem_pages(sk->sk_forward_alloc +
> +			       atomic_read(&sk->sk_rmem_alloc));
> +	amt = sk_mem_pages(msk->backlog_len + sk->sk_forward_alloc +
> +		     atomic_read(&sk->sk_rmem_alloc));

The code here is not aligned properly.

> +	amt -= old_amt;
> +	if (amt)
> +		mem_cgroup_sk_charge(sk, amt, GFP_ATOMIC |
> __GFP_NOFAIL);

I'm not sure if we need to call kmem_cache_charge() here, just like in
__sk_charge().

WDYT?

Thanks,
-Geliang

> +	mptcp_data_unlock(sk);
> +
> +	/* Finally let the subflow restart queuing data. */
> +	mptcp_for_each_subflow(msk, subflow) {
> +		ssk = mptcp_subflow_tcp_sock(subflow);
> +
> +		slow = lock_sock_fast(ssk);
> +		subflow->closing = 0;
> +
> +		if (mptcp_subflow_data_available(ssk))
> +			mptcp_data_ready(sk, ssk);
>  		unlock_sock_fast(ssk, slow);
>  	}
>  }