Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Subject: Re: [RFC PATCH 1/2] ceph: add support for encrypted snapshot names
To:     =?UTF-8?Q?Lu=c3=ads_Henriques?= <lhenriques@suse.de>
Cc:     Jeff Layton <jlayton@kernel.org>,
        Ilya Dryomov <idryomov@gmail.com>, ceph-devel@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20220310172616.16212-1-lhenriques@suse.de>
 <20220310172616.16212-2-lhenriques@suse.de>
 <fdf774cd-3cca-14e5-d5aa-44de70bb89f0@redhat.com>
 <2d69e6dd-b047-13fe-7dc5-2c64190e0e8a@redhat.com>
 <cff2b7ac-d4bb-4096-06a9-79b41b31a57a@redhat.com>
 <87o8288jwd.fsf@brahms.olymp>
From:   Xiubo Li <xiubli@redhat.com>
Message-ID: <7aedb4b9-11e4-cfa1-986f-75cf8706c6c0@redhat.com>
Date:   Tue, 15 Mar 2022 15:28:48 +0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.10.1
MIME-Version: 1.0
In-Reply-To: <87o8288jwd.fsf@brahms.olymp>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk


On 3/15/22 2:32 AM, Luís Henriques wrote:
> 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?

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,