[PATCH] ceph: make sure directories aren't complete after setting crypt context

[PATCH] ceph: make sure directories aren't complete after setting crypt context

[Luís Henriques]

When setting a directory's crypt context, __ceph_dir_clear_complete() needs
to be used otherwise, if it was complete before, any old dentry that's still
around will be valid.

Signed-off-by: Luís Henriques <lhenriques@suse.de>
---
Hi!

Here's a simple way to trigger the bug this patch is fixing:

# cd /cephfs
# ls mydir
nKRhofOAVNsAwVLvDw7a0c9ypsjbZfK3n0Npnmni6j0
# ls mydir/nKRhofOAVNsAwVLvDw7a0c9ypsjbZfK3n0Npnmni6j0/
Cyuer5xT+kBlEPgtwAqSj0WK2taEljP5vHZ,D8VXCJ8  u+46b2XVCt7Obpz0gznZyNLRj79Q2l4KmkwbKOzdQKw
# fscrypt unlock mydir
# touch /mnt/test/mydir/mysubdir/file
touch: cannot touch '/mnt/test/mydir/mysubdir/file': No such file or directory

 fs/ceph/crypto.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
index 35a2ccfe6899..dc1557967032 100644
--- a/fs/ceph/crypto.c
+++ b/fs/ceph/crypto.c
@@ -87,6 +87,10 @@ static int ceph_crypt_get_context(struct inode *inode, void *ctx, size_t len)
 		return -ERANGE;
 
 	memcpy(ctx, cfa->cfa_blob, ctxlen);
+
+	/* Directory isn't complete anymore */
+	if (S_ISDIR(inode->i_mode) && __ceph_dir_is_complete(ci))
+		__ceph_dir_clear_complete(ci);
 	return ctxlen;
 }
 

Re: [PATCH] ceph: make sure directories aren't complete after setting crypt context

[Xiubo Li]

On 16/11/2022 23:37, Luís Henriques wrote:
> When setting a directory's crypt context, __ceph_dir_clear_complete() needs
> to be used otherwise, if it was complete before, any old dentry that's still
> around will be valid.
>
> Signed-off-by: Luís Henriques <lhenriques@suse.de>
> ---
> Hi!
>
> Here's a simple way to trigger the bug this patch is fixing:
>
> # cd /cephfs
> # ls mydir
> nKRhofOAVNsAwVLvDw7a0c9ypsjbZfK3n0Npnmni6j0
> # ls mydir/nKRhofOAVNsAwVLvDw7a0c9ypsjbZfK3n0Npnmni6j0/
> Cyuer5xT+kBlEPgtwAqSj0WK2taEljP5vHZ,D8VXCJ8  u+46b2XVCt7Obpz0gznZyNLRj79Q2l4KmkwbKOzdQKw
> # fscrypt unlock mydir
> # touch /mnt/test/mydir/mysubdir/file
> touch: cannot touch '/mnt/test/mydir/mysubdir/file': No such file or directory
>
>   fs/ceph/crypto.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
> index 35a2ccfe6899..dc1557967032 100644
> --- a/fs/ceph/crypto.c
> +++ b/fs/ceph/crypto.c
> @@ -87,6 +87,10 @@ static int ceph_crypt_get_context(struct inode *inode, void *ctx, size_t len)
>   		return -ERANGE;
>   
>   	memcpy(ctx, cfa->cfa_blob, ctxlen);
> +
> +	/* Directory isn't complete anymore */
> +	if (S_ISDIR(inode->i_mode) && __ceph_dir_is_complete(ci))
> +		__ceph_dir_clear_complete(ci);

Hi Luis,

Good catch!

BTW, why do this in the ceph_crypt_get_context() ? As my understanding 
is that we should mark 'mydir' as incomplete when unlocking it. While as 
I remembered the unlock operation will do:


Step1: get_encpolicy via 'mydir' as ctx
Step2: rm_enckey of ctx from the superblock


Since I am still running the test cases for the file lock patches, so I 
didn't catch logs to confirm the above steps yet.

If I am right IMO then we should mark the dir as incomplete in the Step2 
instead, because for non-unlock operations they may also do the Step1.

Thanks!

- Xiubo

>   	return ctxlen;
>   }
>   
>

Re: [PATCH] ceph: make sure directories aren't complete after setting crypt context

[Xiubo Li]

On 17/11/2022 16:03, Xiubo Li wrote:
>
> On 16/11/2022 23:37, Luís Henriques wrote:
>> When setting a directory's crypt context, __ceph_dir_clear_complete() 
>> needs
>> to be used otherwise, if it was complete before, any old dentry 
>> that's still
>> around will be valid.
>>
>> Signed-off-by: Luís Henriques <lhenriques@suse.de>
>> ---
>> Hi!
>>
>> Here's a simple way to trigger the bug this patch is fixing:
>>
>> # cd /cephfs
>> # ls mydir
>> nKRhofOAVNsAwVLvDw7a0c9ypsjbZfK3n0Npnmni6j0
>> # ls mydir/nKRhofOAVNsAwVLvDw7a0c9ypsjbZfK3n0Npnmni6j0/
>> Cyuer5xT+kBlEPgtwAqSj0WK2taEljP5vHZ,D8VXCJ8 
>> u+46b2XVCt7Obpz0gznZyNLRj79Q2l4KmkwbKOzdQKw
>> # fscrypt unlock mydir
>> # touch /mnt/test/mydir/mysubdir/file
>> touch: cannot touch '/mnt/test/mydir/mysubdir/file': No such file or 
>> directory
>>
>>   fs/ceph/crypto.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
>> index 35a2ccfe6899..dc1557967032 100644
>> --- a/fs/ceph/crypto.c
>> +++ b/fs/ceph/crypto.c
>> @@ -87,6 +87,10 @@ static int ceph_crypt_get_context(struct inode 
>> *inode, void *ctx, size_t len)
>>           return -ERANGE;
>>         memcpy(ctx, cfa->cfa_blob, ctxlen);
>> +
>> +    /* Directory isn't complete anymore */
>> +    if (S_ISDIR(inode->i_mode) && __ceph_dir_is_complete(ci))
>> +        __ceph_dir_clear_complete(ci);
>
> Hi Luis,
>
> Good catch!
>
> BTW, why do this in the ceph_crypt_get_context() ? As my understanding 
> is that we should mark 'mydir' as incomplete when unlocking it. While 
> as I remembered the unlock operation will do:
>
>
> Step1: get_encpolicy via 'mydir' as ctx
> Step2: rm_enckey of ctx from the superblock
>
Sorry, it should be add_enckey.
>
> Since I am still running the test cases for the file lock patches, so 
> I didn't catch logs to confirm the above steps yet.
>
> If I am right IMO then we should mark the dir as incomplete in the 
> Step2 instead, because for non-unlock operations they may also do the 
> Step1.
>
Your patch will work. But probably we should do this just around 
__fscrypt_prepare_readdir() or fscrypt_prepare_readdir() instead ? We 
need to detect that once the 'inode->i_crypt_info' changed then mark the 
dir as incomplete.

