[RFC] SELINUX: Remove obsolete deferred inode security init list.

Commit Message

Konstantin Meskhidze (A) Nov. 14, 2022, 11:18 a.m. UTC

From: Alexander Kozhevnikov <[e-mail]>

This patch is a proposed code optimization for SELinux:

1) Each inode has SELinux security structure attached
   to it, this one need to be initialized at some point.
2) This initialization is done by the function
   inode_doinit_with_dentry ( ).
3) In the kernel releases started from some point in the past
   this function (2) is always called normally from function
   __inode_security_revalidate ( ).
4) Which in turn is always called  from inode_security ( ), which
   is a base point for any selinux calls and always called on
   any access to any inode except a few special cases when
   _inode_security_novalidate ( ) is used.
5) Inode security structure initialization can be done only after
   SELinux is fully initialized and policy is loaded.
6) So, for this purpose there was a special defeferred inode security
   initialization list protected by a spinlock implemented, which was
   populated instead of isec initialization in function
   inode_doinit_with_dentry ( ), if it was called before SELinux full
   initialization, and processed at the time when SELinux policy load
   occurred by calling again inode_doinit_with_dentry ( ) on each inode
   in this list.
7) This list was a part of a default initialization logic before (3) was
   implemented, but now, taking into account new mechanism implemented
   with current approach of inode security revalidation on each access
   (4)-(3)-(2), it looks obsolete and not needed anymore.
8) So deferred initialization, this list and code associated with it can
   be safely removed now, as anyway, if inode isec was not initialized
   before it will be processed on any next inode access.
9) There are two possible positive consequences from this removal:
     a. More clean and simple code, less memory consumption;
     b. This deferred initialization in some cases (for example SELinux
        was switched on manually after system was up quite a long time)
        could take some significant time to process, i.e. system looks
        hung for some notable time. And now this is avoided.

Signed-off-by: Alexander Kozhevnikov <[e-mail]>
Signed-off-by: Konstantin Meskhidze <[e-mail]>
---
 security/selinux/hooks.c          | 70 ++++---------------------------
 security/selinux/include/objsec.h |  3 --
 2 files changed, 7 insertions(+), 66 deletions(-)

Comments

Paul Moore Nov. 14, 2022, 5:45 p.m. UTC | #1

On Mon, Nov 14, 2022 at 6:19 AM Konstantin Meskhidze
<[e-mail]> wrote:
> From: Alexander Kozhevnikov <[e-mail]>
>
> This patch is a proposed code optimization for SELinux:
>
> 1) Each inode has SELinux security structure attached
>    to it, this one need to be initialized at some point.
> 2) This initialization is done by the function
>    inode_doinit_with_dentry ( ).
> 3) In the kernel releases started from some point in the past
>    this function (2) is always called normally from function
>    __inode_security_revalidate ( ).
> 4) Which in turn is always called  from inode_security ( ), which
>    is a base point for any selinux calls and always called on
>    any access to any inode except a few special cases when
>    _inode_security_novalidate ( ) is used.
> 5) Inode security structure initialization can be done only after
>    SELinux is fully initialized and policy is loaded.
> 6) So, for this purpose there was a special defeferred inode security
>    initialization list protected by a spinlock implemented, which was
>    populated instead of isec initialization in function
>    inode_doinit_with_dentry ( ), if it was called before SELinux full
>    initialization, and processed at the time when SELinux policy load
>    occurred by calling again inode_doinit_with_dentry ( ) on each inode
>    in this list.
> 7) This list was a part of a default initialization logic before (3) was
>    implemented, but now, taking into account new mechanism implemented
>    with current approach of inode security revalidation on each access
>    (4)-(3)-(2), it looks obsolete and not needed anymore.
> 8) So deferred initialization, this list and code associated with it can
>    be safely removed now, as anyway, if inode isec was not initialized
>    before it will be processed on any next inode access.
> 9) There are two possible positive consequences from this removal:
>      a. More clean and simple code, less memory consumption;
>      b. This deferred initialization in some cases (for example SELinux
>         was switched on manually after system was up quite a long time)
>         could take some significant time to process, i.e. system looks
>         hung for some notable time. And now this is avoided.
>
> Signed-off-by: Alexander Kozhevnikov <[e-mail]>
> Signed-off-by: Konstantin Meskhidze <[e-mail]>
> ---
>  security/selinux/hooks.c          | 70 ++++---------------------------
>  security/selinux/include/objsec.h |  3 --
>  2 files changed, 7 insertions(+), 66 deletions(-)

Hi Konstantin, Alexander,

