[PATCH] lsm: rust: mark SecurityCtx methods inline

[PATCH] lsm: rust: mark SecurityCtx methods inline

[Alice Ryhl]

I'm seeing Binder generating calls to methods on SecurityCtx such as
from_secid and drop without inlining. Since these methods are really
simple wrappers around C functions, mark the methods to inline to avoid
generating these useless small functions.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/kernel/security.rs | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/rust/kernel/security.rs b/rust/kernel/security.rs
index 25d2b1ac3833..243211050526 100644
--- a/rust/kernel/security.rs
+++ b/rust/kernel/security.rs
@@ -23,6 +23,7 @@ pub struct SecurityCtx {
 
 impl SecurityCtx {
     /// Get the security context given its id.
+    #[inline]
     pub fn from_secid(secid: u32) -> Result<Self> {
         // SAFETY: `struct lsm_context` can be initialized to all zeros.
         let mut ctx: bindings::lsm_context = unsafe { core::mem::zeroed() };
@@ -35,16 +36,19 @@ pub fn from_secid(secid: u32) -> Result<Self> {
     }
 
     /// Returns whether the security context is empty.
+    #[inline]
     pub fn is_empty(&self) -> bool {
         self.ctx.len == 0
     }
 
     /// Returns the length of this security context.
+    #[inline]
     pub fn len(&self) -> usize {
         self.ctx.len as usize
     }
 
     /// Returns the bytes for this security context.
+    #[inline]
     pub fn as_bytes(&self) -> &[u8] {
         let ptr = self.ctx.context;
         if ptr.is_null() {
@@ -61,6 +65,7 @@ pub fn as_bytes(&self) -> &[u8] {
 }
 
 impl Drop for SecurityCtx {
+    #[inline]
     fn drop(&mut self) {
         // SAFETY: By the invariant of `Self`, this frees a context that came from a successful
         // call to `security_secid_to_secctx` and has not yet been destroyed by

---
base-commit: a64dcfb451e254085a7daee5fe51bf22959d52d3
change-id: 20250303-inline-securityctx-6fc1ca669156

Best regards,
-- 
Alice Ryhl <aliceryhl@google.com>

Re: [PATCH] lsm: rust: mark SecurityCtx methods inline

[Casey Schaufler]

On 3/3/2025 7:29 AM, Alice Ryhl wrote:
> I'm seeing Binder generating calls to methods on SecurityCtx such as
> from_secid and drop without inlining. Since these methods are really
> simple wrappers around C functions, mark the methods to inline to avoid
> generating these useless small functions.
>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>  rust/kernel/security.rs | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/rust/kernel/security.rs b/rust/kernel/security.rs
> index 25d2b1ac3833..243211050526 100644
> --- a/rust/kernel/security.rs
> +++ b/rust/kernel/security.rs
> @@ -23,6 +23,7 @@ pub struct SecurityCtx {
>  
>  impl SecurityCtx {
>      /// Get the security context given its id.
> +    #[inline]
>      pub fn from_secid(secid: u32) -> Result<Self> {
>          // SAFETY: `struct lsm_context` can be initialized to all zeros.
>          let mut ctx: bindings::lsm_context = unsafe { core::mem::zeroed() };
> @@ -35,16 +36,19 @@ pub fn from_secid(secid: u32) -> Result<Self> {
>      }
>  
>      /// Returns whether the security context is empty.
> +    #[inline]
>      pub fn is_empty(&self) -> bool {
>          self.ctx.len == 0
>      }
>  
>      /// Returns the length of this security context.
> +    #[inline]
>      pub fn len(&self) -> usize {
>          self.ctx.len as usize
>      }
>  
>      /// Returns the bytes for this security context.
> +    #[inline]
>      pub fn as_bytes(&self) -> &[u8] {
>          let ptr = self.ctx.context;
>          if ptr.is_null() {
> @@ -61,6 +65,7 @@ pub fn as_bytes(&self) -> &[u8] {
>  }
>  
>  impl Drop for SecurityCtx {
> +    #[inline]
>      fn drop(&mut self) {
>          // SAFETY: By the invariant of `Self`, this frees a context that came from a successful
>          // call to `security_secid_to_secctx` and has not yet been destroyed by

I don't speak rust (well, yet?) so I can't talk about that, but this comment
has me concerned. Security contexts (secctx) are not destroyed, they are released.
While SELinux allocates and frees them, Smack maintains a list of contexts that
is never freed. A call to security_release_secctx() on SELinux "destroys" the
secctx, but for Smack does not.

>
> ---
> base-commit: a64dcfb451e254085a7daee5fe51bf22959d52d3
> change-id: 20250303-inline-securityctx-6fc1ca669156
>
> Best regards,

Re: [PATCH] lsm: rust: mark SecurityCtx methods inline

[Andreas Hindborg]

"Alice Ryhl" <aliceryhl@google.com> writes:

> I'm seeing Binder generating calls to methods on SecurityCtx such as
> from_secid and drop without inlining. Since these methods are really
> simple wrappers around C functions, mark the methods to inline to avoid
> generating these useless small functions.
>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>


Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>


Best regards,
Andreas Hindborg

Re: [PATCH] lsm: rust: mark SecurityCtx methods inline

[Alice Ryhl]

On Mon, Mar 3, 2025 at 6:07 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> On 3/3/2025 7:29 AM, Alice Ryhl wrote:
> > I'm seeing Binder generating calls to methods on SecurityCtx such as
> > from_secid and drop without inlining. Since these methods are really
> > simple wrappers around C functions, mark the methods to inline to avoid
> > generating these useless small functions.
> >
> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> > ---
> >  rust/kernel/security.rs | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/rust/kernel/security.rs b/rust/kernel/security.rs
> > index 25d2b1ac3833..243211050526 100644
> > --- a/rust/kernel/security.rs
> > +++ b/rust/kernel/security.rs
> > @@ -23,6 +23,7 @@ pub struct SecurityCtx {
> >
> >  impl SecurityCtx {
> >      /// Get the security context given its id.
> > +    #[inline]
> >      pub fn from_secid(secid: u32) -> Result<Self> {
> >          // SAFETY: `struct lsm_context` can be initialized to all zeros.
> >          let mut ctx: bindings::lsm_context = unsafe { core::mem::zeroed() };
> > @@ -35,16 +36,19 @@ pub fn from_secid(secid: u32) -> Result<Self> {
> >      }
> >
> >      /// Returns whether the security context is empty.
> > +    #[inline]
> >      pub fn is_empty(&self) -> bool {
> >          self.ctx.len == 0
> >      }
> >
> >      /// Returns the length of this security context.
> > +    #[inline]
> >      pub fn len(&self) -> usize {
> >          self.ctx.len as usize
> >      }
> >
> >      /// Returns the bytes for this security context.
> > +    #[inline]
> >      pub fn as_bytes(&self) -> &[u8] {
> >          let ptr = self.ctx.context;
> >          if ptr.is_null() {
> > @@ -61,6 +65,7 @@ pub fn as_bytes(&self) -> &[u8] {
> >  }
> >
> >  impl Drop for SecurityCtx {
> > +    #[inline]
> >      fn drop(&mut self) {
> >          // SAFETY: By the invariant of `Self`, this frees a context that came from a successful
> >          // call to `security_secid_to_secctx` and has not yet been destroyed by
>
> I don't speak rust (well, yet?) so I can't talk about that, but this comment
> has me concerned. Security contexts (secctx) are not destroyed, they are released.
> While SELinux allocates and frees them, Smack maintains a list of contexts that
> is never freed. A call to security_release_secctx() on SELinux "destroys" the
> secctx, but for Smack does not.

It's just a comment on a call to security_release_secctx, I can reword
from "destroy" to "release".

Here's the full context:

// SAFETY: By the invariant of `Self`, this frees a context that came from a
// successful call to `security_secid_to_secctx` and has not yet been destroyed
// by `security_release_secctx`.
unsafe { bindings::security_release_secctx(&mut self.ctx) };

Alice

Re: [PATCH] lsm: rust: mark SecurityCtx methods inline

[Casey Schaufler]

On 3/3/2025 10:40 AM, Alice Ryhl wrote:
> On Mon, Mar 3, 2025 at 6:07 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 3/3/2025 7:29 AM, Alice Ryhl wrote:
>>> I'm seeing Binder generating calls to methods on SecurityCtx such as
>>> from_secid and drop without inlining. Since these methods are really
>>> simple wrappers around C functions, mark the methods to inline to avoid
>>> generating these useless small functions.
>>>
>>> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
>>> ---
>>>  rust/kernel/security.rs | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/rust/kernel/security.rs b/rust/kernel/security.rs
>>> index 25d2b1ac3833..243211050526 100644
>>> --- a/rust/kernel/security.rs
>>> +++ b/rust/kernel/security.rs
>>> @@ -23,6 +23,7 @@ pub struct SecurityCtx {
>>>
>>>  impl SecurityCtx {
>>>      /// Get the security context given its id.
>>> +    #[inline]
>>>      pub fn from_secid(secid: u32) -> Result<Self> {
>>>          // SAFETY: `struct lsm_context` can be initialized to all zeros.
>>>          let mut ctx: bindings::lsm_context = unsafe { core::mem::zeroed() };
>>> @@ -35,16 +36,19 @@ pub fn from_secid(secid: u32) -> Result<Self> {
>>>      }
>>>
>>>      /// Returns whether the security context is empty.
>>> +    #[inline]
>>>      pub fn is_empty(&self) -> bool {
>>>          self.ctx.len == 0
>>>      }
>>>
>>>      /// Returns the length of this security context.
>>> +    #[inline]
>>>      pub fn len(&self) -> usize {
>>>          self.ctx.len as usize
>>>      }
>>>
>>>      /// Returns the bytes for this security context.
>>> +    #[inline]
>>>      pub fn as_bytes(&self) -> &[u8] {
>>>          let ptr = self.ctx.context;
>>>          if ptr.is_null() {
>>> @@ -61,6 +65,7 @@ pub fn as_bytes(&self) -> &[u8] {
>>>  }
>>>
>>>  impl Drop for SecurityCtx {
>>> +    #[inline]
>>>      fn drop(&mut self) {
>>>          // SAFETY: By the invariant of `Self`, this frees a context that came from a successful
>>>          // call to `security_secid_to_secctx` and has not yet been destroyed by
>> I don't speak rust (well, yet?) so I can't talk about that, but this comment
>> has me concerned. Security contexts (secctx) are not destroyed, they are released.
>> While SELinux allocates and frees them, Smack maintains a list of contexts that
>> is never freed. A call to security_release_secctx() on SELinux "destroys" the
>> secctx, but for Smack does not.
> It's just a comment on a call to security_release_secctx, I can reword
> from "destroy" to "release".

That would do nicely. Thank you.

>
> Here's the full context:
>
> // SAFETY: By the invariant of `Self`, this frees a context that came from a
> // successful call to `security_secid_to_secctx` and has not yet been destroyed
> // by `security_release_secctx`.
> unsafe { bindings::security_release_secctx(&mut self.ctx) };
>
> Alice
>

Re: [PATCH] lsm: rust: mark SecurityCtx methods inline

[Paul Moore]

On Mon, Mar 3, 2025 at 10:30 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> I'm seeing Binder generating calls to methods on SecurityCtx such as
> from_secid and drop without inlining. Since these methods are really
> simple wrappers around C functions, mark the methods to inline to avoid
> generating these useless small functions.
>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>  rust/kernel/security.rs | 5 +++++
>  1 file changed, 5 insertions(+)

While this isn't specific to your patch, Casey's comment about "free"
vs "release" is something to keep in mind when working on the LSM
bindings; what happens inside the individual LSMs for a given LSM hook
can vary quite a bit.

I also saw a similar Rust patch where a commenter suggested using
"impersonal facts" in the commit description and I believe that is a
good idea here as well.

Beyond those nitpicks, this looks okay to me based on my *extremely*
limited Rust knowledge.  With the minor requested changes in place,
would you prefer me to take this via the LSM tree, or would you prefer
it to go up to Linus via a more Rust-y tree?

> diff --git a/rust/kernel/security.rs b/rust/kernel/security.rs
> index 25d2b1ac3833..243211050526 100644
> --- a/rust/kernel/security.rs
> +++ b/rust/kernel/security.rs
> @@ -23,6 +23,7 @@ pub struct SecurityCtx {
>
>  impl SecurityCtx {
>      /// Get the security context given its id.
> +    #[inline]
>      pub fn from_secid(secid: u32) -> Result<Self> {
>          // SAFETY: `struct lsm_context` can be initialized to all zeros.
>          let mut ctx: bindings::lsm_context = unsafe { core::mem::zeroed() };
> @@ -35,16 +36,19 @@ pub fn from_secid(secid: u32) -> Result<Self> {
>      }
>
>      /// Returns whether the security context is empty.
> +    #[inline]
>      pub fn is_empty(&self) -> bool {
>          self.ctx.len == 0
>      }
>
>      /// Returns the length of this security context.
> +    #[inline]
>      pub fn len(&self) -> usize {
>          self.ctx.len as usize
>      }
>
>      /// Returns the bytes for this security context.
> +    #[inline]
>      pub fn as_bytes(&self) -> &[u8] {
>          let ptr = self.ctx.context;
>          if ptr.is_null() {
> @@ -61,6 +65,7 @@ pub fn as_bytes(&self) -> &[u8] {
>  }
>
>  impl Drop for SecurityCtx {
> +    #[inline]
>      fn drop(&mut self) {
>          // SAFETY: By the invariant of `Self`, this frees a context that came from a successful
>          // call to `security_secid_to_secctx` and has not yet been destroyed by
>
> ---
> base-commit: a64dcfb451e254085a7daee5fe51bf22959d52d3
> change-id: 20250303-inline-securityctx-6fc1ca669156
>
> Best regards,
> --
> Alice Ryhl <aliceryhl@google.com>

-- 
paul-moore.com

Re: [PATCH] lsm: rust: mark SecurityCtx methods inline

[Miguel Ojeda]

Hi Paul,

On Mon, Mar 3, 2025 at 11:55 PM Paul Moore <paul@paul-moore.com> wrote:
>
> Beyond those nitpicks, this looks okay to me based on my *extremely*
> limited Rust knowledge.  With the minor requested changes in place,
> would you prefer me to take this via the LSM tree, or would you prefer
> it to go up to Linus via a more Rust-y tree?

In general, if a subsystem is willing to take Rust-related patches
through their tree, that is the ideal scenario! So please definitely
feel free to pick it up on your side (and thanks!); otherwise, I can
pick it up with your Acked-by.

Some days ago I wrote a summary of the usual discussion we have around
this (copy-pasting here for convenience):

    So far, what we have been doing is ask maintainers, first, if they
    would be willing take the patches themselves -- they are the experts
    of the subsystem, know what changes are incoming, etc. Some subsystems
    have done this (e.g. KUnit). That is ideal, because the goal is to
    scale and allows maintainers to be in full control.

    Of course, sometimes maintainers are not fully comfortable doing that,
    since they may not have the bandwidth, or the setup, or the Rust
    knowledge. In those cases, we typically ask if they would be willing
    to have a co-maintainer (i.e. in their entry, e.g. like locking did),
    or a sub-maintainer (i.e. in a new entry, e.g. like block did), that
    would take care of the bulk of the work from them.

    I think that is a nice middle-ground -- the advantage of doing it like
    that is that you get the benefits of knowing best what is going on
    without too much work (hopefully), and it may allow you to get more
    and more involved over time and confident on what is going on with the
    Rust callers, typical issues that appear, etc. Plus the sub-maintainer
    gets to learn more about the subsystem, its timelines, procedures,
    etc., which you may welcome (if you are looking for new people to get
    involved).

    I think that would be a nice middle-ground. As far as I understand,
    Andreas would be happy to commit to maintain the Rust side as a
    sub-maintainer (for instance). He would also need to make sure the
    tree builds properly with Rust enabled and so on. He already does
    something similar for Jens. Would that work for you?

    You could take the patches directly with his RoBs or Acked-bys, for
    instance. Or perhaps it makes more sense to take PRs from him (on the
    Rust code only, of course), to save you more work. Andreas does not
    send PRs to anyone yet, but I think it would be a good time for him to
    start learning how to apply patches himself etc.

    If not, then the last fallback would be putting it in the Rust tree as
    a sub-entry or similar.

    I hope that clarifies (and thanks whatever you decide!).

    https://lore.kernel.org/rust-for-linux/CANiq72mpYoig2Ro76K0E-sUtP31fW+0403zYWd6MumCgFKfTDQ@mail.gmail.com/

Cheers,
Miguel

Re: [PATCH] lsm: rust: mark SecurityCtx methods inline

[Alice Ryhl]

On Mon, Mar 3, 2025 at 11:55 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Mon, Mar 3, 2025 at 10:30 AM Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > I'm seeing Binder generating calls to methods on SecurityCtx such as
> > from_secid and drop without inlining. Since these methods are really
> > simple wrappers around C functions, mark the methods to inline to avoid
> > generating these useless small functions.
> >
> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> > ---
> >  rust/kernel/security.rs | 5 +++++
> >  1 file changed, 5 insertions(+)
>
> While this isn't specific to your patch, Casey's comment about "free"
> vs "release" is something to keep in mind when working on the LSM
> bindings; what happens inside the individual LSMs for a given LSM hook
> can vary quite a bit.
>
> I also saw a similar Rust patch where a commenter suggested using
> "impersonal facts" in the commit description and I believe that is a
> good idea here as well.
>
> Beyond those nitpicks, this looks okay to me based on my *extremely*
> limited Rust knowledge.  With the minor requested changes in place,
> would you prefer me to take this via the LSM tree, or would you prefer
> it to go up to Linus via a more Rust-y tree?

It would be great if you could take it, thanks!

Regarding the other patch for "Credential", is your tree also the
correct place for that? It's a bit unclear to me which tree maintains
credentials.

Alice

Re: [PATCH] lsm: rust: mark SecurityCtx methods inline

[Paul Moore]

On Mon, Mar 3, 2025 at 7:04 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
> On Mon, Mar 3, 2025 at 11:55 PM Paul Moore <paul@paul-moore.com> wrote:
> >
> > Beyond those nitpicks, this looks okay to me based on my *extremely*
> > limited Rust knowledge.  With the minor requested changes in place,
> > would you prefer me to take this via the LSM tree, or would you prefer
> > it to go up to Linus via a more Rust-y tree?
>
> In general, if a subsystem is willing to take Rust-related patches
> through their tree, that is the ideal scenario! So please definitely
> feel free to pick it up on your side (and thanks!); otherwise, I can
> pick it up with your Acked-by.
>
> Some days ago I wrote a summary of the usual discussion we have around
> this (copy-pasting here for convenience) ...

Hi Miguel,

Thanks.  Yes, I've seen the summary and the recent threads around Rust
in the Linux kernel.  I don't want to drag all of that up here, but I
will simply say that from the perspective of the LSM framework we're
happy to work with the Rust devs to ensure that the LSM framework is
well supported with Rust bindings.  However, I will add that my own
Rust related efforts are going to be very limited as my understanding
of Rust is still frustratingly low; until that improves I'll be
reliant on others like Alice and you to submit patches for
discussion/acceptance when there are issues.  Thankfully that has
proven to work fairly well over the past few months and I would like
to see that continue.

As far as the mechanics of which tree to merge code, I'll probably
continue to ask in most cases simply so we are all clear on where the
patches will land and how they get up to Linus.  From my perspective
there is no harm in asking, and I *really* want to encourage
cross-subsystem communication as much as I can; I've been seeing an
increasing trend towards compartmentalization across subsystems and I
believe the best way to push back against that is to talk a bit more,
even if it is just a mundane "my tree or yours?".

-- 
paul-moore.com

Re: [PATCH] lsm: rust: mark SecurityCtx methods inline

[Paul Moore]

On Tue, Mar 4, 2025 at 5:37 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> Regarding the other patch for "Credential", is your tree also the
> correct place for that? It's a bit unclear to me which tree maintains
> credentials.

Unfortunately, the cred code seems to be a bit of an orphan with no
clear mailing list or maintainer listed in the MAINTAINERS file.  I've
merged cred related patches in the past that have been rotting on the
mailing lists, and Linus has accepted them; I can do the same here.

I'll probably also toss out a RFC patch to volunteer for the cred
maintainer job, if nothing else having an explicit cred mainainter
entry should help clarify things for people who want to submit code
and don't know where.

-- 
paul-moore.com

Re: [PATCH] lsm: rust: mark SecurityCtx methods inline

[Miguel Ojeda]

On Tue, Mar 4, 2025 at 10:57 PM Paul Moore <paul@paul-moore.com> wrote:
>
> Thanks.  Yes, I've seen the summary and the recent threads around Rust
> in the Linux kernel.  I don't want to drag all of that up here, but I
> will simply say that from the perspective of the LSM framework we're
> happy to work with the Rust devs to ensure that the LSM framework is
> well supported with Rust bindings.  However, I will add that my own
> Rust related efforts are going to be very limited as my understanding
> of Rust is still frustratingly low; until that improves I'll be
> reliant on others like Alice and you to submit patches for
> discussion/acceptance when there are issues.  Thankfully that has
> proven to work fairly well over the past few months and I would like
> to see that continue.
>
> As far as the mechanics of which tree to merge code, I'll probably
> continue to ask in most cases simply so we are all clear on where the
> patches will land and how they get up to Linus.  From my perspective
> there is no harm in asking, and I *really* want to encourage
> cross-subsystem communication as much as I can; I've been seeing an
> increasing trend towards compartmentalization across subsystems and I
> believe the best way to push back against that is to talk a bit more,
> even if it is just a mundane "my tree or yours?".

Definitely agreed on talking more and encouraging more communication,
so if deciding based on a case-by-case is best for you, I think that
is good.

And thanks for helping us here in whatever way you can!

Cheers,
Miguel