Message-ID: <20250219155617.GH19203@1wt.eu>
Date: Wed, 19 Feb 2025 16:56:17 +0100
From: Willy Tarreau <w@....eu>
To: James Bottomley <James.Bottomley@...senPartnership.com>
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, Feb 19, 2025 at 10:46:03AM -0500, James Bottomley wrote:
> > > > 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?

Nobody, but it was not said either that they were used at all!

>  One the things the
> _cleanup_X annotations can do is drop references (or even locks).

OK then!

> >  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.

I'm not trying to assume failure or anything, just saying that it's
probably not always as simple as calling kfree() on anything locally
allocated for error paths to be magically cleaned, and it actually is
more subtle (and Laurent confirmed my concerns illustrating that this
case is precisely covered in glib using transfer of ownership).

And the temptation to return from everywhere since it's the only
required statement (instead of a goto to a collecting place) becomes
great and should sometimes be resisted to.

Regardless I do understand how these cleanups can help in a number of
case, at least to avoid some code duplication.

Regards,
Willy

Powered by blists - more mailing lists