A few comments below, but can you share what testing you've done with
this?  Specifically what you've done to ensure that inodes allocated
before the policy is loaded are properly initialized/validated after
the policy is loaded?

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index f553c370397e..c93b5621d735 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -316,27 +316,7 @@ static struct inode_security_struct *backing_inode_security(struct dentry *dentr
>
>  static void inode_free_security(struct inode *inode)
>  {
> -       struct inode_security_struct *isec = selinux_inode(inode);
> -       struct superblock_security_struct *sbsec;
> -
> -       if (!isec)
> -               return;
> -       sbsec = selinux_superblock(inode->i_sb);
> -       /*
> -        * As not all inode security structures are in a list, we check for
> -        * empty list outside of the lock to make sure that we won't waste
> -        * time taking a lock doing nothing.
> -        *
> -        * The list_del_init() function can be safely called more than once.
> -        * It should not be possible for this function to be called with
> -        * concurrent list_add(), but for better safety against future changes
> -        * in the code, we use list_empty_careful() here.
> -        */
> -       if (!list_empty_careful(&isec->list)) {
> -               spin_lock(&sbsec->isec_lock);
> -               list_del_init(&isec->list);
> -               spin_unlock(&sbsec->isec_lock);
> -       }
> +/* NOTHING TO DO AFTER DEFERRED LIST REMOVAL */
>  }

We should just remove inode_free_security(), as well as
selinux_inode_free_security(), there is no reason to leave them as
empty functions and/or hooks.

> @@ -551,27 +531,6 @@ static int sb_finish_set_opts(struct super_block *sb)
>         /* Initialize the root inode. */
>         rc = inode_doinit_with_dentry(root_inode, root);
>
> -       /* Initialize any other inodes associated with the superblock, e.g.
> -          inodes created prior to initial policy load or inodes created
> -          during get_sb by a pseudo filesystem that directly
> -          populates itself. */
> -       spin_lock(&sbsec->isec_lock);
> -       while (!list_empty(&sbsec->isec_head)) {
> -               struct inode_security_struct *isec =
> -                               list_first_entry(&sbsec->isec_head,
> -                                          struct inode_security_struct, list);
> -               struct inode *inode = isec->inode;
> -               list_del_init(&isec->list);
> -               spin_unlock(&sbsec->isec_lock);
> -               inode = igrab(inode);
> -               if (inode) {
> -                       if (!IS_PRIVATE(inode))
> -                               inode_doinit_with_dentry(inode, NULL);
> -                       iput(inode);
> -               }
> -               spin_lock(&sbsec->isec_lock);
> -       }
> -       spin_unlock(&sbsec->isec_lock);
>         return rc;
>  }

I would suggest ending sb_finish_set_opts() by returning from the
inode_doinit_with_dentry() call, e.g.:

    /* ... */
    return inode_doinit_with_dentry(root_inode, root);
  }

