linux-kernel - Re: [PATCH 2/3] PCI/sysfs: Use __free() in reset_method

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: <d07973e2-0ac1-d3e0-25f0-7b1270ca4a15@linux.intel.com>
Date: Mon, 28 Oct 2024 20:18:39 +0200 (EET)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Keith Busch <kbusch@...nel.org>
cc: Bjorn Helgaas <bhelgaas@...gle.com>, linux-pci@...r.kernel.org, 
    LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/3] PCI/sysfs: Use __free() in reset_method_store()

On Mon, 28 Oct 2024, Ilpo Järvinen wrote:

> On Mon, 28 Oct 2024, Keith Busch wrote:
> 
> > On Mon, Oct 28, 2024 at 07:40:45PM +0200, Ilpo Järvinen wrote:
> > > @@ -1430,7 +1431,7 @@ static ssize_t reset_method_store(struct device *dev,
> > >  				  const char *buf, size_t count)
> > >  {
> > >  	struct pci_dev *pdev = to_pci_dev(dev);
> > > -	char *options, *tmp_options, *name;
> > > +	char *tmp_options, *name;
> > >  	int m, n;
> > >  	u8 reset_methods[PCI_NUM_RESET_METHODS] = { 0 };
> > >  
> > > @@ -1445,7 +1446,7 @@ static ssize_t reset_method_store(struct device *dev,
> > >  		return count;
> > >  	}
> > >  
> > > -	options = kstrndup(buf, count, GFP_KERNEL);
> > > +	char *options __free(kfree) = kstrndup(buf, count, GFP_KERNEL);
> > 
> > We should avoid mixing declarations with code. Please declare it with
> > the cleanup attribute at the top like before, and just initialize it to
> > NULL.
> 
> Hi,
> 
> I don't exactly disagree with you myself and would prefer to keep 
> declarations at top, but I think as done now is exactly what Bjorn wants 
> for the specific case where __free() is used. This was discussed earlier 
> on the list.

Hi again,

I went to archives and found out it had already made itself into 
include/linux/cleanup.h which now says this:

"
 * Now, when a function uses both __free() and guard(), or multiple
 * instances of __free(), the LIFO order of variable definition order
 * matters. GCC documentation says:
 *
 * "When multiple variables in the same scope have cleanup attributes,
 * at exit from the scope their associated cleanup functions are run in
 * reverse order of definition (last defined, first cleanup)."
 *
 * When the unwind order matters it requires that variables be defined
 * mid-function scope rather than at the top of the file.

[...snip examples...]

 * Given that the "__free(...) = NULL" pattern for variables defined at
 * the top of the function poses this potential interdependency problem
 * the recommendation is to always define and assign variables in one
 * statement and not group variable definitions at the top of the
 * function when __free() is used.
"

After reading the documentation for real now myself :-), I realized it's
not just about maintainer preferences but about order of releasing things, 
so it's a BAD PATTERN to put those declarations into the usual place when
using __free().

For completeness, the discussion thread (there might have been another 
thread earlier than these):

https://lore.kernel.org/linux-pci/171140738438.1574931.15717256954707430472.stgit@dwillia2-xfh.jf.intel.com/T/#u

> If I misunderstood the conclusion of the earlier cleanup related 
> discussion, can you please correct me Bjorn?
> 
> 

-- 
 i.