Re: [PATCH mptcp-net 2/6] mptcp: pm: userspace: allow announcing ID0 addr

[Matthieu Baerts <matttbe@kernel.org>]

Replying to Sashiko's review:

>> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
>> index 6e6eeda91ade0..4c96433590e36 100644
>> --- a/net/mptcp/pm_userspace.c
>> +++ b/net/mptcp/pm_userspace.c
>> @@ -212,12 +212,6 @@ int mptcp_pm_nl_announce_doit(struct sk_buff *skb, struct genl_info *info)
>>  	if (err < 0)
>>  		goto announce_err;
>>  
>> -	if (addr_val.addr.id == 0) {
>> -		NL_SET_ERR_MSG_ATTR(info->extack, addr, "invalid addr id");
>> -		err = -EINVAL;
>> -		goto announce_err;
>> -	}
>> -
> 
> If userspace deletes the initial subflow via MPTCP_PM_CMD_SUBFLOW_DESTROY,
> which removes the ID 0 entry from the local address list, can userspace then
> announce an arbitrary IP address with ID 0?
> 
> Since the original ID 0 entry would be missing, it seems
> mptcp_userspace_pm_append_new_local_addr() wouldn't find an address or ID
> match:
> 
> net/mptcp/pm_userspace.c:mptcp_userspace_pm_append_new_local_addr() {
>     ...
> 	if (!addr_match && !id_match) {
> 		...
> 		list_add_tail_rcu(&e->list, &msk->pm.userspace_pm_local_addr_list);
> 		msk->pm.local_addr_used++;
> 		ret = e->addr.id;
> 	}
>     ...
> }
> 
> Would this incorrectly append a new entry with ID 0 and an arbitrary IP
> address, bypassing the validation mentioned in the commit message?

Yes, but the RFC8684 doesn't specify if the ID0 can be replaced by
another address, once removed. But it says: "A host wishing to replace
an existing Address ID MUST first remove the existing one". In this
condition, better to let the responsibility to the userspace daemon.

I can add a note in the commit message.

> Does allowing ID 0 announcements here also create a state inconsistency when
> trying to remove them?
> 
> Looking at mptcp_pm_nl_remove_doit(), there is a hardcoded fast path for ID 0:
> 
> net/mptcp/pm_userspace.c:mptcp_pm_nl_remove_doit() {
>     ...
> 	if (id_val == 0) {
> 		err = mptcp_userspace_pm_remove_id_zero_address(msk);
> 		goto out;
> 	}
>     ...
> }
> 
> Because mptcp_userspace_pm_remove_id_zero_address() doesn't call
> mptcp_pm_announced_remove() and ignores msk->pm.anno_list, would this
> leave the announcement un-cancellable and leak the state in anno_list?
Removed in the next patch.

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