Xiubo Li <xiubli@xxxxxxxxxx> writes: > On 08/03/2023 02:53, Luís Henriques wrote: >> xiubli@xxxxxxxxxx writes: >> >>> From: Jeff Layton <jlayton@xxxxxxxxxx> >>> >>> 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@xxxxxxxxxx> >>> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx> >>> --- >>> 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
