netdev - Re: [PATCH net-next 4/5] devlink: embed driver's priv data callback param into devlink

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZrX3KB10sAoqAoKa@nanopsycho.orion>
Date: Fri, 9 Aug 2024 13:02:00 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Przemek Kitszel <przemyslaw.kitszel@...el.com>, netdev@...r.kernel.org,
	Ido Schimmel <idosch@...dia.com>, Petr Machata <petrm@...dia.com>,
	Andrew Lunn <andrew@...n.ch>,
	Florian Fainelli <f.fainelli@...il.com>,
	Vladimir Oltean <olteanv@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
	Saeed Mahameed <saeedm@...dia.com>,
	Leon Romanovsky <leon@...nel.org>, Tariq Toukan <tariqt@...dia.com>,
	Tony Nguyen <anthony.l.nguyen@...el.com>,
	nex.sw.ncis.osdt.itp.upstreaming@...el.com,
	Wojciech Drewek <wojciech.drewek@...el.com>
Subject: Re: [PATCH net-next 4/5] devlink: embed driver's priv data callback
 param into devlink_resource

Fri, Aug 09, 2024 at 04:41:50AM CEST, kuba@...nel.org wrote:
>On Wed, 7 Aug 2024 08:49:57 +0200 Jiri Pirko wrote:
>> > 	lockdep_assert_held(&devlink->lock);
>> > 
>> > 	resource = devlink_resource_find(devlink, NULL, resource_id);
>> >-	if (WARN_ON(!resource))
>> >+	if (WARN_ON(!resource || occ_priv_size > resource->priv_size))  
>> 
>> Very odd. You allocate a mem in devl_resource_register() and here you
>> copy data to it. Why the void pointer is not enough for you? You can
>> easily alloc struct in the driver and pass a pointer to it.
>> 
>> This is quite weird. Please don't.
>
>The patch is a bit of a half measure, true.
>
>Could you shed more light on the design choices for the resource API,
>tho? Why the tying of objects by driver-defined IDs? It looks like 

The ids are exposed all the way down to the user. They are the same
across the reboots and allow user to use the same scripts. Similar to
port index for example.


>the callback for getting resources occupancy is "added" later once 
>the resource is registered? Is this some legacy of the old locking
>scheme? It's quite unusual.

It's been some while since I reviewed this, but afaik the reason is that
the occupancy was not possible to obtain during reload, yet the resource
itself stayed during reload. This is now not a problem, since
devlink->lock protects it. I don't see why occupancy getter cannot be
put during resource register, you are correct.