lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Fri, 6 Oct 2023 19:07:34 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org, pabeni@...hat.com, davem@...emloft.net,
	edumazet@...gle.com, gal@...dia.com
Subject: Re: [patch net-next] devlink: don't take instance lock for nested
 handle put

Fri, Oct 06, 2023 at 04:48:42PM CEST, kuba@...nel.org wrote:
>On Fri, 6 Oct 2023 09:22:01 +0200 Jiri Pirko wrote:
>> Fri, Oct 06, 2023 at 03:30:29AM CEST, kuba@...nel.org wrote:
>> >> @@ -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?
>
>I can't remember to be honest. But we already assume that we can access
>struct device of a devlink instance without holding the instance lock.
>Because the relationship between devlink objects is usually fairly
>straightforward and non-cyclical.
>
>Isn't the "rel infrastructure"... well.. over-designed?
>
>The user creates a port on an instance A, which spawns instance B.
>Instance A links instance B to itself.
>Instance A cannot disappear before instance B disappears.

It can. mlx5 port sf removal is very nice example of that. It just tells
the FW to remove the sf and returns. The actual SF removal is spawned
after that when processing FW events.


>Also instance A is what controls the destruction of instance B
>so it can unlink it.
>
>We can tell lockdep how the locks nest, too.
>
>> >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?
>
>No exactly, if we want bi-directional relationships we can create 
>the link struct as a:
>
>rel {
>	u32 rel_id;
>	struct devlink *instanceA, *instanceB; // hold reference

Sometimes port, sometimes linecard is one one side (A).


>	struct list_head rel_listA, rel_listB; // under instance locks
>	u32 state;
>	struct list_head ntf_process_queue;
>}
>
>Operations on relationship can take the instance locks (sequentially).
>Notifications from a workqueue.

That is pretty much how that works now.


>Instance dumps would only report rel IDs, but the get the "members" of
>the relationship user needs to issue a separate DL command / syscall.

Oh! At least with process listening on notifications, this may be a bit
painful :/
I need some time to digest this.

Powered by blists - more mailing lists