Messages in this thread | | | Subject | Re: [PATCH v3] ceph: mark directory as non-complete complete after loading key | From | Xiubo Li <> | Date | Wed, 23 Nov 2022 19:58:02 +0800 |
| |
On 23/11/2022 17:59, Luís Henriques wrote: > Xiubo Li <xiubli@redhat.com> writes: > >> On 22/11/2022 22:59, Luís Henriques wrote: >>> When setting a directory's crypt context, ceph_dir_clear_complete() needs to >>> be called otherwise if it was complete before, any existing (old) dentry will >>> still be valid. >>> >>> This patch adds a wrapper around __fscrypt_prepare_readdir() which will >>> ensure a directory is marked as non-complete if key status changes. >>> >>> Signed-off-by: Luís Henriques <lhenriques@suse.de> >>> --- >>> Hi! >>> >>> Hopefully I'm not breaking anything else. I've run a bunch of fstests and >>> didn't saw any (new) failures. >>> >>> Changes since v2: >>> - Created helper wrapper for __fscrypt_prepare_readdir() >>> - Added calls to the new helper >>> >>> Changes since v1: >>> - Moved the __ceph_dir_clear_complete() call from ceph_crypt_get_context() >>> to ceph_lookup(). >>> - Added an __fscrypt_prepare_readdir() wrapper to check key status changes >>> >>> fs/ceph/crypto.c | 35 +++++++++++++++++++++++++++++++++-- >>> fs/ceph/crypto.h | 6 ++++++ >>> fs/ceph/dir.c | 8 ++++---- >>> fs/ceph/mds_client.c | 6 +++--- >>> 4 files changed, 46 insertions(+), 9 deletions(-) >>> >>> diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c >>> index 35a2ccfe6899..5ef65a06af98 100644 >>> --- a/fs/ceph/crypto.c >>> +++ b/fs/ceph/crypto.c >>> @@ -397,8 +397,8 @@ int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname, >>> goto out_inode; >>> } >>> - ret = __fscrypt_prepare_readdir(dir); >>> - if (ret) >>> + ret = ceph_fscrypt_prepare_readdir(dir); >>> + if (ret < 0) >>> goto out_inode; >>> /* >>> @@ -636,3 +636,34 @@ int ceph_fscrypt_encrypt_pages(struct inode *inode, struct page **page, u64 off, >>> } >>> return ret; >>> } >>> + >>> +/** >>> + * ceph_fscrypt_prepare_readdir - simple __fscrypt_prepare_readdir() wrapper >>> + * @dir: directory inode for readdir prep >>> + * >>> + * Simple wrapper around __fscrypt_prepare_readdir() that will mark directory as >>> + * non-complete if this call results in having the directory unlocked. >>> + * >>> + * Returns: >>> + * 1 - if directory was locked and key is now loaded (i.e. dir is unlocked) >>> + * 0 - if directory is still locked >>> + * < 0 - if __fscrypt_prepare_readdir() fails >>> + */ >>> +int ceph_fscrypt_prepare_readdir(struct inode *dir) >>> +{ >>> + bool had_key = fscrypt_has_encryption_key(dir); >>> + int err; >>> + >>> + if (!IS_ENCRYPTED(dir)) >>> + return 0; >>> + >>> + err = __fscrypt_prepare_readdir(dir); >>> + if (err) >>> + return err; >>> + if (!had_key && fscrypt_has_encryption_key(dir)) { >>> + /* directory just got unlocked, mark it as not complete */ >>> + ceph_dir_clear_complete(dir); >>> + return 1; >>> + } >>> + return 0; >>> +} >>> diff --git a/fs/ceph/crypto.h b/fs/ceph/crypto.h >>> index c6ee993f4ec8..cad53ec916fd 100644 >>> --- a/fs/ceph/crypto.h >>> +++ b/fs/ceph/crypto.h >>> @@ -154,6 +154,7 @@ int ceph_fscrypt_decrypt_extents(struct inode *inode, struct page **page, u64 of >>> struct ceph_sparse_extent *map, u32 ext_cnt); >>> int ceph_fscrypt_encrypt_pages(struct inode *inode, struct page **page, u64 off, >>> int len, gfp_t gfp); >>> +int ceph_fscrypt_prepare_readdir(struct inode *dir); >>> static inline struct page *ceph_fscrypt_pagecache_page(struct page *page) >>> { >>> @@ -254,6 +255,11 @@ static inline struct page *ceph_fscrypt_pagecache_page(struct page *page) >>> { >>> return page; >>> } >>> + >>> +static inline int ceph_fscrypt_prepare_readdir(struct inode *dir) >>> +{ >>> + return 0; >>> +} >>> #endif /* CONFIG_FS_ENCRYPTION */ >>> static inline loff_t ceph_fscrypt_page_offset(struct page *page) >>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c >>> index edc2bf0aab83..5829f27d058d 100644 >>> --- a/fs/ceph/dir.c >>> +++ b/fs/ceph/dir.c >>> @@ -343,8 +343,8 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx) >>> ctx->pos = 2; >>> } >>> - err = fscrypt_prepare_readdir(inode); >>> - if (err) >>> + err = ceph_fscrypt_prepare_readdir(inode); >>> + if (err < 0) >>> return err; >>> spin_lock(&ci->i_ceph_lock); >>> @@ -784,8 +784,8 @@ static struct dentry *ceph_lookup(struct inode *dir, struct dentry *dentry, >>> return ERR_PTR(-ENAMETOOLONG); >>> if (IS_ENCRYPTED(dir)) { >>> - err = __fscrypt_prepare_readdir(dir); >>> - if (err) >>> + err = ceph_fscrypt_prepare_readdir(dir); >>> + if (err < 0) >>> return ERR_PTR(err); >>> if (!fscrypt_has_encryption_key(dir)) { >>> spin_lock(&dentry->d_lock); >>> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c >>> index 9518ac8e407d..4becc9eada4b 100644 >>> --- a/fs/ceph/mds_client.c >>> +++ b/fs/ceph/mds_client.c >>> @@ -2551,8 +2551,8 @@ static u8 *get_fscrypt_altname(const struct ceph_mds_request *req, u32 *plen) >>> if (!IS_ENCRYPTED(dir)) >>> goto success; >>> - ret = __fscrypt_prepare_readdir(dir); >>> - if (ret) >>> + ret = ceph_fscrypt_prepare_readdir(dir); >>> + if (ret < 0) >>> return ERR_PTR(ret); >>> /* No key? Just ignore it. */ >>> @@ -2668,7 +2668,7 @@ char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase, int for >>> spin_unlock(&cur->d_lock); >>> parent = dget_parent(cur); >>> - ret = __fscrypt_prepare_readdir(d_inode(parent)); >>> + ret = ceph_fscrypt_prepare_readdir(d_inode(parent)); >>> if (ret < 0) { >>> dput(parent); >>> dput(cur); >> Hi Luis, >> >> This version LGTM. >> >> Just one nit. I think the following: >> >> if (fscrypt_has_encryption_key(d_inode(parent))) { >> >> is no needed any more. >> >> We can just switch to: >> >> if (ret) { >> >> And also other places ? > No, that won't work, and I found that out the hard way :-) > > If the directory is unlocked (i.e. the key is loaded), then the helper > function ceph_fscrypt_prepare_readdir() will return '0' because the status > hasn't change. Thus, we still need to check that here.
Yeah, sounds reasonable!
LGTM and I will run the test later!
Thanks Luis.
- Xiubo
> Cheers,
| |