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 <[email protected]>
---
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);
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 <[email protected]>
> ---
> 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 ?
Thanks!
- Xiubo
>
Xiubo Li <[email protected]> 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 <[email protected]>
>> ---
>> 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.
Cheers,
--
Luís
On 23/11/2022 17:59, Luís Henriques wrote:
> Xiubo Li <[email protected]> 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 <[email protected]>
>>> ---
>>> 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,
Hi Luis,
BTW, which branch are you using to generate this patch ?
Could you rebase to the testing branch ?
Thanks
- Xiubo
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 <[email protected]>
> ---
> 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);
>
Xiubo Li <[email protected]> writes:
> Hi Luis,
>
> BTW, which branch are you using to generate this patch ?
>
> Could you rebase to the testing branch ?
Hmmm, I think I've used the wip-fscrypt branch. OK, I'll resend this
after running a few tests just to make sure.
Cheers,
--
Luís
>
> Thanks
>
> - Xiubo
>
>
> 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 <[email protected]>
>> ---
>> 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);
>>
>