[PATCH 4.9 0/1] selinux: drop super_block backpointer from superblock_security_struct

[PATCH 4.9 0/1] selinux: drop super_block backpointer from superblock_security_struct

[Alexander Grund]

This backports a commit from upstream which I would consider a cleanup
as well as a (minor) performance fix due to less memory being used and
avoiding an unneccessary pointer dereferencing, i.e. the change
from `sbsec->sb->s_root` to `sb->s_root`.

However as it changes the `superblock_security_struct` please check if
this violates any API/ABI stability requirements which I'm not aware of.

Ondrej Mosnacek (1):
  selinux: drop super_block backpointer from superblock_security_struct

 security/selinux/hooks.c          | 5 ++---
 security/selinux/include/objsec.h | 1 -
 2 files changed, 2 insertions(+), 4 deletions(-)

-- 
2.25.1

[PATCH 4.9 1/1] selinux: drop super_block backpointer from superblock_security_struct

[Alexander Grund]

From: Ondrej Mosnacek <omosnace@redhat.com>

commit b159e86b5a2ab826b3a292756072f4cc523675ab upstream.

It appears to have been needed for selinux_complete_init() in the past,
but today it's useless.

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
[AG: Backported to 4.9]
Signed-off-by: Alexander Grund <theflamefire89@gmail.com>
---
 security/selinux/hooks.c          | 5 ++---
 security/selinux/include/objsec.h | 1 -
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index eb503eccbacc8..d446b501334ab 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -390,7 +390,6 @@ static int superblock_alloc_security(struct super_block *sb)
 	mutex_init(&sbsec->lock);
 	INIT_LIST_HEAD(&sbsec->isec_head);
 	spin_lock_init(&sbsec->isec_lock);
-	sbsec->sb = sb;
 	sbsec->sid = SECINITSID_UNLABELED;
 	sbsec->def_sid = SECINITSID_FILE;
 	sbsec->mntpoint_sid = SECINITSID_UNLABELED;
