Re: [PATCH mptcp-net 1/6] mptcp: pm: userspace: properly handle the ID0 case

[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 73094bdbdbf27..6e6eeda91ade0 100644
>> --- a/net/mptcp/pm_userspace.c
>> +++ b/net/mptcp/pm_userspace.c
>> @@ -55,7 +55,10 @@ static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
>>  
>>  	spin_lock_bh(&msk->pm.lock);
>>  	mptcp_for_each_userspace_pm_addr(msk, e) {
>> -		addr_match = mptcp_addresses_equal(&e->addr, &entry->addr, true);
>> +		/* allow matching ID0 when no port is specified */
>> +		addr_match = mptcp_addresses_equal(&e->addr, &entry->addr,
>> +						   e->addr.id != 0 ||
>> +						   entry->addr.port != 0);
>>  		if (addr_match && entry->addr.id == 0 && needs_id)
>>  			entry->addr.id = e->addr.id;
>>  		id_match = (e->addr.id == entry->addr.id);
>>  		if (addr_match || id_match)
>>  			break;
> 
> With ID 0 now always present in userspace_pm_local_addr_list, does this
> id_match check prematurely break the loop for new unassigned addresses?
> 
> If an unconfigured address is evaluated (e.g. from an incoming MP_JOIN
> request), entry->addr.id defaults to 0. Since the pre-added initial address
> (ID 0) is now in the list, e->addr.id == entry->addr.id (0 == 0) will be
> true, falsely detecting an ID collision and preventing the new address from
> being inserted.

I guess it meant "if the IP address is not the same". If it is, now the
right ID0 will be returned instead of a new one. If the address is
different, then yes, id_match should always be false when needs_id is
set. I will adapt that in a v2. (The bug was also present before when
the ID0 was in the list.)

>> @@ -692,6 +695,24 @@ int mptcp_userspace_pm_get_addr(u8 id, struct mptcp_pm_addr_entry *addr,
>>  	return ret;
>>  }
>>  
>> +/* Add the initial local address (ID0) to the local list: easier that way */
>> +void mptcp_pm_userspace_created(struct mptcp_sock *msk, const struct sock *ssk)
>> +{
>> +	struct mptcp_pm_addr_entry *entry;
>> +
>> +	entry = sock_kmalloc((struct sock *)msk, sizeof(*entry), GFP_ATOMIC);
>> +	/* Fine not to handle the ID0 case in memory pressure */
>> +	if (!entry)
>> +		return;
>> +
>> +	memset(entry, 0, sizeof(*entry));
>> +	mptcp_local_address((struct sock_common *)ssk, &entry->addr);
>> +
>> +	spin_lock_bh(&msk->pm.lock);
>> +	list_add_tail_rcu(&entry->list, &msk->pm.userspace_pm_local_addr_list);
> 
> Does this code forget to increment msk->pm.local_addr_used?

No, on purpose.

> When this address is added here, the counter is not incremented. Later, if
> userspace creates a subflow using ID 0 and destroys it, or if subflow creation
> fails, mptcp_userspace_pm_delete_local_addr() will unconditionally decrement
> msk->pm.local_addr_used. This could cause the counter to underflow to 255
> and permanently remove the ID 0 address from the list.

Indeed, an exception is needed there. Will add that in v2.

> 
>> +	spin_unlock_bh(&msk->pm.lock);
>> +}

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