Re: [RFC PATCH 01/11] rust: add abstraction for struct device

[Danilo Krummrich]

On Tue, Jun 04, 2024 at 04:17:29PM +0200, Greg KH wrote:
> On Tue, May 21, 2024 at 10:42:03PM +0200, Danilo Krummrich wrote:
> > On Tue, May 21, 2024 at 11:24:38AM +0200, Greg KH wrote:
> > > On Mon, May 20, 2024 at 10:22:22PM +0200, Danilo Krummrich wrote:
> > > > > > +impl Device {
> > > > > > +    /// Creates a new ref-counted instance of an existing device pointer.
> > > > > > +    ///
> > > > > > +    /// # Safety
> > > > > > +    ///
> > > > > > +    /// Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count.
> > > > > 
> > > > > Callers NEVER care about the reference count of a struct device, anyone
> > > > > poking in that is asking for trouble.
> > > > 
> > > > That's confusing, if not the caller who's passing the device pointer somewhere,
> > > > who else?
> > > > 
> > > > Who takes care that a device' reference count is non-zero when a driver's probe
> > > > function is called?
> > > 
> > > A device's reference count will be non-zero, I'm saying that sometimes,
> > > some driver core functions are called with a 'struct device' that is
> > > NULL, and it can handle it just fine.  Hopefully no callbacks to the
> > > rust code will happen that way, but why aren't you checking just "to be
> > > sure!" otherwise you could have a bug here, and it costs nothing to
> > > verify it, right?
> > 
> > I get your point on that one. But let me explain a bit more why I think that
> > check is not overly helpful here.
> > 
> > In Rust we have the concept of marking functions as 'unsafe'. Unsafe functions
> > need to document their safety preconsitions, i.e. the conditions the caller of
> > the function must guarantee. The caller of an unsafe function needs an unsafe
> > block for it to compile and every unsafe block needs an explanation why it is
> > safe to call this function with the corresponding arguments.
> > 
> > (Ideally, we want to avoid having them in the first place, but for C abstractions
> > we have to deal with raw pointers we receive from the C side and dereferencing a
> > raw pointer is unsafe by definition.)
> > 
> > In this case we have a function that constructs the Rust `Device` structure from
> > a raw (device) pointer we potentially received from the C side. Now we have to
> > decide whether this function is going to be unsafe or safe.
> > 
> > In order for this function to be safe we would need to be able to guarantee that
> > this is a valid, non-null pointer with a non-zero reference count, which
> > unfortunately we can't. Hence, it's going to be an unsafe function.
> 
> But you can verify it is non-null, so why not?

I suggest to check out the code making use of this.

>From the PCI abstractions:

    extern "C" fn probe_callback(
        pdev: *mut bindings::pci_dev,
        id: *const bindings::pci_device_id,
    ) -> core::ffi::c_int {
        // SAFETY: Safe because the core kernel only ever calls the probe callback with a valid
        // `pdev`.
        let dev = unsafe { device::Device::from_raw(&mut (*pdev).dev) };

        [...]
    }

Doing the NULL check would turn this into something like:

    extern "C" fn probe_callback(
        pdev: *mut bindings::pci_dev,
        id: *const bindings::pci_device_id,
    ) -> core::ffi::c_int {
        // SAFETY: Safe because the core kernel only ever calls the probe callback with a valid
        // `pdev`, but we still have to handle `Device::from_raw`'s NULL check.
        let dev = match unsafe { device::Device::from_raw(&mut (*pdev).dev) } {
           Ok(dev) => dev,
           Err(err) => return Error::to_errno(err),
        }
    }

This would be super odd. If `Device::from_raw` reports "Ok" it actually wouldn't
mean everything is well. It would *only* mean that the pointer that was passed
is not NULL. This is counter intuitive; IMHO unsafe functions shouldn't return
any type of result, because it just isn't meaningful.

> 
> > A NULL pointer check would not make it a safe function either, since the pointer
> > could still be an invalid one, or a pointer to a device it's not guaranteed that
> > the reference count is held up for the duration of the function call.
> 
> True, but you just took one huge swatch of "potential crashes" off the
> table.  To ignore that feels odd.
> 
> > Given that, we could add the NULL check and change the safety precondition to
> > "valid pointer to a device with non-zero reference count OR NULL", but I don't
> > see how this improves the situation for the caller, plus we'd need to return
> > `Result<Device>` instead and let the caller handle that the `Device` was not
> > created.
> 
> It better be able to handle if `Device` was not created, as you could
> have been out of memory and nothing would have been allocated.  To not
> check feels very broken.

The abstraction is not allocating a new C struct device, it's just abstracting a
pointer to an existing struct device. There is no OOM case to handle, the
abstraction holding the pointer lives on the stack.

> 
> > > Ok, if you say so, should we bookmark this thread for when this does
> > > happen?  :)
> > 
> > I'm just saying the caller has to validate that or provide a rationale why this
> > is safe anyways, hence it'd be just a duplicate check.
> > 
> > > 
> > > What will the rust code do if it is passed in a NULL pointer?  Will it
> > > crash like C code does?  Or something else?
> > 
> > It mostly calls into C functions with this pointer, depends on what they do.
> > 
> > Checking a few random places, e.g. [1], it seems to crash in most cases.
> > 
> > [1] https://elixir.free-electrons.com/linux/latest/source/drivers/base/core.c#L3863
> 
> Great, then you should check :)

Why isn't the conclusion that the C code should check (as well)? :) Would you
want to add a NULL check at the beginning of device_del()?

In Rust we have a clear separation between safe and unsafe functions with, for
the latter, documented requirements on what's actually allowed to pass in and
which preconditions must be guaranteed. The check happens, just not within the
unsafe function.

> 
> thanks,
> 
> greg k-h
>