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

[Luís Henriques]

Eric Biggers <ebiggers@xxxxxxxxxx> writes:

> 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.

Oh, I see what you mean now, and you're obviously correct.  Thanks for the
detailed explanation.

> 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);
> 		}
> 	}

Ugh, I tend to forget all the details behind these decisions.  If I
remember correctly, we had to work around the fact that the cephfs client
handle directory data in a cumbersome way.  We may not have the full data
for a readdir, for example, and that has to be handled by a lookup.

> 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.

In fact, my initial fix for the cephfs bug was doing just that.  It was a
single patch to ceph_atomic_open() that would simply do:

	if (IS_ENCRYPTED(dir)) {
		set_bit(CEPH_MDS_R_FSCRYPT_FILE, &req->r_req_flags);
		err = __fscrypt_prepare_readdir(dir);
		if (!err && !fscrypt_has_encryption_key(dir)) {
			spin_lock(&dentry->d_lock);
			dentry->d_flags |= DCACHE_NOKEY_NAME;
			spin_unlock(&dentry->d_lock);
		}
	}

What made me want to create a new helper was that I simply needed to call
fscrypt_get_encryption_info() to force the encryption info to be set in
the parent directory.  But this function was only accessible through
__fscrypt_prepare_readdir(), which isn't really a great function name for
what I need here.

Since __fscrypt_prepare_readdir() doesn't seem to be used anywhere else,
maybe it could be removed and fscrypt_get_encryption_info() be exported
instead?

> 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.

Hmm... OK, looks like we'll need to have a look into this.  Thanks for the
heads-up.

Cheers,
-- 
Luís