Message ID | 20250303-inline-securityctx-v1-1-fb7b9b641fdf@google.com (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | Paul Moore |
Headers | show |
Series | lsm: rust: mark SecurityCtx methods inline | expand |
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,
"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 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 >
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>
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 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 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?".
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.
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
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'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(+) --- base-commit: a64dcfb451e254085a7daee5fe51bf22959d52d3 change-id: 20250303-inline-securityctx-6fc1ca669156 Best regards,