* Locking rules for fscrypt_operations->set_context()
@ 2016-09-20 14:30 Richard Weinberger
2016-09-22 0:14 ` Eric Biggers
0 siblings, 1 reply; 2+ messages in thread
From: Richard Weinberger @ 2016-09-20 14:30 UTC (permalink / raw)
To: linux-ext4, linux-fsdevel, linux-kernel; +Cc: Theodore Ts'o, David Gstir
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()?
Thanks,
//richard
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: Locking rules for fscrypt_operations->set_context()
2016-09-20 14:30 Locking rules for fscrypt_operations->set_context() Richard Weinberger
@ 2016-09-22 0:14 ` Eric Biggers
0 siblings, 0 replies; 2+ messages in thread
From: Eric Biggers @ 2016-09-22 0:14 UTC (permalink / raw)
To: Richard Weinberger
Cc: linux-ext4, linux-fsdevel, linux-kernel, Theodore Ts'o, David Gstir
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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2016-09-22 0:14 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-20 14:30 Locking rules for fscrypt_operations->set_context() Richard Weinberger
2016-09-22 0:14 ` Eric Biggers
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).