netdev - RE: [PATCH net-next 3/6] net: gianfar: allocate queues with devm

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] [day] [month] [year] [list]

Message-ID:
 <AS8PR04MB8849B58CD8CC440E7A9F6EAB96722@AS8PR04MB8849.eurprd04.prod.outlook.com>
Date: Fri, 4 Oct 2024 06:31:03 +0000
From: Claudiu Manoil <claudiu.manoil@....com>
To: Maxime Chevallier <maxime.chevallier@...tlin.com>, Rosen Penev
	<rosenp@...il.com>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "andrew@...n.ch"
	<andrew@...n.ch>, "davem@...emloft.net" <davem@...emloft.net>,
	"edumazet@...gle.com" <edumazet@...gle.com>, "kuba@...nel.org"
	<kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH net-next 3/6] net: gianfar: allocate queues with devm

> -----Original Message-----
> From: Maxime Chevallier <maxime.chevallier@...tlin.com>
> Sent: Wednesday, October 2, 2024 10:25 AM
[...]
> 
> On Tue,  1 Oct 2024 14:22:01 -0700
> Rosen Penev <rosenp@...il.com> wrote:
> 
> > There seems to be a mistake here where free_tx_queue is called on
> > failure. Just let devm deal with it.
> 
> Good catch, this looks good to me.
> 

I like your enthusiasm, but there's nothing to catch here.
kfree() does nothing to NULL objects, however the 'constructor' allocates an
array of objects so free_tx_queues() has to iterate over all objects, to free those
allocated before failure.

I don't have a strong opinion regarding the usage of devm_*() API to allocate resources
at device probing time, it saves some lines of code. However I see this as bringing limited
benefits for simple cases like device probe()/remove(), especially when converting old drivers
like this one. And there's also the risk of falling into the trap of thinking that devm_*() takes
care of everything.

-Claudiu