For now for the lock operation it will evict the inode, which will help 
do this for us already. But for unlock case, we need to handle it by 
ourself.

Thanks!

- Xiubo


> Thanks!
>
> - Xiubo
>
>>       return ctxlen;
>>   }
>>

Re: [PATCH] ceph: make sure directories aren't complete after setting crypt context

[Luís Henriques]

Xiubo Li <xiubli@redhat.com> writes:

> On 17/11/2022 16:03, Xiubo Li wrote:
>>
>> On 16/11/2022 23:37, Luís Henriques wrote:
>>> When setting a directory's crypt context, __ceph_dir_clear_complete() needs
>>> to be used otherwise, if it was complete before, any old dentry that's still
>>> around will be valid.
>>>
>>> Signed-off-by: Luís Henriques <lhenriques@suse.de>
>>> ---
>>> Hi!
>>>
>>> Here's a simple way to trigger the bug this patch is fixing:
>>>
>>> # cd /cephfs
>>> # ls mydir
>>> nKRhofOAVNsAwVLvDw7a0c9ypsjbZfK3n0Npnmni6j0
>>> # ls mydir/nKRhofOAVNsAwVLvDw7a0c9ypsjbZfK3n0Npnmni6j0/
>>> Cyuer5xT+kBlEPgtwAqSj0WK2taEljP5vHZ,D8VXCJ8
>>> u+46b2XVCt7Obpz0gznZyNLRj79Q2l4KmkwbKOzdQKw
>>> # fscrypt unlock mydir
>>> # touch /mnt/test/mydir/mysubdir/file
>>> touch: cannot touch '/mnt/test/mydir/mysubdir/file': No such file or
>>> directory
>>>
>>>   fs/ceph/crypto.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
>>> index 35a2ccfe6899..dc1557967032 100644
>>> --- a/fs/ceph/crypto.c
>>> +++ b/fs/ceph/crypto.c
>>> @@ -87,6 +87,10 @@ static int ceph_crypt_get_context(struct inode *inode,
>>> void *ctx, size_t len)
>>>           return -ERANGE;
>>>         memcpy(ctx, cfa->cfa_blob, ctxlen);
>>> +
>>> +    /* Directory isn't complete anymore */
>>> +    if (S_ISDIR(inode->i_mode) && __ceph_dir_is_complete(ci))
>>> +        __ceph_dir_clear_complete(ci);
>>
>> Hi Luis,
>>
>> Good catch!
>>
>> BTW, why do this in the ceph_crypt_get_context() ? As my understanding is that
>> we should mark 'mydir' as incomplete when unlocking it. While as I remembered
>> the unlock operation will do:
>>
>>
>> Step1: get_encpolicy via 'mydir' as ctx
>> Step2: rm_enckey of ctx from the superblock
>>
> Sorry, it should be add_enckey.
>>
>> Since I am still running the test cases for the file lock patches, so I didn't
>> catch logs to confirm the above steps yet.
>>
>> If I am right IMO then we should mark the dir as incomplete in the Step2
>> instead, because for non-unlock operations they may also do the Step1.
>>
> Your patch will work. But probably we should do this just around
> __fscrypt_prepare_readdir() or fscrypt_prepare_readdir() instead ? We need to
> detect that once the 'inode->i_crypt_info' changed then mark the dir as
> incomplete.
>
> For now for the lock operation it will evict the inode, which will help do this
> for us already. But for unlock case, we need to handle it by ourself.

OK, that makes sense and to be honest I thought that there should be
another place for doing this. Unfortunately, I didn't found it: in the
test case I have the fscrypt_prepare_readdir() isn't called:

   # cd /cephfs
   # ls mydir
   nKRhofOAVNsAwVLvDw7a0c9ypsjbZfK3n0Npnmni6j0
   # ls mydir/nKRhofOAVNsAwVLvDw7a0c9ypsjbZfK3n0Npnmni6j0/
   Cyuer5xT+kBlEPgtwAqSj0WK2taEljP5vHZ,D8VXCJ8 u+46b2XVCt7Obpz0gznZyNLRj79Q2l4KmkwbKOzdQKw

At this point readdir was executed, of course.  And
__ceph_dir_set_complete() is also used to indicate that we have the full
contents.  However, executing the following commands won't result in any
new readdir():

   # fscrypt unlock mydir
   # touch /mnt/test/mydir/mysubdir/file

and since the encryption key is set at the sb level, I couldn't find a way
to detect changes in inode->i_crypt_info.  ceph_d_revalidate() is invoked
but at that point I don't thing we have a way to know what is changing.

Any ideas?

Cheers,
-- 
Luís

>
> Thanks!
>
> - Xiubo
>
>
>> Thanks!
>>
>> - Xiubo
>>
>>>       return ctxlen;
>>>   }
>>>
>

