On Tue, 2023-09-12 at 12:19 -0400, Stefan Berger wrote: > On 9/4/23 09:40, Roberto Sassu wrote: > > From: Roberto Sassu <[email protected]> > > > > 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 <[email protected]> > > Reviewed-by: Casey Schaufler <[email protected]> > > --- > > > > @@ -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); > > }
