Re: [TECH TOPIC] Rust for Linux

[Greg KH <greg@kroah.com>]

On Wed, Jul 07, 2021 at 09:27:32AM +0200, Julia Lawall wrote:
> 
> 
> On Wed, 7 Jul 2021, Laurent Pinchart wrote:
> 
> > On Tue, Jul 06, 2021 at 10:36:14PM +0200, Linus Walleij wrote:
> > > On Tue, Jul 6, 2021 at 10:00 PM Roland Dreier wrote:
> > >
> > > > One area I see where Rust could make a big improvement for drivers is
> > > > in using RAII for error paths.  Drivers often have a lot of code like
> > > >
> > > >     if (something())
> > > >         return err;
> > > >
> > > >     if (something_else())
> > > >         goto undo_something;
> > > >
> > > >     if (a_third_thing())
> > > >         goto undo_two_things;
> > > >
> > > > and so on, which is difficult to get right in the first place and even
> > > > harder to keep correct in the face of changes.
> > >
> > > Yes.
> > >
> > > > "devres" / devm_xxx was an attempt to deal with this in C, but it only
> > > > solves some cases of this and has not received a lot of adoption (we
> > > > can argue about the reasons).
> > >
> > > Really? From my point of view that is adopted all over the map.
> > > I add new users all the time and use it as much as I can when
> > > writing new drivers.
> > >
> > > $ git grep devm_  | wc -l
> > > 26112
> > >
> > > Dmitry in the input subsystem even insist to use it for e.g. powering
> > > down and disabling regulators on remove(), like recently in
> > > drivers/input/touchscreen/cy8ctma140.c
> > >
> > > /* Called from the registered devm action */
> > > static void cy8ctma140_power_off_action(void *d)
> > > {
> > >         struct cy8ctma140 *ts = d;
> > >
> > >         cy8ctma140_power_down(ts);
> > > }
> > > (...)
> > > error = devm_add_action_or_reset(dev, cy8ctma140_power_off_action, ts);
> > > if (error) {
> > >         dev_err(dev, "failed to install power off handler\n");
> > >         return error;
> > > }
> > >
> > > I think it's a formidable success, people just need to learn to do it more.
> >
> > devres is interesting and has good use cases, but the devm_k*alloc() are
> > a nightmare. They're used through drivers without any consideration of
> > life time management of the allocated memory, to allocate data
> > structures that can be accessed in userspace-controlled code paths.
> > Open a device node, keep it open, unplug the device, close the device
> > node, and in many cases you'll find that the kernel can crash :-( When
> > the first line of the probe function of a driver that exposes a chardev
> > if a devm_kzalloc(), it's usually a sign that something is wrong.
> 
> Where should the free have been?  Will Rust help in this case?  Will it
> result in a memory leak?

This is going to be the "interesting" part of the rust work, where it
has to figure out how to map the reference counted objects that we
currently have in the driver model across to rust-controlled objects and
keep everything in sync properly.

For the normal code, the fact that the memory was assigned to one
specific object (the struct device) but yet referenced from another
object (the cdev).  devm_* users like this do not seem to realize there
are two separate object lifecycles happening here as the interactions
are subtle at times.

I am looking forward to how the rust implementation is going to handle
all of this as I have no idea.

thanks,

greg k-h