Re: [PATCH v2 01/10] rust: pass module name to `Module::init`

[Danilo Krummrich]

On Wed, Jul 10, 2024 at 04:02:04PM +0200, Greg KH wrote:
> On Thu, Jun 20, 2024 at 11:24:17PM +0200, Danilo Krummrich wrote:
> > Ok, good you point this out. We should definitely discuss this point then and
> > build some consensus around it.
> > 
> > I propose to focus on this point first and follow up with the discussion of the
> > rest of the series afterwards.
> > 
> > Let me explain why I am convinced that it's very important to have abstractions
> > in place in general and from the get-go.
> > 
> > In general, having abstractions for C APIs is the foundation of being able to
> > make use of a lot of advantages Rust has to offer.
> > 
> > The most obvious one are all the safety aspects. For instance, with an
> > abstraction we have to get exactly one piece of code right in terms of pointer
> > validity, lifetimes, type safety, API semantics, etc. and in all other places
> > (e.g. drivers) we get the compiler to check those things for us through the
> > abstraction.
> > 
> > Now, the abstraction can be buggy or insufficient and hence there is no absolute
> > safety guarantee. *But*, if we get this one thing right, there is nothing a
> > driver can mess up by itself trying to do stupid things anymore.
> > 
> > If we just call the C code instead we have to get it right everywhere instead.
> 
> I too want a pony.  But unfortunatly you are shaving a yak here instead :)
> 
> I'm not saying to call C code from rust, I'm saying call rust code from
> C.
> 
> Have a "normal" pci_driver structure with C functions that THEN call
> into the rust code that you want to implement them, with a pointer to
> the proper structure.  That gives you everything that you really want
> here, EXCEPT you don't have to build the whole tower of drivers and
> busses and the like.

Please find my reply below where you expand this point.

> 
> > Now, you could approach this top-down instead and argue that we could at least
> > benefit from Rust for the driver specific parts.
> > 
> > Unfortunately, this doesn't really work out either. Also driver specific code is
> > typically (very) closely connected to kernel APIs. If you want to use the safety
> > aspects of Rust for the driver specific parts you inevitably end up writing
> > abstractions for the C APIs in your driver.
> > 
> > There are basically three options you can end up with:
> > 
> > (1) An abstraction for the C API within your driver that is actually generic
> >     for every driver, and hence shouldn't be there.
> > (2) Your driver specific code is full of raw pointers and `unsafe {}` calls,
> >     which in the end just means that you ended up baking the abstraction into
> >     your driver specific code.
> > (3) You ignore everything, put everything in a huge `unsafe {}` block and
> >     compile C code with the Rust compiler. (Admittedly, maybe slightly
> >     overstated, but not that far off either.)
> > 
> > The latter is also the reason why it doesn't make sense to only have
> > abstractions for some things, but not for other.
> > 
> > If an abstraction for B is based on A, but we don't start with A, then B ends up
> > implementing (at least partially) the abstraction for A as well. For instance,
> > if we don't implement `driver::Registration` then the PCI abstractions (and
> > platform, usb, etc.) have to implement it.
> > 
> > It really comes down to the point that it just bubbles up. We really have to do
> > this bottom-up, otherwise we just end up moving those abstractions up, layer by
> > layer, where they don't belong to and we re-implement them over and over again.
> 
> I think we are talking past each other.

Given your proposal below, that is correct.

I read your previous mail as if you question having abstractions for C APIs at
all. And some parts of your latest reply still read like that to me, but for the
rest of my reply I will assume that's not the case.

> 
> Here is an example .c file that you can use today for your "implement a
> PCI driver in rust" wish in a mere 34 lines of .c code:

Your proposal below actually only discards a rather small amount of the proposed
abstractions, in particular:

  (1) the code to create a struct pci_driver and the code to call
      pci_register_driver() and pci_unregister_driver() (implemented in Patch 2)
  (2) the generic code to create the struct pci_device_id table (or any other ID
      table) (implemented in Patch 3)

As I understand you, the concern here really seems to be about the complexity
vs. what we get from it and that someone has to maintain this.

I got the point and I take it seriously. As already mentioned, I'm also willing
to take responsibility for the code and offer to maintain it.

But back to the technical details.

Let's start with (2):

I agree that this seems a bit complicated. I'd propose to remove the
abstractions in device_id.rs (i.e. Patch 3) for now and let the PCI abstraction
simply create a struct pci_device_id table directly.

As for (1):

This just makes a pretty small part of the abstractions and, it's really only
about creating the struct pci_driver (or another driver type) instance and
call the corresponding register() / unregister() functions, since we already
have the Rust module support upstream.

As in C, we really just call register() from module_init() and unregister() from
module_exit() and the code for that, without comments, is around 50 lines of
code.

As mentioned this is implemented in Patch 2; Hence, please see my reply on Patch
2, where I put a rather long and detailed explanation which hopefully clarifies
things.

Now, what do we get from that compared to the proposal below?

  - The driver's probe() and remove() functions get called with the
    corresponding abstraction types directly instead of raw pointers; this moves
    the required  `unsafe {}` blocks to a cental place which otherwise *every*
    driver would need to implement itself (obviously it'd be the same for future
    suspend, resume, shutdown, etc. callbacks).

  - More complex drivers can do the work required to be done in module_init()
    and module_exit() in Rust directly, which allows them to attach the lifetime
    of structures to the lifetime of the `Module` structure in Rust which avoids
    the need for explicit cleanup in module_exit() since they can just implement
    in Rust's drop() trait.

The latter may sound a bit less important than it actually is, since it can break
the design, safety and soundness of Rust types. Let me explain:

