linux-kernel - Re: [PATCH v2] ACPI: NFIT: Fix local use of 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <6529b493cc785_5c0d2947d@dwillia2-mobl3.amr.corp.intel.com.notmuch>
Date:   Fri, 13 Oct 2023 14:20:19 -0700
From:   Dan Williams <dan.j.williams@...el.com>
To:     "Wilczynski, Michal" <michal.wilczynski@...el.com>,
        Dan Williams <dan.j.williams@...el.com>,
        <nvdimm@...ts.linux.dev>, <linux-acpi@...r.kernel.org>
CC:     <rafael@...nel.org>, <vishal.l.verma@...el.com>, <lenb@...nel.org>,
        <dave.jiang@...el.com>, <ira.weiny@...el.com>,
        <linux-kernel@...r.kernel.org>,
        Andy Shevchenko <andy.shevchenko@...il.com>
Subject: Re: [PATCH v2] ACPI: NFIT: Fix local use of devm_*()

Wilczynski, Michal wrote:
> 
> 
> On 10/13/2023 7:05 PM, Dan Williams wrote:
> > Wilczynski, Michal wrote:
> >> On 10/13/2023 6:38 PM, Dan Williams wrote:
> >>> Michal Wilczynski wrote:
> >>>> devm_*() family of functions purpose is managing memory attached to a
> >>>> device. So in general it should only be used for allocations that should
> >>>> last for the whole lifecycle of the device. 
> >>> No, this assertion is not accurate, if it were strictly true then
> >>> devm_kfree() should be deleted. This patch is only a cleanup to switch
> >>> the automatic cleanup pattern from devm to the new cleanup.h helpers.
> >> The memory in question is only used locally in a function, so there is no reason
> >> to use devm_*() family of functions. I think devm_kfree() is more for special
> >> cases where the memory is meant to be used for the whole lifecycle of device,
> >> but some special case occurs and it's not and it needs to be freed.
> >>
> >> This is an incorrect API usage. Would you propose to change all memory
> >> allocations currently being done to devm_*() family simply because devm_kfree()
> >> exists ?
> > Michal, please work with someone else to get these cleanups upstream, I
> > am done with this thread.
> 
> I'm really sorry if I offended you, I didn't mean to.

Hey, it happens.

I am not offended, just frustrated.

Going forward, my advice for anyone doing advocacy for a patch set is to
be clear about "what happens if upstream does not take this patch?".
When you view it from that angle the patch changelog that would have
received an immediate Reviewed-by with no additional comment from me is
something along the lines of:

"The new cleanup.h facilities that arrived in v6.5-rc1 can replace the
the usage of devm semantics in acpi_nfit_init_interleave_set(). That
routine appears to only be using devm to avoid goto statements. The new
__free() annotation at variable declaration time can achieve the same
effect more efficiently.

There is no end user visible side effects of this patch, I was motivated
to send this cleanup to practice using the new helpers."

As Linus mentions, subtlety is difficult to convey in mailing list
interactions, and you may not have picked up on it, but the frustration
for me began with the claim of a memory leak that turned out to be
false. That was the prompt to consider "what other claims should I be
careful about now that the fundamental justification for touching this
old code has gone away."

So if you want to try again with the justification of the patch limited
to a cleanup, we can move forward.