| Message ID | 20231206-alice-file-v2-2-af617c0d9d94@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | File abstractions needed by Rust Binder | expand |
On 12/6/23 12:59, Alice Ryhl wrote: > +/// Wraps the kernel's `struct cred`. > +/// > +/// # Invariants > +/// > +/// Instances of this type are always ref-counted, that is, a call to `get_cred` ensures that the > +/// allocation remains valid at least until the matching call to `put_cred`. > +#[repr(transparent)] > +pub struct Credential(pub(crate) Opaque<bindings::cred>); Why is the field `pub(crate)`? [...] > +// SAFETY: The type invariants guarantee that `Credential` is always ref-counted. > +unsafe impl AlwaysRefCounted for Credential { > + fn inc_ref(&self) { > + // SAFETY: The existence of a shared reference means that the refcount is nonzero. > + unsafe { bindings::get_cred(self.0.get()) }; > + } > + > + unsafe fn dec_ref(obj: core::ptr::NonNull<Self>) { > + // SAFETY: The safety requirements guarantee that the refcount is nonzero. Can you also justify the `cast()`?
On Wed, Dec 06, 2023 at 11:59:47AM +0000, Alice Ryhl wrote: [...] > @@ -151,6 +152,21 @@ pub fn as_ptr(&self) -> *mut bindings::file { > self.0.get() > } > > + /// Returns the credentials of the task that originally opened the file. > + pub fn cred(&self) -> &Credential { I wonder whether it would be helpful if we use explicit lifetime here: pub fn cred<'file>(&'file self) -> &'file Credential It might be easier for people to get. For example, the lifetime of the returned Credential reference is constrainted by 'file, the lifetime of the file reference. But yes, maybe need to hear others' feedback first. Regards, Boqun > + // SAFETY: Since the caller holds a reference to the file, it is guaranteed that its > + // refcount does not hit zero during this function call. > + // > + // It's okay to read the `f_cred` field without synchronization as `f_cred` is never > + // changed after initialization of the file. > + let ptr = unsafe { (*self.as_ptr()).f_cred }; > + > + // SAFETY: The signature of this function ensures that the caller will only access the > + // returned credential while the file is still valid, and the C side ensures that the > + // credential stays valid at least as long as the file. > + unsafe { Credential::from_ptr(ptr) } > + } > + > /// Returns the flags associated with the file. > /// > /// The flags are a combination of the constants in [`flags`]. > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs > index ce9abceab784..097fe9bb93ed 100644 > --- a/rust/kernel/lib.rs > +++ b/rust/kernel/lib.rs > @@ -33,6 +33,7 @@ > #[cfg(not(testlib))] > mod allocator; > mod build_assert; > +pub mod cred; > pub mod error; > pub mod file; > pub mod init; > > -- > 2.43.0.rc2.451.g8631bc7472-goog > >
Benno Lossin <benno.lossin@proton.me> writes: > On 12/6/23 12:59, Alice Ryhl wrote: > > +/// Wraps the kernel's `struct cred`. > > +/// > > +/// # Invariants > > +/// > > +/// Instances of this type are always ref-counted, that is, a call to `get_cred` ensures that the > > +/// allocation remains valid at least until the matching call to `put_cred`. > > +#[repr(transparent)] > > +pub struct Credential(pub(crate) Opaque<bindings::cred>); > > Why is the field `pub(crate)`? Historical accident. It isn't needed anymore. I'll remove it. > > + unsafe fn dec_ref(obj: core::ptr::NonNull<Self>) { > > + // SAFETY: The safety requirements guarantee that the refcount is nonzero. > > Can you also justify the `cast()`? Will do. Alice
Boqun Feng <boqun.feng@gmail.com> writes: > On Wed, Dec 06, 2023 at 11:59:47AM +0000, Alice Ryhl wrote: > [...] > > @@ -151,6 +152,21 @@ pub fn as_ptr(&self) -> *mut bindings::file { > > self.0.get() > > } > > > > + /// Returns the credentials of the task that originally opened the file. > > + pub fn cred(&self) -> &Credential { > > I wonder whether it would be helpful if we use explicit lifetime here: > > pub fn cred<'file>(&'file self) -> &'file Credential > > It might be easier for people to get. For example, the lifetime of the > returned Credential reference is constrainted by 'file, the lifetime of > the file reference. > > But yes, maybe need to hear others' feedback first. > > Regards, > Boqun That would trigger a compiler warning because the lifetime is unnecessary. The safety comment explains what the signature means. I think that should be enough. Alice
On Mon, Dec 11, 2023 at 03:34:29PM +0000, Alice Ryhl wrote: > Boqun Feng <boqun.feng@gmail.com> writes: > > On Wed, Dec 06, 2023 at 11:59:47AM +0000, Alice Ryhl wrote: > > [...] > > > @@ -151,6 +152,21 @@ pub fn as_ptr(&self) -> *mut bindings::file { > > > self.0.get() > > > } > > > > > > + /// Returns the credentials of the task that originally opened the file. > > > + pub fn cred(&self) -> &Credential { > > > > I wonder whether it would be helpful if we use explicit lifetime here: > > > > pub fn cred<'file>(&'file self) -> &'file Credential > > > > It might be easier for people to get. For example, the lifetime of the > > returned Credential reference is constrainted by 'file, the lifetime of > > the file reference. > > > > But yes, maybe need to hear others' feedback first. > > > > Regards, > > Boqun > > That would trigger a compiler warning because the lifetime is > unnecessary. > We can disable that warning if people need the information. Code is mostly for reading, less often for compilation and changes. > The safety comment explains what the signature means. I think that > should be enough. > For someone who has a good understanding of Rust lifetime (and the elision), yes. But I'm wondering whether all the people feel the same way. Regards, Boqun > Alice
On 12/11/23 18:35, Boqun Feng wrote: > On Mon, Dec 11, 2023 at 03:34:29PM +0000, Alice Ryhl wrote: >> Boqun Feng <boqun.feng@gmail.com> writes: >>> On Wed, Dec 06, 2023 at 11:59:47AM +0000, Alice Ryhl wrote: >>> [...] >>>> @@ -151,6 +152,21 @@ pub fn as_ptr(&self) -> *mut bindings::file { >>>> self.0.get() >>>> } >>>> >>>> + /// Returns the credentials of the task that originally opened the file. >>>> + pub fn cred(&self) -> &Credential { >>> >>> I wonder whether it would be helpful if we use explicit lifetime here: >>> >>> pub fn cred<'file>(&'file self) -> &'file Credential >>> >>> It might be easier for people to get. For example, the lifetime of the >>> returned Credential reference is constrainted by 'file, the lifetime of >>> the file reference. >>> >>> But yes, maybe need to hear others' feedback first. >>> >>> Regards, >>> Boqun >> >> That would trigger a compiler warning because the lifetime is >> unnecessary. >> > > We can disable that warning if people need the information. Code is > mostly for reading, less often for compilation and changes. > >> The safety comment explains what the signature means. I think that >> should be enough. >> > > For someone who has a good understanding of Rust lifetime (and the > elision), yes. But I'm wondering whether all the people feel the same > way. So in this example, I think it should be straight forward what happens to the lifetimes, since there is only one to begin with. If we want to do this, I think we should have some rules around it. A general piece of advice I can offer is this: - when there are no lifetimes in the return type of a function, then you probably do not care about the lifetimes (they will all be distinct and there are no relationships between them). - when there is a single input and a single output lifetime, then they are the same. - the other cases are not so simple, but most of the time they will require explicit annotations. I left out some details, you can read more at [1]. Most cases where it is not obvious which lifetime relations exist are already rejected by the compiler. The only exception is if there is a `&self` or `&mut self` parameter, then that one has precedence. So we could also explicitly annotate those. Since they are also rare, I think this would be fine. [1]: https://doc.rust-lang.org/nomicon/lifetime-elision.html
On Mon, Dec 11, 2023 at 6:35 PM Boqun Feng <boqun.feng@gmail.com> wrote: > On Mon, Dec 11, 2023 at 03:34:29PM +0000, Alice Ryhl wrote: > > The safety comment explains what the signature means. I think that > > should be enough. > > > > For someone who has a good understanding of Rust lifetime (and the > elision), yes. But I'm wondering whether all the people feel the same > way. The safety comment doesn't require understanding of lifetime elision to be understood: "The signature of this function ensures that the caller will only access the returned credential while the file is still valid." Yes, if you don't know the syntax for lifetimes, you'll have to trust me that this is what the signature means. But I think that's the case either way. I don't think it needs to be changed. Alice
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h index beed3ef1fbc3..6d1bd2229aab 100644 --- a/rust/bindings/bindings_helper.h +++ b/rust/bindings/bindings_helper.h @@ -7,6 +7,7 @@ */ #include <kunit/test.h> +#include <linux/cred.h> #include <linux/errname.h> #include <linux/file.h> #include <linux/fs.h> diff --git a/rust/helpers.c b/rust/helpers.c index 03141a3608a4..10ed69f76424 100644 --- a/rust/helpers.c +++ b/rust/helpers.c @@ -23,6 +23,7 @@ #include <kunit/test-bug.h> #include <linux/bug.h> #include <linux/build_bug.h> +#include <linux/cred.h> #include <linux/err.h> #include <linux/errname.h> #include <linux/fs.h> @@ -164,6 +165,18 @@ struct file *rust_helper_get_file(struct file *f) } EXPORT_SYMBOL_GPL(rust_helper_get_file); +const struct cred *rust_helper_get_cred(const struct cred *cred) +{ + return get_cred(cred); +} +EXPORT_SYMBOL_GPL(rust_helper_get_cred); + +void rust_helper_put_cred(const struct cred *cred) +{ + put_cred(cred); +} +EXPORT_SYMBOL_GPL(rust_helper_put_cred); + /* * `bindgen` binds the C `size_t` type as the Rust `usize` type, so we can * use it in contexts where Rust expects a `usize` like slice (array) indices. diff --git a/rust/kernel/cred.rs b/rust/kernel/cred.rs new file mode 100644 index 000000000000..497058ec89bb --- /dev/null +++ b/rust/kernel/cred.rs @@ -0,0 +1,64 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! Credentials management. +//! +//! C header: [`include/linux/cred.h`](../../../../include/linux/cred.h) +//! +//! Reference: <https://www.kernel.org/doc/html/latest/security/credentials.html> + +use crate::{ + bindings, + types::{AlwaysRefCounted, Opaque}, +}; + +/// Wraps the kernel's `struct cred`. +/// +/// # Invariants +/// +/// Instances of this type are always ref-counted, that is, a call to `get_cred` ensures that the +/// allocation remains valid at least until the matching call to `put_cred`. +#[repr(transparent)] +pub struct Credential(pub(crate) Opaque<bindings::cred>); + +// SAFETY: By design, the only way to access a `Credential` is via an immutable reference or an +// `ARef`. This means that the only situation in which a `Credential` can be accessed mutably is +// when the refcount drops to zero and the destructor runs. It is safe for that to happen on any +// thread, so it is ok for this type to be `Send`. +unsafe impl Send for Credential {} + +// SAFETY: It's OK to access `Credential` through shared references from other threads because +// we're either accessing properties that don't change or that are properly synchronised by C code. +unsafe impl Sync for Credential {} + +impl Credential { + /// Creates a reference to a [`Credential`] from a valid pointer. + /// + /// # Safety + /// + /// The caller must ensure that `ptr` is valid and remains valid for the lifetime of the + /// returned [`Credential`] reference. + pub unsafe fn from_ptr<'a>(ptr: *const bindings::cred) -> &'a Credential { + // SAFETY: The safety requirements guarantee the validity of the dereference, while the + // `Credential` type being transparent makes the cast ok. + unsafe { &*ptr.cast() } + } + + /// Returns the effective UID of the given credential. + pub fn euid(&self) -> bindings::kuid_t { + // SAFETY: By the type invariant, we know that `self.0` is valid. + unsafe { (*self.0.get()).euid } + } +} + +// SAFETY: The type invariants guarantee that `Credential` is always ref-counted. +unsafe impl AlwaysRefCounted for Credential { + fn inc_ref(&self) { + // SAFETY: The existence of a shared reference means that the refcount is nonzero. + unsafe { bindings::get_cred(self.0.get()) }; + } + + unsafe fn dec_ref(obj: core::ptr::NonNull<Self>) { + // SAFETY: The safety requirements guarantee that the refcount is nonzero. + unsafe { bindings::put_cred(obj.cast().as_ptr()) }; + } +} diff --git a/rust/kernel/file.rs b/rust/kernel/file.rs index 29e1aacacd06..a88140794a8d 100644 --- a/rust/kernel/file.rs +++ b/rust/kernel/file.rs @@ -7,6 +7,7 @@ use crate::{ bindings, + cred::Credential, error::{code::*, Error, Result}, types::{ARef, AlwaysRefCounted, Opaque}, }; @@ -151,6 +152,21 @@ pub fn as_ptr(&self) -> *mut bindings::file { self.0.get() } + /// Returns the credentials of the task that originally opened the file. + pub fn cred(&self) -> &Credential { + // SAFETY: Since the caller holds a reference to the file, it is guaranteed that its + // refcount does not hit zero during this function call. + // + // It's okay to read the `f_cred` field without synchronization as `f_cred` is never + // changed after initialization of the file. + let ptr = unsafe { (*self.as_ptr()).f_cred }; + + // SAFETY: The signature of this function ensures that the caller will only access the + // returned credential while the file is still valid, and the C side ensures that the + // credential stays valid at least as long as the file. + unsafe { Credential::from_ptr(ptr) } + } + /// Returns the flags associated with the file. /// /// The flags are a combination of the constants in [`flags`]. diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index ce9abceab784..097fe9bb93ed 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -33,6 +33,7 @@ #[cfg(not(testlib))] mod allocator; mod build_assert; +pub mod cred; pub mod error; pub mod file; pub mod init;