On Wed, 2012-04-18 at 19:39 +0100, Al Viro wrote: > On Wed, Apr 18, 2012 at 02:07:52PM -0400, Mimi Zohar wrote: > > > >From the 'ima: defer calling __fput()' patch description: > > > > ima_file_free(), which is called on __fput(), updates the file data > > hash stored as an extended attribute to reflect file changes. If a > > file is closed before it is munmapped, __fput() is called with the > > mmap_sem taken. With IMA-appraisal enabled, this results in an > > mmap_sem/i_mutex lockdep. ima_defer_fput() increments the f_count to > > defer the __fput() being called until after the mmap_sem is released. > > > > The number of __fput() calls needing to be deferred is minimal. Only > > those files in policy, that were closed prior to the munmap and were > > mmapped write, need to defer the __fput(). > > > > With this patch, on a clean F16 install, from boot to login, only > > 5 out of ~100,000 mmap_sem held fput() calls were deferred. > > Assuming that it's commit 3cee52ffe8ca925bb1e96f804daa87f7e2e34e46 > Author: Mimi Zohar <zohar@xxxxxxxxxxxxxxxxxx> > Date: Fri Feb 24 06:23:12 2012 -0500 > > ima: defer calling __fput() > in your tree, the NAK still stands. For starters, but you are creating a > different locking rules for IMA-enabled builds and for everything else. > Moreover, this deferral is done only for files opened for write; the > rules are convoluted as hell *and* inviting abuses. Yes, that is the updated version. For performance, we limited deferring the __fput() to only those files that could possibly change - open for write, were closed before being munmapped, and that IMA-appraisal maintains a file data hash as an xattr. If the main concern is different locking rules when IMA is enabled, then we could remove the IMA criteria and rename ima_defer_fput() to something more generic. As for "*and* inviting abuses", I'm not sure what you mean. > NAKed at least until you come up with formal proof that there's no other > lock where fput() would be possible and ->i_mutex was not allowed. This > is not a way to go; that kind of kludges leads to locking code that is > impossible to reason about. On __fput(), we need to update the security.ima xattr with a hash of the file data. The original thread discussion suggested changing the xattr locking. The filesystems seem to do their own xattr locking, but in fs/xattr.c the i_mutex is taken before accessing the inode setxattr/removexattr ops. hm, lockdep isn't complaining about anything else. Not sure if that qualifies as formal proof. > PS: BTW, what the hell is "fput already scheduled" codepath about? > Why is it pr_info() and not an outright BUG_ON()? I'll fix this. thanks, Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
