On Mon, Nov 14, 2022 at 6:19 AM Konstantin Meskhidze
<konstantin.meskhidze@xxxxxxxxxx> wrote:
> From: Alexander Kozhevnikov <alexander.kozhevnikov@xxxxxxxxxxxxxxxxxxx>
>
> 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 <alexander.kozhevnikov@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@xxxxxxxxxx>
> ---
> 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.com
