Message-ID: <e42e8e79a539849419e475ef8041e87b3bccbbfe.camel@HansenPartnership.com>
Date: Wed, 19 Feb 2025 10:46:03 -0500
From: James Bottomley <James.Bottomley@...senPartnership.com>
To: Willy Tarreau <w@....eu>
Cc: "Martin K. Petersen" <martin.petersen@...cle.com>, Dan Carpenter
 <dan.carpenter@...aro.org>, Christoph Hellwig <hch@...radead.org>, Miguel
 Ojeda <miguel.ojeda.sandonis@...il.com>, rust-for-linux
 <rust-for-linux@...r.kernel.org>, Linus Torvalds
 <torvalds@...ux-foundation.org>,  Greg KH <gregkh@...uxfoundation.org>,
 David Airlie <airlied@...il.com>,  linux-kernel@...r.kernel.org,
 ksummit@...ts.linux.dev
Subject: Re: Rust kernel policy

On Wed, 2025-02-19 at 16:33 +0100, Willy Tarreau wrote:
> Hi James,
> 
> On Wed, Feb 19, 2025 at 10:15:00AM -0500, James Bottomley wrote:
> > On Wed, 2025-02-19 at 09:46 -0500, Martin K. Petersen wrote:
> > > 
> > > James,
> > > 
> > > > Could we possibly fix a lot of this by adopting the _cleanup_
> > > > annotations[1]? I've been working in systemd code recently and
> > > > they seem to make great use of this for error leg
> > > > simplification.
> > > 
> > > We already have this:
> > > 
> > >   include/linux/cleanup.h
> > > 
> > > I like using cleanup attributes for some error handling. However,
> > > I'm finding that in many cases I want to do a bit more than a
> > > simple kfree(). And at that point things get syntactically messy
> > > in the variable declarations and harder to read than just doing a
> > > classic goto style unwind.
> > 
> > So the way systemd solves this is that they define a whole bunch of
> > _cleanup_<type>_ annotations which encode the additional logic.  It
> > does mean you need a globally defined function for each cleanup
> > type, but judicious use of cleanup types seems to mean they only
> > have a few dozen of these.
> 
> I may be missing something obvious, but this seems super dangerous to
> me to perform lightly without reference counting, as it increases the
> risks of use-after-free and double-free in case one of the allocated
> objects in question can sometimes be returned.

Who said anything about not reference counting?  One the things the
_cleanup_X annotations can do is drop references (or even locks).

>  Users of such mechanisms must be extremely cautious never to ever
> return a pointer derivated from a variable tagged as such, or to
> properly NULL-assign the original object for it not to double-free.
> So it might in the end require to be careful about null-setting on
> return instead of explicitly freeing what was explicitly allocated.
> I'm not sure about the overall benefit.
> Also I suspect it encourages to multiply the return points, which
> makes it even more difficult to possibly fix what needs to be fixed
> without coming from a locally allocated variable (e.g. restore a
> state in a parser etc). Maybe it's just me not seeing the whole
> picture, but as a general case I prefer to forget a free() call
> (worst case: memory leak) than forget a foo=NULL that may result in a
> double free, and the description here makes me think the latter might
> more easily happen.

Well we could all speculate about the mess we'll make with any new
tool.  All I'm saying is that another project with a large code base
(systemd), which you can go an look at, managed to use these
annotations very successfully to simplify their error legs. Perhaps
there are reasons why the kernel can't be as successful, but I think
assuming failure from the outset isn't the best way to flush these
reasons out.

Regards,

James

Powered by blists - more mailing lists