rust/kernel/security.rs | 5 +++++ 1 file changed, 5 insertions(+)
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>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
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
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
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
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
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
"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
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,
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
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
>
© 2016 - 2026 Red Hat, Inc.