Re: [RFC PATCH mptcp-next 2/3] mptcp: pm: kernel: retry subflows closed after an error

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mptcp: pm: infinite loop when net.mptcp.subflow_retry_timeout is 0
- [Medium] mptcp: pm: retry list slot exhaustion due to uncollected disabled entries
--

commit d5d7b6b49191fe292c451a4a994147ebff6534b3
Author: Anil Kaushik <anilkaushikwireless@gmail.com>

mptcp: pm: kernel: retry subflows closed after an error

This commit introduces a mechanism to retry creating subflows that were
closed due to an error, using the in-kernel path manager. It adds a sysctl
to configure the exponential backoff retry interval, up to a defined maximum
delay.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/440

> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index d7c5b50b34cc9..2683e359695cd 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
[ ... ]
> @@ -493,6 +509,250 @@ static void mptcp_pm_free_announced_list(struct mptcp_sock *msk)
>  	}
>  }
>  
> +/* Subflows closed after an error can be re-created later on, see the
> + * 'subflow_retry_timeout' sysctl. Entries are kept until the msk is destroyed
> + * to track the exponential backoff, and are disabled when the corresponding
> + * local endpoint or remote address is removed.
> + */
> +static void mptcp_pm_retry_timer(struct timer_list *timer)
[ ... ]
> +/* To be called while pm->lock is held */
> +static void mptcp_pm_retry_arm(struct mptcp_sock *msk,
> +			       struct mptcp_pm_retry *entry)
> +{
> +	unsigned long max = mptcp_subflow_retry_timeout(sock_net((struct sock *)msk));
> +	unsigned long timeout = TCP_TIMEOUT_INIT << min_t(u8, entry->attempts, 16);
> +
> +	entry->pending = false;
> +	entry->timer_done = false;
> +	if (entry->attempts < U8_MAX)
> +		entry->attempts++;
> +
> +	sk_reset_timer((struct sock *)msk, &entry->timer,
> +		       jiffies + min(timeout, max));

[Severity: High]
What happens if the sysctl is dynamically changed to 0 while there are active
entries in the retry list?

It appears mptcp_pm_retry_arm() calculates max as 0, which results in arming
the timer with jiffies + 0. The timer would then fire immediately and schedule
the PM worker mptcp_pm_retry_subflows(). If the connection fails again, the
worker immediately calls mptcp_pm_retry_arm() again.

Could this create an infinite, zero-delay loop between the timer softirq and
the PM worker queue that monopolizes a CPU core? Perhaps the code should
explicitly check if the sysctl is 0 and halt retries for existing entries,
rather than clamping the timer delay to 0.

> +}
> +
> +static bool mptcp_pm_retry_path_error(int err)
[ ... ]
> +static void mptcp_pm_retry_add(struct mptcp_sock *msk,
> +			       const struct mptcp_subflow_context *subflow)
> +{
[ ... ]
> +	spin_lock_bh(&msk->pm.lock);
> +	if (msk->pm.status & BIT(MPTCP_PM_DESTROYING))
> +		goto unlock;
> +
> +	list_for_each_entry(entry, &msk->pm.retry_list, list) {
> +		if (mptcp_addresses_equal(&entry->local.addr, &local, false) &&
> +		    mptcp_addresses_equal(&entry->remote, &remote, true))
> +			goto found;
> +		nr++;
> +	}
> +
> +	if (nr >= MPTCP_PM_RETRY_MAX)
> +		goto unlock;

[Severity: Medium]
When mptcp_pm_retry_disable() marks retry list entries as disabled, they are
not removed from the retry_list.

During mptcp_pm_retry_add(), the list iteration counts all entries via nr++,
including disabled ones. If normal network events accumulate 8 disabled
entries over the lifetime of a connection, it looks like this limit will be
reached and no new subflows can ever be retried.

Can this lead to permanent slot exhaustion on this connection? Should disabled
entries be evicted or skipped in the count to avoid this?

> +
> +	entry = kzalloc_obj(*entry, GFP_ATOMIC);
> +	if (!entry)
> +		goto unlock;
> +
> +	entry->sock = msk;
> +	timer_setup(&entry->timer, mptcp_pm_retry_timer, 0);
> +	list_add_tail(&entry->list, &msk->pm.retry_list);
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012171740.464517-1-anilkaushikwireless@gmail.com?part=2