Since filenames in encrypted directories are already encrypted and shown
as a base64-encoded string when the directory is locked, snapshot names
should show a similar behaviour.
Signed-off-by: Luís Henriques <[email protected]>
---
fs/ceph/dir.c | 15 +++++++++++++++
fs/ceph/inode.c | 10 +++++++++-
2 files changed, 24 insertions(+), 1 deletion(-)
Support on the MDS for names that'll be > MAX_NAME when base64 encoded is
still TBD. I thought it would be something easy to do, but snapshots
don't seem to make use of the CDir/CDentry (which is where alternate_name
is stored on the MDS). I'm still looking into this, but I may need some
help there :-(
Cheers,
--
Luís
diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index a449f4a07c07..20ae600ee7cd 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -1065,6 +1065,13 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
op = CEPH_MDS_OP_MKSNAP;
dout("mksnap dir %p snap '%pd' dn %p\n", dir,
dentry, dentry);
+ /* XXX missing support for alternate_name in snapshots */
+ if (IS_ENCRYPTED(dir) && (dentry->d_name.len >= 189)) {
+ dout("encrypted snapshot name too long: %pd len: %d\n",
+ dentry, dentry->d_name.len);
+ err = -ENAMETOOLONG;
+ goto out;
+ }
} else if (ceph_snap(dir) == CEPH_NOSNAP) {
dout("mkdir dir %p dn %p mode 0%ho\n", dir, dentry, mode);
op = CEPH_MDS_OP_MKDIR;
@@ -1109,6 +1116,14 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
!req->r_reply_info.head->is_target &&
!req->r_reply_info.head->is_dentry)
err = ceph_handle_notrace_create(dir, dentry);
+
+ /*
+ * If we have created a snapshot we need to clear the cache, otherwise
+ * snapshot will show encrypted filenames in readdir.
+ */
+ if (ceph_snap(dir) == CEPH_SNAPDIR)
+ d_drop(dentry);
+
out_req:
ceph_mdsc_put_request(req);
out:
diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index 8b0832271fdf..080824610b73 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -182,6 +182,13 @@ struct inode *ceph_get_snapdir(struct inode *parent)
ci->i_rbytes = 0;
ci->i_btime = ceph_inode(parent)->i_btime;
+ /* if encrypted, just borough fscrypt_auth from parent */
+ if (IS_ENCRYPTED(parent)) {
+ struct ceph_inode_info *pci = ceph_inode(parent);
+ inode->i_flags |= S_ENCRYPTED;
+ ci->fscrypt_auth_len = pci->fscrypt_auth_len;
+ ci->fscrypt_auth = pci->fscrypt_auth;
+ }
if (inode->i_state & I_NEW) {
inode->i_op = &ceph_snapdir_iops;
inode->i_fop = &ceph_snapdir_fops;
@@ -632,7 +639,8 @@ void ceph_free_inode(struct inode *inode)
kfree(ci->i_symlink);
#ifdef CONFIG_FS_ENCRYPTION
- kfree(ci->fscrypt_auth);
+ if (ceph_snap(inode) != CEPH_SNAPDIR)
+ kfree(ci->fscrypt_auth);
#endif
fscrypt_free_inode(inode);
kmem_cache_free(ceph_inode_cachep, ci);
On 2/24/22 7:21 PM, Luís Henriques wrote:
> Since filenames in encrypted directories are already encrypted and shown
> as a base64-encoded string when the directory is locked, snapshot names
> should show a similar behaviour.
>
> Signed-off-by: Luís Henriques <[email protected]>
> ---
> fs/ceph/dir.c | 15 +++++++++++++++
> fs/ceph/inode.c | 10 +++++++++-
> 2 files changed, 24 insertions(+), 1 deletion(-)
>
> Support on the MDS for names that'll be > MAX_NAME when base64 encoded is
> still TBD. I thought it would be something easy to do, but snapshots
> don't seem to make use of the CDir/CDentry (which is where alternate_name
> is stored on the MDS). I'm still looking into this, but I may need some
> help there :-(
Yeah, good catch. The snapshot handler in MDS hasn't handled this case
yet, though the kclient has passed it to MDS server.
The snapshot alternate_name raw ciphertext should be stored in SnapInfo
struct along with the 'name'.
>
> Cheers,
> --
> Luís
>
> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> index a449f4a07c07..20ae600ee7cd 100644
> --- a/fs/ceph/dir.c
> +++ b/fs/ceph/dir.c
> @@ -1065,6 +1065,13 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
> op = CEPH_MDS_OP_MKSNAP;
> dout("mksnap dir %p snap '%pd' dn %p\n", dir,
> dentry, dentry);
> + /* XXX missing support for alternate_name in snapshots */
> + if (IS_ENCRYPTED(dir) && (dentry->d_name.len >= 189)) {
> + dout("encrypted snapshot name too long: %pd len: %d\n",
> + dentry, dentry->d_name.len);
> + err = -ENAMETOOLONG;
> + goto out;
> + }
We should fix the MDS side bug and then this workaroud will be no needed.
- Xiubo
> } else if (ceph_snap(dir) == CEPH_NOSNAP) {
> dout("mkdir dir %p dn %p mode 0%ho\n", dir, dentry, mode);
> op = CEPH_MDS_OP_MKDIR;
> @@ -1109,6 +1116,14 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
> !req->r_reply_info.head->is_target &&
> !req->r_reply_info.head->is_dentry)
> err = ceph_handle_notrace_create(dir, dentry);
> +
> + /*
> + * If we have created a snapshot we need to clear the cache, otherwise
> + * snapshot will show encrypted filenames in readdir.
> + */
> + if (ceph_snap(dir) == CEPH_SNAPDIR)
> + d_drop(dentry);
> +
> out_req:
> ceph_mdsc_put_request(req);
> out:
> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> index 8b0832271fdf..080824610b73 100644
> --- a/fs/ceph/inode.c
> +++ b/fs/ceph/inode.c
> @@ -182,6 +182,13 @@ struct inode *ceph_get_snapdir(struct inode *parent)
> ci->i_rbytes = 0;
> ci->i_btime = ceph_inode(parent)->i_btime;
>
> + /* if encrypted, just borough fscrypt_auth from parent */
> + if (IS_ENCRYPTED(parent)) {
> + struct ceph_inode_info *pci = ceph_inode(parent);
> + inode->i_flags |= S_ENCRYPTED;
> + ci->fscrypt_auth_len = pci->fscrypt_auth_len;
> + ci->fscrypt_auth = pci->fscrypt_auth;
> + }
> if (inode->i_state & I_NEW) {
> inode->i_op = &ceph_snapdir_iops;
> inode->i_fop = &ceph_snapdir_fops;
> @@ -632,7 +639,8 @@ void ceph_free_inode(struct inode *inode)
>
> kfree(ci->i_symlink);
> #ifdef CONFIG_FS_ENCRYPTION
> - kfree(ci->fscrypt_auth);
> + if (ceph_snap(inode) != CEPH_SNAPDIR)
> + kfree(ci->fscrypt_auth);
> #endif
> fscrypt_free_inode(inode);
> kmem_cache_free(ceph_inode_cachep, ci);
>
On 2/24/22 7:21 PM, Luís Henriques wrote:
> Since filenames in encrypted directories are already encrypted and shown
> as a base64-encoded string when the directory is locked, snapshot names
> should show a similar behaviour.
>
> Signed-off-by: Luís Henriques <[email protected]>
> ---
> fs/ceph/dir.c | 15 +++++++++++++++
> fs/ceph/inode.c | 10 +++++++++-
> 2 files changed, 24 insertions(+), 1 deletion(-)
>
> Support on the MDS for names that'll be > MAX_NAME when base64 encoded is
> still TBD. I thought it would be something easy to do, but snapshots
> don't seem to make use of the CDir/CDentry (which is where alternate_name
> is stored on the MDS). I'm still looking into this, but I may need some
> help there :-(
>
> Cheers,
> --
> Luís
>
> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> index a449f4a07c07..20ae600ee7cd 100644
> --- a/fs/ceph/dir.c
> +++ b/fs/ceph/dir.c
> @@ -1065,6 +1065,13 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
> op = CEPH_MDS_OP_MKSNAP;
> dout("mksnap dir %p snap '%pd' dn %p\n", dir,
> dentry, dentry);
> + /* XXX missing support for alternate_name in snapshots */
> + if (IS_ENCRYPTED(dir) && (dentry->d_name.len >= 189)) {
> + dout("encrypted snapshot name too long: %pd len: %d\n",
> + dentry, dentry->d_name.len);
> + err = -ENAMETOOLONG;
> + goto out;
> + }
> } else if (ceph_snap(dir) == CEPH_NOSNAP) {
> dout("mkdir dir %p dn %p mode 0%ho\n", dir, dentry, mode);
> op = CEPH_MDS_OP_MKDIR;
> @@ -1109,6 +1116,14 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
> !req->r_reply_info.head->is_target &&
> !req->r_reply_info.head->is_dentry)
> err = ceph_handle_notrace_create(dir, dentry);
> +
> + /*
> + * If we have created a snapshot we need to clear the cache, otherwise
> + * snapshot will show encrypted filenames in readdir.
> + */
Do you mean dencrypted filenames ?
- Xiubo
> + if (ceph_snap(dir) == CEPH_SNAPDIR)
> + d_drop(dentry);
> +
> out_req:
> ceph_mdsc_put_request(req);
> out:
> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> index 8b0832271fdf..080824610b73 100644
> --- a/fs/ceph/inode.c
> +++ b/fs/ceph/inode.c
> @@ -182,6 +182,13 @@ struct inode *ceph_get_snapdir(struct inode *parent)
> ci->i_rbytes = 0;
> ci->i_btime = ceph_inode(parent)->i_btime;
>
> + /* if encrypted, just borough fscrypt_auth from parent */
> + if (IS_ENCRYPTED(parent)) {
> + struct ceph_inode_info *pci = ceph_inode(parent);
> + inode->i_flags |= S_ENCRYPTED;
> + ci->fscrypt_auth_len = pci->fscrypt_auth_len;
> + ci->fscrypt_auth = pci->fscrypt_auth;
> + }
> if (inode->i_state & I_NEW) {
> inode->i_op = &ceph_snapdir_iops;
> inode->i_fop = &ceph_snapdir_fops;
> @@ -632,7 +639,8 @@ void ceph_free_inode(struct inode *inode)
>
> kfree(ci->i_symlink);
> #ifdef CONFIG_FS_ENCRYPTION
> - kfree(ci->fscrypt_auth);
> + if (ceph_snap(inode) != CEPH_SNAPDIR)
> + kfree(ci->fscrypt_auth);
> #endif
> fscrypt_free_inode(inode);
> kmem_cache_free(ceph_inode_cachep, ci);
>
On 2/25/22 5:48 PM, Luís Henriques wrote:
> Xiubo Li <[email protected]> writes:
>
>> On 2/24/22 7:21 PM, Luís Henriques wrote:
>>> Since filenames in encrypted directories are already encrypted and shown
>>> as a base64-encoded string when the directory is locked, snapshot names
>>> should show a similar behaviour.
>>>
>>> Signed-off-by: Luís Henriques <[email protected]>
>>> ---
>>> fs/ceph/dir.c | 15 +++++++++++++++
>>> fs/ceph/inode.c | 10 +++++++++-
>>> 2 files changed, 24 insertions(+), 1 deletion(-)
>>>
>>> Support on the MDS for names that'll be > MAX_NAME when base64 encoded is
>>> still TBD. I thought it would be something easy to do, but snapshots
>>> don't seem to make use of the CDir/CDentry (which is where alternate_name
>>> is stored on the MDS). I'm still looking into this, but I may need some
>>> help there :-(
>>>
>>> Cheers,
>>> --
>>> Luís
>>>
>>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
>>> index a449f4a07c07..20ae600ee7cd 100644
>>> --- a/fs/ceph/dir.c
>>> +++ b/fs/ceph/dir.c
>>> @@ -1065,6 +1065,13 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
>>> op = CEPH_MDS_OP_MKSNAP;
>>> dout("mksnap dir %p snap '%pd' dn %p\n", dir,
>>> dentry, dentry);
>>> + /* XXX missing support for alternate_name in snapshots */
>>> + if (IS_ENCRYPTED(dir) && (dentry->d_name.len >= 189)) {
>>> + dout("encrypted snapshot name too long: %pd len: %d\n",
>>> + dentry, dentry->d_name.len);
>>> + err = -ENAMETOOLONG;
>>> + goto out;
>>> + }
>>> } else if (ceph_snap(dir) == CEPH_NOSNAP) {
>>> dout("mkdir dir %p dn %p mode 0%ho\n", dir, dentry, mode);
>>> op = CEPH_MDS_OP_MKDIR;
>>> @@ -1109,6 +1116,14 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
>>> !req->r_reply_info.head->is_target &&
>>> !req->r_reply_info.head->is_dentry)
>>> err = ceph_handle_notrace_create(dir, dentry);
>>> +
>>> + /*
>>> + * If we have created a snapshot we need to clear the cache, otherwise
>>> + * snapshot will show encrypted filenames in readdir.
>>> + */
>> Do you mean dencrypted filenames ?
> What I see without this d_drop() is that, if I run an 'ls' in a snapshot
> directory immediately after creating it, the filenames in that snapshot
> will be encrypted. Maybe there's a bug somewhere else and this d_drop()
> isn't the right fix...?
Maybe should fix this in ceph_fill_trace() in
} else if ((req->r_op == CEPH_MDS_OP_LOOKUPSNAP ||... {
}
?
I still haven't gotten where will encrypt it yet in mksnap case. Because
the MDS will set the 'rinfo->head->is_target' but won't set the
'rinfo->head->is_dentry', so in this case the dentry should keep the
human readable name.
- Xiubo
> Cheers,
Xiubo Li <[email protected]> writes:
> On 2/24/22 7:21 PM, Luís Henriques wrote:
>> Since filenames in encrypted directories are already encrypted and shown
>> as a base64-encoded string when the directory is locked, snapshot names
>> should show a similar behaviour.
>>
>> Signed-off-by: Luís Henriques <[email protected]>
>> ---
>> fs/ceph/dir.c | 15 +++++++++++++++
>> fs/ceph/inode.c | 10 +++++++++-
>> 2 files changed, 24 insertions(+), 1 deletion(-)
>>
>> Support on the MDS for names that'll be > MAX_NAME when base64 encoded is
>> still TBD. I thought it would be something easy to do, but snapshots
>> don't seem to make use of the CDir/CDentry (which is where alternate_name
>> is stored on the MDS). I'm still looking into this, but I may need some
>> help there :-(
>>
>> Cheers,
>> --
>> Luís
>>
>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
>> index a449f4a07c07..20ae600ee7cd 100644
>> --- a/fs/ceph/dir.c
>> +++ b/fs/ceph/dir.c
>> @@ -1065,6 +1065,13 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
>> op = CEPH_MDS_OP_MKSNAP;
>> dout("mksnap dir %p snap '%pd' dn %p\n", dir,
>> dentry, dentry);
>> + /* XXX missing support for alternate_name in snapshots */
>> + if (IS_ENCRYPTED(dir) && (dentry->d_name.len >= 189)) {
>> + dout("encrypted snapshot name too long: %pd len: %d\n",
>> + dentry, dentry->d_name.len);
>> + err = -ENAMETOOLONG;
>> + goto out;
>> + }
>> } else if (ceph_snap(dir) == CEPH_NOSNAP) {
>> dout("mkdir dir %p dn %p mode 0%ho\n", dir, dentry, mode);
>> op = CEPH_MDS_OP_MKDIR;
>> @@ -1109,6 +1116,14 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
>> !req->r_reply_info.head->is_target &&
>> !req->r_reply_info.head->is_dentry)
>> err = ceph_handle_notrace_create(dir, dentry);
>> +
>> + /*
>> + * If we have created a snapshot we need to clear the cache, otherwise
>> + * snapshot will show encrypted filenames in readdir.
>> + */
>
> Do you mean dencrypted filenames ?
What I see without this d_drop() is that, if I run an 'ls' in a snapshot
directory immediately after creating it, the filenames in that snapshot
will be encrypted. Maybe there's a bug somewhere else and this d_drop()
isn't the right fix...?
Cheers,
--
Luís
>
> - Xiubo
>
>
>> + if (ceph_snap(dir) == CEPH_SNAPDIR)
>> + d_drop(dentry);
>> +
>> out_req:
>> ceph_mdsc_put_request(req);
>> out:
>> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
>> index 8b0832271fdf..080824610b73 100644
>> --- a/fs/ceph/inode.c
>> +++ b/fs/ceph/inode.c
>> @@ -182,6 +182,13 @@ struct inode *ceph_get_snapdir(struct inode *parent)
>> ci->i_rbytes = 0;
>> ci->i_btime = ceph_inode(parent)->i_btime;
>> + /* if encrypted, just borough fscrypt_auth from parent */
>> + if (IS_ENCRYPTED(parent)) {
>> + struct ceph_inode_info *pci = ceph_inode(parent);
>> + inode->i_flags |= S_ENCRYPTED;
>> + ci->fscrypt_auth_len = pci->fscrypt_auth_len;
>> + ci->fscrypt_auth = pci->fscrypt_auth;
>> + }
>> if (inode->i_state & I_NEW) {
>> inode->i_op = &ceph_snapdir_iops;
>> inode->i_fop = &ceph_snapdir_fops;
>> @@ -632,7 +639,8 @@ void ceph_free_inode(struct inode *inode)
>> kfree(ci->i_symlink);
>> #ifdef CONFIG_FS_ENCRYPTION
>> - kfree(ci->fscrypt_auth);
>> + if (ceph_snap(inode) != CEPH_SNAPDIR)
>> + kfree(ci->fscrypt_auth);
>> #endif
>> fscrypt_free_inode(inode);
>> kmem_cache_free(ceph_inode_cachep, ci);
>>
>
Xiubo Li <[email protected]> writes:
> On 2/24/22 7:21 PM, Luís Henriques wrote:
>> Since filenames in encrypted directories are already encrypted and shown
>> as a base64-encoded string when the directory is locked, snapshot names
>> should show a similar behaviour.
>>
>> Signed-off-by: Luís Henriques <[email protected]>
>> ---
>> fs/ceph/dir.c | 15 +++++++++++++++
>> fs/ceph/inode.c | 10 +++++++++-
>> 2 files changed, 24 insertions(+), 1 deletion(-)
>>
>> Support on the MDS for names that'll be > MAX_NAME when base64 encoded is
>> still TBD. I thought it would be something easy to do, but snapshots
>> don't seem to make use of the CDir/CDentry (which is where alternate_name
>> is stored on the MDS). I'm still looking into this, but I may need some
>> help there :-(
>
> Yeah, good catch. The snapshot handler in MDS hasn't handled this case yet,
> though the kclient has passed it to MDS server.
>
> The snapshot alternate_name raw ciphertext should be stored in SnapInfo struct
> along with the 'name'.
>
>
>>
>> Cheers,
>> --
>> Luís
>>
>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
>> index a449f4a07c07..20ae600ee7cd 100644
>> --- a/fs/ceph/dir.c
>> +++ b/fs/ceph/dir.c
>> @@ -1065,6 +1065,13 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
>> op = CEPH_MDS_OP_MKSNAP;
>> dout("mksnap dir %p snap '%pd' dn %p\n", dir,
>> dentry, dentry);
>> + /* XXX missing support for alternate_name in snapshots */
>> + if (IS_ENCRYPTED(dir) && (dentry->d_name.len >= 189)) {
>> + dout("encrypted snapshot name too long: %pd len: %d\n",
>> + dentry, dentry->d_name.len);
>> + err = -ENAMETOOLONG;
>> + goto out;
>> + }
>
> We should fix the MDS side bug and then this workaroud will be no needed.
Yep, I've been looking into that too but it's taking a bit to understand
all that's going on there. I'm still trying, but the MDS code (and C++ in
general) is a bit... challenging.
Cheers,
--
Luís
>
> - Xiubo
>
>> } else if (ceph_snap(dir) == CEPH_NOSNAP) {
>> dout("mkdir dir %p dn %p mode 0%ho\n", dir, dentry, mode);
>> op = CEPH_MDS_OP_MKDIR;
>> @@ -1109,6 +1116,14 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
>> !req->r_reply_info.head->is_target &&
>> !req->r_reply_info.head->is_dentry)
>> err = ceph_handle_notrace_create(dir, dentry);
>> +
>> + /*
>> + * If we have created a snapshot we need to clear the cache, otherwise
>> + * snapshot will show encrypted filenames in readdir.
>> + */
>> + if (ceph_snap(dir) == CEPH_SNAPDIR)
>> + d_drop(dentry);
>> +
>> out_req:
>> ceph_mdsc_put_request(req);
>> out:
>> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
>> index 8b0832271fdf..080824610b73 100644
>> --- a/fs/ceph/inode.c
>> +++ b/fs/ceph/inode.c
>> @@ -182,6 +182,13 @@ struct inode *ceph_get_snapdir(struct inode *parent)
>> ci->i_rbytes = 0;
>> ci->i_btime = ceph_inode(parent)->i_btime;
>> + /* if encrypted, just borough fscrypt_auth from parent */
>> + if (IS_ENCRYPTED(parent)) {
>> + struct ceph_inode_info *pci = ceph_inode(parent);
>> + inode->i_flags |= S_ENCRYPTED;
>> + ci->fscrypt_auth_len = pci->fscrypt_auth_len;
>> + ci->fscrypt_auth = pci->fscrypt_auth;
>> + }
>> if (inode->i_state & I_NEW) {
>> inode->i_op = &ceph_snapdir_iops;
>> inode->i_fop = &ceph_snapdir_fops;
>> @@ -632,7 +639,8 @@ void ceph_free_inode(struct inode *inode)
>> kfree(ci->i_symlink);
>> #ifdef CONFIG_FS_ENCRYPTION
>> - kfree(ci->fscrypt_auth);
>> + if (ceph_snap(inode) != CEPH_SNAPDIR)
>> + kfree(ci->fscrypt_auth);
>> #endif
>> fscrypt_free_inode(inode);
>> kmem_cache_free(ceph_inode_cachep, ci);
>>
>
Xiubo Li <[email protected]> writes:
> On 2/25/22 5:48 PM, Luís Henriques wrote:
>> Xiubo Li <[email protected]> writes:
>>
>>> On 2/24/22 7:21 PM, Luís Henriques wrote:
>>>> Since filenames in encrypted directories are already encrypted and shown
>>>> as a base64-encoded string when the directory is locked, snapshot names
>>>> should show a similar behaviour.
>>>>
>>>> Signed-off-by: Luís Henriques <[email protected]>
>>>> ---
>>>> fs/ceph/dir.c | 15 +++++++++++++++
>>>> fs/ceph/inode.c | 10 +++++++++-
>>>> 2 files changed, 24 insertions(+), 1 deletion(-)
>>>>
>>>> Support on the MDS for names that'll be > MAX_NAME when base64 encoded is
>>>> still TBD. I thought it would be something easy to do, but snapshots
>>>> don't seem to make use of the CDir/CDentry (which is where alternate_name
>>>> is stored on the MDS). I'm still looking into this, but I may need some
>>>> help there :-(
>>>>
>>>> Cheers,
>>>> --
>>>> Luís
>>>>
>>>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
>>>> index a449f4a07c07..20ae600ee7cd 100644
>>>> --- a/fs/ceph/dir.c
>>>> +++ b/fs/ceph/dir.c
>>>> @@ -1065,6 +1065,13 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
>>>> op = CEPH_MDS_OP_MKSNAP;
>>>> dout("mksnap dir %p snap '%pd' dn %p\n", dir,
>>>> dentry, dentry);
>>>> + /* XXX missing support for alternate_name in snapshots */
>>>> + if (IS_ENCRYPTED(dir) && (dentry->d_name.len >= 189)) {
>>>> + dout("encrypted snapshot name too long: %pd len: %d\n",
>>>> + dentry, dentry->d_name.len);
>>>> + err = -ENAMETOOLONG;
>>>> + goto out;
>>>> + }
>>>> } else if (ceph_snap(dir) == CEPH_NOSNAP) {
>>>> dout("mkdir dir %p dn %p mode 0%ho\n", dir, dentry, mode);
>>>> op = CEPH_MDS_OP_MKDIR;
>>>> @@ -1109,6 +1116,14 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
>>>> !req->r_reply_info.head->is_target &&
>>>> !req->r_reply_info.head->is_dentry)
>>>> err = ceph_handle_notrace_create(dir, dentry);
>>>> +
>>>> + /*
>>>> + * If we have created a snapshot we need to clear the cache, otherwise
>>>> + * snapshot will show encrypted filenames in readdir.
>>>> + */
>>> Do you mean dencrypted filenames ?
>> What I see without this d_drop() is that, if I run an 'ls' in a snapshot
>> directory immediately after creating it, the filenames in that snapshot
>> will be encrypted. Maybe there's a bug somewhere else and this d_drop()
>> isn't the right fix...?
>
> Maybe should fix this in ceph_fill_trace() in
Hmm... maybe, I'll have to check. Thank's for the suggestion. I'll try
to investigate this before sending a new revision which, hopefully, will
have some MDS-side changes to support the altname (which I'm still trying
to untangle).
Cheers,
--
Luís
>
> } else if ((req->r_op == CEPH_MDS_OP_LOOKUPSNAP ||... {
>
> }
>
> ?
>
> I still haven't gotten where will encrypt it yet in mksnap case. Because the MDS
> will set the 'rinfo->head->is_target' but won't set the
> 'rinfo->head->is_dentry', so in this case the dentry should keep the
> human readable name.
>
> - Xiubo
>
>
>> Cheers,
>
On Thu, 2022-02-24 at 11:21 +0000, Lu?s Henriques wrote:
> Since filenames in encrypted directories are already encrypted and shown
> as a base64-encoded string when the directory is locked, snapshot names
> should show a similar behaviour.
>
> Signed-off-by: Lu?s Henriques <[email protected]>
> ---
> fs/ceph/dir.c | 15 +++++++++++++++
> fs/ceph/inode.c | 10 +++++++++-
> 2 files changed, 24 insertions(+), 1 deletion(-)
>
> Support on the MDS for names that'll be > MAX_NAME when base64 encoded is
> still TBD. I thought it would be something easy to do, but snapshots
> don't seem to make use of the CDir/CDentry (which is where alternate_name
> is stored on the MDS). I'm still looking into this, but I may need some
> help there :-(
>
> Cheers,
> --
> Lu?s
>
> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> index a449f4a07c07..20ae600ee7cd 100644
> --- a/fs/ceph/dir.c
> +++ b/fs/ceph/dir.c
> @@ -1065,6 +1065,13 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
> op = CEPH_MDS_OP_MKSNAP;
> dout("mksnap dir %p snap '%pd' dn %p\n", dir,
> dentry, dentry);
> + /* XXX missing support for alternate_name in snapshots */
> + if (IS_ENCRYPTED(dir) && (dentry->d_name.len >= 189)) {
> + dout("encrypted snapshot name too long: %pd len: %d\n",
> + dentry, dentry->d_name.len);
> + err = -ENAMETOOLONG;
> + goto out;
> + }
Where does 189 come from? You probably want to use CEPH_NOHASH_NAME_MAX.
> } else if (ceph_snap(dir) == CEPH_NOSNAP) {
> dout("mkdir dir %p dn %p mode 0%ho\n", dir, dentry, mode);
> op = CEPH_MDS_OP_MKDIR;
> @@ -1109,6 +1116,14 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
> !req->r_reply_info.head->is_target &&
> !req->r_reply_info.head->is_dentry)
> err = ceph_handle_notrace_create(dir, dentry);
> +
> + /*
> + * If we have created a snapshot we need to clear the cache, otherwise
> + * snapshot will show encrypted filenames in readdir.
> + */
> + if (ceph_snap(dir) == CEPH_SNAPDIR)
> + d_drop(dentry);
> +
This looks hacky, but I just caught up on the discussion between you and
Xiubo, so I assume you're addressing that.
> out_req:
> ceph_mdsc_put_request(req);
> out:
> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> index 8b0832271fdf..080824610b73 100644
> --- a/fs/ceph/inode.c
> +++ b/fs/ceph/inode.c
> @@ -182,6 +182,13 @@ struct inode *ceph_get_snapdir(struct inode *parent)
> ci->i_rbytes = 0;
> ci->i_btime = ceph_inode(parent)->i_btime;
>
> + /* if encrypted, just borough fscrypt_auth from parent */
> + if (IS_ENCRYPTED(parent)) {
> + struct ceph_inode_info *pci = ceph_inode(parent);
> + inode->i_flags |= S_ENCRYPTED;
> + ci->fscrypt_auth_len = pci->fscrypt_auth_len;
> + ci->fscrypt_auth = pci->fscrypt_auth;
> + }
> if (inode->i_state & I_NEW) {
> inode->i_op = &ceph_snapdir_iops;
> inode->i_fop = &ceph_snapdir_fops;
> @@ -632,7 +639,8 @@ void ceph_free_inode(struct inode *inode)
>
> kfree(ci->i_symlink);
> #ifdef CONFIG_FS_ENCRYPTION
> - kfree(ci->fscrypt_auth);
> + if (ceph_snap(inode) != CEPH_SNAPDIR)
> + kfree(ci->fscrypt_auth);
Can a snapdir inode outlive its parent?
> #endif
> fscrypt_free_inode(inode);
> kmem_cache_free(ceph_inode_cachep, ci);
--
Jeff Layton <[email protected]>
On 2/25/22 5:48 PM, Luís Henriques wrote:
> Xiubo Li <[email protected]> writes:
>
>> On 2/24/22 7:21 PM, Luís Henriques wrote:
>>> Since filenames in encrypted directories are already encrypted and shown
>>> as a base64-encoded string when the directory is locked, snapshot names
>>> should show a similar behaviour.
>>>
>>> Signed-off-by: Luís Henriques <[email protected]>
>>> ---
>>> fs/ceph/dir.c | 15 +++++++++++++++
>>> fs/ceph/inode.c | 10 +++++++++-
>>> 2 files changed, 24 insertions(+), 1 deletion(-)
>>>
>>> Support on the MDS for names that'll be > MAX_NAME when base64 encoded is
>>> still TBD. I thought it would be something easy to do, but snapshots
>>> don't seem to make use of the CDir/CDentry (which is where alternate_name
>>> is stored on the MDS). I'm still looking into this, but I may need some
>>> help there :-(
>>>
>>> Cheers,
>>> --
>>> Luís
>>>
>>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
>>> index a449f4a07c07..20ae600ee7cd 100644
>>> --- a/fs/ceph/dir.c
>>> +++ b/fs/ceph/dir.c
>>> @@ -1065,6 +1065,13 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
>>> op = CEPH_MDS_OP_MKSNAP;
>>> dout("mksnap dir %p snap '%pd' dn %p\n", dir,
>>> dentry, dentry);
>>> + /* XXX missing support for alternate_name in snapshots */
>>> + if (IS_ENCRYPTED(dir) && (dentry->d_name.len >= 189)) {
>>> + dout("encrypted snapshot name too long: %pd len: %d\n",
>>> + dentry, dentry->d_name.len);
>>> + err = -ENAMETOOLONG;
>>> + goto out;
>>> + }
>>> } else if (ceph_snap(dir) == CEPH_NOSNAP) {
>>> dout("mkdir dir %p dn %p mode 0%ho\n", dir, dentry, mode);
>>> op = CEPH_MDS_OP_MKDIR;
>>> @@ -1109,6 +1116,14 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
>>> !req->r_reply_info.head->is_target &&
>>> !req->r_reply_info.head->is_dentry)
>>> err = ceph_handle_notrace_create(dir, dentry);
>>> +
>>> + /*
>>> + * If we have created a snapshot we need to clear the cache, otherwise
>>> + * snapshot will show encrypted filenames in readdir.
>>> + */
>> Do you mean dencrypted filenames ?
> What I see without this d_drop() is that, if I run an 'ls' in a snapshot
> directory immediately after creating it, the filenames in that snapshot
> will be encrypted. Maybe there's a bug somewhere else and this d_drop()
> isn't the right fix...?
BTW, how did you reproduce this ?
The snapshot name hasn't encrypted yet ? Did you add one patch to do this ?
Locally I have one patch to support this, but not finished yet:
diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 914a6e68bb56..bc9b2b0c9c66 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -2464,7 +2464,7 @@ static u8 *get_fscrypt_altname(const struct
ceph_mds_request *req, u32 *plen)
char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64
*pbase, int for_wire)
{
struct dentry *cur;
- struct inode *inode;
+ struct inode *inode, *pinode = NULL;
char *path;
int pos;
unsigned seq;
@@ -2485,18 +2485,29 @@ char *ceph_mdsc_build_path(struct dentry
*dentry, int *plen, u64 *pbase, int for
for (;;) {
struct dentry *parent;
+ parent = dget_parent(cur);
spin_lock(&cur->d_lock);
+ pinode = d_inode(parent);
+ ihold(pinode);
+ if (ceph_snap(pinode) == CEPH_SNAPDIR) {
+ struct ceph_vino vino = {
+ .ino = ceph_ino(pinode),
+ .snap = CEPH_NOSNAP,
+ };
+ pinode = ceph_find_inode(pinode->i_sb, vino);
+ BUG_ON(!pinode);
+ }
+
inode = d_inode(cur);
if (inode && ceph_snap(inode) == CEPH_SNAPDIR) {
dout("build_path path+%d: %p SNAPDIR\n",
pos, cur);
spin_unlock(&cur->d_lock);
- parent = dget_parent(cur);
} else if (for_wire && inode && dentry != cur &&
ceph_snap(inode) == CEPH_NOSNAP) {
spin_unlock(&cur->d_lock);
pos++; /* get rid of any prepended '/' */
break;
- } else if (!for_wire ||
!IS_ENCRYPTED(d_inode(cur->d_parent))) {
+ } else if (!for_wire || !IS_ENCRYPTED(pinode)) {
pos -= cur->d_name.len;
if (pos < 0) {
spin_unlock(&cur->d_lock);
@@ -2504,7 +2515,6 @@ char *ceph_mdsc_build_path(struct dentry *dentry,
int *plen, u64 *pbase, int for
}
memcpy(path + pos, cur->d_name.name,
cur->d_name.len);
spin_unlock(&cur->d_lock);
- parent = dget_parent(cur);
} else {
int len, ret;
char buf[FSCRYPT_BASE64URL_CHARS(NAME_MAX)];
@@ -2516,20 +2526,21 @@ char *ceph_mdsc_build_path(struct dentry
*dentry, int *plen, u64 *pbase, int for
memcpy(buf, cur->d_name.name, cur->d_name.len);
len = cur->d_name.len;
spin_unlock(&cur->d_lock);
- parent = dget_parent(cur);
- ret = __fscrypt_prepare_readdir(d_inode(parent));
+ ret = __fscrypt_prepare_readdir(pinode);
if (ret < 0) {
dput(parent);
dput(cur);
+ iput(pinode);
return ERR_PTR(ret);
}
- if (fscrypt_has_encryption_key(d_inode(parent))) {
- len =
ceph_encode_encrypted_fname(d_inode(parent), cur, buf);
+ if (fscrypt_has_encryption_key(pinode)) {
+ len =
ceph_encode_encrypted_fname(pinode, cur, buf);
if (len < 0) {
dput(parent);
dput(cur);
+ iput(pinode);
return ERR_PTR(len);
}
}
@@ -2552,7 +2563,11 @@ char *ceph_mdsc_build_path(struct dentry *dentry,
int *plen, u64 *pbase, int for
break;
path[pos] = '/';
+ iput(pinode);
+ pinode = NULL;
}
+ if (pinode)
+ iput(pinode);
inode = d_inode(cur);
base = inode ? ceph_ino(inode) : 0;
dput(cur);
Are you doing the same thing too ? If so I will leave this to you, or I
can send one patch to support it.
Thanks
- Xiubo
> Cheers,
Xiubo Li <[email protected]> writes:
> On 2/25/22 5:48 PM, Luís Henriques wrote:
>> Xiubo Li <[email protected]> writes:
>>
>>> On 2/24/22 7:21 PM, Luís Henriques wrote:
>>>> Since filenames in encrypted directories are already encrypted and shown
>>>> as a base64-encoded string when the directory is locked, snapshot names
>>>> should show a similar behaviour.
>>>>
>>>> Signed-off-by: Luís Henriques <[email protected]>
>>>> ---
>>>> fs/ceph/dir.c | 15 +++++++++++++++
>>>> fs/ceph/inode.c | 10 +++++++++-
>>>> 2 files changed, 24 insertions(+), 1 deletion(-)
>>>>
>>>> Support on the MDS for names that'll be > MAX_NAME when base64 encoded is
>>>> still TBD. I thought it would be something easy to do, but snapshots
>>>> don't seem to make use of the CDir/CDentry (which is where alternate_name
>>>> is stored on the MDS). I'm still looking into this, but I may need some
>>>> help there :-(
>>>>
>>>> Cheers,
>>>> --
>>>> Luís
>>>>
>>>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
>>>> index a449f4a07c07..20ae600ee7cd 100644
>>>> --- a/fs/ceph/dir.c
>>>> +++ b/fs/ceph/dir.c
>>>> @@ -1065,6 +1065,13 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
>>>> op = CEPH_MDS_OP_MKSNAP;
>>>> dout("mksnap dir %p snap '%pd' dn %p\n", dir,
>>>> dentry, dentry);
>>>> + /* XXX missing support for alternate_name in snapshots */
>>>> + if (IS_ENCRYPTED(dir) && (dentry->d_name.len >= 189)) {
>>>> + dout("encrypted snapshot name too long: %pd len: %d\n",
>>>> + dentry, dentry->d_name.len);
>>>> + err = -ENAMETOOLONG;
>>>> + goto out;
>>>> + }
>>>> } else if (ceph_snap(dir) == CEPH_NOSNAP) {
>>>> dout("mkdir dir %p dn %p mode 0%ho\n", dir, dentry, mode);
>>>> op = CEPH_MDS_OP_MKDIR;
>>>> @@ -1109,6 +1116,14 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
>>>> !req->r_reply_info.head->is_target &&
>>>> !req->r_reply_info.head->is_dentry)
>>>> err = ceph_handle_notrace_create(dir, dentry);
>>>> +
>>>> + /*
>>>> + * If we have created a snapshot we need to clear the cache, otherwise
>>>> + * snapshot will show encrypted filenames in readdir.
>>>> + */
>>> Do you mean dencrypted filenames ?
>> What I see without this d_drop() is that, if I run an 'ls' in a snapshot
>> directory immediately after creating it, the filenames in that snapshot
>> will be encrypted. Maybe there's a bug somewhere else and this d_drop()
>> isn't the right fix...?
>
> BTW, how did you reproduce this ?
>
> The snapshot name hasn't encrypted yet ? Did you add one patch to do this ?
I don't think I understand what you're referring to. I haven't looked
into you patch (probably won't be able to do in before Wednesday) but if
you remove the d_drop() in ceph_mkdir() in this patch, here's what I use
to reproduce the issue:
# mkdir mydir
# fscrypt encrypt mydir
# cd mydir
# create a few files
# mkdir .snap/snapshot-01
# ls -l .snap/snapshot-01
And this would show the contents 'snapshot-01' but with the filenames
encrypted, even with 'mydir' isn't locked.
With this d_drop(), this behaviour will go away, i.e. you'll see the
correct (unencrypted) filenames.
Also, after locking 'mydir' (fscrypt lock mydir), an 'ls' in the snapshot
directory ('ls mydir/.snap') will show the _encrypted_ snapshot names and
an 'ls' in the snapshot itself ('ls mydir/.snap/<ENCRYPTED NAME>') will
show the encrypted filenames as in an 'ls mydir'.
Cheers,
--
Luís
> Locally I have one patch to support this, but not finished yet:
>
>
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index 914a6e68bb56..bc9b2b0c9c66 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -2464,7 +2464,7 @@ static u8 *get_fscrypt_altname(const struct
> ceph_mds_request *req, u32 *plen)
> char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase, int
> for_wire)
> {
> struct dentry *cur;
> - struct inode *inode;
> + struct inode *inode, *pinode = NULL;
> char *path;
> int pos;
> unsigned seq;
> @@ -2485,18 +2485,29 @@ char *ceph_mdsc_build_path(struct dentry *dentry, int
> *plen, u64 *pbase, int for
> for (;;) {
> struct dentry *parent;
>
> + parent = dget_parent(cur);
> spin_lock(&cur->d_lock);
> + pinode = d_inode(parent);
> + ihold(pinode);
> + if (ceph_snap(pinode) == CEPH_SNAPDIR) {
> + struct ceph_vino vino = {
> + .ino = ceph_ino(pinode),
> + .snap = CEPH_NOSNAP,
> + };
> + pinode = ceph_find_inode(pinode->i_sb, vino);
> + BUG_ON(!pinode);
> + }
> +
> inode = d_inode(cur);
> if (inode && ceph_snap(inode) == CEPH_SNAPDIR) {
> dout("build_path path+%d: %p SNAPDIR\n",
> pos, cur);
> spin_unlock(&cur->d_lock);
> - parent = dget_parent(cur);
> } else if (for_wire && inode && dentry != cur &&
> ceph_snap(inode) == CEPH_NOSNAP) {
> spin_unlock(&cur->d_lock);
> pos++; /* get rid of any prepended '/' */
> break;
> - } else if (!for_wire || !IS_ENCRYPTED(d_inode(cur->d_parent))) {
> + } else if (!for_wire || !IS_ENCRYPTED(pinode)) {
> pos -= cur->d_name.len;
> if (pos < 0) {
> spin_unlock(&cur->d_lock);
> @@ -2504,7 +2515,6 @@ char *ceph_mdsc_build_path(struct dentry *dentry, int
> *plen, u64 *pbase, int for
> }
> memcpy(path + pos, cur->d_name.name, cur->d_name.len);
> spin_unlock(&cur->d_lock);
> - parent = dget_parent(cur);
> } else {
> int len, ret;
> char buf[FSCRYPT_BASE64URL_CHARS(NAME_MAX)];
> @@ -2516,20 +2526,21 @@ char *ceph_mdsc_build_path(struct dentry *dentry, int
> *plen, u64 *pbase, int for
> memcpy(buf, cur->d_name.name, cur->d_name.len);
> len = cur->d_name.len;
> spin_unlock(&cur->d_lock);
> - parent = dget_parent(cur);
>
> - ret = __fscrypt_prepare_readdir(d_inode(parent));
> + ret = __fscrypt_prepare_readdir(pinode);
> if (ret < 0) {
> dput(parent);
> dput(cur);
> + iput(pinode);
> return ERR_PTR(ret);
> }
>
> - if (fscrypt_has_encryption_key(d_inode(parent))) {
> - len =
> ceph_encode_encrypted_fname(d_inode(parent), cur, buf);
> + if (fscrypt_has_encryption_key(pinode)) {
> + len = ceph_encode_encrypted_fname(pinode, cur,
> buf);
> if (len < 0) {
> dput(parent);
> dput(cur);
> + iput(pinode);
> return ERR_PTR(len);
> }
> }
> @@ -2552,7 +2563,11 @@ char *ceph_mdsc_build_path(struct dentry *dentry, int
> *plen, u64 *pbase, int for
> break;
>
> path[pos] = '/';
> + iput(pinode);
> + pinode = NULL;
> }
> + if (pinode)
> + iput(pinode);
> inode = d_inode(cur);
> base = inode ? ceph_ino(inode) : 0;
> dput(cur);
>
>
> Are you doing the same thing too ? If so I will leave this to you, or I can send
> one patch to support it.
>
> Thanks
>
> - Xiubo
>
>
>
>
>> Cheers,
>
Jeff Layton <[email protected]> writes:
> On Thu, 2022-02-24 at 11:21 +0000, Luís Henriques wrote:
>> Since filenames in encrypted directories are already encrypted and shown
>> as a base64-encoded string when the directory is locked, snapshot names
>> should show a similar behaviour.
>>
>> Signed-off-by: Luís Henriques <[email protected]>
>> ---
>> fs/ceph/dir.c | 15 +++++++++++++++
>> fs/ceph/inode.c | 10 +++++++++-
>> 2 files changed, 24 insertions(+), 1 deletion(-)
>>
>> Support on the MDS for names that'll be > MAX_NAME when base64 encoded is
>> still TBD. I thought it would be something easy to do, but snapshots
>> don't seem to make use of the CDir/CDentry (which is where alternate_name
>> is stored on the MDS). I'm still looking into this, but I may need some
>> help there :-(
>>
>> Cheers,
>> --
>> Luís
>>
>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
>> index a449f4a07c07..20ae600ee7cd 100644
>> --- a/fs/ceph/dir.c
>> +++ b/fs/ceph/dir.c
>> @@ -1065,6 +1065,13 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
>> op = CEPH_MDS_OP_MKSNAP;
>> dout("mksnap dir %p snap '%pd' dn %p\n", dir,
>> dentry, dentry);
>> + /* XXX missing support for alternate_name in snapshots */
>> + if (IS_ENCRYPTED(dir) && (dentry->d_name.len >= 189)) {
>> + dout("encrypted snapshot name too long: %pd len: %d\n",
>> + dentry, dentry->d_name.len);
>> + err = -ENAMETOOLONG;
>> + goto out;
>> + }
>
> Where does 189 come from? You probably want to use CEPH_NOHASH_NAME_MAX.
>
Yeah, this is just a temporary workaround while the support for altnames
isn't implemented in snapshots. (189 is the max size that will result in
a base64-encoded that is < MAX_NAME; 190 will be result in a filename that
is too long).
>> } else if (ceph_snap(dir) == CEPH_NOSNAP) {
>> dout("mkdir dir %p dn %p mode 0%ho\n", dir, dentry, mode);
>> op = CEPH_MDS_OP_MKDIR;
>> @@ -1109,6 +1116,14 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
>> !req->r_reply_info.head->is_target &&
>> !req->r_reply_info.head->is_dentry)
>> err = ceph_handle_notrace_create(dir, dentry);
>> +
>> + /*
>> + * If we have created a snapshot we need to clear the cache, otherwise
>> + * snapshot will show encrypted filenames in readdir.
>> + */
>> + if (ceph_snap(dir) == CEPH_SNAPDIR)
>> + d_drop(dentry);
>> +
>
> This looks hacky, but I just caught up on the discussion between you and
> Xiubo, so I assume you're addressing that.
Right, I still need to investigate this further. It may actually be a bug
somewhere else. Right now I was trying to get the MDS code written and
decided to look at this later. I just thought I could send out this RFC
anyway in case someone had an idea -- and Xiubo already gave some
suggestions (which I still have to look at...).
>
>> out_req:
>> ceph_mdsc_put_request(req);
>> out:
>> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
>> index 8b0832271fdf..080824610b73 100644
>> --- a/fs/ceph/inode.c
>> +++ b/fs/ceph/inode.c
>> @@ -182,6 +182,13 @@ struct inode *ceph_get_snapdir(struct inode *parent)
>> ci->i_rbytes = 0;
>> ci->i_btime = ceph_inode(parent)->i_btime;
>>
>> + /* if encrypted, just borough fscrypt_auth from parent */
>> + if (IS_ENCRYPTED(parent)) {
>> + struct ceph_inode_info *pci = ceph_inode(parent);
>> + inode->i_flags |= S_ENCRYPTED;
>> + ci->fscrypt_auth_len = pci->fscrypt_auth_len;
>> + ci->fscrypt_auth = pci->fscrypt_auth;
>> + }
>> if (inode->i_state & I_NEW) {
>> inode->i_op = &ceph_snapdir_iops;
>> inode->i_fop = &ceph_snapdir_fops;
>> @@ -632,7 +639,8 @@ void ceph_free_inode(struct inode *inode)
>>
>> kfree(ci->i_symlink);
>> #ifdef CONFIG_FS_ENCRYPTION
>> - kfree(ci->fscrypt_auth);
>> + if (ceph_snap(inode) != CEPH_SNAPDIR)
>> + kfree(ci->fscrypt_auth);
>
> Can a snapdir inode outlive its parent?
Good question. That actually occurred to me and I assumed it can not.
But maybe a better/safer option is to create a new copy of fscrypt_auth
into the snapdir and kfree it here.
Cheers,
--
Luís
>
>> #endif
>> fscrypt_free_inode(inode);
>> kmem_cache_free(ceph_inode_cachep, ci);
>
> --
> Jeff Layton <[email protected]>
On 2/26/22 10:58 PM, Luís Henriques wrote:
> Xiubo Li <[email protected]> writes:
>
>> On 2/25/22 5:48 PM, Luís Henriques wrote:
>>> Xiubo Li <[email protected]> writes:
>>>
>>>> On 2/24/22 7:21 PM, Luís Henriques wrote:
>>>>> Since filenames in encrypted directories are already encrypted and shown
>>>>> as a base64-encoded string when the directory is locked, snapshot names
>>>>> should show a similar behaviour.
>>>>>
>>>>> Signed-off-by: Luís Henriques <[email protected]>
>>>>> ---
>>>>> fs/ceph/dir.c | 15 +++++++++++++++
>>>>> fs/ceph/inode.c | 10 +++++++++-
>>>>> 2 files changed, 24 insertions(+), 1 deletion(-)
>>>>>
>>>>> Support on the MDS for names that'll be > MAX_NAME when base64 encoded is
>>>>> still TBD. I thought it would be something easy to do, but snapshots
>>>>> don't seem to make use of the CDir/CDentry (which is where alternate_name
>>>>> is stored on the MDS). I'm still looking into this, but I may need some
>>>>> help there :-(
>>>>>
>>>>> Cheers,
>>>>> --
>>>>> Luís
>>>>>
>>>>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
>>>>> index a449f4a07c07..20ae600ee7cd 100644
>>>>> --- a/fs/ceph/dir.c
>>>>> +++ b/fs/ceph/dir.c
>>>>> @@ -1065,6 +1065,13 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
>>>>> op = CEPH_MDS_OP_MKSNAP;
>>>>> dout("mksnap dir %p snap '%pd' dn %p\n", dir,
>>>>> dentry, dentry);
>>>>> + /* XXX missing support for alternate_name in snapshots */
>>>>> + if (IS_ENCRYPTED(dir) && (dentry->d_name.len >= 189)) {
>>>>> + dout("encrypted snapshot name too long: %pd len: %d\n",
>>>>> + dentry, dentry->d_name.len);
>>>>> + err = -ENAMETOOLONG;
>>>>> + goto out;
>>>>> + }
>>>>> } else if (ceph_snap(dir) == CEPH_NOSNAP) {
>>>>> dout("mkdir dir %p dn %p mode 0%ho\n", dir, dentry, mode);
>>>>> op = CEPH_MDS_OP_MKDIR;
>>>>> @@ -1109,6 +1116,14 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
>>>>> !req->r_reply_info.head->is_target &&
>>>>> !req->r_reply_info.head->is_dentry)
>>>>> err = ceph_handle_notrace_create(dir, dentry);
>>>>> +
>>>>> + /*
>>>>> + * If we have created a snapshot we need to clear the cache, otherwise
>>>>> + * snapshot will show encrypted filenames in readdir.
>>>>> + */
>>>> Do you mean dencrypted filenames ?
>>> What I see without this d_drop() is that, if I run an 'ls' in a snapshot
>>> directory immediately after creating it, the filenames in that snapshot
>>> will be encrypted. Maybe there's a bug somewhere else and this d_drop()
>>> isn't the right fix...?
>> BTW, how did you reproduce this ?
>>
>> The snapshot name hasn't encrypted yet ? Did you add one patch to do this ?
> I don't think I understand what you're referring to. I haven't looked
> into you patch (probably won't be able to do in before Wednesday) but if
> you remove the d_drop() in ceph_mkdir() in this patch, here's what I use
> to reproduce the issue:
>
> # mkdir mydir
> # fscrypt encrypt mydir
> # cd mydir
> # create a few files
> # mkdir .snap/snapshot-01
> # ls -l .snap/snapshot-01
This is my test by using the branch 'wip-fscrypt' branch:
[root@lxbceph1 kcephfs]# cd mydir/
[root@lxbceph1 mydir]# mkdir a b cd
[root@lxbceph1 mydir]# ls
a b c
[root@lxbceph1 mydir]# ls .snap
[root@lxbceph1 mydir]# mkdir .snap/mydir_snap1
[root@lxbceph1 mydir]# ls .snap/mydir_snap1
a b c
[root@lxbceph1 mydir]# ls .snap/
mydir_snap1
[root@lxbceph1 mydir]# touch file1 file2 file3
[root@lxbceph1 mydir]# mkdir .snap/mydir_snap2
[root@lxbceph1 mydir]# ls .snap/mydir_snap2
a b c file1 file2 file3
[root@lxbceph1 mydir]# ls .snap/
mydir_snap1 mydir_snap2
[root@lxbceph1 mydir]# ls
a b c file1 file2 file3
[root@lxbceph1 mydir]# cd ..
[root@lxbceph1 kcephfs]# fscrypt lock mydir/
"mydir/" is now locked.
[root@lxbceph1 kcephfs]# cd mydir/
[root@lxbceph1 mydir]# ls
5D7Q8T0rdSRiJZnvbpCWRFQnLb3BxO4-zyVHsFCH98o
LWbsdS2rvuCALUXE0TrTqbuElw4zU6cZHg62-LY5GMA
u2nEDclQZAdtetYqQ7aCWNFwu8-1FDH9vI6pM4o6ZN8
-fKqUSM9DGlT1KNE-pkygEXAdjwf9fTDROA_6ZkDEio
m42jW1zJs75o2dx0bEHNmEWx9GmYXxHveSmBFagwPOw
ZLYwBnb7WT78Saz5RFEOdrKn3OLb6AfHk-IElAmEVps
[root@lxbceph1 mydir]# ls .snap/
mydir_snap1 mydir_snap2
[root@lxbceph1 mydir]# ls .snap/mydir_snap1
LWbsdS2rvuCALUXE0TrTqbuElw4zU6cZHg62-LY5GMA
m42jW1zJs75o2dx0bEHNmEWx9GmYXxHveSmBFagwPOw
ZLYwBnb7WT78Saz5RFEOdrKn3OLb6AfHk-IElAmEVps
[root@lxbceph1 mydir]# ls .snap/mydir_snap2
5D7Q8T0rdSRiJZnvbpCWRFQnLb3BxO4-zyVHsFCH98o
LWbsdS2rvuCALUXE0TrTqbuElw4zU6cZHg62-LY5GMA
u2nEDclQZAdtetYqQ7aCWNFwu8-1FDH9vI6pM4o6ZN8
-fKqUSM9DGlT1KNE-pkygEXAdjwf9fTDROA_6ZkDEio
m42jW1zJs75o2dx0bEHNmEWx9GmYXxHveSmBFagwPOw
ZLYwBnb7WT78Saz5RFEOdrKn3OLb6AfHk-IElAmEVps
[root@lxbceph1 mydir]# cd ..
[root@lxbceph1 kcephfs]# fscrypt unlock mydir/
Enter custom passphrase for protector "l":
"mydir/" is now unlocked and ready for use.
[root@lxbceph1 kcephfs]# cd mydir/
[root@lxbceph1 mydir]# ls
a b c file1 file2 file3
[root@lxbceph1 mydir]# ls .snap/mydir_snap1
a b c
[root@lxbceph1 mydir]# ls .snap/mydir_snap2
a b c file1 file2 file3
[root@lxbceph1 mydir]#
We can see that only the "mydir_snap1" and "mydir_snap2" snapshot names
are not encrypted when the 'mydir' is locked, my patch above is fixing
this issue. All the others work as expected.
>
> And this would show the contents 'snapshot-01' but with the filenames
> encrypted, even with 'mydir' isn't locked.
>
> With this d_drop(), this behaviour will go away, i.e. you'll see the
> correct (unencrypted) filenames.
The tests above without this changes in your patch.
> Also, after locking 'mydir' (fscrypt lock mydir), an 'ls' in the snapshot
> directory ('ls mydir/.snap') will show the _encrypted_ snapshot names and
> an 'ls' in the snapshot itself ('ls mydir/.snap/<ENCRYPTED NAME>') will
> show the encrypted filenames as in an 'ls mydir'.
Not sure whether I missed something here, so strange I couldn't
reproduce it.
BTW, which branch were you using to test this ?
I will post my patch to fix the issue I mentioned.
- Xiubo
> Cheers,