Xiubo Li <xiubli@xxxxxxxxxx> 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@xxxxxxx> >>> --- >>> 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; >>> } >>> >
