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

[Stefan Berger <stefanb@linux.ibm.com>]

On 1/18/22 15:42, Mimi Zohar wrote:
> 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".

'policy' is so generic. It can be the internal representation of the policy.


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

Hm, it cannot also be the point to work around the naming just to avoid 
it and come up with ambiguous names...

'policy_dentry' explains much better what it is but following this style 
guide that is then "Hungarian" notation, which is 'asinine.'

What about 'policy_file'?

    Stefan

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