Re: [PATCH v2 1/7] fs: ima: Remove S_IMA and IS_IMA()

[Christian Brauner <brauner@kernel.org>]

On Thu, Nov 28, 2024 at 11:06:14AM +0100, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.com>
> 
> Commit 196f518128d2e ("IMA: explicit IMA i_flag to remove global lock on
> inode_delete") introduced the new S_IMA inode flag to determine whether or
> not an inode was processed by IMA. In that way, it was not necessary to
> take the global lock on inode delete.
> 
> Since commit 4de2f084fbff ("ima: Make it independent from 'integrity'
> LSM"), the pointer of the inode integrity metadata managed by IMA has been
> moved to the inode security blob, from the rb-tree. The pointer is not NULL
> only if the inode has been processed by IMA, i.e. ima_inode_get() has been
> called for that inode.
> 
> Thus, since the IS_IMA() check can be now implemented by trivially testing
> whether or not the pointer of inode integrity metadata is NULL, remove the
> S_IMA definition in include/linux/fs.h and also the IS_IMA() macro.
> 
> Remove also the IS_IMA() invocation in ima_rdwr_violation_check(), since
> whether the inode was processed by IMA will be anyway detected by a
> subsequent call to ima_iint_find(). It does not have an additional overhead
> since the decision can be made in constant time, as opposed to logarithm
> when the inode integrity metadata was stored in the rb-tree.
> 
> Suggested-by: Shu Han <ebpqwerty472123@gmail.com>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  include/linux/fs.h                | 3 +--
>  security/integrity/ima/ima_iint.c | 5 -----
>  security/integrity/ima/ima_main.c | 2 +-
>  3 files changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 3559446279c1..b33363becbdd 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2261,7 +2261,7 @@ struct super_operations {
>  #define S_NOCMTIME	(1 << 7)  /* Do not update file c/mtime */
>  #define S_SWAPFILE	(1 << 8)  /* Do not truncate: swapon got its bmaps */
>  #define S_PRIVATE	(1 << 9)  /* Inode is fs-internal */
> -#define S_IMA		(1 << 10) /* Inode has an associated IMA struct */
> +/* #define S_IMA	(1 << 10) Inode has an associated IMA struct (unused) */

As Jan said, that line should be deleted. Otherwise,
Reviewed-by: Christian Brauner <brauner@kernel.org>