All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] selinux: remove duplicated LABEL_INITIALIZED check routine
@ 2021-06-02  5:48 Austin Kim
  2021-06-02 14:32 ` Paul Moore
  0 siblings, 1 reply; 3+ messages in thread
From: Austin Kim @ 2021-06-02  5:48 UTC (permalink / raw)
  To: paul, stephen.smalley.work, eparis
  Cc: selinux, linux-kernel, austin.kim, kernel-team

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


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH] selinux: remove duplicated LABEL_INITIALIZED check routine
  2021-06-02  5:48 [PATCH] selinux: remove duplicated LABEL_INITIALIZED check routine Austin Kim
@ 2021-06-02 14:32 ` Paul Moore
  2021-06-02 23:41   ` Austin Kim
  0 siblings, 1 reply; 3+ messages in thread
From: Paul Moore @ 2021-06-02 14:32 UTC (permalink / raw)
  To: Austin Kim
  Cc: Stephen Smalley, Eric Paris, selinux, linux-kernel, austin.kim,
	kernel-team

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] selinux: remove duplicated LABEL_INITIALIZED check routine
  2021-06-02 14:32 ` Paul Moore
@ 2021-06-02 23:41   ` Austin Kim
  0 siblings, 0 replies; 3+ messages in thread
From: Austin Kim @ 2021-06-02 23:41 UTC (permalink / raw)
  To: Paul Moore
  Cc: Stephen Smalley, Eric Paris, selinux, linux-kernel,
	김동현,
	kernel-team

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-06-02 23:41 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-02  5:48 [PATCH] selinux: remove duplicated LABEL_INITIALIZED check routine Austin Kim
2021-06-02 14:32 ` Paul Moore
2021-06-02 23:41   ` Austin Kim

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.