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;
Message-ID: <fbdade37017dd836881c5ecd98fae7313de5b5bb.camel@linux.ibm.com>
Subject: Re: [PATCH v8 07/19] ima: Move dentry into ima_namespace and others
 onto stack
From:   Mimi Zohar <zohar@linux.ibm.com>
To:     Stefan Berger <stefanb@linux.ibm.com>,
        Stefan Berger <stefanb@linux.vnet.ibm.com>,
        linux-integrity@vger.kernel.org
Cc:     serge@hallyn.com, christian.brauner@ubuntu.com,
        containers@lists.linux.dev, dmitry.kasatkin@gmail.com,
        ebiederm@xmission.com, krzysztof.struczynski@huawei.com,
        roberto.sassu@huawei.com, mpeters@redhat.com, lhinds@redhat.com,
        lsturman@redhat.com, puiterwi@redhat.com, jejb@linux.ibm.com,
        jamjoom@us.ibm.com, linux-kernel@vger.kernel.org,
        paul@paul-moore.com, rgb@redhat.com,
        linux-security-module@vger.kernel.org, jmorris@namei.org
Date:   Tue, 18 Jan 2022 15:42:14 -0500
In-Reply-To: <d53a66a2-17e2-54b3-f115-efd7c58080a7@linux.ibm.com>
References: <20220104170416.1923685-1-stefanb@linux.vnet.ibm.com>
         <20220104170416.1923685-8-stefanb@linux.vnet.ibm.com>
         <a7c5ac94b4c4d87b407353f74ff87bc0b13542a4.camel@linux.ibm.com>
         <d53a66a2-17e2-54b3-f115-efd7c58080a7@linux.ibm.com>
Content-Type: text/plain; charset="ISO-8859-15"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Tue, 2022-01-18 at 15:12 -0500, Stefan Berger wrote:
> On 1/13/22 15:28, Mimi Zohar wrote:
> > Hi Stefan,
> >
> > Nobody refers to the IMA securityfs files as dentries.  The Subject
> > line is suppose to provide a hint about the patch.  How about changing
> > the "Subject" line to "ima: Move IMA securityfs files into
> > ima_namespaces or onto stack".
> >
> > On Tue, 2022-01-04 at 12:04 -0500, Stefan Berger wrote:
> >> From: Stefan Berger <stefanb@linux.ibm.com>
> >>
> >> Move the policy file dentry into the ima_namespace for reuse by
> >> virtualized SecurityFS and for being able to remove it from
> >> the filesystem. Move the other dentries onto the stack.
> > Missing is an explanation why the other IMA securityfs files can be on
> > the stack.  Maybe start out by saying that the ns_ima_init securityfs
> > files are never deleted.  Then transition into the IMA namespaced
> > securityfs files and how they will be deleted.
> 
> How about this:
> 
> ima: Move IMA securityfs files into ima_namespace or onto stack
> 
> Move the IMA policy file's dentry into the ima_namespace for reuse by
> virtualized securityfs and for being able to remove the file from the
> filesystem using securityfs_remove().

How about "Move the IMA securityfs policy file ..."

> Move the other files' dentries onto the stack since they are not needed

How about "Move the other IMA securityfs files ..."

> outside the function where they are created in. Also, their cleanup is
> automatically handled by the filesystem upon umount of a virtualized
> secruityfs instance, so they don't need to be explicitly freed anymore.
> 
> When moving the dentry 'ima_policy' into ima_namespace rename it to
> 'policy_dentry' to clarify its datatype and avoid a name clash with
> 'int ima_policy' from ima_policy.c.

To prevent namespace pollution, static variables need to be prefixed
(e.g. "ima_").  This is not a concern with variables inside the
ima_namespace structure.  Why not just rename the variable "policy".

Refer to the section on "Naming" in Documentation/process/coding-
style.rst.

thanks,

Mimi

> 
> 
> 
> >> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> >> ---
> >>   security/integrity/ima/ima.h    |  2 ++
> >>   security/integrity/ima/ima_fs.c | 32 ++++++++++++++++++--------------
> >>   2 files changed, 20 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> >> index 82b3f6a98320..224b09617c52 100644
> >> --- a/security/integrity/ima/ima.h
> >> +++ b/security/integrity/ima/ima.h
> >> @@ -140,6 +140,8 @@ struct ima_namespace {
> >>   	struct mutex ima_write_mutex;
> >>   	unsigned long ima_fs_flags;
> >>   	int valid_policy;
> >> +
> >> +	struct dentry *policy_dentry;
> > None of the other securityfs files are renamed.  Why is "ima_policy"
> > being renamed to "policy_dentry"?  If there is a need, it should be
> > documented in the patch description.
> >
> > thanks,
> >
> > Mimi
> >
> >>   } __randomize_layout;
> >>   extern struct ima_namespace init_ima_ns;
> >>