On Thu, Apr 11, 2019 at 4:03 PM Mimi Zohar <zohar@xxxxxxxxxxxxx> wrote:
> > + mutex_lock(&iint->mutex);
> > + if (atomic_read(&inode->i_writecount) == 1) {
>
> This test limits the number of opened writers. Only if there is a
> single writer opened, will the xattr be updated. Is this what you
> intended?
Certainly no, I misunderstood that to be a flag. Will fix.
> Your testing should open the same file for write multiple times.
Indeed. I was just wondering what happened with 'locksettings.db' as
it failed to update. Duh..
> > + clear_bit(IMA_UPDATE_XATTR, &iint->atomic_flags);
> > + if (!IS_I_VERSION(inode) ||
> > + !inode_eq_iversion(inode, iint->version)) {
> > + iint->flags &= ~IMA_COLLECTED;
> > + ima_update_xattr(iint, file);
>
> Relatively recently there were some changes to iversion so that it
> isn't being updated as frequently. Can we use i_version here?
Thanks.
> > +void security_file_sync(struct file *file)
> > +{
> > + ima_file_update(file);
> > +}
> > +
>
> Either this is an LSM hook or it isn't. If it's an LSM hook it needs
> to be similar to the existing hooks. If it's an IMA hook, like
> ima_file_check() or ima_file_free(), then call it directly.
>
> Normally the function name is related to the LSM hook name. For
> example, I would name it ima_file_sync.
Ok. I guess ima hook it is then, doubt any lsm will ever need it?
--
Janne
