Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [RFC PATCH v3 2/4] ceph: handle encrypted snapshot names in
 subdirectories
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: <20220317154521.6615-1-lhenriques@suse.de>
 <20220317154521.6615-3-lhenriques@suse.de>
 <61d831de-1589-3a19-8f46-a162099e75df@redhat.com>
 <878rt7h6qs.fsf@brahms.olymp>
From:   Xiubo Li <xiubli@redhat.com>
Message-ID: <15c60a74-73a9-a509-2b0e-2d9c6bfd9398@redhat.com>
Date:   Fri, 18 Mar 2022 19:28:46 +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: <878rt7h6qs.fsf@brahms.olymp>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk


On 3/18/22 6:53 PM, Luís Henriques wrote:
> Xiubo Li <xiubli@redhat.com> writes:
>
>> On 3/17/22 11:45 PM, Luís Henriques wrote:
>>> When creating a snapshot, the .snap directories for every subdirectory will
>>> show the snapshot name in the "long format":
>>>
>>>     # mkdir .snap/my-snap
>>>     # ls my-dir/.snap/
>>>     _my-snap_1099511627782
>>>
>>> Encrypted snapshots will need to be able to handle these snapshot names by
>>> encrypting/decrypting only the snapshot part of the string ('my-snap').
>>>
>>> Also, since the MDS prevents snapshot names to be bigger than 240 characters
>>> it is necessary to adapt CEPH_NOHASH_NAME_MAX to accommodate this extra
>>> limitation.
>>>
>>> Signed-off-by: Luís Henriques <lhenriques@suse.de>
>>> ---
>>>    fs/ceph/crypto.c | 189 ++++++++++++++++++++++++++++++++++++++++-------
>>>    fs/ceph/crypto.h |  11 ++-
>>>    2 files changed, 169 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
>>> index beb73bbdd868..caa9863dee93 100644
>>> --- a/fs/ceph/crypto.c
>>> +++ b/fs/ceph/crypto.c
>>> @@ -128,16 +128,100 @@ void ceph_fscrypt_as_ctx_to_req(struct ceph_mds_request *req, struct ceph_acl_se
>>>    	swap(req->r_fscrypt_auth, as->fscrypt_auth);
>>>    }
>>>    -int ceph_encode_encrypted_dname(const struct inode *parent, struct qstr
>>> *d_name, char *buf)
>>> +/*
>>> + * User-created snapshots can't start with '_'.  Snapshots that start with this
>>> + * character are special (hint: there aren't real snapshots) and use the
>>> + * following format:
>>> + *
>>> + *   _<SNAPSHOT-NAME>_<INODE-NUMBER>
>>> + *
>>> + * where:
>>> + *  - <SNAPSHOT-NAME> - the real snapshot name that may need to be decrypted,
>>> + *  - <INODE-NUMBER> - the inode number for the actual snapshot
>>> + *
>>> + * This function parses these snapshot names and returns the inode
>>> + * <INODE-NUMBER>.  'name_len' will also bet set with the <SNAPSHOT-NAME>
>>> + * length.
>>> + */
>>> +static struct inode *parse_longname(const struct inode *parent, const char *name,
>>> +				    int *name_len)
>>>    {
>>> +	struct inode *dir = NULL;
>>> +	struct ceph_vino vino = { .snap = CEPH_NOSNAP };
>>> +	char *inode_number;
>>> +	char *name_end;
>>> +	int orig_len = *name_len;
>>> +	int ret = -EIO;
>>> +
>>> +	/* Skip initial '_' */
>>> +	name++;
>>> +	name_end = strrchr(name, '_');
>>> +	if (!name_end) {
>>> +		dout("Failed to parse long snapshot name: %s\n", name);
>>> +		return ERR_PTR(-EIO);
>>> +	}
>>> +	*name_len = (name_end - name);
>>> +	if (*name_len <= 0) {
>>> +		pr_err("Failed to parse long snapshot name\n");
>>> +		return ERR_PTR(-EIO);
>>> +	}
>>> +
>>> +	/* Get the inode number */
>>> +	inode_number = kmemdup_nul(name_end + 1,
>>> +				   orig_len - *name_len - 2,
>>> +				   GFP_KERNEL);
>>> +	if (!inode_number)
>>> +		return ERR_PTR(-ENOMEM);
>>> +	ret = kstrtou64(inode_number, 0, &vino.ino);
>>> +	if (ret) {
>>> +		dout("Failed to parse inode number: %s\n", name);
>>> +		dir = ERR_PTR(ret);
>>> +		goto out;
>>> +	}
>>> +
>>> +	/* And finally the inode */
>>> +	dir = ceph_find_inode(parent->i_sb, vino);
>>> +	if (!dir) {
>>> +		/* This can happen if we're not mounting cephfs on the root */
>>> +		dir = ceph_get_inode(parent->i_sb, vino, NULL);
>> In this case IMO you should lookup the inode from MDS instead create it in the
>> cache, which won't setup the encryption info needed.
>>
>> So later when you try to use this to dencrypt the snapshot names, you will hit
>> errors ? And also the case Jeff mentioned in previous thread could happen.
> No, I don't see any errors.  The reason is that if we get a I_NEW inode,
> we do not have the keys to even decrypt the names.  If you mount a
> filesystem using as root a directory that is inside an encrypted
> directory, you'll see the encrypted snapshot name:
>
>   # mkdir mydir
>   # fscrypt encrypt mydir
>   # mkdir -p mydir/a/b/c/d
>   # mkdir mydir/a/.snap/myspan
>   # umount ...
>   # mount <mon>:<port>:/a
>   # ls .snap
>
> And we simply can't decrypt it because for that we'd need to have access
> to the .fscrypt in the original filesystem mount root.

Should we resolve this issue ? Something like try to copy the .fscrypt 
when mounting '/a' ?

-- Xiubo