Re: Locking rules for fscrypt_operations->set_context()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]
  Powered by Linux