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] [day] [month] [year] [list]

Date:   Thu, 3 Jun 2021 08:41:27 +0900
From:   Austin Kim <austindh.kim@...il.com>
To:     Paul Moore <paul@...l-moore.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

2021년 6월 2일 (수) 오후 11:32, Paul Moore <paul@...l-moore.com>님이 작성:
>
> 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.
>

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