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
