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 ?
I never seen the generic/123 failure yet. And just now I ran the test
for many times locally it worked fine.
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 ?
Thanks
- Xiubo
Cheers,
