[PATCH] selinux: remove duplicated LABEL_INITIALIZED check routine

[PATCH] selinux: remove duplicated LABEL_INITIALIZED check routine

[Austin Kim]

The 'isec->initialized == LABEL_INITIALIZED' is checked twice in a row,
since selinux was mainlined from Linux-2.6.12-rc2.

Since 'isec->initialized' is protected using spin_lock(&isec->lock)
within various APIs, it had better remove first exceptional routine.

With this commit, the code look simpler, easier to read and maintain.

Signed-off-by: Austin Kim <austindh.kim@gmail.com>
---
 security/selinux/hooks.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index fc6a3ab7e179..a236b0a33665 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1448,9 +1448,6 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
 	struct dentry *dentry;
 	int rc = 0;
 
-	if (isec->initialized == LABEL_INITIALIZED)
-		return 0;
-
 	spin_lock(&isec->lock);
 	if (isec->initialized == LABEL_INITIALIZED)
 		goto out_unlock;
-- 
2.20.1

Re: [PATCH] selinux: remove duplicated LABEL_INITIALIZED check routine

[Paul Moore]

On Wed, Jun 2, 2021 at 1:48 AM Austin Kim <austindh.kim@gmail.com> wrote:
>
> The 'isec->initialized == LABEL_INITIALIZED' is checked twice in a row,
> since selinux was mainlined from Linux-2.6.12-rc2.
>
> Since 'isec->initialized' is protected using spin_lock(&isec->lock)
> within various APIs, it had better remove first exceptional routine.
>
> With this commit, the code look simpler, easier to read and maintain.
>
> Signed-off-by: Austin Kim <austindh.kim@gmail.com>
> ---
>  security/selinux/hooks.c | 3 ---
>  1 file changed, 3 deletions(-)

This is a common pattern when dealing with lock protected variables:
first check the variable before taking the lock (fast path) and if
necessary take the lock and re-check the variable when we know we have
exclusive access.

In the majority of cases the SELinux inode initialization value goes
from LABEL_INVALID to LABEL_INITIALIZED and stays there; while there
is an invalidation function/hook that is used by some
network/distributed filesystems, it isn't a common case to the best of
my knowledge.  With that understanding it makes perfect sense to do a
quick check to first see if the inode is initialized in
inode_doinit_with_dentry() and return quickly, without taking a lock,
if it is already initialized.  In the case where the inode has not
been previously initialized, or has been invalidated, we take the
spinlock to guarantee we are not racing with another task and re-check
the initialization value to ensure that another task hasn't
initialized the inode and act accordingly.

The existing code is correct.

-- 
paul moore
www.paul-moore.com

Re: [PATCH] selinux: remove duplicated LABEL_INITIALIZED check routine

[Austin Kim]

2021년 6월 2일 (수) 오후 11:32, Paul Moore <paul@paul-moore.com>님이 작성:
>
> On Wed, Jun 2, 2021 at 1:48 AM Austin Kim <austindh.kim@gmail.com> wrote:
> >
> > The 'isec->initialized == LABEL_INITIALIZED' is checked twice in a row,
> > since selinux was mainlined from Linux-2.6.12-rc2.
> >
> > Since 'isec->initialized' is protected using spin_lock(&isec->lock)
> > within various APIs, it had better remove first exceptional routine.
> >
> > With this commit, the code look simpler, easier to read and maintain.
> >
> > Signed-off-by: Austin Kim <austindh.kim@gmail.com>
> > ---
> >  security/selinux/hooks.c | 3 ---
> >  1 file changed, 3 deletions(-)
>
> This is a common pattern when dealing with lock protected variables:
> first check the variable before taking the lock (fast path) and if
> necessary take the lock and re-check the variable when we know we have
> exclusive access.
>
> In the majority of cases the SELinux inode initialization value goes
> from LABEL_INVALID to LABEL_INITIALIZED and stays there; while there
> is an invalidation function/hook that is used by some
> network/distributed filesystems, it isn't a common case to the best of
> my knowledge.  With that understanding it makes perfect sense to do a
> quick check to first see if the inode is initialized in
> inode_doinit_with_dentry() and return quickly, without taking a lock,
> if it is already initialized.  In the case where the inode has not
> been previously initialized, or has been invalidated, we take the
> spinlock to guarantee we are not racing with another task and re-check
> the initialization value to ensure that another task hasn't
> initialized the inode and act accordingly.
>
> The existing code is correct.
>

Understood, after looking into all routines related to 'isec->initialized' again
where 'isec->initialized' statement is not always protected
spin_lock(&isec->lock) during initialization progress.

Thanks for valuable feedback.

> --
> paul moore
> www.paul-moore.com