Re: [PATCH] ceph: make sure directories aren't complete after setting crypt context

[Xiubo Li]

On 18/11/2022 02:02, Luís Henriques wrote:
> Xiubo Li <xiubli@redhat.com> writes:
>
>> On 17/11/2022 16:03, Xiubo Li wrote:
>>> On 16/11/2022 23:37, Luís Henriques wrote:
>>>> When setting a directory's crypt context, __ceph_dir_clear_complete() needs
>>>> to be used otherwise, if it was complete before, any old dentry that's still
>>>> around will be valid.
>>>>
>>>> Signed-off-by: Luís Henriques <lhenriques@suse.de>
>>>> ---
>>>> Hi!
>>>>
>>>> Here's a simple way to trigger the bug this patch is fixing:
>>>>
>>>> # cd /cephfs
>>>> # ls mydir
>>>> nKRhofOAVNsAwVLvDw7a0c9ypsjbZfK3n0Npnmni6j0
>>>> # ls mydir/nKRhofOAVNsAwVLvDw7a0c9ypsjbZfK3n0Npnmni6j0/
>>>> Cyuer5xT+kBlEPgtwAqSj0WK2taEljP5vHZ,D8VXCJ8
>>>> u+46b2XVCt7Obpz0gznZyNLRj79Q2l4KmkwbKOzdQKw
>>>> # fscrypt unlock mydir
>>>> # touch /mnt/test/mydir/mysubdir/file
>>>> touch: cannot touch '/mnt/test/mydir/mysubdir/file': No such file or
>>>> directory
>>>>
>>>>    fs/ceph/crypto.c | 4 ++++
>>>>    1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
>>>> index 35a2ccfe6899..dc1557967032 100644
>>>> --- a/fs/ceph/crypto.c
>>>> +++ b/fs/ceph/crypto.c
>>>> @@ -87,6 +87,10 @@ static int ceph_crypt_get_context(struct inode *inode,
>>>> void *ctx, size_t len)
>>>>            return -ERANGE;
>>>>          memcpy(ctx, cfa->cfa_blob, ctxlen);
>>>> +
>>>> +    /* Directory isn't complete anymore */
>>>> +    if (S_ISDIR(inode->i_mode) && __ceph_dir_is_complete(ci))
>>>> +        __ceph_dir_clear_complete(ci);
>>> Hi Luis,
>>>
>>> Good catch!
>>>
>>> BTW, why do this in the ceph_crypt_get_context() ? As my understanding is that
>>> we should mark 'mydir' as incomplete when unlocking it. While as I remembered
>>> the unlock operation will do:
>>>
>>>
>>> Step1: get_encpolicy via 'mydir' as ctx
>>> Step2: rm_enckey of ctx from the superblock
>>>
>> Sorry, it should be add_enckey.
>>> Since I am still running the test cases for the file lock patches, so I didn't
>>> catch logs to confirm the above steps yet.
>>>
>>> If I am right IMO then we should mark the dir as incomplete in the Step2
>>> instead, because for non-unlock operations they may also do the Step1.
>>>
>> Your patch will work. But probably we should do this just around
>> __fscrypt_prepare_readdir() or fscrypt_prepare_readdir() instead ? We need to
>> detect that once the 'inode->i_crypt_info' changed then mark the dir as
>> incomplete.
>>
>> For now for the lock operation it will evict the inode, which will help do this
>> for us already. But for unlock case, we need to handle it by ourself.
> OK, that makes sense and to be honest I thought that there should be
> another place for doing this. Unfortunately, I didn't found it: in the
> test case I have the fscrypt_prepare_readdir() isn't called:
>
>     # cd /cephfs
>     # ls mydir
>     nKRhofOAVNsAwVLvDw7a0c9ypsjbZfK3n0Npnmni6j0
>     # ls mydir/nKRhofOAVNsAwVLvDw7a0c9ypsjbZfK3n0Npnmni6j0/
>     Cyuer5xT+kBlEPgtwAqSj0WK2taEljP5vHZ,D8VXCJ8 u+46b2XVCt7Obpz0gznZyNLRj79Q2l4KmkwbKOzdQKw
>
> At this point readdir was executed, of course.  And
> __ceph_dir_set_complete() is also used to indicate that we have the full
> contents.  However, executing the following commands won't result in any
> new readdir():
>
>     # fscrypt unlock mydir
>     # touch /mnt/test/mydir/mysubdir/file
>
> and since the encryption key is set at the sb level, I couldn't find a way
> to detect changes in inode->i_crypt_info.  ceph_d_revalidate() is invoked
> but at that point I don't thing we have a way to know what is changing.
>
> Any ideas?

# ls mydir/
zy94Zt01M90xwYq+nxJsEvea+HYq49mqVgrUBkYrJAU
# ls mydir/zy94Zt01M90xwYq+nxJsEvea+HYq49mqVgrUBkYrJAU/
24iNa8ICYc6nZZIkL,4n1sOtp9KiaTGtpq8PCZDQ6LU 
ivG4rQYzg6YENagbTZaV2kh2sVouEdlkmBZPfgVDl48

I have added the debug logs in all the places just like:

