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 | 9 +++++++++
fs/ceph/inode.c | 13 +++++++++++++
2 files changed, 22 insertions(+)
diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index 6df2a91af236..123e3b9c8161 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -1075,6 +1075,15 @@ 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);
+ /*
+ * Encrypted snapshots require d_revalidate to force a
+ * LOOKUPSNAP to cleanup dcache
+ */
+ if (IS_ENCRYPTED(dir)) {
+ spin_lock(&dentry->d_lock);
+ dentry->d_flags |= DCACHE_NOKEY_NAME;
+ spin_unlock(&dentry->d_lock);
+ }
} 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;
diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index b573a0f33450..81d3d554d261 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -182,6 +182,19 @@ struct inode *ceph_get_snapdir(struct inode *parent)
ci->i_rbytes = 0;
ci->i_btime = ceph_inode(parent)->i_btime;
+ /* if encrypted, just borrow fscrypt_auth from parent */
+ if (IS_ENCRYPTED(parent)) {
+ struct ceph_inode_info *pci = ceph_inode(parent);
+
+ ci->fscrypt_auth = kmemdup(pci->fscrypt_auth,
+ pci->fscrypt_auth_len,
+ GFP_KERNEL);
+ if (ci->fscrypt_auth) {
+ inode->i_flags |= S_ENCRYPTED;
+ ci->fscrypt_auth_len = pci->fscrypt_auth_len;
+ } else
+ dout("Failed to alloc memory for fscrypt_auth in snapdir\n");
+ }
if (inode->i_state & I_NEW) {
inode->i_op = &ceph_snapdir_iops;
inode->i_fop = &ceph_snapdir_fops;
On 3/11/22 1:26 AM, 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 | 9 +++++++++
> fs/ceph/inode.c | 13 +++++++++++++
> 2 files changed, 22 insertions(+)
>
> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> index 6df2a91af236..123e3b9c8161 100644
> --- a/fs/ceph/dir.c
> +++ b/fs/ceph/dir.c
> @@ -1075,6 +1075,15 @@ 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);
> + /*
> + * Encrypted snapshots require d_revalidate to force a
> + * LOOKUPSNAP to cleanup dcache
> + */
> + if (IS_ENCRYPTED(dir)) {
> + spin_lock(&dentry->d_lock);
> + dentry->d_flags |= DCACHE_NOKEY_NAME;
I think this is not correct fix of this issue.
Actually this dentry's name is a KEY NAME, which is human readable name.
DCACHE_NOKEY_NAME means the base64_encoded names. This usually will be
set when filling a new dentry if the directory is locked. If the
directory is unlocked the directory inode will be set with the key.
The root cause should be the snapshot's inode doesn't correctly set the
encrypt stuff when you are reading from it.
NOTE: when you are 'ls -l .snap/snapXXX' the snapXXX dentry name is
correct, it's just corrupted for the file or directory names under snapXXX/.
> + spin_unlock(&dentry->d_lock);
> + }
> } 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;
> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> index b573a0f33450..81d3d554d261 100644
> --- a/fs/ceph/inode.c
> +++ b/fs/ceph/inode.c
> @@ -182,6 +182,19 @@ struct inode *ceph_get_snapdir(struct inode *parent)
> ci->i_rbytes = 0;
> ci->i_btime = ceph_inode(parent)->i_btime;
>
> + /* if encrypted, just borrow fscrypt_auth from parent */
> + if (IS_ENCRYPTED(parent)) {
> + struct ceph_inode_info *pci = ceph_inode(parent);
> +
> + ci->fscrypt_auth = kmemdup(pci->fscrypt_auth,
> + pci->fscrypt_auth_len,
> + GFP_KERNEL);
> + if (ci->fscrypt_auth) {
> + inode->i_flags |= S_ENCRYPTED;
> + ci->fscrypt_auth_len = pci->fscrypt_auth_len;
> + } else
> + dout("Failed to alloc memory for fscrypt_auth in snapdir\n");
> + }
Here I think Jeff has already commented it in your last version, it
should fail by returning NULL ?
- Xiubo
> if (inode->i_state & I_NEW) {
> inode->i_op = &ceph_snapdir_iops;
> inode->i_fop = &ceph_snapdir_fops;
>
On 3/12/22 4:30 PM, Xiubo Li wrote:
>
> On 3/11/22 1:26 AM, 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 | 9 +++++++++
>> fs/ceph/inode.c | 13 +++++++++++++
>> 2 files changed, 22 insertions(+)
>>
>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
>> index 6df2a91af236..123e3b9c8161 100644
>> --- a/fs/ceph/dir.c
>> +++ b/fs/ceph/dir.c
>> @@ -1075,6 +1075,15 @@ 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);
>> + /*
>> + * Encrypted snapshots require d_revalidate to force a
>> + * LOOKUPSNAP to cleanup dcache
>> + */
>> + if (IS_ENCRYPTED(dir)) {
>> + spin_lock(&dentry->d_lock);
>> + dentry->d_flags |= DCACHE_NOKEY_NAME;
>
> I think this is not correct fix of this issue.
>
> Actually this dentry's name is a KEY NAME, which is human readable name.
>
> DCACHE_NOKEY_NAME means the base64_encoded names. This usually will be
> set when filling a new dentry if the directory is locked. If the
> directory is unlocked the directory inode will be set with the key.
>
> The root cause should be the snapshot's inode doesn't correctly set
> the encrypt stuff when you are reading from it.
>
> NOTE: when you are 'ls -l .snap/snapXXX' the snapXXX dentry name is
> correct, it's just corrupted for the file or directory names under
> snapXXX/.
>
When mksnap in ceph_mkdir() before sending the request out it will
create a new inode for the snapshot dentry and then will fill the
ci->fscrypt_auth from .snap's inode, please see
ceph_mkdir()->ceph_new_inode().
And in the mksnap request reply it will try to fill the ci->fscrypt_auth
again but failed because it was already filled. This time the auth info
is from .snap's parent dir from MDS side. In this patch in theory they
should be the same, but I am still not sure why when decrypting the
dentry names in snapXXX will fail.
I just guess it possibly will depend on the inode number from the
related inode or something else. Before the request reply it seems the
inode isn't set the inode number ?
- Xiubo
>
>> + spin_unlock(&dentry->d_lock);
>> + }
>> } 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;
>> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
>> index b573a0f33450..81d3d554d261 100644
>> --- a/fs/ceph/inode.c
>> +++ b/fs/ceph/inode.c
>> @@ -182,6 +182,19 @@ struct inode *ceph_get_snapdir(struct inode
>> *parent)
>> ci->i_rbytes = 0;
>> ci->i_btime = ceph_inode(parent)->i_btime;
>> + /* if encrypted, just borrow fscrypt_auth from parent */
>> + if (IS_ENCRYPTED(parent)) {
>> + struct ceph_inode_info *pci = ceph_inode(parent);
>> +
>> + ci->fscrypt_auth = kmemdup(pci->fscrypt_auth,
>> + pci->fscrypt_auth_len,
>> + GFP_KERNEL);
>> + if (ci->fscrypt_auth) {
>> + inode->i_flags |= S_ENCRYPTED;
>> + ci->fscrypt_auth_len = pci->fscrypt_auth_len;
>> + } else
>> + dout("Failed to alloc memory for fscrypt_auth in
>> snapdir\n");
>> + }
>
> Here I think Jeff has already commented it in your last version, it
> should fail by returning NULL ?
>
> - Xiubo
>
>> if (inode->i_state & I_NEW) {
>> inode->i_op = &ceph_snapdir_iops;
>> inode->i_fop = &ceph_snapdir_fops;
>>
Xiubo Li <[email protected]> writes:
> On 3/14/22 10:45 AM, Xiubo Li wrote:
>>
>> On 3/12/22 4:30 PM, Xiubo Li wrote:
>>>
>>> On 3/11/22 1:26 AM, 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 | 9 +++++++++
>>>> fs/ceph/inode.c | 13 +++++++++++++
>>>> 2 files changed, 22 insertions(+)
>>>>
>>>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
>>>> index 6df2a91af236..123e3b9c8161 100644
>>>> --- a/fs/ceph/dir.c
>>>> +++ b/fs/ceph/dir.c
>>>> @@ -1075,6 +1075,15 @@ 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);
>>>> + /*
>>>> + * Encrypted snapshots require d_revalidate to force a
>>>> + * LOOKUPSNAP to cleanup dcache
>>>> + */
>>>> + if (IS_ENCRYPTED(dir)) {
>>>> + spin_lock(&dentry->d_lock);
>>>> + dentry->d_flags |= DCACHE_NOKEY_NAME;
>>>
>>> I think this is not correct fix of this issue.
>>>
>>> Actually this dentry's name is a KEY NAME, which is human readable name.
>>>
>>> DCACHE_NOKEY_NAME means the base64_encoded names. This usually will be set
>>> when filling a new dentry if the directory is locked. If the directory is
>>> unlocked the directory inode will be set with the key.
>>>
>>> The root cause should be the snapshot's inode doesn't correctly set the
>>> encrypt stuff when you are reading from it.
>>>
>>> NOTE: when you are 'ls -l .snap/snapXXX' the snapXXX dentry name is correct,
>>> it's just corrupted for the file or directory names under snapXXX/.
>>>
>> When mksnap in ceph_mkdir() before sending the request out it will create a
>> new inode for the snapshot dentry and then will fill the ci->fscrypt_auth from
>> .snap's inode, please see ceph_mkdir()->ceph_new_inode().
>>
>> And in the mksnap request reply it will try to fill the ci->fscrypt_auth again
>> but failed because it was already filled. This time the auth info is from
>> .snap's parent dir from MDS side. In this patch in theory they should be the
>> same, but I am still not sure why when decrypting the dentry names in snapXXX
>> will fail.
>>
>> I just guess it possibly will depend on the inode number from the related
>> inode or something else. Before the request reply it seems the inode isn't set
>> the inode number ?
>>
> It should be the ci_nonce's problem.
>
> In the ceph_mkdir()->ceph_new_inode() it will generate a new random nonce and
> then setup the fscrypt context for the inode of .snap/snapXXX. But this context
> is not correct, because the context of .snap/snapXXX should always be inherit
> from .snap's parent, which will be sent from the MDS in the request reply.
Hmm, OK, let me look closer into this. What you're saying makes sense and
you're probably right. Thank you for the hints.
Cheers,
--
Luís
>
>
>> - Xiubo
>>
>>>
>>>> + spin_unlock(&dentry->d_lock);
>>>> + }
>>>> } 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;
>>>> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
>>>> index b573a0f33450..81d3d554d261 100644
>>>> --- a/fs/ceph/inode.c
>>>> +++ b/fs/ceph/inode.c
>>>> @@ -182,6 +182,19 @@ struct inode *ceph_get_snapdir(struct inode *parent)
>>>> ci->i_rbytes = 0;
>>>> ci->i_btime = ceph_inode(parent)->i_btime;
>>>> + /* if encrypted, just borrow fscrypt_auth from parent */
>>>> + if (IS_ENCRYPTED(parent)) {
>>>> + struct ceph_inode_info *pci = ceph_inode(parent);
>>>> +
>>>> + ci->fscrypt_auth = kmemdup(pci->fscrypt_auth,
>>>> + pci->fscrypt_auth_len,
>>>> + GFP_KERNEL);
>>>> + if (ci->fscrypt_auth) {
>>>> + inode->i_flags |= S_ENCRYPTED;
>>>> + ci->fscrypt_auth_len = pci->fscrypt_auth_len;
>>>> + } else
>>>> + dout("Failed to alloc memory for fscrypt_auth in snapdir\n");
>>>> + }
>>>
>>> Here I think Jeff has already commented it in your last version, it should
>>> fail by returning NULL ?
>>>
>>> - Xiubo
>>>
>>>> if (inode->i_state & I_NEW) {
>>>> inode->i_op = &ceph_snapdir_iops;
>>>> inode->i_fop = &ceph_snapdir_fops;
>>>>
>
On 3/14/22 10:45 AM, Xiubo Li wrote:
>
> On 3/12/22 4:30 PM, Xiubo Li wrote:
>>
>> On 3/11/22 1:26 AM, 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 | 9 +++++++++
>>> fs/ceph/inode.c | 13 +++++++++++++
>>> 2 files changed, 22 insertions(+)
>>>
>>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
>>> index 6df2a91af236..123e3b9c8161 100644
>>> --- a/fs/ceph/dir.c
>>> +++ b/fs/ceph/dir.c
>>> @@ -1075,6 +1075,15 @@ 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);
>>> + /*
>>> + * Encrypted snapshots require d_revalidate to force a
>>> + * LOOKUPSNAP to cleanup dcache
>>> + */
>>> + if (IS_ENCRYPTED(dir)) {
>>> + spin_lock(&dentry->d_lock);
>>> + dentry->d_flags |= DCACHE_NOKEY_NAME;
>>
>> I think this is not correct fix of this issue.
>>
>> Actually this dentry's name is a KEY NAME, which is human readable name.
>>
>> DCACHE_NOKEY_NAME means the base64_encoded names. This usually will
>> be set when filling a new dentry if the directory is locked. If the
>> directory is unlocked the directory inode will be set with the key.
>>
>> The root cause should be the snapshot's inode doesn't correctly set
>> the encrypt stuff when you are reading from it.
>>
>> NOTE: when you are 'ls -l .snap/snapXXX' the snapXXX dentry name is
>> correct, it's just corrupted for the file or directory names under
>> snapXXX/.
>>
> When mksnap in ceph_mkdir() before sending the request out it will
> create a new inode for the snapshot dentry and then will fill the
> ci->fscrypt_auth from .snap's inode, please see
> ceph_mkdir()->ceph_new_inode().
>
> And in the mksnap request reply it will try to fill the
> ci->fscrypt_auth again but failed because it was already filled. This
> time the auth info is from .snap's parent dir from MDS side. In this
> patch in theory they should be the same, but I am still not sure why
> when decrypting the dentry names in snapXXX will fail.
>
> I just guess it possibly will depend on the inode number from the
> related inode or something else. Before the request reply it seems the
> inode isn't set the inode number ?
>
It should be the ci_nonce's problem.
In the ceph_mkdir()->ceph_new_inode() it will generate a new random
nonce and then setup the fscrypt context for the inode of .snap/snapXXX.
But this context is not correct, because the context of .snap/snapXXX
should always be inherit from .snap's parent, which will be sent from
the MDS in the request reply.
> - Xiubo
>
>>
>>> + spin_unlock(&dentry->d_lock);
>>> + }
>>> } 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;
>>> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
>>> index b573a0f33450..81d3d554d261 100644
>>> --- a/fs/ceph/inode.c
>>> +++ b/fs/ceph/inode.c
>>> @@ -182,6 +182,19 @@ struct inode *ceph_get_snapdir(struct inode
>>> *parent)
>>> ci->i_rbytes = 0;
>>> ci->i_btime = ceph_inode(parent)->i_btime;
>>> + /* if encrypted, just borrow fscrypt_auth from parent */
>>> + if (IS_ENCRYPTED(parent)) {
>>> + struct ceph_inode_info *pci = ceph_inode(parent);
>>> +
>>> + ci->fscrypt_auth = kmemdup(pci->fscrypt_auth,
>>> + pci->fscrypt_auth_len,
>>> + GFP_KERNEL);
>>> + if (ci->fscrypt_auth) {
>>> + inode->i_flags |= S_ENCRYPTED;
>>> + ci->fscrypt_auth_len = pci->fscrypt_auth_len;
>>> + } else
>>> + dout("Failed to alloc memory for fscrypt_auth in
>>> snapdir\n");
>>> + }
>>
>> Here I think Jeff has already commented it in your last version, it
>> should fail by returning NULL ?
>>
>> - Xiubo
>>
>>> if (inode->i_state & I_NEW) {
>>> inode->i_op = &ceph_snapdir_iops;
>>> inode->i_fop = &ceph_snapdir_fops;
>>>
Xiubo Li <[email protected]> writes:
> On 3/14/22 10:45 AM, Xiubo Li wrote:
>>
>> On 3/12/22 4:30 PM, Xiubo Li wrote:
>>>
>>> On 3/11/22 1:26 AM, 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 | 9 +++++++++
>>>> fs/ceph/inode.c | 13 +++++++++++++
>>>> 2 files changed, 22 insertions(+)
>>>>
>>>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
>>>> index 6df2a91af236..123e3b9c8161 100644
>>>> --- a/fs/ceph/dir.c
>>>> +++ b/fs/ceph/dir.c
>>>> @@ -1075,6 +1075,15 @@ 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);
>>>> + /*
>>>> + * Encrypted snapshots require d_revalidate to force a
>>>> + * LOOKUPSNAP to cleanup dcache
>>>> + */
>>>> + if (IS_ENCRYPTED(dir)) {
>>>> + spin_lock(&dentry->d_lock);
>>>> + dentry->d_flags |= DCACHE_NOKEY_NAME;
>>>
>>> I think this is not correct fix of this issue.
>>>
>>> Actually this dentry's name is a KEY NAME, which is human readable name.
>>>
>>> DCACHE_NOKEY_NAME means the base64_encoded names. This usually will be set
>>> when filling a new dentry if the directory is locked. If the directory is
>>> unlocked the directory inode will be set with the key.
>>>
>>> The root cause should be the snapshot's inode doesn't correctly set the
>>> encrypt stuff when you are reading from it.
>>>
>>> NOTE: when you are 'ls -l .snap/snapXXX' the snapXXX dentry name is correct,
>>> it's just corrupted for the file or directory names under snapXXX/.
>>>
>> When mksnap in ceph_mkdir() before sending the request out it will create a
>> new inode for the snapshot dentry and then will fill the ci->fscrypt_auth from
>> .snap's inode, please see ceph_mkdir()->ceph_new_inode().
>>
>> And in the mksnap request reply it will try to fill the ci->fscrypt_auth again
>> but failed because it was already filled. This time the auth info is from
>> .snap's parent dir from MDS side. In this patch in theory they should be the
>> same, but I am still not sure why when decrypting the dentry names in snapXXX
>> will fail.
>>
>> I just guess it possibly will depend on the inode number from the related
>> inode or something else. Before the request reply it seems the inode isn't set
>> the inode number ?
>>
> It should be the ci_nonce's problem.
OK, you were right. However, I don't see a simple way around it. And I
don't think that adding a fscrypt new interface to copy an existent nonce
makes sense.
So, here's another possible option: instead of setting the
DCACHE_NOKEY_NAME flag, we could simply do d_invalidate(dentry) before
leaving ceph_mkdir (if we're creating an encrypted snapshot, of course).
Would this be acceptable?
Cheers,
--
Luís
> In the ceph_mkdir()->ceph_new_inode() it will generate a new random nonce and
> then setup the fscrypt context for the inode of .snap/snapXXX. But this context
> is not correct, because the context of .snap/snapXXX should always be inherit
> from .snap's parent, which will be sent from the MDS in the request reply.
>
>
>> - Xiubo
>>
>>>
>>>> + spin_unlock(&dentry->d_lock);
>>>> + }
>>>> } 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;
>>>> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
>>>> index b573a0f33450..81d3d554d261 100644
>>>> --- a/fs/ceph/inode.c
>>>> +++ b/fs/ceph/inode.c
>>>> @@ -182,6 +182,19 @@ struct inode *ceph_get_snapdir(struct inode *parent)
>>>> ci->i_rbytes = 0;
>>>> ci->i_btime = ceph_inode(parent)->i_btime;
>>>> + /* if encrypted, just borrow fscrypt_auth from parent */
>>>> + if (IS_ENCRYPTED(parent)) {
>>>> + struct ceph_inode_info *pci = ceph_inode(parent);
>>>> +
>>>> + ci->fscrypt_auth = kmemdup(pci->fscrypt_auth,
>>>> + pci->fscrypt_auth_len,
>>>> + GFP_KERNEL);
>>>> + if (ci->fscrypt_auth) {
>>>> + inode->i_flags |= S_ENCRYPTED;
>>>> + ci->fscrypt_auth_len = pci->fscrypt_auth_len;
>>>> + } else
>>>> + dout("Failed to alloc memory for fscrypt_auth in snapdir\n");
>>>> + }
>>>
>>> Here I think Jeff has already commented it in your last version, it should
>>> fail by returning NULL ?
>>>
>>> - Xiubo
>>>
>>>> if (inode->i_state & I_NEW) {
>>>> inode->i_op = &ceph_snapdir_iops;
>>>> inode->i_fop = &ceph_snapdir_fops;
>>>>
>
Xiubo Li <[email protected]> writes:
<...>
> I think there has one simple way. Just think about without setting the
> fscrypt_auth for the '.snap' dir's inode, that is without your this
> patch it works well.
>
> That's because when we create a snapshot under '.snap' dir, since the '.snap'
> dir related inode doesn't have the fscrypt_auth been filled, so when creating a
> new inode for the snapshot it won't fill the fscrypt_auth for the new inode. And
> then in the handle_reply() it can fill the fscrypt auth as expected.
>
> You can make sure that in the ceph_new_inode() just skip setting the
> fscrypt_auth for the new inode if the parent dir is a snapdir, that is
> '.snap/'. And this will just leave it to be filled in the handle_reply().
Ah! That's it! Great suggestion, I'll go test this and send out a new
version later. (And I think I'll need to rebase my patches on top of the
latest changes too.)
Cheers,
--
Luís
On 3/15/22 2:32 AM, Luís Henriques wrote:
> Xiubo Li <[email protected]> writes:
>
>> On 3/14/22 10:45 AM, Xiubo Li wrote:
>>> On 3/12/22 4:30 PM, Xiubo Li wrote:
>>>> On 3/11/22 1:26 AM, 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 | 9 +++++++++
>>>>> fs/ceph/inode.c | 13 +++++++++++++
>>>>> 2 files changed, 22 insertions(+)
>>>>>
>>>>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
>>>>> index 6df2a91af236..123e3b9c8161 100644
>>>>> --- a/fs/ceph/dir.c
>>>>> +++ b/fs/ceph/dir.c
>>>>> @@ -1075,6 +1075,15 @@ 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);
>>>>> + /*
>>>>> + * Encrypted snapshots require d_revalidate to force a
>>>>> + * LOOKUPSNAP to cleanup dcache
>>>>> + */
>>>>> + if (IS_ENCRYPTED(dir)) {
>>>>> + spin_lock(&dentry->d_lock);
>>>>> + dentry->d_flags |= DCACHE_NOKEY_NAME;
>>>> I think this is not correct fix of this issue.
>>>>
>>>> Actually this dentry's name is a KEY NAME, which is human readable name.
>>>>
>>>> DCACHE_NOKEY_NAME means the base64_encoded names. This usually will be set
>>>> when filling a new dentry if the directory is locked. If the directory is
>>>> unlocked the directory inode will be set with the key.
>>>>
>>>> The root cause should be the snapshot's inode doesn't correctly set the
>>>> encrypt stuff when you are reading from it.
>>>>
>>>> NOTE: when you are 'ls -l .snap/snapXXX' the snapXXX dentry name is correct,
>>>> it's just corrupted for the file or directory names under snapXXX/.
>>>>
>>> When mksnap in ceph_mkdir() before sending the request out it will create a
>>> new inode for the snapshot dentry and then will fill the ci->fscrypt_auth from
>>> .snap's inode, please see ceph_mkdir()->ceph_new_inode().
>>>
>>> And in the mksnap request reply it will try to fill the ci->fscrypt_auth again
>>> but failed because it was already filled. This time the auth info is from
>>> .snap's parent dir from MDS side. In this patch in theory they should be the
>>> same, but I am still not sure why when decrypting the dentry names in snapXXX
>>> will fail.
>>>
>>> I just guess it possibly will depend on the inode number from the related
>>> inode or something else. Before the request reply it seems the inode isn't set
>>> the inode number ?
>>>
>> It should be the ci_nonce's problem.
> OK, you were right. However, I don't see a simple way around it. And I
> don't think that adding a fscrypt new interface to copy an existent nonce
> makes sense.
>
> So, here's another possible option: instead of setting the
> DCACHE_NOKEY_NAME flag, we could simply do d_invalidate(dentry) before
> leaving ceph_mkdir (if we're creating an encrypted snapshot, of course).
> Would this be acceptable?
I think there has one simple way. Just think about without setting the
fscrypt_auth for the '.snap' dir's inode, that is without your this
patch it works well.
That's because when we create a snapshot under '.snap' dir, since the
'.snap' dir related inode doesn't have the fscrypt_auth been filled, so
when creating a new inode for the snapshot it won't fill the
fscrypt_auth for the new inode. And then in the handle_reply() it can
fill the fscrypt auth as expected.
You can make sure that in the ceph_new_inode() just skip setting the
fscrypt_auth for the new inode if the parent dir is a snapdir, that is
'.snap/'. And this will just leave it to be filled in the handle_reply().
-- Xiubo
>
> Cheers,