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: <Z-00UkrBC1TRnoqA@smile.fi.intel.com>
Date: Wed, 2 Apr 2025 15:57:54 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Peter Zijlstra <peterz@...radead.org>
Cc: Przemek Kitszel <przemyslaw.kitszel@...el.com>,
	linux-kernel@...r.kernel.org, linux-mm@...ck.org, vbabka@...e.cz,
	torvalds@...ux-foundation.org, intel-wired-lan@...ts.osuosl.org,
	netdev@...r.kernel.org, linux-toolchains@...r.kernel.org
Subject: Re: [RFC] slab: introduce auto_kfree macro

On Wed, Apr 02, 2025 at 02:22:24PM +0200, Peter Zijlstra wrote:
> On Wed, Apr 02, 2025 at 02:19:35PM +0200, Peter Zijlstra wrote:
> > On Wed, Apr 02, 2025 at 01:32:51PM +0300, Andy Shevchenko wrote:
> > > On Tue, Apr 01, 2025 at 03:44:08PM +0200, Przemek Kitszel wrote:
> > > > Add auto_kfree macro that acts as a higher level wrapper for manual
> > > > __free(kfree) invocation, and sets the pointer to NULL - to have both
> > > > well defined behavior also for the case code would lack other assignement.
> > > > 
> > > > Consider the following code:
> > > > int my_foo(int arg)
> > > > {
> > > > 	struct my_dev_foo *foo __free(kfree); /* no assignement */
> > > > 
> > > > 	foo = kzalloc(sizeof(*foo), GFP_KERNEL);
> > > > 	/* ... */
> > > > }
> > > > 
> > > > So far it is fine and even optimal in terms of not assigning when
> > > > not needed. But it is typical to don't touch (and sadly to don't
> > > > think about) code that is not related to the change, so let's consider
> > > > an extension to the above, namely an "early return" style to check
> > > > arg prior to allocation:
> > > > int my_foo(int arg)
> > > > {
> > > >         struct my_dev_foo *foo __free(kfree); /* no assignement */
> > > > +
> > > > +	if (!arg)
> > > > +		return -EINVAL;
> > > >         foo = kzalloc(sizeof(*foo), GFP_KERNEL);
> > > >         /* ... */
> > > > }
> > > > Now we have uninitialized foo passed to kfree, what likely will crash.
> > > > One could argue that `= NULL` should be added to this patch, but it is
> > > > easy to forgot, especially when the foo declaration is outside of the
> > > > default git context.
> > 
> > The compiler *should* complain. But neither GCC nor clang actually
> > appear to warn in this case.
> > 
> > I don't think we should be making dodgy macros like you propose to work
> > around this compiler deficiency. Instead I would argue we ought to get
> > both compilers fixed asap, and then none of this will be needed.
> 
> Ah, I think the problem is that the cleanup function takes a pointer to
> the object, and pointers to uninitialized values are generally
> considered okay.
> 
> The compilers would have to explicitly disallow this for the cleanup
> functions.

Hmm... What I have heard is that the cleanup is basically a port of
C++ destructor code to C, and it might be related to the virtual functions
that are may be absent for the basic classes. But not an expert here,
just speculating based on my poor knowledge of C++.

-- 
With Best Regards,
Andy Shevchenko



Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