On Sat, Sep 22, 2012 at 1:59 AM, Dave Chinner <david@xxxxxxxxxxxxx> wrote: > On Fri, Sep 21, 2012 at 05:14:28PM +0300, Dmitry Kasatkin wrote: >> On some filesystems calling i_op->fiemap() takes the i_mutex, >> on others it doesn't. > > Exactly by design. Many fiesystems don't require the i_mutex for > fiemap to be safe. e.g. the extent map is not protected by the > i_mutex in XFS or ext4, and so holding the i_mutex over fiemap is > incorrect. > Please read an explanation bellow, where I tell what is EVM. But in my opinion it is a a bit wrong design if caller does not know if any lock will or will not be locked. Linux kernel has many non-locking functions and its locking wrappers. Many callees requires and state that caller must lock something. > As a result, extent maps can change even when someone is holding the > i_mutex. Any design that requires i_mutex to provide stable, > unchanging extent maps is fundamentally broken.... > It is not necessarily that i_mutex to be locked to provide stable unchanging extent maps. FS might not require that. But the caller might require some sort of atomicity. > So that's a NACK from me... > >> This change is needed in preparation for EVM to include a hash of >> the extent data to be used in the HMAC calculation. EVM is called >> when the i_mutex has already been taken. > > What's EVM? What's HMAC? and what is it actually checksumming? The > extent map, or the data in the file? And if it is the extent map, > what's the purpose of using the extent map for this? You need to > explain how this is intended to be used, not use TLAs that people > unfamiliar with your application will not understand... > EVM - Extended Verification Module: security/integrity/evm That is a part of integrity subsystem which provides protection against offline modification of a inode metadata: uid, gid, perm, ino, xattrs, etc.. EVM has several hooks, which are called when those above metadata changes. And they are called when i_mutex is locked. i_mutex is used to prevent racing between some security verification and file metadata attribute update. One of the patches we are working on now would like to use extent map information to prevent certain attack. Of course as above we do it in EVM hooks which are called under i_mutex. And more over we need to ensure atomicity when reading extent map and updating xattrs - just the same as it works now for other inode metadata. So, we need to be able to call i_op->fiemap under i_mutex locked. It works fine for ext4, because ext4 does not lock i_mutex for files, but it does not work for ext3/ext2, because it does not have extent and use i_mutex to lock it. In my opinion, moving i_mutex locking to ioctl_fiemap is quite reasonable idea. It does not expose any requirement to FS to lock or not to lock i_mutex. - Dmitry > Cheers, > > Dave. > -- > Dave Chinner > david@xxxxxxxxxxxxx -- 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
