On Sun, 2017-01-08 at 10:57 -0800, James Bottomley wrote: > On Sun, 2017-01-08 at 19:18 +0100, Christoph Hellwig wrote: > > On Sun, Jan 08, 2017 at 09:59:25AM -0800, James Bottomley wrote: > > > Hey, that's not really true: the inode lock (i_rwsem) is used in > > > all sorts of generic places, including generic_file_write_iter(). > > > That's, I think, why ima is using it to try to prevent writes > > > while it measures the file. > > > > But all these are _below_ file_operations. The only place where take > > them in the VFS is for namespace locking, e.g. before calling into > > inode_operations (to generalize a little). > > Definitely agree we need an abstraction with defined semantics. > > > > > So the answer here is that ima needs to stop playing with > > > > i_rwsem. > > > > > > Isn't there a happy medium? most sensible filesystems will allow > > > shared reading (unless they want to tank performance) so we can > > > rely on the fact that even if a fs does use i_rwsem internally on > > > the read path, it will have to be shared. > > > > At least for direct I/O that doesn't always have to be true. > > I'm unsure about the DIO case, so lets try defining the semantics and > see if they're implementable for DIO, otherwise simply exclude it. > > > > So simply replacing the inode_lock() in ima > > > with inode_lock_shared() should do what ima wants and not interact > > > badly even if the underlying FS uses i_rwsem. If there's ever a FS > > > that takes it exclusively in the read path, ima can simply > > > blacklist > > > it. > > > > IFF we actually allow recursive readers for rw_semaphores this would > > work around the issue (but I'm not sure about that fact, at least > > in the past we didn't). It won't fix IMA for all the file systems > > use other synchronization for reads, e.g. the cluster locks in ocfs2 > > or gfs2. It won't fix NFS which will exhibit exacly the same issue > > as Mimi reported. > > > > Last but not least it won't solve the problem that IMA has never been > > designed and does neither document the requires it has from a file > > system, nor is there any systematic testing for it. It will keep on > > breaking because it has all kinds of weird implicit assumptions never > > written down or verified, and the test coverage for it is basically > > non-existing. > > OK, so how about we define it. I think we need two vfs calls: > > inode_block_local_writes(inode) > inode_unblock_local_writes(inode) > > With semantics that between these two, all write attempts to the file > backed by the inode on this system block but reads of the underlying > file are allowed (I added local so we don't have to implement for > remote filesystems). inode_block_local_writes() will block until all > local writes to the file have finished, so you're guaranteed the file > only allows reads when it succeeds. > > As for implementation in the vfs, I suspect an outstanding write count > in the inode might be the better way? As a reference point, what you're suggesting is similar to the current locks that prevent writing to an executable, while it is being executed (eg. bprm). Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
