linux-kernel - Re: [PATCH 1/7] rust: file: add Rust abstraction for `struct file`

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1cT5uqAdTaJ2sJVCILPrVHczVq1z2BZfCXy3wOHJKhMx7RFSATPcdAFpYT9y6h6nZBvHcQsYbeJMXMqB6u3i85yDRIO6iGwUoerIaSWdEn4=@proton.me>
Date:   Wed, 29 Nov 2023 23:17:48 +0000
From:   Benno Lossin <benno.lossin@...ton.me>
To:     Alice Ryhl <aliceryhl@...gle.com>
Cc:     brauner@...nel.org, a.hindborg@...sung.com, alex.gaynor@...il.com,
        arve@...roid.com, bjorn3_gh@...tonmail.com, boqun.feng@...il.com,
        cmllamas@...gle.com, dan.j.williams@...el.com, dxu@...uu.xyz,
        gary@...yguo.net, gregkh@...uxfoundation.org,
        joel@...lfernandes.org, keescook@...omium.org,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        maco@...roid.com, ojeda@...nel.org, peterz@...radead.org,
        rust-for-linux@...r.kernel.org, surenb@...gle.com,
        tglx@...utronix.de, tkjos@...roid.com, viro@...iv.linux.org.uk,
        wedsonaf@...il.com, willy@...radead.org
Subject: Re: [PATCH 1/7] rust: file: add Rust abstraction for `struct file`

On 29.11.23 22:27, Alice Ryhl wrote:
> Another example:
> 
> 	void set_nonblocking_and_fput(struct file *file) {
> 		// Let's just ignore the lock for this example.
> 		file->f_flags |= O_NONBLOCK;
> 
> 		fput(file);
> 	}
> 
> This method takes a file, sets it to non-blocking, and then destroys the
> ref-count. What are the ownership semantics? Well, the caller should own
> an `fget` ref-count, and we consume that ref-count. The equivalent Rust
> code would be to take an `ARef<File>`:
> 
> 	fn set_nonblocking_and_fput(file: ARef<File>) {
> 		file.set_flag(O_NONBLOCK);
> 
> 		// When `file` goes out of scope here, the destructor
> 		// runs and calls `fput`. (Since that's what we defined
> 		// `ARef` to do on drop in `fn dec_ref`.)
> 	}
> 
> You can also explicitly call the destructor with `drop(file)`:
> 
> 	fn set_nonblocking_and_fput(file: ARef<File>) {
> 		file.set_flag(O_NONBLOCK);
> 		drop(file);
> 
> 		// When `file` goes out of scope, the destructor does
> 		// *not* run. This is because `drop(file)` is a move
> 		// (due to the signature of drop), and if you perform a
> 		// move, then the destructor no longer runs at the end
> 		// of the scope.

I want to note that while the destructor does not run at the end of the
scope, it still *does* run: the `drop(file)` call runs the destructor.

> 	}
> 
> And note that this would not compile, because we give up ownership of
> the `ARef` by passing it to `drop`:
> 
> 	fn set_nonblocking_and_fput(file: ARef<File>) {
> 		drop(file);
> 		file.set_flag(O_NONBLOCK);
> 	}
>

[...]

>>> +// SAFETY: The type invariants guarantee that `File` is always ref-counted.
>>> +unsafe impl AlwaysRefCounted for File {
>>> +    fn inc_ref(&self) {
>>> +        // SAFETY: The existence of a shared reference means that the refcount is nonzero.
>>> +        unsafe { bindings::get_file(self.0.get()) };
>>> +    }
>>
>> Why inc_ref() and not just get_file()?
> 
> Whenever you see an impl block that uses the keyword "for", then the
> code is implementing a trait. In this case, the trait being implemented
> is AlwaysRefCounted, which allows File to work with ARef.
> 
> It has to be `inc_ref` because that's what AlwaysRefCounted calls this
> method.

I am not sure if the Rust term "trait" is well-known, so for a bit more
context, I am quoting the Rust Book [1]:

    A *trait* defines functionality a particular type has and can share
    with other types. We can use traits to define shared behavior in an
    abstract way. We can use *trait bounds* to specify that a generic type
    can be any type that has certain behavior.

[1]: https://doc.rust-lang.org/book/ch10-02-traits.html

We have created an abstraction over reference counting:
the trait `AlwaysRefCounted` and the struct `ARef<T>` where `T`
implements `AlwaysRefCounted`.
As Alice already explained, `ARef<T>` is a pointer that owns a refcount
on the object. Because `ARef<T>` needs to know how to increment and
decrement that counter. For example, when you want to create another
`ARef<T>` you can `clone()` it and therefore `ARef<T>` needs to
increment the refcount. And when you drop it, `ARef<T>` needs to
decrement it.
The "`ARef<T>` knows how to inc/dec the refcount" part is done by the
`AlwaysRefCounted` trait. And there we chose to name the functions
`inc_ref` and `dec_ref`, since these are the *general*/*abstract*
operations and not any specific refcount adjustment.



Hope that also helped and did not create confusion.

--
Cheers,
Benno