Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210602054802.GA984@raspberrypi> <CAHC9VhQdAt2EQqP3pQM=5TifTYuXxnU1QOvOT-aFaDaGiLLJXQ@mail.gmail.com>
In-Reply-To: <CAHC9VhQdAt2EQqP3pQM=5TifTYuXxnU1QOvOT-aFaDaGiLLJXQ@mail.gmail.com>
From:   Austin Kim <austindh.kim@gmail.com>
Date:   Thu, 3 Jun 2021 08:41:27 +0900
Message-ID: <CADLLry7D8OYStDAFsP4dFzS0fwV6bM2__ZEB2py+oca+R5Zuog@mail.gmail.com>
Subject: Re: [PATCH] selinux: remove duplicated LABEL_INITIALIZED check routine
To:     Paul Moore <paul@paul-moore.com>
Cc:     Stephen Smalley <stephen.smalley.work@gmail.com>,
        Eric Paris <eparis@parisplace.org>, selinux@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        =?UTF-8?B?6rmA64+Z7ZiE?= <austin.kim@lge.com>,
        kernel-team@lge.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

2021=EB=85=84 6=EC=9B=94 2=EC=9D=BC (=EC=88=98) =EC=98=A4=ED=9B=84 11:32, P=
aul Moore <paul@paul-moore.com>=EB=8B=98=EC=9D=B4 =EC=9E=91=EC=84=B1:
>
> On Wed, Jun 2, 2021 at 1:48 AM Austin Kim <austindh.kim@gmail.com> wrote:
> >
> > The 'isec->initialized =3D=3D 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