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

[Eric Biggers <ebiggers@kernel.org>]

On Mon, Aug 24, 2020 at 08:03:35AM -0400, Jeff Layton wrote:
> On Fri, 2020-08-21 at 19:34 -0700, Eric Biggers wrote:
> > 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.
> > 
> 
> It's probably possible. I think we'd just need to attach the nascent
> inode to the MDS request tracking structure, and convert that from using
> iget5_locked to inode_insert5.
> 
> Would we need to do this for all inode types or just symlinks?

It would be all inodes, since fscrypt_prepare_new_inode() will handle all types
of inodes.

- Eric