Re: [PATCH mptcp-net] mptcp: pm: kernel: c-flag: mark as used only when it is

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] mptcp: infinite loop in fill_local_addresses_vec_c_flag() on skipped endpoint
--

commit 13a1b915ae070fcc7bed06cf022912abbebad663
Author: Matthieu Baerts (NGI0) <matttbe@kernel.org>

mptcp: pm: kernel: c-flag: mark as used only when it is

This commit moves the clearing of local->addr.id in msk->pm.id_avail_bitmap
in fill_local_addresses_vec_c_flag() until after checking that the address
families match and the local endpoint is not msk->mpc_endpoint_id.

> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
> index d3014bf57bf3c..3e473a81389d3 100644
> --- a/net/mptcp/pm_kernel.c
> +++ b/net/mptcp/pm_kernel.c
> @@ -582,14 +582,14 @@ fill_local_addresses_vec_c_flag(struct mptcp_sock *msk,
>  		if (!select_local_address(pernet, msk, local))
>  			break;
>  
> -		__clear_bit(local->addr.id, msk->pm.id_avail_bitmap);
> -
>  		if (!mptcp_pm_addr_families_match(sk, &local->addr, remote))
>  			continue;
>  
>  		if (local->addr.id == msk->mpc_endpoint_id)
>  			continue;
>  
> +		__clear_bit(local->addr.id, msk->pm.id_avail_bitmap);

[Severity: Critical]
Could moving __clear_bit() after the continue statements cause an infinite
loop in fill_local_addresses_vec_c_flag()?

In net/mptcp/pm_kernel.c, select_local_address() always scans
pernet->endp_list from the head and returns the first subflow endpoint whose
ID is still set in msk->pm.id_avail_bitmap:

net/mptcp/pm_kernel.c:select_local_address() {
	...
	list_for_each_entry_rcu(entry, &pernet->endp_list, list) {
		if (!(entry->flags & MPTCP_PM_ADDR_FLAG_SUBFLOW))
			continue;

		if (!test_bit(entry->addr.id, msk->pm.id_avail_bitmap))
			continue;

		new_local->addr = entry->addr;
		new_local->flags = entry->flags;
		new_local->ifindex = entry->ifindex;
		found = true;
		break;
	}
	...
}

When __mptcp_pm_kernel_worker() -> mptcp_pm_nl_add_addr_received() ->
fill_local_addresses_vec() -> fill_local_addresses_vec_c_flag() runs and the
selected endpoint either fails mptcp_pm_addr_families_match() or matches
msk->mpc_endpoint_id, the loop takes a continue branch without clearing
local->addr.id in msk->pm.id_avail_bitmap or incrementing
msk->pm.local_addr_used.

On the next iteration of while (msk->pm.local_addr_used < endp_subflow_max),
wouldn't select_local_address() return the exact same endpoint again and loop
indefinitely?

> +
>  		msk->pm.local_addr_used++;
>  		msk->pm.extra_subflows++;
>  		i++;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-mptcp-pm-cflag-clear-when-used-v1-1-516c09b4ee65@kernel.org?part=1