Re: [PATCH mptcp-next v3 6/8] mptcp: netlink: store lsk ref in mptcp_pm_addr_entry

[Kishen Maloor <kishen.maloor@intel.com>]

On 2/1/22 3:31 AM, Paolo Abeni wrote:
> Hello,
> 
> On Thu, 2022-01-27 at 19:38 -0500, Kishen Maloor wrote:
>> @@ -157,6 +157,33 @@ static void lsk_list_release(struct pm_nl_pernet *pernet,
>>  	}
>>  }
>>  
>> +static struct mptcp_local_lsk *lsk_list_find_or_create(struct net *net,
>> +						       struct pm_nl_pernet *pernet,
>> +						       struct mptcp_pm_addr_entry *entry,
>> +						       int *createlsk_err)
>> +{
>> +	struct mptcp_local_lsk *lsk_ref;
>> +	struct socket *lsk;
>> +	int err;
>> +
>> +	lsk_ref = lsk_list_find(pernet, &entry->addr);
>> +
>> +	if (!lsk_ref) {
>> +		err = mptcp_pm_nl_create_listen_socket(net, entry, &lsk);
> 
> What happens if multiple cores call 'lsk_list_find_or_create'
> simultaneously? Is that possible/expected?
> 

That is technically possible, yes.

> I think the expected behaviour in that scenario is creating a single
> new lsk, and have all the callers fetching such instances. If the race
> happens on mptcp_pm_nl_create_listen_socket() it looks like only one
> caller will get a valid lsk reference, all the others will get back an
> error.
> 

That's right, there will be at most one socket created per addr+port.
I think there shouldn't be a problem with simultaneous calls for
different addresses.

But for a scenario where the simultaneous calls refer to the same addr+port,
I could (as you suggest below) add another lsk_list_find() following a failed
lsk_list_find_or_create() to then either a) obtain a lsk_ref to a
recently created lsk (by the parallel call), or b) fail more definitively.

> Possibly calling again lsk_list_find() in case of failure could address
> the above.
> 
> If the race is not possible, it should be at least documented in a
> comment why it can't happen.
> 
> Side note: using:
> 
> 	if (lsk_ref)
> 		return lsk_ref;
> 
> instead of:
> 	if (!lsk_ref) { //...
> 
> will reduce the indentation level.
> 
> /P
>