@@ -643,7 +642,7 @@ static int selinux_get_mnt_opts(const struct super_block *sb,
 		opts->mnt_opts_flags[i++] = DEFCONTEXT_MNT;
 	}
 	if (sbsec->flags & ROOTCONTEXT_MNT) {
-		struct dentry *root = sbsec->sb->s_root;
+		struct dentry *root = sb->s_root;
 		struct inode_security_struct *isec = backing_inode_security(root);
 
 		rc = security_sid_to_context(isec->sid, &context, &len);
@@ -699,7 +698,7 @@ static int selinux_set_mnt_opts(struct super_block *sb,
 	int rc = 0, i;
 	struct superblock_security_struct *sbsec = sb->s_security;
 	const char *name = sb->s_type->name;
-	struct dentry *root = sbsec->sb->s_root;
+	struct dentry *root = sb->s_root;
 	struct inode_security_struct *root_isec;
 	u32 fscontext_sid = 0, context_sid = 0, rootcontext_sid = 0;
 	u32 defcontext_sid = 0;
diff --git a/security/selinux/include/objsec.h b/security/selinux/include/objsec.h
index c21e135460a5e..e4ad5fa58c6b0 100644
--- a/security/selinux/include/objsec.h
+++ b/security/selinux/include/objsec.h
@@ -63,7 +63,6 @@ struct file_security_struct {
 };
 
 struct superblock_security_struct {
-	struct super_block *sb;		/* back pointer to sb object */
 	u32 sid;			/* SID of file system superblock */
 	u32 def_sid;			/* default SID for labeling */
 	u32 mntpoint_sid;		/* SECURITY_FS_USE_MNTPOINT context for files */
-- 
2.25.1

Re: [PATCH 4.9 0/1] selinux: drop super_block backpointer from superblock_security_struct

[Greg KH]

On Mon, Aug 08, 2022 at 01:59:21PM +0200, Alexander Grund wrote:
> This backports a commit from upstream which I would consider a cleanup
> as well as a (minor) performance fix due to less memory being used and
> avoiding an unneccessary pointer dereferencing, i.e. the change
> from `sbsec->sb->s_root` to `sb->s_root`.
> 
> However as it changes the `superblock_security_struct` please check if
> this violates any API/ABI stability requirements which I'm not aware of.

There is no internal stable API/ABI in the kernel, including in the
stable releases.

But, we only take patches that actually do something.  This one doesn't
do anything at all, and has no measurable performance or bugfix that I
can determine at all.

So no need for it, sorry,

greg k-h

Re: [PATCH 4.9 0/1] selinux: drop super_block backpointer from superblock_security_struct

[Alexander Grund]

On 08.08.22 15:28, Greg KH wrote:
> But, we only take patches that actually do something.  This one doesn't
> do anything at all, and has no measurable performance or bugfix that I
> can determine at all.

Isn't "doing less" also worth the patch?
I mean this patch removes a superflous pointer of the superblock struct
making the kernel use less memory.
It also saves a code line and operation during init and removes the
(somewhat hidden in syntax) superflous indirect access (and hence memory read)
of a pointer already available (likely even in a register) during get/set_mnt_opts.

Of course the effect here is small but I think cleanups are always good to avoid
a "death by a thousand cuts" scenario, i.e. that even small things help.

But of course you may disagree, just wanted to provide my reasoning especially
as you wrote earlier that deleting code is always good and I thought this
patch fits that.

Thanks,
Alex

Re: [PATCH 4.9 0/1] selinux: drop super_block backpointer from superblock_security_struct

[Greg KH]

On Thu, Aug 11, 2022 at 11:46:09AM +0200, Alexander Grund wrote:
> On 08.08.22 15:28, Greg KH wrote:
> > But, we only take patches that actually do something.  This one doesn't
> > do anything at all, and has no measurable performance or bugfix that I
> > can determine at all.
> 
> Isn't "doing less" also worth the patch?

Not if it doesn't actually fix something that a user sees.

> I mean this patch removes a superflous pointer of the superblock struct
> making the kernel use less memory.
> It also saves a code line and operation during init and removes the
> (somewhat hidden in syntax) superflous indirect access (and hence memory read)
> of a pointer already available (likely even in a register) during get/set_mnt_opts.
> 
> Of course the effect here is small but I think cleanups are always good to avoid
> a "death by a thousand cuts" scenario, i.e. that even small things help.

We do not take "cleanup patches" in stable trees without it being a
requirement for a real fix.  Please read the stable kernel rules
document again for what is actually allowed.

thanks,

greg k-h

Re: [PATCH 4.9 0/1] selinux: drop super_block backpointer from superblock_security_struct

[Greg KH]

On Thu, Aug 11, 2022 at 11:46:09AM +0200, Alexander Grund wrote:
> On 08.08.22 15:28, Greg KH wrote:
> > But, we only take patches that actually do something.  This one doesn't
> > do anything at all, and has no measurable performance or bugfix that I
> > can determine at all.
> 
> Isn't "doing less" also worth the patch?
> I mean this patch removes a superflous pointer of the superblock struct
> making the kernel use less memory.

Also, how much measurable memory does this save?  You did not quantify
it.

Re: [PATCH 4.9 0/1] selinux: drop super_block backpointer from superblock_security_struct

[Alexander Grund]

On 11.08.22 12:21, Greg KH wrote:
> On Thu, Aug 11, 2022 at 11:46:09AM +0200, Alexander Grund wrote:
>> I mean this patch removes a superflous pointer of the superblock struct
>> making the kernel use less memory.
> 
> Also, how much measurable memory does this save?  You did not quantify
> it.

It saves one pointer, i.e. usually 8 Byte, per superblock when using SELinux.

I don't know how many of those superblocks will be allocated in total on usual systems
as I'm not familiar with the details of the filesystems.
However following one callchain leads to

> /*
> * Common helper for pseudo-filesystems (sockfs, pipefs, bdev - stuff that
> * will never be mountable)
> */
> struct dentry *mount_pseudo_xattr(struct file_system_type *fs_type, char *name,

So it seems one superblock even for each pseudo-fs of which there can be many.

A quick experiment [1] on my phone shows about 300 superblocks allocated which means
that the patch saves a bit over 2kB of memory.
So not that much on usual systems but could be much for some embedded systems.

I hope that helps,
Alex

[1] For the experiment I added a debug counter to `superblock_alloc_security`

Re: [PATCH 4.9 0/1] selinux: drop super_block backpointer from superblock_security_struct

[Greg KH]

On Thu, Aug 11, 2022 at 01:04:35PM +0200, Alexander Grund wrote:
> On 11.08.22 12:21, Greg KH wrote:
> > On Thu, Aug 11, 2022 at 11:46:09AM +0200, Alexander Grund wrote:
> >> I mean this patch removes a superflous pointer of the superblock struct
> >> making the kernel use less memory.
> > 
> > Also, how much measurable memory does this save?  You did not quantify
> > it.
> 
> It saves one pointer, i.e. usually 8 Byte, per superblock when using SELinux.
> 
> I don't know how many of those superblocks will be allocated in total on usual systems
> as I'm not familiar with the details of the filesystems.
> However following one callchain leads to
> 
> > /*
> > * Common helper for pseudo-filesystems (sockfs, pipefs, bdev - stuff that
> > * will never be mountable)
> > */
> > struct dentry *mount_pseudo_xattr(struct file_system_type *fs_type, char *name,
> 
> So it seems one superblock even for each pseudo-fs of which there can be many.
> 
> A quick experiment [1] on my phone shows about 300 superblocks allocated which means
> that the patch saves a bit over 2kB of memory.
> So not that much on usual systems but could be much for some embedded systems.

Again, we are not adding "slim the kernel down by an infinitesimal %"
patches to older stable kernels, especially ones that only have a few
more months to live.  Let's stick with real bugfixes please, that's what
matters here.

thanks,

greg k-h