Re: [RFC PATCH 1/2] ceph: add support for encrypted snapshot names

[Xiubo Li <xiubli@redhat.com>]

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 <lhenriques@suse.de>
>>> ---
>>>   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;
>>>