| Message ID | 20241128100621.461743-2-roberto.sassu@huaweicloud.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ima: Remove unnecessary inode locks | expand |
On Thu 28-11-24 11:06:14, 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) */ Well, I guess you can just delete this line. These are internal kernel flags so we can do whatever we want with them. Otherwise the patch looks good. Feel free to add: Acked-by: Jan Kara <jack@suse.cz> Honza > #define S_AUTOMOUNT (1 << 11) /* Automount/referral quasi-directory */ > #define S_NOSEC (1 << 12) /* no suid or xattr security attributes */ > #ifdef CONFIG_FS_DAX > @@ -2319,7 +2319,6 @@ static inline bool sb_rdonly(const struct super_block *sb) { return sb->s_flags > #endif > > #define IS_PRIVATE(inode) ((inode)->i_flags & S_PRIVATE) > -#define IS_IMA(inode) ((inode)->i_flags & S_IMA) > #define IS_AUTOMOUNT(inode) ((inode)->i_flags & S_AUTOMOUNT) > #define IS_NOSEC(inode) ((inode)->i_flags & S_NOSEC) > #define IS_DAX(inode) ((inode)->i_flags & S_DAX) > diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c > index 00b249101f98..9d9fc7a911ad 100644 > --- a/security/integrity/ima/ima_iint.c > +++ b/security/integrity/ima/ima_iint.c > @@ -26,9 +26,6 @@ static struct kmem_cache *ima_iint_cache __ro_after_init; > */ > struct ima_iint_cache *ima_iint_find(struct inode *inode) > { > - if (!IS_IMA(inode)) > - return NULL; > - > return ima_inode_get_iint(inode); > } > > @@ -102,7 +99,6 @@ struct ima_iint_cache *ima_inode_get(struct inode *inode) > > ima_iint_init_always(iint, inode); > > - inode->i_flags |= S_IMA; > ima_inode_set_iint(inode, iint); > > return iint; > @@ -118,7 +114,6 @@ void ima_inode_free_rcu(void *inode_security) > { > struct ima_iint_cache **iint_p = inode_security + ima_blob_sizes.lbs_inode; > > - /* *iint_p should be NULL if !IS_IMA(inode) */ > if (*iint_p) > ima_iint_free(*iint_p); > } > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > index 06132cf47016..cea0afbbc28d 100644 > --- a/security/integrity/ima/ima_main.c > +++ b/security/integrity/ima/ima_main.c > @@ -126,7 +126,7 @@ static void ima_rdwr_violation_check(struct file *file, > bool send_tomtou = false, send_writers = false; > > if (mode & FMODE_WRITE) { > - if (atomic_read(&inode->i_readcount) && IS_IMA(inode)) { > + if (atomic_read(&inode->i_readcount)) { > if (!iint) > iint = ima_iint_find(inode); > /* IMA_MEASURE is set from reader side */ > -- > 2.47.0.118.gfd3785337b >
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>
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) */ #define S_AUTOMOUNT (1 << 11) /* Automount/referral quasi-directory */ #define S_NOSEC (1 << 12) /* no suid or xattr security attributes */ #ifdef CONFIG_FS_DAX @@ -2319,7 +2319,6 @@ static inline bool sb_rdonly(const struct super_block *sb) { return sb->s_flags #endif #define IS_PRIVATE(inode) ((inode)->i_flags & S_PRIVATE) -#define IS_IMA(inode) ((inode)->i_flags & S_IMA) #define IS_AUTOMOUNT(inode) ((inode)->i_flags & S_AUTOMOUNT) #define IS_NOSEC(inode) ((inode)->i_flags & S_NOSEC) #define IS_DAX(inode) ((inode)->i_flags & S_DAX) diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c index 00b249101f98..9d9fc7a911ad 100644 --- a/security/integrity/ima/ima_iint.c +++ b/security/integrity/ima/ima_iint.c @@ -26,9 +26,6 @@ static struct kmem_cache *ima_iint_cache __ro_after_init; */ struct ima_iint_cache *ima_iint_find(struct inode *inode) { - if (!IS_IMA(inode)) - return NULL; - return ima_inode_get_iint(inode); } @@ -102,7 +99,6 @@ struct ima_iint_cache *ima_inode_get(struct inode *inode) ima_iint_init_always(iint, inode); - inode->i_flags |= S_IMA; ima_inode_set_iint(inode, iint); return iint; @@ -118,7 +114,6 @@ void ima_inode_free_rcu(void *inode_security) { struct ima_iint_cache **iint_p = inode_security + ima_blob_sizes.lbs_inode; - /* *iint_p should be NULL if !IS_IMA(inode) */ if (*iint_p) ima_iint_free(*iint_p); } diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 06132cf47016..cea0afbbc28d 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -126,7 +126,7 @@ static void ima_rdwr_violation_check(struct file *file, bool send_tomtou = false, send_writers = false; if (mode & FMODE_WRITE) { - if (atomic_read(&inode->i_readcount) && IS_IMA(inode)) { + if (atomic_read(&inode->i_readcount)) { if (!iint) iint = ima_iint_find(inode); /* IMA_MEASURE is set from reader side */