On Mon, Sep 24, 2012 at 12:18 PM, Dave Chinner [off-list ref] wrote:

On Mon, Sep 24, 2012 at 11:13:30AM +0300, Kasatkin, Dmitry wrote:

quoted

On Sat, Sep 22, 2012 at 1:59 AM, Dave Chinner [off-list ref] wrote:

quoted

On Fri, Sep 21, 2012 at 05:14:28PM +0300, Dmitry Kasatkin wrote:

quoted

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 [off-list ref]
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 [off-list ref]
    Signed-off-by: Josef Bacik [off-list ref]
    Signed-off-by: Al Viro [off-list ref]

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.

I see.

quoted

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()....

Fair enough.

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.

quoted

quoted

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 use filemap_write_and_wait() before using fiemap.
And it seems to be persistent between reboots on ext4.

In our case i_mutex is not for FS itself, but for EVM to ensure
that hash over extents and its value in xattrs are stay in sync.

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 what is the way at VFS to ensure that what ever is returned in fiemap
stays the same over reboots?

ioctl_fiemap() uses filemap_write_and_wait()...
And it seems to work well for me as well...

quoted

quoted

So that's a NACK from me...

quoted

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*

quoted

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?

What inode metadata are you talking about?

do_truncate:
	mutex_lock(&dentry->d_inode->i_mutex);
	ret = notify_change(dentry, &newattrs);
	mutex_unlock(&dentry->d_inode->i_mutex);

chmod_common:
	error = notify_change(path->dentry, &newattrs);
	mutex_unlock(&inode->i_mutex);

chown_common:
	mutex_lock(&inode->i_mutex);
	error = security_path_chown(path, user, group);
	if (!error)
		error = notify_change(path->dentry, &newattrs);
	mutex_unlock(&inode->i_mutex);

It all happens with i_mutex locked.
They all call then
	if (inode->i_op->setattr)
		error = inode->i_op->setattr(dentry, attr);

quoted

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?

That is a simple attack of modifying inode block map so that different
files points to the same blocks.
FSCK calls them "multiply claimed blocks"...

quoted

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.

Really, as I said, we are under i_mutex not to protect FS,
but to make sure that extent content and extended attribute stays in sync.

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....

That is a tough case then if VFS is unaware.
For integrity protection it might be a requirement not to use such
ways to modify file system.


But ok.. You are aware of the problem.
What is your suggestion on how we could get some integrity measurement
at VFS layer which
allows to identify if inode block mapping has changed?

Thanks,

Dmitry

Cheers,

Dave.
--
Dave Chinner
david@fromorbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html