> @@ -1430,9 +1381,7 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
>                 if (!dentry) {
>                         /*
>                          * this is can be hit on boot when a file is accessed
> -                        * before the policy is loaded.  When we load policy we
> -                        * may find inodes that have no dentry on the
> -                        * sbsec->isec_head list.  No reason to complain as these
> +                        * before the policy is loaded. No reason to complain as these
>                          * will get fixed up the next time we go through
>                          * inode_doinit with a dentry, before these inodes could
>                          * be used again by userspace.

There are some typos at the start of this comment that are worth
fixing here since you are updating the comment block, e.g.:

  /*
   * This can be hit on boot when a file is accessed
   * before the policy is loaded ...

Paul Moore Nov. 14, 2022, 6:09 p.m. UTC | #2

On Mon, Nov 14, 2022 at 12:45 PM Paul Moore <[e-mail]> wrote:
>
> On Mon, Nov 14, 2022 at 6:19 AM Konstantin Meskhidze
> <[e-mail]> wrote:
> > From: Alexander Kozhevnikov <[e-mail]>
> >
> > This patch is a proposed code optimization for SELinux:
> >
> > 1) Each inode has SELinux security structure attached
> >    to it, this one need to be initialized at some point.
> > 2) This initialization is done by the function
> >    inode_doinit_with_dentry ( ).
> > 3) In the kernel releases started from some point in the past
> >    this function (2) is always called normally from function
> >    __inode_security_revalidate ( ).
> > 4) Which in turn is always called  from inode_security ( ), which
> >    is a base point for any selinux calls and always called on
> >    any access to any inode except a few special cases when
> >    _inode_security_novalidate ( ) is used.
> > 5) Inode security structure initialization can be done only after
> >    SELinux is fully initialized and policy is loaded.
> > 6) So, for this purpose there was a special defeferred inode security
> >    initialization list protected by a spinlock implemented, which was
> >    populated instead of isec initialization in function
> >    inode_doinit_with_dentry ( ), if it was called before SELinux full
> >    initialization, and processed at the time when SELinux policy load
> >    occurred by calling again inode_doinit_with_dentry ( ) on each inode
> >    in this list.
> > 7) This list was a part of a default initialization logic before (3) was
> >    implemented, but now, taking into account new mechanism implemented
> >    with current approach of inode security revalidation on each access
> >    (4)-(3)-(2), it looks obsolete and not needed anymore.
> > 8) So deferred initialization, this list and code associated with it can
> >    be safely removed now, as anyway, if inode isec was not initialized
> >    before it will be processed on any next inode access.
> > 9) There are two possible positive consequences from this removal:
> >      a. More clean and simple code, less memory consumption;
> >      b. This deferred initialization in some cases (for example SELinux
> >         was switched on manually after system was up quite a long time)
> >         could take some significant time to process, i.e. system looks
> >         hung for some notable time. And now this is avoided.
> >
> > Signed-off-by: Alexander Kozhevnikov <[e-mail]>
> > Signed-off-by: Konstantin Meskhidze <[e-mail]>
> > ---
> >  security/selinux/hooks.c          | 70 ++++---------------------------
> >  security/selinux/include/objsec.h |  3 --
> >  2 files changed, 7 insertions(+), 66 deletions(-)
>
> Hi Konstantin, Alexander,
>
> A few comments below, but can you share what testing you've done with
> this?  Specifically what you've done to ensure that inodes allocated
> before the policy is loaded are properly initialized/validated after
> the policy is loaded?

To be more specific, I'm curious about the cases where
__inode_security_revalidate() is called without the ability to sleep;
in those cases it is not possible to call inode_doinit_with_dentry()
to revalidate the inode's label.  With the current solution that is
not so much of an issue as sb_finish_set_opts() can block, but in your
proposed solution I worry this may be an issue.

--
paul-moore.com

Paul Moore Dec. 9, 2022, 9:06 p.m. UTC | #3

On Thu, Dec 8, 2022 at 7:29 AM Alexander Kozhevnikov
<[e-mail]> wrote:
>      Hi, Paul,
>
> Finally, I hope that I've got answers on all questions which were found
> open on previous attempt:
> 1) RCU accesses. There was a bug in printout code (isec pointers were
> messed up with inode pointers), unfortunately. Now the picture is clear.
> All inodes which were accessed on RCU-walk mode got another access in
> Ref-walk mode right after and got successfully initialized. This is
> exactly as VFS manual suggested.
> 2) Inodes which are left without initialization are coupled with
> directories which are mount points for some other filesystems and
> according to VFS path lookup logic this dentries are substituted by
> root dentries from underlying filesystems by handling mount points
> during link_path_walk(). So this inodes do not have a chance to be
> accessed until this mount points exist. As those generally are special
> filesystems like /sys/fs/cgroup (very good example) there is almost no
> chance for unmount of them.
> The chain of events is quite simple: upper directory created, inode
> added to deferred list, another filesystem mounted to this directory,
> inode is not accessible anymore.
> So, hope that this time I have quite good explanation of the story.

Thanks for the update Alexander.  It sounds like the VFS RCU fallback
is working properly, which should address my worry about revalidating
inodes while in a critical section.

I would suggest updating your patchset based on the previous feedback
and reposting.

Patch

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index f553c370397e..c93b5621d735 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -316,27 +316,7 @@  static struct inode_security_struct *backing_inode_security(struct dentry *dentr
 
 static void inode_free_security(struct inode *inode)
 {
-	struct inode_security_struct *isec = selinux_inode(inode);
-	struct superblock_security_struct *sbsec;
-
-	if (!isec)
-		return;
-	sbsec = selinux_superblock(inode->i_sb);
-	/*
-	 * As not all inode security structures are in a list, we check for
-	 * empty list outside of the lock to make sure that we won't waste
-	 * time taking a lock doing nothing.
-	 *
-	 * The list_del_init() function can be safely called more than once.
-	 * It should not be possible for this function to be called with
-	 * concurrent list_add(), but for better safety against future changes
-	 * in the code, we use list_empty_careful() here.
-	 */
-	if (!list_empty_careful(&isec->list)) {
-		spin_lock(&sbsec->isec_lock);
-		list_del_init(&isec->list);
-		spin_unlock(&sbsec->isec_lock);
-	}
+/* NOTHING TO DO AFTER DEFERRED LIST REMOVAL */
 }
 
 struct selinux_mnt_opts {
@@ -551,27 +531,6 @@  static int sb_finish_set_opts(struct super_block *sb)
 	/* Initialize the root inode. */
 	rc = inode_doinit_with_dentry(root_inode, root);
 
-	/* Initialize any other inodes associated with the superblock, e.g.
-	   inodes created prior to initial policy load or inodes created
-	   during get_sb by a pseudo filesystem that directly
-	   populates itself. */
-	spin_lock(&sbsec->isec_lock);
-	while (!list_empty(&sbsec->isec_head)) {
-		struct inode_security_struct *isec =
-				list_first_entry(&sbsec->isec_head,
-					   struct inode_security_struct, list);
-		struct inode *inode = isec->inode;
-		list_del_init(&isec->list);
-		spin_unlock(&sbsec->isec_lock);
-		inode = igrab(inode);
-		if (inode) {
-			if (!IS_PRIVATE(inode))
-				inode_doinit_with_dentry(inode, NULL);
-			iput(inode);
-		}
-		spin_lock(&sbsec->isec_lock);
-	}
-	spin_unlock(&sbsec->isec_lock);
 	return rc;
 }
 
