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...)
Cheers,
--
Luís
>
> if (flags & LOOKUP_RCU) {
> parent = READ_ONCE(dentry->d_parent);
> dir = d_inode_rcu(parent);
> @@ -1782,8 +1786,8 @@ static int ceph_d_revalidate(struct dentry *dentry, unsigned int flags)
> inode = d_inode(dentry);
> }
>
> - dout("d_revalidate %p '%pd' inode %p offset 0x%llx\n", dentry,
> - dentry, inode, ceph_dentry(dentry)->offset);
> + dout("d_revalidate %p '%pd' inode %p offset 0x%llx nokey %d\n", dentry,
> + dentry, inode, ceph_dentry(dentry)->offset, !!(dentry->d_flags & DCACHE_NOKEY_NAME));
>
> mdsc = ceph_sb_to_client(dir->i_sb)->mdsc;
>
> --
>
> 2.31.1
>
