Re: [PATCH v8 07/19] ima: Move dentry into ima_namespace and others onto stack

[Mimi Zohar]

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@xxxxxxxxxxxxx>
> >>
> >> 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@xxxxxxxxxxxxx>
> >> ---
> >>   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;
> >>