Re: [PATCH mptcp-net 3/6] mptcp: pm: userspace: no ID0 exception for RM_ADDR

[Matthieu Baerts <matttbe@kernel.org>]

Replying to Sashiko's review:

>> After the first patch of this series, the initial ID0 address is present
>> in the local addr list when the connection has been created.
> 
> Does adding the initial ID 0 address to the local address list cause
> secondary subflow creation to fail for incoming joins on unannounced
> addresses?
> 
> When a secondary subflow is initiated using a new address with ID 0,
> mptcp_userspace_pm_append_new_local_addr() loops over the list.
> 
> The new entry will match the ID 0 of the initial address (id_match = true)
> but its IP differs (addr_match = false).
> 
> This causes the loop to break, bypassing allocation and ID assignment,
> and returns -EINVAL.

Same comment as on patch 1/6.

> Does destroying the initial subflow (ID 0) via userspace PM cause an
> underflow of msk->pm.local_addr_used?
> 
> If the initial ID 0 connection address is added without incrementing
> the local_addr_used counter, later calling MPTCP_PM_CMD_SUBFLOW_DESTROY
> on ID 0 will unconditionally decrement it in
> mptcp_userspace_pm_delete_local_addr().
> 
> Could this wrap the 8-bit counter to 255 and corrupt the statistics
> exported to userspace?

Same comment as on patch 1/6.
>> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
>> index 4c96433590e36..9c7fab6a59bf4 100644
>> --- a/net/mptcp/pm_userspace.c
>> +++ b/net/mptcp/pm_userspace.c
> 
> [ ... ]
> 
>> @@ -315,11 +284,6 @@ int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info)
>>  
>>  	sk = (struct sock *)msk;
>>  
>> -	if (id_val == 0) {
>> -		err = mptcp_userspace_pm_remove_id_zero_address(msk);
>> -		goto out;
>> -	}
>> -
>>  	lock_sock(sk);
>>  
>>  	spin_lock_bh(&msk->pm.lock);
> 
> This is a pre-existing issue, but does decrementing sk_omem_alloc eagerly
> while freeing memory via RCU allow a local user in a user namespace to
> bypass socket memory limits?
> 
> Later in mptcp_pm_nl_remove_doit():
> 
> 	kfree_rcu_mightsleep(match);
> 	/* Adjust sk_omem_alloc like sock_kfree_s() does, to match
> 	 * with allocation of this memory by sock_kmemdup()
> 	 */
> 	atomic_sub(sizeof(*match), &sk->sk_omem_alloc);
> 
> If a user with CAP_NET_ADMIN repeatedly announces and removes addresses in
> a tight loop via MPTCP_PM_CMD_ANNOUNCE and MPTCP_PM_CMD_REMOVE, the
> sk_omem_alloc counter is decremented immediately, but the actual freeing
> is deferred to RCU grace periods.
> 
> Could this potentially lead to memory exhaustion (OOM) on the host?

Mmh, I don't think we need to handle this case with CAP_NET_ADMIN, plus
the netlink communication will not allow too many loops I suppose.

(+ pre-existing issue: for a follow-up patch anyway)

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