netdev - Re: [PATCH v2 7/8] fwctl/mlx5: Support for communicating with mlx5 fw

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: <20240801132519.GD2809814@nvidia.com>
Date: Thu, 1 Aug 2024 10:25:19 -0300
From: Jason Gunthorpe <jgg@...dia.com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: Jonathan Corbet <corbet@....net>, Itay Avraham <itayavr@...dia.com>,
	Jakub Kicinski <kuba@...nel.org>, Leon Romanovsky <leon@...nel.org>,
	linux-doc@...r.kernel.org, linux-rdma@...r.kernel.org,
	netdev@...r.kernel.org, Paolo Abeni <pabeni@...hat.com>,
	Saeed Mahameed <saeedm@...dia.com>,
	Tariq Toukan <tariqt@...dia.com>,
	Andy Gospodarek <andrew.gospodarek@...adcom.com>,
	Aron Silverton <aron.silverton@...cle.com>,
	Dan Williams <dan.j.williams@...el.com>,
	David Ahern <dsahern@...nel.org>,
	Christoph Hellwig <hch@...radead.org>, Jiri Pirko <jiri@...dia.com>,
	Leonid Bloch <lbloch@...dia.com>,
	Leon Romanovsky <leonro@...dia.com>, linux-cxl@...r.kernel.org,
	patches@...ts.linux.dev
Subject: Re: [PATCH v2 7/8] fwctl/mlx5: Support for communicating with mlx5 fw

On Wed, Jul 31, 2024 at 12:52:32PM +0100, Jonathan Cameron wrote:

> Thanks for the explanation.  I clearly needed more coffee that day :)
> Personally I find this to be a confusing use of scoped cleanup
> as we aren't associating a constructor / destructor with scope, but
> rather sort of 'adopting ownership / destructor'.

It is a pretty typical "move" concept for reference counting, as you
might see in other languages.

> Assuming my caffeine level is better today, maybe device managed is
> more appropriate?

Oh, perhaps controversial, but I dislike devm, so I'd rather not :) It
makes exciting bugs, IMHO. Someone (Laurent?) gave a nice presentation
on some of its nasty edge cases at LPC a few years ago.

> devm_add_action_or_reset to associate the destructor by placing
> it immediately after the setup path for both the allocate and unregister.
> Should run in very nearly same order for teardown as what you have here.

Yes, in this case it would work technically.

I think devm would be more code lines, more memory usage, and more
failure cases though.

So, I'm interested that cleanup could be a better option. I view it as
positive that the success path remove() flow is actually documented
what order it is doing things. Order is very important there and I've
seen enough places get things wrong here over the years..

One of the nasty traps of devm is you have to know to have your
creation order match your required destruction order, and *everything*
has to use devm or it can't be ordered.

Mind you cleanup has the same order trap, but you don't have to use
cleanup during remove(), and maybe it was overzealous to do so here.

Thanks,
Jason