Messages in this thread | | | Subject | Re: [PATCH] ceph: make sure directories aren't complete after setting crypt context | From | Xiubo Li <> | Date | Mon, 21 Nov 2022 08:53:49 +0800 |
| |
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,
| |