linux-kernel - Re: [PATCH] selinux: remove duplicated LABEL

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHC9VhQdAt2EQqP3pQM=5TifTYuXxnU1QOvOT-aFaDaGiLLJXQ@mail.gmail.com>
Date:   Wed, 2 Jun 2021 10:32:06 -0400
From:   Paul Moore <paul@...l-moore.com>
To:     Austin Kim <austindh.kim@...il.com>
Cc:     Stephen Smalley <stephen.smalley.work@...il.com>,
        Eric Paris <eparis@...isplace.org>, selinux@...r.kernel.org,
        linux-kernel@...r.kernel.org, austin.kim@....com,
        kernel-team@....com
Subject: Re: [PATCH] selinux: remove duplicated LABEL_INITIALIZED check routine

On Wed, Jun 2, 2021 at 1:48 AM Austin Kim <austindh.kim@...il.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@...il.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