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