On Tue, 2023-09-12 at 12:19 -0400, Stefan Berger wrote:
> On 9/4/23 09:40, Roberto Sassu wrote:
> > From: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> >
> > Before the security field of kernel objects could be shared among LSMs with
> > the LSM stacking feature, IMA and EVM had to rely on an alternative storage
> > of inode metadata. The association between inode metadata and inode is
> > maintained through an rbtree.
> >
> > With the reservation mechanism offered by the LSM infrastructure, the
> > rbtree is no longer necessary, as each LSM could reserve a space in the
> > security blob for each inode. Thus, request from the 'integrity' LSM a
> > space in the security blob for the pointer of inode metadata
> > (integrity_iint_cache structure).
> >
> > Prefer this to allocating the integrity_iint_cache structure directly, as
> > IMA would require it only for a subset of inodes. Always allocating it
> > would cause a waste of memory.
> >
> > Introduce two primitives for getting and setting the pointer of
> > integrity_iint_cache in the security blob, respectively
> > integrity_inode_get_iint() and integrity_inode_set_iint(). This would make
> > the code more understandable, as they directly replace rbtree operations.
> >
> > Locking is not needed, as access to inode metadata is not shared, it is per
> > inode.
> >
> > Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> > Reviewed-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
> > ---
> >
> > @@ -145,10 +91,8 @@ static void integrity_inode_free(struct inode *inode)
> > if (!IS_IMA(inode))
> > return;
>
> I think you can remove this check !IS_IMA() as well since the next
> function called here integrity_iint_find() already has this check:
>
> struct integrity_iint_cache *integrity_iint_find(struct inode *inode)
> {
> if (!IS_IMA(inode))
> return NULL;
>
> return integrity_inode_get_iint(inode);
> }
I agree, thanks!
Roberto
> >
> > - write_lock(&integrity_iint_lock);
> > - iint = __integrity_iint_find(inode);
> > - rb_erase(&iint->rb_node, &integrity_iint_tree);
> > - write_unlock(&integrity_iint_lock);
> > + iint = integrity_iint_find(inode); <--------------
> > + integrity_inode_set_iint(inode, NULL);
> >
> > iint_free(iint);
> > }