@@ -1378,6 +1337,10 @@  static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
 	if (isec->initialized == LABEL_INITIALIZED)
 		return 0;
 
+	sbsec = selinux_superblock(inode->i_sb);
+	if (!(sbsec->flags & SE_SBINITIALIZED))
+		return 0;
+
 	spin_lock(&isec->lock);
 	if (isec->initialized == LABEL_INITIALIZED)
 		goto out_unlock;
@@ -1385,18 +1348,6 @@  static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
 	if (isec->sclass == SECCLASS_FILE)
 		isec->sclass = inode_mode_to_security_class(inode->i_mode);
 
-	sbsec = selinux_superblock(inode->i_sb);
-	if (!(sbsec->flags & SE_SBINITIALIZED)) {
-		/* Defer initialization until selinux_complete_init,
-		   after the initial policy is loaded and the security
-		   server is ready to handle calls. */
-		spin_lock(&sbsec->isec_lock);
-		if (list_empty(&isec->list))
-			list_add(&isec->list, &sbsec->isec_head);
-		spin_unlock(&sbsec->isec_lock);
-		goto out_unlock;
-	}
-
 	sclass = isec->sclass;
 	task_sid = isec->task_sid;
 	sid = isec->sid;
@@ -1430,9 +1381,7 @@  static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
 		if (!dentry) {
 			/*
 			 * this is can be hit on boot when a file is accessed
-			 * before the policy is loaded.  When we load policy we
-			 * may find inodes that have no dentry on the
-			 * sbsec->isec_head list.  No reason to complain as these
+			 * before the policy is loaded. No reason to complain as these
 			 * will get fixed up the next time we go through
 			 * inode_doinit with a dentry, before these inodes could
 			 * be used again by userspace.
@@ -1486,9 +1435,7 @@  static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
 			}
 			/*
 			 * This can be hit on boot when a file is accessed
-			 * before the policy is loaded.  When we load policy we
-			 * may find inodes that have no dentry on the
-			 * sbsec->isec_head list.  No reason to complain as
+			 * before the policy is loaded. No reason to complain as
 			 * these will get fixed up the next time we go through
 			 * inode_doinit() with a dentry, before these inodes
 			 * could be used again by userspace.
@@ -2543,8 +2490,6 @@  static int selinux_sb_alloc_security(struct super_block *sb)
 	struct superblock_security_struct *sbsec = selinux_superblock(sb);
 
 	mutex_init(&sbsec->lock);
-	INIT_LIST_HEAD(&sbsec->isec_head);
-	spin_lock_init(&sbsec->isec_lock);
 	sbsec->sid = SECINITSID_UNLABELED;
 	sbsec->def_sid = SECINITSID_FILE;
 	sbsec->mntpoint_sid = SECINITSID_UNLABELED;
@@ -2808,7 +2753,6 @@  static int selinux_inode_alloc_security(struct inode *inode)
 	u32 sid = current_sid();
 
 	spin_lock_init(&isec->lock);
-	INIT_LIST_HEAD(&isec->list);
 	isec->inode = inode;
 	isec->sid = SECINITSID_UNLABELED;
 	isec->sclass = SECCLASS_FILE;
diff --git a/security/selinux/include/objsec.h b/security/selinux/include/objsec.h
index 2953132408bf..58f752af38cf 100644
--- a/security/selinux/include/objsec.h
+++ b/security/selinux/include/objsec.h
@@ -45,7 +45,6 @@  enum label_initialized {
 
 struct inode_security_struct {
 	struct inode *inode;	/* back pointer to inode object */
-	struct list_head list;	/* list of inode_security_struct */
 	u32 task_sid;		/* SID of creating task */
 	u32 sid;		/* SID of this object */
 	u16 sclass;		/* security class of this object */
@@ -67,8 +66,6 @@  struct superblock_security_struct {
 	unsigned short behavior;	/* labeling behavior */
 	unsigned short flags;		/* which mount options were specified */
 	struct mutex lock;
-	struct list_head isec_head;
-	spinlock_t isec_lock;
 };
 
 struct msg_security_struct {