Re: [PATCH mptcp-net 1/2] mptcp: pm: in-kernel: always set as unavail when removing addr

[Matthieu Baerts <matttbe@kernel.org>]

Hi Mat,

On 26/01/2026 19:34, Matthieu Baerts wrote:
> Hi Mat,
> 
> Thank you for the review!
> 
> On 15/01/2026 05:56, Mat Martineau wrote:
>> On Mon, 15 Dec 2025, Matthieu Baerts (NGI0) wrote:
>>
>>> Syzkaller managed to find a combination of actions that was generating
>>> this warning:
> 
> (...)
> 
>>> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
>>> index f59d21e7579c..51bcfcec882d 100644
>>> --- a/net/mptcp/pm_kernel.c
>>> +++ b/net/mptcp/pm_kernel.c
>>> @@ -1057,10 +1057,8 @@ static bool mptcp_pm_remove_anno_addr(struct
>>> mptcp_sock *msk,
>>>     ret = mptcp_remove_anno_list_by_saddr(msk, addr);
>>>     if (ret || force) {
>>>         spin_lock_bh(&msk->pm.lock);
>>> -        if (ret) {
>>> -            __set_bit(addr->id, msk->pm.id_avail_bitmap);
>>> +        if (ret)
>>>             msk->pm.add_addr_signaled--;
>>> -        }
>>>         mptcp_pm_remove_addr(msk, &list);
>>>         spin_unlock_bh(&msk->pm.lock);
>>>     }
>>> @@ -1098,17 +1096,14 @@ static int
>>> mptcp_nl_remove_subflow_and_signal_addr(struct net *net,
>>>                       !(entry->flags & MPTCP_PM_ADDR_FLAG_IMPLICIT));
>>>
>>>         list.ids[0] = mptcp_endp_get_local_id(msk, addr);
>>> -        if (remove_subflow) {
>>> -            spin_lock_bh(&msk->pm.lock);
>>> -            mptcp_pm_rm_subflow(msk, &list);
>>> -            spin_unlock_bh(&msk->pm.lock);
>>> -        }
>>>
>>> -        if (entry->flags & MPTCP_PM_ADDR_FLAG_SUBFLOW) {
>>> -            spin_lock_bh(&msk->pm.lock);
>>> +        spin_lock_bh(&msk->pm.lock);
>>> +        if (remove_subflow)
>>> +            mptcp_pm_rm_subflow(msk, &list);
>>> +        if (entry->flags & MPTCP_PM_ADDR_FLAG_SUBFLOW)
>>>             __mark_subflow_endp_available(msk, list.ids[0]);
>>> -            spin_unlock_bh(&msk->pm.lock);
>>> -        }
>>> +        __set_bit(addr->id, msk->pm.id_avail_bitmap);
>>
>> There's not any harm in setting this bit a second time if it was also
>> set in __mark_subflow_endp_available().
>>
>> However, __mark_subflow_endp_available() has some logic around ID 0 and
>> mpc_endpoint_id. Is that relevant in this code path or is the new
>> __set_bit() doing the correct thing by always clearing based on addr->id?
> 
> Good point. Even if there is no harm, no need to set the bit for ID 0. I
> will look at that!

I just re-checked this: addr->id here is always positive because that's
the endpoint ID, not the ID used on the wire (list.ids[0]) which can be
0 if this endpoint is linked to the initial subflow. So we don't need
the same logic around ID 0 and mpc_endpoint_id.

Still, I can add this before calling __set_bit() not to clear the bit a
second time:

  else /* mark endp ID as available, e.g. Signal or MPC endp */

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.