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

["Luís Henriques" <lhenriques@suse.de>]

Xiubo Li <xiubli@redhat.com> 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 <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.

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;
>>>>
>