Re: [PATCH 1/2] fscrypt: new helper function - fscrypt_prepare_atomic_open()

[Eric Biggers]

On Tue, Mar 14, 2023 at 10:15:11AM +0000, Luís Henriques wrote:
> Eric Biggers <ebiggers@xxxxxxxxxx> writes:
> 
> > On Mon, Mar 13, 2023 at 12:33:09PM +0000, Luís Henriques wrote:
> >> + * The regular open path will use fscrypt_file_open for that, but in the
> >> + * atomic open a different approach is required.
> >
> > This should actually be fscrypt_prepare_lookup, not fscrypt_file_open, right?
> 
> Ups, I missed this comment.
> 
> I was comparing the regular open() with the atomic_open() paths.  I think
> I really mean fscrypt_file_open() because that's where the encryption info
> is (or may be) set by calling fscrypt_require_key().  atomic_open needs
> something similar, but combined with a lookup.
> 
> Maybe I can rephrase it to:
> 
>   The reason for getting the encryption info before checking if the
>   directory has the encryption key is because the key may be available but
>   the encryption info isn't yet set (maybe due to a drop_caches).  The
>   regular open path will call fscrypt_file_open which uses function
>   fscrypt_require_key for setting the encryption info if needed.  The
>   atomic open needs to do something similar.
> 

No, regular open is two parts: ->lookup and ->open.  fscrypt_prepare_lookup()
sets up the directory's key, whereas fscrypt_file_open() sets up the file's key.

Your proposed fscrypt_prepare_atomic_open() sets up the directory's key.  So it
is really fscrypt_prepare_lookup() that is its equivalent.

However, that raises the question of why doesn't ceph just use
fscrypt_prepare_lookup()?  It seems the answer is that ceph wants to handle the
filenames encryption and no-key name encoding itself.  And for that reason, its
->lookup() does the following and does *not* use fscrypt_prepare_lookup():

	if (IS_ENCRYPTED(dir)) {
		err = ceph_fscrypt_prepare_readdir(dir);
		if (err < 0)
			return ERR_PTR(err);
		if (!fscrypt_has_encryption_key(dir)) {
			spin_lock(&dentry->d_lock);
			dentry->d_flags |= DCACHE_NOKEY_NAME;
			spin_unlock(&dentry->d_lock);
		}
	}

So, actually I think this patch doesn't make sense.  If ceph is doing the above
in its ->lookup() anyway, then it just should do the exact same thing in its
->atomic_open() too.

If you want to add a new fscrypt_* helper function which *just* sets up the
given directory's key and sets the NOKEY_NAME flag on the given dentry
accordingly, that could make sense.  However, it should be called from *both*
->lookup() and ->atomic_open(), not just ->atomic_open().

It's also worth mentioning that setting up the filename separately from the
NOKEY_NAME flag makes ceph have the same race condition that I had fixed for the
other filesystems in commit b01531db6cec ("fscrypt: fix race where ->lookup()
marks plaintext dentry as ciphertext").  It's not a huge deal, but it can cause
some odd behavior, so it's worth thinking about whether it can be solved.

- Eric