Re: [PATCH v16 25/68] ceph: make d_revalidate call fscrypt revalidator for encrypted dentries

["Luís Henriques" <lhenriques@suse.de>]

Xiubo Li <xiubli@redhat.com> writes:

> On 08/03/2023 02:53, Luís Henriques wrote:
>> xiubli@redhat.com writes:
>>
>>> From: Jeff Layton <jlayton@kernel.org>
>>>
>>> If we have a dentry which represents a no-key name, then we need to test
>>> whether the parent directory's encryption key has since been added.  Do
>>> that before we test anything else about the dentry.
>>>
>>> Reviewed-by: Xiubo Li <xiubli@redhat.com>
>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>>> ---
>>>   fs/ceph/dir.c | 8 ++++++--
>>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
>>> index d3c2853bb0f1..5ead9f59e693 100644
>>> --- a/fs/ceph/dir.c
>>> +++ b/fs/ceph/dir.c
>>> @@ -1770,6 +1770,10 @@ static int ceph_d_revalidate(struct dentry *dentry, unsigned int flags)
>>>   	struct inode *dir, *inode;
>>>   	struct ceph_mds_client *mdsc;
>>>   +	valid = fscrypt_d_revalidate(dentry, flags);
>>> +	if (valid <= 0)
>>> +		return valid;
>>> +
>> This patch has confused me in the past, and today I found myself
>> scratching my head again looking at it.
>>
>> So, I've started seeing generic/123 test failing when running it with
>> test_dummy_encryption.  I was almost sure that this test used to run fine
>> before, but I couldn't find any evidence (somehow I lost my old testing
>> logs...).
>>
>> Anyway, the test is quite simple:
>>
>> 1. Creates a directory with write permissions for root only
>> 2. Writes into a file in that directory
>> 3. Uses 'su' to try to modify that file as a different user, and
>>     gets -EPERM
>>
>> All these steps run fine, and the test should pass.  *However*, in the
>> test cleanup function, a simple 'rm -rf <dir>' will fail with -ENOTEMPTY.
>> 'strace' shows that calling unlinkat() to remove the file got a '-ENOENT'
>> and then -ENOTEMPTY for the directory.
>>
>> Some digging allowed me to figure out that running commands with 'su' will
>> drop caches (I see 'su (874): drop_caches: 2' in the log).  And this is
>> how I ended up looking at this patch.  fscrypt_d_revalidate() will return
>> '0' if the parent directory does has a key (fscrypt_has_encryption_key()).
>> Can we really say here that the dentry is *not* valid in that case?  Or
>> should that '<= 0' be a '< 0'?
>>
>> (But again, this patch has confused me before...)
>
> Luis,
>
> Could you reproduce it with the latest testing branch ?

Yes, I'm seeing this with the latest code.

> I never seen the generic/123 failure yet. And just now I ran the test for many
> times locally it worked fine.

That's odd.  With 'test_dummy_encryption' mount option I can reproduce it
every time.

> From the generic/123 test code it will never touch the key while testing, that
> means the dentries under the test dir will always have the keyed name. And then
> the 'fscrypt_d_revalidate()' should return 1 always.
>
> Only when we remove the key will it trigger evicting the inodes and then when we
> add the key back will the 'fscrypt_d_revalidate()' return 0 by checking the
> 'fscrypt_has_encryption_key()'.
>
> As I remembered we have one or more fixes about this those days, not sure
> whether you were hitting those bugs we have already fixed ?

Yeah, I remember now, and I guess there's yet another one here!

I'll look closer into this and see if I can find out something else.  I'm
definitely seeing 'fscrypt_d_revalidate()' returning 0, so probably the
bug is in the error paths, when the 'fsgqa' user tries to write into the
file.

Thanks for your feedback, Xiubo.

Cheers,
-- 
Luís