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 <[email protected]>
---
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
On Wed, Jun 2, 2021 at 1:48 AM Austin Kim <[email protected]> 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 <[email protected]>
> ---
> 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
http://www.paul-moore.com
2021년 6월 2일 (수) 오후 11:32, Paul Moore <[email protected]>님이 작성:
>
> On Wed, Jun 2, 2021 at 1:48 AM Austin Kim <[email protected]> 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 <[email protected]>
> > ---
> > 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
> http://www.paul-moore.com