Re: [PATCH mptcp-net] mptcp: pm: only set fullmesh for subflow endp

[Matthieu Baerts <matttbe@kernel.org>]

Hi Paolo,

On 16/01/2025 19:37, Paolo Abeni wrote:
> On 1/16/25 7:15 PM, Matthieu Baerts (NGI0) wrote:
>> With the in-kernel path-manager, it is possible to change the 'fullmesh'
>> flag. The code in mptcp_pm_nl_fullmesh() expects to change it only on
>> 'subflow' endpoints, to recreate more or less subflows using the linked
>> address.
>>
>> Unfortunately, the set_flags() hook was a bit more permissive, and
>> allowed 'implicit' endpoints to get the 'fullmesh' flag while it is not
>> allowed before.
>>
>> That's what syzbot found, triggering the following warning:

(...)

>> Fixes: 73c762c1f07d ("mptcp: set fullmesh flag in pm_netlink")
>> Reported-by: syzbot+cd16e79c1e45f3fe0377@syzkaller.appspotmail.com
>> Closes: https://lore.kernel.org/6786ac51.050a0220.216c54.00a6.GAE@google.com
>> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/540
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>>  net/mptcp/pm_netlink.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>> index fef01692eaed404e272359df691264f797240d10..7131dc2f312d923a31bb4b0460b6b4663376fbd0 100644
>> --- a/net/mptcp/pm_netlink.c
>> +++ b/net/mptcp/pm_netlink.c
>> @@ -1986,7 +1986,8 @@ int mptcp_pm_nl_set_flags(struct mptcp_pm_addr_entry *local,
>>  		return -EINVAL;
>>  	}
>>  	if ((local->flags & MPTCP_PM_ADDR_FLAG_FULLMESH) &&
>> -	    (entry->flags & MPTCP_PM_ADDR_FLAG_SIGNAL)) {
>> +	    ((entry->flags & MPTCP_PM_ADDR_FLAG_SIGNAL) ||
>> +	     (entry->flags & MPTCP_PM_ADDR_FLAG_IMPLICIT))) {
>>  		spin_unlock_bh(&pernet->lock);
>>  		NL_SET_ERR_MSG_ATTR(info->extack, attr, "invalid addr flags");
>>  		return -EINVAL;
> 
> LGTM, thanks!
> 
> slight preference for:
> 
> 		entry->flags & (MPTCP_PM_ADDR_FLAG_SIGNAL ||
> 			        MPTCP_PM_ADDR_FLAG_IMPLICIT)
> 
> but definitely not strong opinion about it.

Good point! I agree with you. That was in fact my v2, then I switched to
a version with:

  flags & (SUBFLOW | SIGNAL | IMPLICIT) == SUBFLOW

but then I realised the case with no "type" (so not subflow, signal nor
implicit) was OK in fact, even if it will recreate the subflows for no
reasons, but that was already the case before.

So I switched back to v1, and forgot about v2 :)

Can I add your Acked-by or you prefer not to?

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