In Rust a type instance should cleanup after itself when it goes out of scope.

Let's make up an example:

```
// DISCLAIMER: Obviously, this is not how we actually handle memory allocations
// in Rust, it's just an example.
struct Buffer {
	ptr: *const u8,
}

impl Buffer {
	fn alloc(size: u8) -> Result<Self> {
		let ptr = Kmalloc::alloc(size, GFP_KERNEL)?;

		// Return an instance of `Buffer` initialized with a pointer to
		// the above memory allocation.
		Ok(Self {
			ptr,
		})
	}
}

impl Drop for Buffer {
	fn drop(&mut self) {
		// SAFETY: `Self` is always holding a pointer to valid memory
		// allocated with `Kmalloc`.
		unsafe { Kmalloc::free(self.ptr) };
	}
}
```

(Side note: `Kmalloc::free` is unsafe since it has no control on whether the
pointer passed to it is valid and was allocated with `Kmalloc` previously.)

In Rust's module_init() you could now attach an instance of `Buffer` to the
`Module` instance, which lives until module_exit(), which looks like this:

```
struct MyModule {
	buffer: Buffer,
}

impl kernel::Moduke for MyModule {
	fn init(module: &'static ThisModule) -> Result<Self> {
		Ok(MyModule {
			buffer: Buffer::alloc(0x100)?,
		})
	}
}
```

Note that we don't need to implement module_exit() here, since `MyModule` lives
until module_exit() and hence `buffer` has the same lifetime. Once `buffer` goes
out of scope it frees itself due to the drop() trait. In fact, buffer can never
leak it's memory.

With the proposed approach below we can't do this, we'd need to play dirty and
unsafe tricks in order to not entirely break the design of `Buffer`, or any
other more complex data structure that the module needs.

> 
> ------------------------------------------------------------------------
> // SPDX-License-Identifier: GPL-2.0-only
> #include <linux/module.h>
> #include <linux/pci.h>
> #include "my_pci_rust_bindings.h"
> 
> #define PCI_DEVICE_ID_FOO		0x0f00
> #define PCI_VENDOR_ID_FOO		0x0f00
> 
> static int probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> {
> 	return my_rust_probe(pdev);
> }
> 
> static void remove(struct pci_dev *pdev)
> {
> 	my_rust_remove(pdev);
> }
> 
> static const struct pci_device_id pci_ids[] = {
> 	{PCI_DEVICE(PCI_VENDOR_ID_FOO, PCI_DEVICE_ID_FOO)},
> 	{}
> };
> MODULE_DEVICE_TABLE(pci, pci_ids);
> 
> static struct pci_driver my_rust_pci_driver = {
> 	.name = "my_pci_rust_driver",
> 	.id_table = pci_ids,
> 	.probe = probe,
> 	.remove = remove,
> };
> module_pci_driver(my_rust_pci_driver);
> 
> MODULE_DESCRIPTION("Driver for my fancy PCI device");
> MODULE_LICENSE("GPL v2");
> ------------------------------------------------------------------------
> 
> Now, all you have to do is provide a my_rust_probe() and
> my_rust_remove() call in your rust code, and handle the conversion of a
> struct pci_dev to whatever you want to use (which you had to do anyway),
> and you are set!
> 
> That .c code above is "obviously" correct, and much simpler and worlds
> easier for all of us to understand instead of the huge .rs files that
> this patch series was attempting to implement.
> 
> You have to draw the c/rust line somewhere, you all seem to be wanting
> to draw that line at "no .c in my module at all!" and I'm saying "put 34
> lines of .c in your module and you will save a LOT of headache now."
> 
> All of the "real" work you want to do for your driver is behind the
> probe/remove callbacks (ok, add a suspend/resume as well, forgot that),
> and stop worrying about all of the bindings and other mess to tie into
> the driver model for a specific bus type and the lunacy that the device
> id mess was.
> 
> In short, KEEP IT SIMPLE!
> 
> Then, after we are comfortable with stuff like the above, slowly think
> about moving the line back a bit more, if you really even need to.  But
> why would you need to?  Only reason I can think of is if you wanted to
> write an entire bus in rust, and for right now, I would strongly suggest
> anyone not attempt that.

We're not trying to do that. Also, please note that all the abstractions are
*only* making use of APIs that C drivers use directly, just abstracting them
in a way, such that we can actually use the strength of Rust.

> 
> The "joy" of writing a driver in rust is that a driver consumes from
> EVERYWHERE in the kernel (as you well know.)  Having to write bindings
> and mappings for EVERYTHING all at once, when all you really want to do
> is implement the logic between probe/remove is rough, I'm sorry.  I'm
> suggesting that you stop at the above line for now, which should make
> your life easier as the .c code is obviously correct, and anything you
> do in the rust side is your problem, not mine as a bus maintainer :)

As explained above, we already have the module abstraction in place. Really all
that we're left with is creating the struct pci_driver and call register() /
remove() from it.

Now, this could be fully done in the PCI abstraction. The only reason we have
the `Registration` and `DriverOps` (again there are a few misunderstandings
around that, that I try to clarify in the corresponding thread) in "driver.rs"
is to not duplicate code for maintainability.

What I'm trying to say, the stuff in "driver.rs" is not even abstracting things
from drivers/base/, but is Rust helper code for subsystems to abstract their
stuff (e.g. struct pci_driver).

And again, I'm willing to take responsibility for the code and offer to maintain
it - I really want to do this the proper way.

- Danilo

(cutting of the rest since it was based on the misunderstanding mentioned above)