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: <20211011190204.123934-1-dave@stgolabs.net>
In-Reply-To: <20211011190204.123934-1-dave@stgolabs.net>
From:   Paul Moore <paul@paul-moore.com>
Date:   Mon, 11 Oct 2021 19:13:34 -0400
Message-ID: <CAHC9VhQngdvJCDWtguSH8EXwYhgeQwG8Zbp5jA3bheNVCZaigA@mail.gmail.com>
Subject: Re: [PATCH] selinux: consilidate comments from inode_doinit_with_dentry
 wrt !dentry
To:     Davidlohr Bueso <dave@stgolabs.net>
Cc:     Stephen Smalley <stephen.smalley.work@gmail.com>,
        selinux@vger.kernel.org, linux-kernel@vger.kernel.org,
        Davidlohr Bueso <dbueso@suse.de>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Oct 11, 2021 at 3:02 PM Davidlohr Bueso <dave@stgolabs.net> wrote:
>
> Instead of having the same big comment twice for the same scenario,
> just have it in out_invalid.
>
> Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
> ---
>  security/selinux/hooks.c | 30 ++++++++++--------------------
>  1 file changed, 10 insertions(+), 20 deletions(-)

Hi Davidlohr,

Thanks for the patch, but I think the comment is a bit more
discoverable/helpful when it is integrated into the main flow of the
code and not the error handling at the bottom.  However, if you wanted
to change the second instance to refer to the prior instance I think
that would be okay.

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index e7ebd45ca345..0e0013cc7ba8 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1502,18 +1502,8 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
>                         if (!dentry)
>                                 dentry = d_find_any_alias(inode);
>                 }
> -               if (!dentry) {
> -                       /*
> -                        * this is can be hit on boot when a file is accessed
> -                        * before the policy is loaded.  When we load policy we
> -                        * may find inodes that have no dentry on the
> -                        * sbsec->isec_head list.  No reason to complain as these
> -                        * will get fixed up the next time we go through
> -                        * inode_doinit with a dentry, before these inodes could
> -                        * be used again by userspace.
> -                        */
> +               if (!dentry)
>                         goto out_invalid;
> -               }
>
>                 rc = inode_doinit_use_xattr(inode, dentry, sbsec->def_sid,
>                                             &sid);
> @@ -1559,15 +1549,6 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
>                                 if (!dentry)
>                                         dentry = d_find_any_alias(inode);
>                         }
> -                       /*
> -                        * This can be hit on boot when a file is accessed
> -                        * before the policy is loaded.  When we load policy we
> -                        * may find inodes that have no dentry on the
> -                        * sbsec->isec_head list.  No reason to complain as
> -                        * these will get fixed up the next time we go through
> -                        * inode_doinit() with a dentry, before these inodes
> -                        * could be used again by userspace.
> -                        */
>                         if (!dentry)
>                                 goto out_invalid;
>                         rc = selinux_genfs_get_sid(dentry, sclass,
> @@ -1607,6 +1588,15 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
>         return rc;
>
>  out_invalid:
> +       /*
> +        * This is can be hit on boot when a file is accessed
> +        * before the policy is loaded.  When we load policy we
> +        * may find inodes that have no dentry on the
> +        * sbsec->isec_head list.  No reason to complain as these
> +        * will get fixed up the next time we go through
> +        * inode_doinit with a dentry, before these inodes could
> +        * be used again by userspace.
> +        */
>         spin_lock(&isec->lock);
>         if (isec->initialized == LABEL_PENDING) {
>                 isec->initialized = LABEL_INVALID;
> --
> 2.26.2

-- 
paul moore
www.paul-moore.com