Re: [RFC PATCH 00/14] ceph+fscrypt: together at last (contexts and filenames)

[Eric Biggers <ebiggers@kernel.org>]

On Fri, Aug 21, 2020 at 08:58:35PM -0400, Jeff Layton wrote:
> > > Ceph (and most other netfs') will need to pre-create a crypto context
> > > when creating a new inode as we'll need to encrypt some things before we
> > > have an inode. This patchset stores contexts in an xattr, but that's
> > > probably not ideal for the final implementation [1].
> > 
> > Coincidentally, I've currently working on solving a similar problem.  On ext4,
> > the inode number can't be assigned, and the encryption xattr can't be set, until
> > the jbd2 transaction which creates the inode.  Also, if the new inode is a
> > symlink, then fscrypt_encrypt_symlink() has to be called during the transaction.
> > Together, these imply that fscrypt_get_encryption_info() has to be called during
> > the transaction.
> > 
> 
> Yes, similar problem. I started looking at symlinks today, and got a
> little ways into a patchset to refactor some fscrypt code to handle
> them, but I don't think it's quite right yet. A more general solution
> would be nice.
> 
> > That's what we do, currently.  However, it's technically wrong and can deadlock,
> > since fscrypt_get_encryption_info() isn't GFP_NOFS-safe (and it can't be).
> > 
> > f2fs appears to have a similar problem, though I'm still investigating.
> > 
> > To fix this, I'm planning to add new functions:
> > 
> >    - fscrypt_prepare_new_inode() will set up the fscrypt_info for a new
> >      'struct inode' which hasn't necessarily had an inode number assigned yet.
> >      It won't set the encryption xattr yet.
> > 
> 
> I more or less have that in 02/14, I think, but if you have something
> else in mind, I'm happy to follow suit.
[...]
> > > Symlink handling in fscrypt will also need to be refactored a bit, as we
> > > won't have an inode before we'll need to encrypt its contents.
> > 
> > Will there be an in-memory inode allocated yet (a 'struct inode'), just with no
> > inode number assigned yet?  If so, my work-in-progress patchset I mentioned
> > earlier should be sufficient to address this.  The order would be:
> > 
> > 	1. fscrypt_prepare_new_inode()
> > 	2. fscrypt_encrypt_symlink()
> > 	3. Assign inode number
> > 
> > 
> > Or does ceph not have a 'struct inode' at all until step (3)?
> 
> No, generally ceph doesn't create an inode until the reply comes in. I
> think we'll need to be able to create a context and encrypt the symlink
> before we issue the call to the server. I started hacking at the fscrypt
> code for this today, but I didn't get very far.
> 
> FWIW, ceph is a bit of an odd netfs protocol in that there is a standard
> "trace" that holds info about dentries and inodes that are created or
> modified as a result of an operation. Most of the dentry/inode cache
> manipulation is done at that point, which is done as part of the reply
> processing.

Your patch "fscrypt: add fscrypt_new_context_from_parent" takes in a directory
and generates an fscrypt_context (a.k.a. an encryption xattr) for a new file
that will be created in that directory.

fscrypt_prepare_new_inode() from my work-in-progress patches would do a bit more
than that.  It would actually set up a "struct fscrypt_info" for a new inode.
That includes the encryption key and all information needed to build the
fscrypt_context.  So, afterwards it will be possible to call
fscrypt_encrypt_symlink() before the fscrypt_context is "saved to disk".
IIUC, that's part of what ceph will need.

The catch is that there will still have to be a 'struct inode' to associate the
'struct fscrypt_info' with.  It won't have to have ->i_ino set yet, but some
other fields (at least ->i_mode and ->i_sb) will have to be set, since lots of
code in fs/crypto/ uses those fields.

I think it would be possible to refactor things to make 'struct fscrypt_info'
more separate from 'struct inode', so that filesystems could create a
'struct fscrypt_info' that isn't associated with an inode yet, then encrypt a
symlink target using it (not caching it in ->i_link as we currently do).

However, it would require a lot of changes.

So I'm wondering if it would be easier to instead change ceph to create and
start initializing the 'struct inode' earlier.  It doesn't have to have an inode
number assigned or be added to the inode cache yet; it just needs to be
allocated in memory and some basic fields need to be initialized.  In theory
it's possible, right?  I'd expect that local filesystems aren't even that much
different, in principle; they start initializing a new 'struct inode' in memory
first, and only later do they *really* create the inode by allocating an inode
number and saving the changes to disk.

- Eric