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

Jiri Pirko
2023-10-06T07:22:08Z

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?