@@ -784,7 +786,9 @@ static struct dentry *ceph_lookup(struct inode *dir, 
struct dentry *dentry,
                 return ERR_PTR(-ENAMETOOLONG);

         if (IS_ENCRYPTED(dir)) {
+       printk("%s fscrypt_has_encryption_key(dir): %d ======\n", 
__func__, fscrypt_has_encryption_key(dir));
                 err = __fscrypt_prepare_readdir(dir);
+       printk("%s fscrypt_has_encryption_key(dir): %d ======\n", 
__func__, fscrypt_has_encryption_key(dir));
                 if (err)
                         return ERR_PTR(err);
                 if (!fscrypt_has_encryption_key(dir)) {


# fscrypt unlock mydir
Enter custom passphrase for protector "l":
"mydir" is now unlocked and ready for use.

And after "mydir/" was unlocked and then when doing:

# touch mydir/dir/file2

And when lookup the "dir/" dentry I can see the "mydir/" encryption key 
changed:

709 <7>[79125.023676] ceph:  __ceph_caps_issued_mask ino 0x10000000004 
cap 000000004dc11892 issued pAsLsXsFs (mask As)
710 <7>[79125.023687] ceph:  __touch_cap 000000007071b095 cap 
000000004dc11892 mds0
711 <7>[79125.023823] ceph:  lookup 000000007071b095 dentry 
00000000f97501b2 'dir'
712 <4>[79125.023838] ceph_lookup fscrypt_has_encryption_key(dir): 0 ======
713 <4>[79125.024186] ceph_lookup fscrypt_has_encryption_key(dir): 1 ======
714 <7>[79125.024194] ceph:   dir 000000007071b095 flags are 0x0
715 <7>[79125.024269] ceph:  do_request on 00000000a93fafef

I am thinking could we just make __fscrypt_prepare_readdir(), which will 
return 0 when the key is already set or successfully set, to return 1 
instead of 0 to mark that the key changed ?

Thanks!

- Xiubo

>
> Cheers,

Re: [PATCH] ceph: make sure directories aren't complete after setting crypt context

[Luís Henriques]

Xiubo Li <xiubli@redhat.com> writes:

> On 18/11/2022 02:02, Luís Henriques wrote:
>> Xiubo Li <xiubli@redhat.com> writes:
>>
>>> On 17/11/2022 16:03, Xiubo Li wrote:
>>>> On 16/11/2022 23:37, Luís Henriques wrote:
>>>>> When setting a directory's crypt context, __ceph_dir_clear_complete() needs
>>>>> to be used otherwise, if it was complete before, any old dentry that's still
>>>>> around will be valid.
>>>>>
>>>>> Signed-off-by: Luís Henriques <lhenriques@suse.de>
>>>>> ---
>>>>> Hi!
>>>>>
>>>>> Here's a simple way to trigger the bug this patch is fixing:
>>>>>
>>>>> # cd /cephfs
>>>>> # ls mydir
>>>>> nKRhofOAVNsAwVLvDw7a0c9ypsjbZfK3n0Npnmni6j0
>>>>> # ls mydir/nKRhofOAVNsAwVLvDw7a0c9ypsjbZfK3n0Npnmni6j0/
>>>>> Cyuer5xT+kBlEPgtwAqSj0WK2taEljP5vHZ,D8VXCJ8
>>>>> u+46b2XVCt7Obpz0gznZyNLRj79Q2l4KmkwbKOzdQKw
>>>>> # fscrypt unlock mydir
>>>>> # touch /mnt/test/mydir/mysubdir/file
>>>>> touch: cannot touch '/mnt/test/mydir/mysubdir/file': No such file or
>>>>> directory
>>>>>
>>>>>    fs/ceph/crypto.c | 4 ++++
>>>>>    1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
>>>>> index 35a2ccfe6899..dc1557967032 100644
>>>>> --- a/fs/ceph/crypto.c
>>>>> +++ b/fs/ceph/crypto.c
>>>>> @@ -87,6 +87,10 @@ static int ceph_crypt_get_context(struct inode *inode,
>>>>> void *ctx, size_t len)
>>>>>            return -ERANGE;
>>>>>          memcpy(ctx, cfa->cfa_blob, ctxlen);
>>>>> +
>>>>> +    /* Directory isn't complete anymore */
>>>>> +    if (S_ISDIR(inode->i_mode) && __ceph_dir_is_complete(ci))
>>>>> +        __ceph_dir_clear_complete(ci);
>>>> Hi Luis,
>>>>
>>>> Good catch!
>>>>
>>>> BTW, why do this in the ceph_crypt_get_context() ? As my understanding is that
>>>> we should mark 'mydir' as incomplete when unlocking it. While as I remembered
>>>> the unlock operation will do:
>>>>
>>>>
>>>> Step1: get_encpolicy via 'mydir' as ctx
>>>> Step2: rm_enckey of ctx from the superblock
>>>>
>>> Sorry, it should be add_enckey.
>>>> Since I am still running the test cases for the file lock patches, so I didn't
>>>> catch logs to confirm the above steps yet.
>>>>
>>>> If I am right IMO then we should mark the dir as incomplete in the Step2
>>>> instead, because for non-unlock operations they may also do the Step1.
>>>>
>>> Your patch will work. But probably we should do this just around
>>> __fscrypt_prepare_readdir() or fscrypt_prepare_readdir() instead ? We need to
>>> detect that once the 'inode->i_crypt_info' changed then mark the dir as
>>> incomplete.
>>>
>>> For now for the lock operation it will evict the inode, which will help do this
>>> for us already. But for unlock case, we need to handle it by ourself.
>> OK, that makes sense and to be honest I thought that there should be
>> another place for doing this. Unfortunately, I didn't found it: in the
>> test case I have the fscrypt_prepare_readdir() isn't called:
>>
>>     # cd /cephfs
>>     # ls mydir
>>     nKRhofOAVNsAwVLvDw7a0c9ypsjbZfK3n0Npnmni6j0
>>     # ls mydir/nKRhofOAVNsAwVLvDw7a0c9ypsjbZfK3n0Npnmni6j0/
>>     Cyuer5xT+kBlEPgtwAqSj0WK2taEljP5vHZ,D8VXCJ8 u+46b2XVCt7Obpz0gznZyNLRj79Q2l4KmkwbKOzdQKw
>>
>> At this point readdir was executed, of course.  And
>> __ceph_dir_set_complete() is also used to indicate that we have the full
>> contents.  However, executing the following commands won't result in any
>> new readdir():
>>
>>     # fscrypt unlock mydir
>>     # touch /mnt/test/mydir/mysubdir/file
>>
>> and since the encryption key is set at the sb level, I couldn't find a way
>> to detect changes in inode->i_crypt_info.  ceph_d_revalidate() is invoked
>> but at that point I don't thing we have a way to know what is changing.
>>
>> Any ideas?
>
> # ls mydir/
> zy94Zt01M90xwYq+nxJsEvea+HYq49mqVgrUBkYrJAU
> # ls mydir/zy94Zt01M90xwYq+nxJsEvea+HYq49mqVgrUBkYrJAU/
> 24iNa8ICYc6nZZIkL,4n1sOtp9KiaTGtpq8PCZDQ6LU
> ivG4rQYzg6YENagbTZaV2kh2sVouEdlkmBZPfgVDl48
>
> I have added the debug logs in all the places just like:
>
> @@ -784,7 +786,9 @@ static struct dentry *ceph_lookup(struct inode *dir, struct
> dentry *dentry,
>                 return ERR_PTR(-ENAMETOOLONG);
>
>         if (IS_ENCRYPTED(dir)) {
> +       printk("%s fscrypt_has_encryption_key(dir): %d ======\n", __func__,
> fscrypt_has_encryption_key(dir));
>                 err = __fscrypt_prepare_readdir(dir);
> +       printk("%s fscrypt_has_encryption_key(dir): %d ======\n", __func__,
> fscrypt_has_encryption_key(dir));
>                 if (err)
>                         return ERR_PTR(err);
>                 if (!fscrypt_has_encryption_key(dir)) {
>
>
> # fscrypt unlock mydir
> Enter custom passphrase for protector "l":
> "mydir" is now unlocked and ready for use.
>
> And after "mydir/" was unlocked and then when doing:
>
> # touch mydir/dir/file2
>
> And when lookup the "dir/" dentry I can see the "mydir/" encryption key changed:
>
> 709 <7>[79125.023676] ceph:  __ceph_caps_issued_mask ino 0x10000000004 cap
> 000000004dc11892 issued pAsLsXsFs (mask As)
> 710 <7>[79125.023687] ceph:  __touch_cap 000000007071b095 cap 000000004dc11892
> mds0
> 711 <7>[79125.023823] ceph:  lookup 000000007071b095 dentry 00000000f97501b2
> 'dir'
> 712 <4>[79125.023838] ceph_lookup fscrypt_has_encryption_key(dir): 0 ======
> 713 <4>[79125.024186] ceph_lookup fscrypt_has_encryption_key(dir): 1 ======
> 714 <7>[79125.024194] ceph:   dir 000000007071b095 flags are 0x0
> 715 <7>[79125.024269] ceph:  do_request on 00000000a93fafef
>
> I am thinking could we just make __fscrypt_prepare_readdir(), which will return
> 0 when the key is already set or successfully set, to return 1 instead of 0 to
> mark that the key changed ?

OK, I see what you mean.  Thanks.  What about simply detect this change
here instead of changing __fscrypt_prepare_readdir() semantics?  Because I
think that would require changes in several other places, including other
filesystems.

What about something like this?

diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index edc2bf0aab83..499ec75d2496 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -784,6 +784,8 @@ static struct dentry *ceph_lookup(struct inode *dir, struct dentry *dentry,
 		return ERR_PTR(-ENAMETOOLONG);
 
 	if (IS_ENCRYPTED(dir)) {
+		bool had_key = fscrypt_has_encryption_key(dir);
+
 		err = __fscrypt_prepare_readdir(dir);
 		if (err)
 			return ERR_PTR(err);
@@ -791,6 +793,8 @@ static struct dentry *ceph_lookup(struct inode *dir, struct dentry *dentry,
 			spin_lock(&dentry->d_lock);
 			dentry->d_flags |= DCACHE_NOKEY_NAME;
 			spin_unlock(&dentry->d_lock);
+		} else if (!had_key) {
+			__ceph_dir_clear_complete(ceph_inode(dir));
 		}
 	}
 
Cheers,
-- 
Luís

Re: [PATCH] ceph: make sure directories aren't complete after setting crypt context

[Xiubo Li]

On 18/11/2022 19:12, Luís Henriques wrote:
> Xiubo Li <xiubli@redhat.com> writes:
>
>> On 18/11/2022 02:02, Luís Henriques wrote:
>>> Xiubo Li <xiubli@redhat.com> writes:
>>>
>>>> On 17/11/2022 16:03, Xiubo Li wrote:
>>>>> On 16/11/2022 23:37, Luís Henriques wrote:
>>>>>> When setting a directory's crypt context, __ceph_dir_clear_complete() needs
>>>>>> to be used otherwise, if it was complete before, any old dentry that's still
>>>>>> around will be valid.
>>>>>>
>>>>>> Signed-off-by: Luís Henriques <lhenriques@suse.de>
>>>>>> ---
>>>>>> Hi!
>>>>>>
>>>>>> Here's a simple way to trigger the bug this patch is fixing:
>>>>>>
>>>>>> # cd /cephfs
>>>>>> # ls mydir
>>>>>> nKRhofOAVNsAwVLvDw7a0c9ypsjbZfK3n0Npnmni6j0
>>>>>> # ls mydir/nKRhofOAVNsAwVLvDw7a0c9ypsjbZfK3n0Npnmni6j0/
>>>>>> Cyuer5xT+kBlEPgtwAqSj0WK2taEljP5vHZ,D8VXCJ8
>>>>>> u+46b2XVCt7Obpz0gznZyNLRj79Q2l4KmkwbKOzdQKw
>>>>>> # fscrypt unlock mydir
>>>>>> # touch /mnt/test/mydir/mysubdir/file
>>>>>> touch: cannot touch '/mnt/test/mydir/mysubdir/file': No such file or
>>>>>> directory
>>>>>>
>>>>>>     fs/ceph/crypto.c | 4 ++++
>>>>>>     1 file changed, 4 insertions(+)
>>>>>>
>>>>>> diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
>>>>>> index 35a2ccfe6899..dc1557967032 100644
>>>>>> --- a/fs/ceph/crypto.c
>>>>>> +++ b/fs/ceph/crypto.c
>>>>>> @@ -87,6 +87,10 @@ static int ceph_crypt_get_context(struct inode *inode,
>>>>>> void *ctx, size_t len)
>>>>>>             return -ERANGE;
>>>>>>           memcpy(ctx, cfa->cfa_blob, ctxlen);
>>>>>> +
>>>>>> +    /* Directory isn't complete anymore */
>>>>>> +    if (S_ISDIR(inode->i_mode) && __ceph_dir_is_complete(ci))
>>>>>> +        __ceph_dir_clear_complete(ci);
>>>>> Hi Luis,
>>>>>
>>>>> Good catch!
>>>>>
>>>>> BTW, why do this in the ceph_crypt_get_context() ? As my understanding is that
>>>>> we should mark 'mydir' as incomplete when unlocking it. While as I remembered
>>>>> the unlock operation will do:
>>>>>
>>>>>
>>>>> Step1: get_encpolicy via 'mydir' as ctx
>>>>> Step2: rm_enckey of ctx from the superblock
>>>>>
>>>> Sorry, it should be add_enckey.
>>>>> Since I am still running the test cases for the file lock patches, so I didn't
>>>>> catch logs to confirm the above steps yet.
>>>>>
>>>>> If I am right IMO then we should mark the dir as incomplete in the Step2
>>>>> instead, because for non-unlock operations they may also do the Step1.
>>>>>
>>>> Your patch will work. But probably we should do this just around
>>>> __fscrypt_prepare_readdir() or fscrypt_prepare_readdir() instead ? We need to
>>>> detect that once the 'inode->i_crypt_info' changed then mark the dir as
>>>> incomplete.
>>>>
>>>> For now for the lock operation it will evict the inode, which will help do this
>>>> for us already. But for unlock case, we need to handle it by ourself.
>>> OK, that makes sense and to be honest I thought that there should be
>>> another place for doing this. Unfortunately, I didn't found it: in the
>>> test case I have the fscrypt_prepare_readdir() isn't called:
>>>
>>>      # cd /cephfs
>>>      # ls mydir
>>>      nKRhofOAVNsAwVLvDw7a0c9ypsjbZfK3n0Npnmni6j0
>>>      # ls mydir/nKRhofOAVNsAwVLvDw7a0c9ypsjbZfK3n0Npnmni6j0/
>>>      Cyuer5xT+kBlEPgtwAqSj0WK2taEljP5vHZ,D8VXCJ8 u+46b2XVCt7Obpz0gznZyNLRj79Q2l4KmkwbKOzdQKw
>>>
>>> At this point readdir was executed, of course.  And
>>> __ceph_dir_set_complete() is also used to indicate that we have the full
>>> contents.  However, executing the following commands won't result in any
>>> new readdir():
>>>
>>>      # fscrypt unlock mydir
>>>      # touch /mnt/test/mydir/mysubdir/file
>>>
>>> and since the encryption key is set at the sb level, I couldn't find a way
>>> to detect changes in inode->i_crypt_info.  ceph_d_revalidate() is invoked
>>> but at that point I don't thing we have a way to know what is changing.
>>>
>>> Any ideas?
>> # ls mydir/
>> zy94Zt01M90xwYq+nxJsEvea+HYq49mqVgrUBkYrJAU
>> # ls mydir/zy94Zt01M90xwYq+nxJsEvea+HYq49mqVgrUBkYrJAU/
>> 24iNa8ICYc6nZZIkL,4n1sOtp9KiaTGtpq8PCZDQ6LU
>> ivG4rQYzg6YENagbTZaV2kh2sVouEdlkmBZPfgVDl48
>>
>> I have added the debug logs in all the places just like:
>>
>> @@ -784,7 +786,9 @@ static struct dentry *ceph_lookup(struct inode *dir, struct
>> dentry *dentry,
>>                  return ERR_PTR(-ENAMETOOLONG);
>>
>>          if (IS_ENCRYPTED(dir)) {
>> +       printk("%s fscrypt_has_encryption_key(dir): %d ======\n", __func__,
>> fscrypt_has_encryption_key(dir));
>>                  err = __fscrypt_prepare_readdir(dir);
>> +       printk("%s fscrypt_has_encryption_key(dir): %d ======\n", __func__,
>> fscrypt_has_encryption_key(dir));
>>                  if (err)
>>                          return ERR_PTR(err);
>>                  if (!fscrypt_has_encryption_key(dir)) {
>>
>>
>> # fscrypt unlock mydir
>> Enter custom passphrase for protector "l":
>> "mydir" is now unlocked and ready for use.
>>
>> And after "mydir/" was unlocked and then when doing:
>>
>> # touch mydir/dir/file2
>>
>> And when lookup the "dir/" dentry I can see the "mydir/" encryption key changed:
>>
>> 709 <7>[79125.023676] ceph:  __ceph_caps_issued_mask ino 0x10000000004 cap
>> 000000004dc11892 issued pAsLsXsFs (mask As)
>> 710 <7>[79125.023687] ceph:  __touch_cap 000000007071b095 cap 000000004dc11892
>> mds0
>> 711 <7>[79125.023823] ceph:  lookup 000000007071b095 dentry 00000000f97501b2
>> 'dir'
>> 712 <4>[79125.023838] ceph_lookup fscrypt_has_encryption_key(dir): 0 ======
>> 713 <4>[79125.024186] ceph_lookup fscrypt_has_encryption_key(dir): 1 ======
>> 714 <7>[79125.024194] ceph:   dir 000000007071b095 flags are 0x0
>> 715 <7>[79125.024269] ceph:  do_request on 00000000a93fafef
>>
>> I am thinking could we just make __fscrypt_prepare_readdir(), which will return
>> 0 when the key is already set or successfully set, to return 1 instead of 0 to
>> mark that the key changed ?
> OK, I see what you mean.  Thanks.  What about simply detect this change
> here instead of changing __fscrypt_prepare_readdir() semantics?  Because I
> think that would require changes in several other places, including other
> filesystems.
>
> What about something like this?
>
> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> index edc2bf0aab83..499ec75d2496 100644
> --- a/fs/ceph/dir.c
> +++ b/fs/ceph/dir.c
> @@ -784,6 +784,8 @@ static struct dentry *ceph_lookup(struct inode *dir, struct dentry *dentry,
>   		return ERR_PTR(-ENAMETOOLONG);
>   
>   	if (IS_ENCRYPTED(dir)) {
> +		bool had_key = fscrypt_has_encryption_key(dir);
> +
>   		err = __fscrypt_prepare_readdir(dir);
>   		if (err)
>   			return ERR_PTR(err);
> @@ -791,6 +793,8 @@ static struct dentry *ceph_lookup(struct inode *dir, struct dentry *dentry,
>   			spin_lock(&dentry->d_lock);
>   			dentry->d_flags |= DCACHE_NOKEY_NAME;
>   			spin_unlock(&dentry->d_lock);
> +		} else if (!had_key) {
> +			__ceph_dir_clear_complete(ceph_inode(dir));
>   		}
>   	}
>   

Yeah. Could we just add one helper, such as:

bool ceph_fscrypt_prepare_readdir(dir)

{

     bool had_key = fscrypt_has_encryption_key(dir);

     err = __fscrypt_prepare_readdir(dir);

     if (!err && !had_key || err = -ENOKEY && had_key) {

         return true;   // key changed

     }

     return false;  // key not changed

}

Thanks!

- Xiubo

> Cheers,

Re: [PATCH] ceph: make sure directories aren't complete after setting crypt context

[Luís Henriques]

Xiubo Li <xiubli@redhat.com> writes:

> On 18/11/2022 19:12, Luís Henriques wrote:
>> Xiubo Li <xiubli@redhat.com> writes:
>>
>>> On 18/11/2022 02:02, Luís Henriques wrote:
>>>> Xiubo Li <xiubli@redhat.com> writes:
>>>>
>>>>> On 17/11/2022 16:03, Xiubo Li wrote:
>>>>>> On 16/11/2022 23:37, Luís Henriques wrote:
>>>>>>> When setting a directory's crypt context, __ceph_dir_clear_complete() needs
>>>>>>> to be used otherwise, if it was complete before, any old dentry that's still
>>>>>>> around will be valid.
>>>>>>>
>>>>>>> Signed-off-by: Luís Henriques <lhenriques@suse.de>
>>>>>>> ---
>>>>>>> Hi!
>>>>>>>
>>>>>>> Here's a simple way to trigger the bug this patch is fixing:
>>>>>>>
>>>>>>> # cd /cephfs
>>>>>>> # ls mydir
>>>>>>> nKRhofOAVNsAwVLvDw7a0c9ypsjbZfK3n0Npnmni6j0
>>>>>>> # ls mydir/nKRhofOAVNsAwVLvDw7a0c9ypsjbZfK3n0Npnmni6j0/
>>>>>>> Cyuer5xT+kBlEPgtwAqSj0WK2taEljP5vHZ,D8VXCJ8
>>>>>>> u+46b2XVCt7Obpz0gznZyNLRj79Q2l4KmkwbKOzdQKw
>>>>>>> # fscrypt unlock mydir
>>>>>>> # touch /mnt/test/mydir/mysubdir/file
>>>>>>> touch: cannot touch '/mnt/test/mydir/mysubdir/file': No such file or
>>>>>>> directory
>>>>>>>
>>>>>>>     fs/ceph/crypto.c | 4 ++++
>>>>>>>     1 file changed, 4 insertions(+)
>>>>>>>
>>>>>>> diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
>>>>>>> index 35a2ccfe6899..dc1557967032 100644
>>>>>>> --- a/fs/ceph/crypto.c
>>>>>>> +++ b/fs/ceph/crypto.c
>>>>>>> @@ -87,6 +87,10 @@ static int ceph_crypt_get_context(struct inode *inode,
>>>>>>> void *ctx, size_t len)
>>>>>>>             return -ERANGE;
>>>>>>>           memcpy(ctx, cfa->cfa_blob, ctxlen);
>>>>>>> +
>>>>>>> +    /* Directory isn't complete anymore */
>>>>>>> +    if (S_ISDIR(inode->i_mode) && __ceph_dir_is_complete(ci))
>>>>>>> +        __ceph_dir_clear_complete(ci);
>>>>>> Hi Luis,
>>>>>>
>>>>>> Good catch!
>>>>>>
>>>>>> BTW, why do this in the ceph_crypt_get_context() ? As my understanding is that
>>>>>> we should mark 'mydir' as incomplete when unlocking it. While as I remembered
>>>>>> the unlock operation will do:
>>>>>>
>>>>>>
>>>>>> Step1: get_encpolicy via 'mydir' as ctx
>>>>>> Step2: rm_enckey of ctx from the superblock
>>>>>>
>>>>> Sorry, it should be add_enckey.
>>>>>> Since I am still running the test cases for the file lock patches, so I didn't
>>>>>> catch logs to confirm the above steps yet.
>>>>>>
>>>>>> If I am right IMO then we should mark the dir as incomplete in the Step2
>>>>>> instead, because for non-unlock operations they may also do the Step1.
>>>>>>
>>>>> Your patch will work. But probably we should do this just around
>>>>> __fscrypt_prepare_readdir() or fscrypt_prepare_readdir() instead ? We need to
>>>>> detect that once the 'inode->i_crypt_info' changed then mark the dir as
>>>>> incomplete.
>>>>>
>>>>> For now for the lock operation it will evict the inode, which will help do this
>>>>> for us already. But for unlock case, we need to handle it by ourself.
>>>> OK, that makes sense and to be honest I thought that there should be
>>>> another place for doing this. Unfortunately, I didn't found it: in the
>>>> test case I have the fscrypt_prepare_readdir() isn't called:
>>>>
>>>>      # cd /cephfs
>>>>      # ls mydir
>>>>      nKRhofOAVNsAwVLvDw7a0c9ypsjbZfK3n0Npnmni6j0
>>>>      # ls mydir/nKRhofOAVNsAwVLvDw7a0c9ypsjbZfK3n0Npnmni6j0/
>>>>      Cyuer5xT+kBlEPgtwAqSj0WK2taEljP5vHZ,D8VXCJ8 u+46b2XVCt7Obpz0gznZyNLRj79Q2l4KmkwbKOzdQKw
>>>>
>>>> At this point readdir was executed, of course.  And
>>>> __ceph_dir_set_complete() is also used to indicate that we have the full
>>>> contents.  However, executing the following commands won't result in any
>>>> new readdir():
>>>>
>>>>      # fscrypt unlock mydir
>>>>      # touch /mnt/test/mydir/mysubdir/file
>>>>
>>>> and since the encryption key is set at the sb level, I couldn't find a way
>>>> to detect changes in inode->i_crypt_info.  ceph_d_revalidate() is invoked
>>>> but at that point I don't thing we have a way to know what is changing.
>>>>
>>>> Any ideas?
>>> # ls mydir/
>>> zy94Zt01M90xwYq+nxJsEvea+HYq49mqVgrUBkYrJAU
>>> # ls mydir/zy94Zt01M90xwYq+nxJsEvea+HYq49mqVgrUBkYrJAU/
>>> 24iNa8ICYc6nZZIkL,4n1sOtp9KiaTGtpq8PCZDQ6LU
>>> ivG4rQYzg6YENagbTZaV2kh2sVouEdlkmBZPfgVDl48
>>>
>>> I have added the debug logs in all the places just like:
>>>
>>> @@ -784,7 +786,9 @@ static struct dentry *ceph_lookup(struct inode *dir, struct
>>> dentry *dentry,
>>>                  return ERR_PTR(-ENAMETOOLONG);
>>>
>>>          if (IS_ENCRYPTED(dir)) {
>>> +       printk("%s fscrypt_has_encryption_key(dir): %d ======\n", __func__,
>>> fscrypt_has_encryption_key(dir));
>>>                  err = __fscrypt_prepare_readdir(dir);
>>> +       printk("%s fscrypt_has_encryption_key(dir): %d ======\n", __func__,
>>> fscrypt_has_encryption_key(dir));
>>>                  if (err)
>>>                          return ERR_PTR(err);
>>>                  if (!fscrypt_has_encryption_key(dir)) {
>>>
>>>
>>> # fscrypt unlock mydir
>>> Enter custom passphrase for protector "l":
>>> "mydir" is now unlocked and ready for use.
>>>
>>> And after "mydir/" was unlocked and then when doing:
>>>
>>> # touch mydir/dir/file2
>>>
>>> And when lookup the "dir/" dentry I can see the "mydir/" encryption key changed:
>>>
>>> 709 <7>[79125.023676] ceph:  __ceph_caps_issued_mask ino 0x10000000004 cap
>>> 000000004dc11892 issued pAsLsXsFs (mask As)
>>> 710 <7>[79125.023687] ceph:  __touch_cap 000000007071b095 cap 000000004dc11892
>>> mds0
>>> 711 <7>[79125.023823] ceph:  lookup 000000007071b095 dentry 00000000f97501b2
>>> 'dir'
>>> 712 <4>[79125.023838] ceph_lookup fscrypt_has_encryption_key(dir): 0 ======
>>> 713 <4>[79125.024186] ceph_lookup fscrypt_has_encryption_key(dir): 1 ======
>>> 714 <7>[79125.024194] ceph:   dir 000000007071b095 flags are 0x0
>>> 715 <7>[79125.024269] ceph:  do_request on 00000000a93fafef
>>>
>>> I am thinking could we just make __fscrypt_prepare_readdir(), which will return
>>> 0 when the key is already set or successfully set, to return 1 instead of 0 to
>>> mark that the key changed ?
>> OK, I see what you mean.  Thanks.  What about simply detect this change
>> here instead of changing __fscrypt_prepare_readdir() semantics?  Because I
>> think that would require changes in several other places, including other
>> filesystems.
>>
>> What about something like this?
>>
>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
>> index edc2bf0aab83..499ec75d2496 100644
>> --- a/fs/ceph/dir.c
>> +++ b/fs/ceph/dir.c
>> @@ -784,6 +784,8 @@ static struct dentry *ceph_lookup(struct inode *dir, struct dentry *dentry,
>>   		return ERR_PTR(-ENAMETOOLONG);
>>     	if (IS_ENCRYPTED(dir)) {
>> +		bool had_key = fscrypt_has_encryption_key(dir);
>> +
>>   		err = __fscrypt_prepare_readdir(dir);
>>   		if (err)
>>   			return ERR_PTR(err);
>> @@ -791,6 +793,8 @@ static struct dentry *ceph_lookup(struct inode *dir, struct dentry *dentry,
>>   			spin_lock(&dentry->d_lock);
>>   			dentry->d_flags |= DCACHE_NOKEY_NAME;
>>   			spin_unlock(&dentry->d_lock);
>> +		} else if (!had_key) {
>> +			__ceph_dir_clear_complete(ceph_inode(dir));
>>   		}
>>   	}
>>   
>
> Yeah. Could we just add one helper, such as:

Sure, that makes sense.  I'll send out a new version soon.  Thanks!

Cheers,
-- 
Luís


> bool ceph_fscrypt_prepare_readdir(dir)
>
> {
>
>     bool had_key = fscrypt_has_encryption_key(dir);
>
>     err = __fscrypt_prepare_readdir(dir);
>
>     if (!err && !had_key || err = -ENOKEY && had_key) {
>
>         return true;   // key changed
>
>     }
>
>     return false;  // key not changed
>
> }
>
> Thanks!
>
> - Xiubo
>
>> Cheers,
>