Messages in this thread | | | Date | Mon, 17 Apr 2023 08:45:40 -0400 | Subject | Re: [PATCH] overlayfs: Trigger file re-evaluation by IMA / EVM after writes | From | Stefan Berger <> |
| |
On 4/17/23 06:05, Jeff Layton wrote: > On Sun, 2023-04-16 at 21:57 -0400, Stefan Berger wrote: >> >> On 4/7/23 09:29, Jeff Layton wrote:
>>> >>> Note that there Stephen is correct that calling getattr is probably >>> going to be less efficient here since we're going to end up calling >>> generic_fillattr unnecessarily, but I still think it's the right thing >>> to do. >> >> I was wondering whether to use the existing inode_eq_iversion() for all >> other filesystems than overlayfs, nfs, and possibly other ones (which ones?) >> where we would use the vfs_getattr_nosec() via a case on inode->i_sb->s_magic? >> If so, would this function be generic enough to be a public function for libfs.c? >> >> I'll hopefully be able to test the proposed patch tomorrow. >> >> > > No, you don't want to use inode_eq_iversion here because (as the comment > over it says):
In the ima_check_last_writer() case the usage of inode_eq_iversion() was correct since at this point no record of its value was made and therefore no writer needed to change the i_value again due to IMA:
update = test_and_clear_bit(IMA_UPDATE_XATTR, &iint->atomic_flags); if (!IS_I_VERSION(inode) || !inode_eq_iversion(inode, iint->version) || (iint->flags & IMA_NEW_FILE)) { iint->flags &= ~(IMA_DONE_MASK | IMA_NEW_FILE); iint->measured_pcrs = 0; if (update) ima_update_xattr(iint, file); }
The record of the value is only made when the actual measurement is done in ima_collect_measurement()
Compared to this the usage of vfs_getattr_nosec() is expensive since it resets the flag.
if ((request_mask & STATX_CHANGE_COOKIE) && IS_I_VERSION(inode)) { stat->result_mask |= STATX_CHANGE_COOKIE; stat->change_cookie = inode_query_iversion(inode); }
idmap = mnt_idmap(path->mnt); if (inode->i_op->getattr) return inode->i_op->getattr(idmap, path, stat, request_mask, query_flags);
Also, many filesystems will have their getattr now called as well.
I understand Christian's argument about the maintenance headache to a certain degree...
Stefan
> > * Note that we don't need to set the QUERIED flag in this case, as the value > * in the inode is not being recorded for later use. > > The IMA code _does_ record the value for later use. Furthermore, it's > not valid to use inode_eq_iversion on a non-IS_I_VERSION inode, so it's > better to just use vfs_getattr_nosec which allows IMA to avoid all of > those gory details. > > Thanks,
| |