Message-ID: <28fa3f97-4c7c-212e-2be2-fb1c05f7f576@asahilina.net>
Date:   Fri, 10 Mar 2023 07:16:00 +0900
From:   Asahi Lina <lina@...hilina.net>
To:     Faith Ekstrand <faith.ekstrand@...labora.com>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        Miguel Ojeda <ojeda@...nel.org>,
        Alex Gaynor <alex.gaynor@...il.com>,
        Wedson Almeida Filho <wedsonaf@...il.com>,
        Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
        Björn Roy Baron <bjorn3_gh@...tonmail.com>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        Christian König <christian.koenig@....com>,
        Luben Tuikov <luben.tuikov@....com>,
        Jarkko Sakkinen <jarkko@...nel.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>
Cc:     Alyssa Rosenzweig <alyssa@...enzweig.io>,
        Karol Herbst <kherbst@...hat.com>,
        Ella Stanforth <ella@...unix.org>, Mary <mary@...y.zone>,
        linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        rust-for-linux@...r.kernel.org, linux-media@...r.kernel.org,
        linaro-mm-sig@...ts.linaro.org, linux-sgx@...r.kernel.org,
        asahi@...ts.linux.dev
Subject: Re: [PATCH RFC 03/18] rust: drm: file: Add File abstraction

