Nouveau - Feb 2025 - [RFC PATCH 0/3] gpu: nova-core: add basic timer subdevice implementation

On Thu, Feb 27, 2025 at 01:25:10PM -0800, Boqun Feng
wrote:
> > The design pattern says that 'share it with the rest of the
world' is
> > a bug. A driver following the pattern cannot do that, it must contain
> > the driver objects within the driver scope and free them. In C we
> 
> I cannot speak for Danilo, but IIUC, the 'share it with the rest of the
> world' things are the ones that drivers can share, for example, I
> suppose (not a network expert) a NIC driver can share the packet object
> with the upper layer of netowrk.
I'm having a bit of trouble parsing this sentence..

In your example a NIC driver passing a packet into the network stack
would still be within the world of the driver.

Outside the world would be assigning the packet to a global
variable.

It is a very similar idea to what you explained about the module
lifetime, just instead of module __exit being the terminal point, it
is driver remove().
> > It appears to me that the main issue here is that nobody has figured
> > out how to make rust have rules that can enforce that design pattern.
> 
> Most of the cases, it should be naturally achieved, because you already
> bind the objects into your module or driver, otherwise they would be
> already cancelled and freed. 
I'm getting the feeling you can probably naturally achieve the
required destructors, but I think Danillo is concerned that since it
isn't *mandatory* it isn't safe/sound.
> So I think in Rust you can have the "design pattern", the
difference is
> instead of putting cancel/free functions carefully in some remove()
> function, you will need to (still!) carefully arrange the fields in your
> driver/module data structure, and you can have more fine grained control
> by writting the drop() function for the driver/module data structure.
That all makes sense, but again, the challenge seems to be making that
mandatory so it is all safe.

If you can show people a sketch how you think that could work it would
probably help.
> I feel I'm still missing some contexts why Devres<T> is related
to the
> "design pattern", so I will just skip this part for now... Hope
we are
> on the same page of the "design pattern" in Rust?
There is a requirement that a certain C destructor function be
*guaranteed* to be called during remove().

Ie if I told you that the C functions *required* hrtimer_cancel() be
called in the device_driver remove for correctness, how could you
accomplish this? And avoid an UAF of the hrtimer from other threads?
And do it without adding new locks. And prevent the driver author from
escaping these requirements.

Then you have the design pattern I'm talking about.

Is it clearer?

Jason

On Thu, Feb 27, 2025 at 06:00:13PM -0400, Jason Gunthorpe
wrote:
> On Thu, Feb 27, 2025 at 01:25:10PM -0800, Boqun Feng wrote:
> > 
> > Most of the cases, it should be naturally achieved, because you
already
> > bind the objects into your module or driver, otherwise they would be
> > already cancelled and freed. 
> 
> I'm getting the feeling you can probably naturally achieve the
> required destructors, but I think Danillo is concerned that since it
> isn't *mandatory* it isn't safe/sound.
Of course you can "naturally" achieve the required destructors, I even
explained
that in [1]. :-)

And yes, for *device resources* it is unsound if we do not ensure that the
device resource is actually dropped at device unbind.

Maybe some example code does help. Look at this example where we assume that
pci::Device::iomap_region() does return a pci::Bar instead of a
Devres<pci::Bar>, which it actually does. (The example won't compile,
since,
for readability, it's heavily simplified.)

    fn probe(pdev: &mut pci::Device, _info: &Self::IdInfo) ->
Result<Pin<KBox<Self>>> {
        let bar: pci::Bar = pdev.iomap_region()?;

        let drvdata = Arc::new(bar, GFP_KERNEL)?;

        let drm = drm::device::Device::new(pdev.as_ref(), drvdata)?;
        let reg = drm::drv::Registration::new(drm)?;

        // Everything in `Self` is dropped on remove(), hence the DRM driver is
        // unregistered on remove().
        Ok(KBox::new(Self(reg), GFP_KERNEL)?)
    }

This is already broken, because now the lifetime of the pci::Bar is bound to the
DRM device and the DRM device is allowed to outlive remove().

Subsequently, pci_iounmap() and pci_release_region() are not called in remove(),
but whenever the DRM device is dropped.

The fact that this is possible with safe code, makes this API unsound.

Now let's assume iomap_region() would return a Devres<pci::Device>.
That fixes
the problem, because even if the DRM device keeps the Devres<pci::Device>
object
alive, it is still dropped when the driver is unbound, and subsequently
pci_iounmap() and pci_release_region() are called when they're supposed to
be
called.

Note that this would not be a problem if pci::Device would not be a device
resource. Other stuff may be perfectly fine to bind to the lifetime of the DRM
device.

[1] https://lore.kernel.org/rust-for-linux/Z8CX__mIlFUFEkIh at cassiopeiae/