On Mon, Sep 24, 2012 at 11:13:30AM +0300, Kasatkin, Dmitry wrote:
> 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.
We've got plenty of interfaces where this is true. Look at .fsync,
for example:
$ gl -n 1 02c24a82
commit 02c24a82187d5a628c68edfe71ae60dc135cd178
Author: Josef Bacik <josef@xxxxxxxxxx>
Date: Sat Jul 16 20:44:56 2011 -0400
fs: push i_mutex and filemap_write_and_wait down into ->fsync() handlers
Btrfs needs to be able to control how filemap_write_and_wait_range() is called
in fsync to make it less of a painful operation, so push down taking i_mutex and
the calling of filemap_write_and_wait() down into the ->fsync() handlers. Some
file systems can drop taking the i_mutex altogether it seems, like ext3 and
ocfs2. For correctness sake I just pushed everything down in all cases to make
sure that we keep the current behavior the same for everybody, and then each
individual fs maintainer can make up their mind about what to do from there.
Thanks,
Acked-by: Jan Kara <jack@xxxxxxx>
Signed-off-by: Josef Bacik <josef@xxxxxxxxxx>
Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
i.e. i_mutex used to be wrapped around the outside of .fsync, but
that proved to be a problem for btrfs and other filesystems that *do
not need i_mutex* during fsync. Hence the locking got pushed down
into the filesystems, and those that did not need i_mutex got rid of
it. Now the only rule for calling .fsync is that you can't hold
i_mutex when calling it.
> Linux kernel has many non-locking functions and its locking wrappers.
> Many callees requires and state that caller must lock something.
Yes, they do, just like many callees require and state that the
caller must *not* lock something. Say, like might_sleep()....
The requirement for calling .fiemap/.fync/.fallocate/etc is that the
"caller must not hold i_mutex". Essentially, all new inode/file
methods we have added in the past couple of years make the
assumption that serialisation is the problem of the filesystem, not
the VFS. This has been done because there is not a
"one-size-fits-all" method for fine-grained locking in filesystems.
> > 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.
fiemap is not the interface you want, then. it gives *zero*
guarantee that what it returns actually reflects what is on disk.
What is on disk can change the moment the filesystem drops the lock
that protects the extent map, and because i_mutex doesn't protect
the extent map, you aren't getting the atomicity you require....
i.e. fiemap provides a point in time snapshot that is out of date
even before the snapshot is returned to the caller regardless of the
locks held by the caller.
> > 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..
*groan*
> 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.
Sounds great, until you consider the fact that inode metadata can be
updated without i_mutex being held. e.g. fallocate vs fiemap. Have
you ever had a look at how many different ioctls various filesystems
implement that modify metadata without needing to hold i_mutex?
> One of the patches we are working on now would like to use extent map
> information to prevent certain attack.
I'm not a mind reader. What attack may that be?
> 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.
Fundamentally, extent maps are not protected by i_mutex, and that's
where your design breaks down. What you want cannot be provided by
i_mutex, and that's one of the reasons why i_mutex is not held above
.fiemap - it's not appropriate locking for the operation being
performed.
Here's a beautiful example: XFS_IOC_SWAPEXT.
This filesystem specific ioctl() atomically swaps the extent maps
between two inodes. And it doesn't hold i_mutex to do it, and it
doesn't even inform the VFS that it has happened. Indeed, it is
*completely transparent* to the VFS because it's a special interface
to the filesystem used by filesystem maintenance tools, namely the
online defragmenter xfs_fsr. That means running xfs_fsr will really
screw with any EVM requirement that special security attributes and
extent maps are kept in sync....
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
