Re: Locking rules for fscrypt_operations->set_context()

[Eric Biggers <ebiggers@google.com>]

On Tue, Sep 20, 2016 at 04:30:06PM +0200, Richard Weinberger wrote:
> Hi!
> 
> To my understanding ->setxattr() is always being called with i_mutex held.
> ->set_context() in ext4 stores the security context using ext4_xattr_set(),
> but the fs crypto framework does not lock the inode itself.
> So, depending on the call path, ext4_xattr_set() is sometimes being
> called with i_mutex held and some times not.
> 
> What are the locking rules for fscrypt_operations and especially ->set_context()?

Hi Richard, this is a great question.  I would like to document somewhere the
semantics of each of the fscrypt_operations, but I am still figuring them out
myself.

With regards to ->set_context(), it is called in two distinct situtations:

  (1) when a user process uses FS_IOC_SET_ENCRYPTION_POLICY to set the
      encryption policy on an empty directory

  (2) when an encryption policy is inherited by a newly created file in an
      encrypted directory

In case (1), I think there needs to be an inode_lock() added.  For ext4 and
f2fs, it looks like setting an xattr without inode_lock() isn't problematic by
itself.  Instead, the problem I see is that fscrypt_process_policy() does
several operations, including the ->empty_dir() check, which aren't guaranteed
to be atomic if the directory inode is not locked with inode_lock().

In case (2), I don't think it matters whether inode_lock() is held, since the
inode is still being initialized and is still "locked" in a different way, in
the I_NEW state.  There are also other xattrs being set in __ext4_new_inode(),
seemingly without inode_lock(), which I *think* is fine.

So I am currently thinking that fscrypt_process_policy() should be fixed to do
inode_lock(), and ->set_context() should be documented as a filesystem internal
operation (not necessarily related to ->setxattr()) that is called on either an
inode_lock()-ed inode or on an I_NEW inode.

Eric