Re: [PATCH] mptcp: pm: Fix address ID overflow when all IDs are exhausted

Hi,

Thank you for this patch. Here is a very quick review done in between
tasks.

(-people who were in Cc: no need to add them when sending only to the
MPTCP ML.)

Also, please use a prefix: mptcp-net for fixes, mptcp-next for features.

The CI reported a few issues at build time, please check that.

> When all MPTCP address IDs (1-255) are exhausted, find_next_zero_bit()
> returns MPTCP_PM_MAX_ADDR_ID + 1 (256). This value overflows when stored
> in the u8 field e->addr.id, resulting in ID 0 being stored.

Are you sure? With the in-kernel PM? Do you have a reproducer?

> ID 0 has special meaning in MPTCP (it's reserved for the initial connection),
> so this overflow can cause confusion and incorrect behavior, including
> unintentional ID 0 reuse or address conflicts.

A fix should have a Fixes tag, please add one.

Also, if you were assisted by a tool, please add the Assisted-by tag.

> Signed-off-by: luoqing <[email protected]>

For "legal" reasons, you are supposed to put your full name. Having only
one "word" for your full name, without capital letters looks wrong, no?

>
> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
> index 331f6fa99014..1bd81c94b971 100644
> --- a/net/mptcp/pm_kernel.c
> +++ b/net/mptcp/pm_kernel.c
> @@ -795,9 +795,14 @@ static int mptcp_pm_nl_append_new_local_addr(struct pm_nl_pernet *pernet,
>
>  	if (!entry->addr.id) {
>  find_next:
> -		entry->addr.id = find_next_zero_bit(pernet->id_bitmap,
> -						    MPTCP_PM_MAX_ADDR_ID + 1,
> -						    pernet->next_id);
> +		unsigned int id = find_next_zero_bit(pernet->id_bitmap,
> +						     MPTCP_PM_MAX_ADDR_ID + 1,
> +						     pernet->next_id);
> +		if (id > MPTCP_PM_MAX_ADDR_ID) {
> +			ret = -ENOSPC;
> +			goto out;
> +		}
> +		entry->addr.id = id;
>  		if (!entry->addr.id && pernet->next_id != 1) {

The overflow is a normal case: find_next_zero_bit() will find the next
zero bit from next_id. If there it didn't find any, this block here
restart find_next_zero_bit() but from the beginning.

>  			pernet->next_id = 1;
>  			goto find_next;
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index ad6ba658e5a5..0a1e835ae049 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -74,10 +74,17 @@ static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
>  			goto append_err;
>  		}
>
> -		if (!e->addr.id && needs_id)
> -			e->addr.id = find_next_zero_bit(id_bitmap,

Do you need to change this line? In case of error, 'e' is dropped.

> -							MPTCP_PM_MAX_ADDR_ID + 1,
> -							1);
> +		if (!e->addr.id && needs_id) {
> +			unsigned int id = find_next_zero_bit(id_bitmap,
> +							     MPTCP_PM_MAX_ADDR_ID + 1,
> +							     1);
> +			if (id > MPTCP_PM_MAX_ADDR_ID) {
> +				sock_kfree_s(sk, e, sizeof(*e));
> +				ret = -ENOSPC;
> +				goto append_err;
> +			}
> +			e->addr.id = id;

I didn't check the code, but here, it looks like such check is needed.
But please, provide a test reproducing the issue, e.g. by adding a test
in userspace_pm.sh. (Be careful that such test should also run on older
kernels where the limits are lower, see what is done in pm_netlink.sh)

-- 
Matthieu Baerts (NGI0) <[email protected]>