Re: [patch net-next] devlink: don't take instance lock for nested handle put

[Jiri Pirko <jiri@resnulli.us>]

Fri, Oct 06, 2023 at 03:30:29AM CEST, kuba@kernel.org wrote:
>On Tue,  3 Oct 2023 09:43:49 +0200 Jiri Pirko wrote:
>> To fix this, don't take the devlink instance lock when putting nested
>> handle. Instead, rely on devlink reference to access relevant pointers
>> within devlink structure. Also, make sure that the device does
>
>struct device ?

Yes.


>
>> not disappear by taking a reference in devlink_alloc_ns().
>
>> @@ -310,6 +299,7 @@ static void devlink_release(struct work_struct *work)
>>  
>>  	mutex_destroy(&devlink->lock);
>>  	lockdep_unregister_key(&devlink->lock_key);
>> +	put_device(devlink->dev);
>
>IDK.. holding references until all references are gone may lead 
>to reference cycles :(

I don't follow. What seems to be the problematic flow? I can't spot any
reference cycle, do you?


>
>>  	kfree(devlink);
>>  }
>
>> @@ -92,9 +93,8 @@ int devlink_nl_put_nested_handle(struct sk_buff *msg, struct net *net,
>>  		return -EMSGSIZE;
>>  	if (devlink_nl_put_handle(msg, devlink))
>>  		goto nla_put_failure;
>> -	if (!net_eq(net, devlink_net(devlink))) {
>> -		int id = peernet2id_alloc(net, devlink_net(devlink),
>> -					  GFP_KERNEL);
>> +	if (!net_eq(net, devl_net)) {
>> +		int id = peernet2id_alloc(net, devl_net, GFP_KERNEL);
>>  
>>  		if (nla_put_s32(msg, DEVLINK_ATTR_NETNS_ID, id))
>>  			return -EMSGSIZE;
>
>Looks like pure refapeernet2id_allocctoring. But are you sure that the netns can't
>disappear? We're not holding the lock, the instance may get moved.

Yeah, I think you are right. I can do peernet2id_alloc during devlink
init/netnschange and store id into devlink structure. That should solve
this.


>
>Overall I feel like recording the references on the objects will be
>an endless source of locking pain. Would it be insane if we held 
>the relationships as independent objects? Not as attributes of either
>side? 

How exactly do you envision this? rel struct would hold the bus/name
strings direcly?