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