On 10/03/2023 06.16, Faith Ekstrand wrote:
> On Tue, 2023-03-07 at 23:25 +0900, Asahi Lina wrote:
>> A DRM File is the DRM counterpart to a kernel file structure,
>> representing an open DRM file descriptor. Add a Rust abstraction to
>> allow drivers to implement their own File types that implement the
>> DriverFile trait.
>>
>> Signed-off-by: Asahi Lina <lina@...hilina.net>
>> ---
>>  rust/bindings/bindings_helper.h |   1 +
>>  rust/kernel/drm/drv.rs          |   7 ++-
>>  rust/kernel/drm/file.rs         | 113
>> ++++++++++++++++++++++++++++++++++++++++
>>  rust/kernel/drm/mod.rs          |   1 +
>>  4 files changed, 120 insertions(+), 2 deletions(-)
>>
>> diff --git a/rust/bindings/bindings_helper.h
>> b/rust/bindings/bindings_helper.h
>> index 2a999138c4ae..7d7828faf89c 100644
>> --- a/rust/bindings/bindings_helper.h
>> +++ b/rust/bindings/bindings_helper.h
>> @@ -8,6 +8,7 @@
>>  
>>  #include <drm/drm_device.h>
>>  #include <drm/drm_drv.h>
>> +#include <drm/drm_file.h>
>>  #include <drm/drm_ioctl.h>
>>  #include <linux/delay.h>
>>  #include <linux/device.h>
>> diff --git a/rust/kernel/drm/drv.rs b/rust/kernel/drm/drv.rs
>> index 29a465515dc9..1dcb651e1417 100644
>> --- a/rust/kernel/drm/drv.rs
>> +++ b/rust/kernel/drm/drv.rs
>> @@ -144,6 +144,9 @@ pub trait Driver {
>>      /// Should be either `drm::gem::Object<T>` or
>> `drm::gem::shmem::Object<T>`.
>>      type Object: AllocImpl;
>>  
>> +    /// The type used to represent a DRM File (client)
>> +    type File: drm::file::DriverFile;
>> +
>>      /// Driver metadata
>>      const INFO: DriverInfo;
>>  
>> @@ -213,8 +216,8 @@ macro_rules! drm_device_register {
>>  impl<T: Driver> Registration<T> {
>>      const VTABLE: bindings::drm_driver = drm_legacy_fields! {
>>          load: None,
>> -        open: None, // TODO: File abstraction
>> -        postclose: None, // TODO: File abstraction
>> +        open: Some(drm::file::open_callback::<T::File>),
>> +        postclose: Some(drm::file::postclose_callback::<T::File>),
>>          lastclose: None,
>>          unload: None,
>>          release: None,
>> diff --git a/rust/kernel/drm/file.rs b/rust/kernel/drm/file.rs
>> new file mode 100644
>> index 000000000000..48751e93c38a
>> --- /dev/null
>> +++ b/rust/kernel/drm/file.rs
>> @@ -0,0 +1,113 @@
>> +// SPDX-License-Identifier: GPL-2.0 OR MIT
>> +
>> +//! DRM File objects.
>> +//!
>> +//! C header:
>> [`include/linux/drm/drm_file.h`](../../../../include/linux/drm/drm_fi
>> le.h)
>> +
>> +use crate::{bindings, drm, error::Result};
>> +use alloc::boxed::Box;
>> +use core::marker::PhantomData;
>> +use core::ops::Deref;
>> +
>> +/// Trait that must be implemented by DRM drivers to represent a DRM
>> File (a client instance).
>> +pub trait DriverFile {
>> +    /// The parent `Driver` implementation for this `DriverFile`.
>> +    type Driver: drm::drv::Driver;
>> +
>> +    /// Open a new file (called when a client opens the DRM device).
>> +    fn open(device: &drm::device::Device<Self::Driver>) ->
>> Result<Box<Self>>;
>> +}
>> +
>> +/// An open DRM File.
>> +///
>> +/// # Invariants
>> +/// `raw` is a valid pointer to a `drm_file` struct.
>> +#[repr(transparent)]
>> +pub struct File<T: DriverFile> {
>> +    raw: *mut bindings::drm_file,
>> +    _p: PhantomData<T>,
>> +}
>> +
>> +pub(super) unsafe extern "C" fn open_callback<T: DriverFile>(
>> +    raw_dev: *mut bindings::drm_device,
>> +    raw_file: *mut bindings::drm_file,
>> +) -> core::ffi::c_int {
>> +    let drm = core::mem::ManuallyDrop::new(unsafe {
>> drm::device::Device::from_raw(raw_dev) });
> 
> Maybe you can help educate me a bit here... This feels like a really
> sketchy pattern.  We're creating a Device from a pointer, an operation
> which inherently consumes a reference but then marking it ManuallyDrop
> so drm_device_put() never gets called.  It took me a while but I think
> I figured out what you're trying to do: Make it so all the Rust stuff
> works with Device, not drm_device but it still feels really wrong.  It
> works, it just feels like there's a lot of unsafe abstraction juggling
> happening here and I expect this operation is going to be pretty common
> in the Rust abstraction layer.

So I think this is going to be a pretty common pattern in this kind of
abstraction. The problem is that, of course, in C there is no
distinction between an owned reference and a borrowed one. Here we have
a borrowed reference to a struct drm_device, and we need to turn it into
a &Device (which is the Rust equivalent type). But for &Device to exist
we need a Device to exist in the first place, and Device normally
implies ownership of the underlying drm_device.

We could just acquire a reference here, but then we're needlessly
grabbing a ref only to drop it at the end of the function, which is
pointless when the caller is holding another reference for us while the
callback runs. And of course Rust likes to claim to offer zero-cost
abstractions, so it would be kind of sad to have to do that... ^^

Just doing drm::device::Device::from_raw(raw_dev) is a ticking time
bomb, because we haven't acquired a reference (which would normally be
required). If that Device ever gets dropped, we've messed up the
refcounting and stolen the caller's reference. We could try to ensure it
gets passed to core::mem::forget in all paths out, but that gets
error-prone very quickly when trying to cover error paths. So instead,
we put it into a ManuallyDrop. That takes care of neutering the ref
drop, so we don't have to worry about messing that up. Then the only
remaining safety requirement is that that the ManuallyDrop<Device> never
escape the callback function, and that's easy to ensure: we only pass a
&ref to the user (which via auto-deref ends up being a &Device), and
then nothing bad can happen. If the user wants an owned reference to the
device to keep around, they can call .clone() on it and that's when the
incref happens.

Basically, ManuallyDrop<T> where T is a reference counted type
represents a borrowed reference to a T coming from the C side. You can
see another use of this pattern in gem::Object, which contains a
ManuallyDrop<Device> that represents a borrowed reference to the device
that owns that object. The DRM core (as far as I know!) guarantees that
DRM devices outlive all of their GEM objects, so we can materialize a
borrowed reference and as long as it never leaves the GEM object, it
will be sound. Then we can take &Device references from it whenever we
want, and the usual Rust borrow checker rules ensure we can't do
something illegal.

~~ Lina

Powered by blists - more mailing lists