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

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

Xiubo Li <xiubli@redhat.com> writes:

> On 2/25/22 5:48 PM, Luís Henriques wrote:
>> Xiubo Li <xiubli@redhat.com> writes:
>>
>>> On 2/24/22 7:21 PM, 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   | 15 +++++++++++++++
>>>>    fs/ceph/inode.c | 10 +++++++++-
>>>>    2 files changed, 24 insertions(+), 1 deletion(-)
>>>>
>>>> Support on the MDS for names that'll be > MAX_NAME when base64 encoded is
>>>> still TBD.  I thought it would be something easy to do, but snapshots
>>>> don't seem to make use of the CDir/CDentry (which is where alternate_name
>>>> is stored on the MDS).  I'm still looking into this, but I may need some
>>>> help there :-(
>>>>
>>>> Cheers,
>>>> --
>>>> Luís
>>>>
>>>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
>>>> index a449f4a07c07..20ae600ee7cd 100644
>>>> --- a/fs/ceph/dir.c
>>>> +++ b/fs/ceph/dir.c
>>>> @@ -1065,6 +1065,13 @@ 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);
>>>> +		/* XXX missing support for alternate_name in snapshots */
>>>> +		if (IS_ENCRYPTED(dir) && (dentry->d_name.len >= 189)) {
>>>> +			dout("encrypted snapshot name too long: %pd len: %d\n",
>>>> +			     dentry, dentry->d_name.len);
>>>> +			err = -ENAMETOOLONG;
>>>> +			goto out;
>>>> +		}
>>>>    	} 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;
>>>> @@ -1109,6 +1116,14 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
>>>>    	    !req->r_reply_info.head->is_target &&
>>>>    	    !req->r_reply_info.head->is_dentry)
>>>>    		err = ceph_handle_notrace_create(dir, dentry);
>>>> +
>>>> +	/*
>>>> +	 * If we have created a snapshot we need to clear the cache, otherwise
>>>> +	 * snapshot will show encrypted filenames in readdir.
>>>> +	 */
>>> Do you mean dencrypted filenames ?
>> What I see without this d_drop() is that, if I run an 'ls' in a snapshot
>> directory immediately after creating it, the filenames in that snapshot
>> will be encrypted.  Maybe there's a bug somewhere else and this d_drop()
>> isn't the right fix...?
>
> BTW, how did you reproduce this ?
>
> The snapshot name hasn't encrypted yet ? Did you add one patch to do this ?

I don't think I understand what you're referring to.  I haven't looked
into you patch (probably won't be able to do in before Wednesday) but if
you remove the d_drop() in ceph_mkdir() in this patch, here's what I use
to reproduce the issue:

	# mkdir mydir
	# fscrypt encrypt mydir
	# cd mydir
	# create a few files
	# mkdir .snap/snapshot-01
	# ls -l .snap/snapshot-01

And this would show the contents 'snapshot-01' but with the filenames
encrypted, even with 'mydir' isn't locked.

With this d_drop(), this behaviour will go away, i.e. you'll see the
correct (unencrypted) filenames.

Also, after locking 'mydir' (fscrypt lock mydir), an 'ls' in the snapshot
directory ('ls mydir/.snap') will show the _encrypted_ snapshot names and
an 'ls' in the snapshot itself ('ls mydir/.snap/<ENCRYPTED NAME>') will
show the encrypted filenames as in an 'ls mydir'.

Cheers,
-- 
Luís


> Locally I have one patch to support this, but not finished yet:
>
>
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index 914a6e68bb56..bc9b2b0c9c66 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -2464,7 +2464,7 @@ static u8 *get_fscrypt_altname(const struct
> ceph_mds_request *req, u32 *plen)
>  char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase, int
> for_wire)
>  {
>         struct dentry *cur;
> -       struct inode *inode;
> +       struct inode *inode, *pinode = NULL;
>         char *path;
>         int pos;
>         unsigned seq;
> @@ -2485,18 +2485,29 @@ char *ceph_mdsc_build_path(struct dentry *dentry, int
> *plen, u64 *pbase, int for
>         for (;;) {
>                 struct dentry *parent;
>
> +               parent = dget_parent(cur);
>                 spin_lock(&cur->d_lock);
> +               pinode = d_inode(parent);
> +               ihold(pinode);
> +               if (ceph_snap(pinode) == CEPH_SNAPDIR) {
> +                       struct ceph_vino vino = {
> +                               .ino = ceph_ino(pinode),
> +                               .snap = CEPH_NOSNAP,
> +                       };
> +                       pinode = ceph_find_inode(pinode->i_sb, vino);
> +                       BUG_ON(!pinode);
> +               }
> +
>                 inode = d_inode(cur);
>                 if (inode && ceph_snap(inode) == CEPH_SNAPDIR) {
>                         dout("build_path path+%d: %p SNAPDIR\n",
>                              pos, cur);
>                         spin_unlock(&cur->d_lock);
> -                       parent = dget_parent(cur);
>                 } else if (for_wire && inode && dentry != cur &&
> ceph_snap(inode) == CEPH_NOSNAP) {
>                         spin_unlock(&cur->d_lock);
>                         pos++; /* get rid of any prepended '/' */
>                         break;
> -               } else if (!for_wire || !IS_ENCRYPTED(d_inode(cur->d_parent))) {
> +               } else if (!for_wire || !IS_ENCRYPTED(pinode)) {
>                         pos -= cur->d_name.len;
>                         if (pos < 0) {
>                                 spin_unlock(&cur->d_lock);
> @@ -2504,7 +2515,6 @@ char *ceph_mdsc_build_path(struct dentry *dentry, int
> *plen, u64 *pbase, int for
>                         }
>                         memcpy(path + pos, cur->d_name.name, cur->d_name.len);
>                         spin_unlock(&cur->d_lock);
> -                       parent = dget_parent(cur);
>                 } else {
>                         int len, ret;
>                         char buf[FSCRYPT_BASE64URL_CHARS(NAME_MAX)];
> @@ -2516,20 +2526,21 @@ char *ceph_mdsc_build_path(struct dentry *dentry, int
> *plen, u64 *pbase, int for
>                         memcpy(buf, cur->d_name.name, cur->d_name.len);
>                         len = cur->d_name.len;
>                         spin_unlock(&cur->d_lock);
> -                       parent = dget_parent(cur);
>
> -                       ret = __fscrypt_prepare_readdir(d_inode(parent));
> +                       ret = __fscrypt_prepare_readdir(pinode);
>                         if (ret < 0) {
>                                 dput(parent);
>                                 dput(cur);
> +                               iput(pinode);
>                                 return ERR_PTR(ret);
>                         }
>
> -                       if (fscrypt_has_encryption_key(d_inode(parent))) {
> -                               len =
>  ceph_encode_encrypted_fname(d_inode(parent), cur, buf);
> +                       if (fscrypt_has_encryption_key(pinode)) {
> +                               len = ceph_encode_encrypted_fname(pinode, cur,
> buf);
>                                 if (len < 0) {
>                                         dput(parent);
>                                         dput(cur);
> +                                       iput(pinode);
>                                         return ERR_PTR(len);
>                                 }
>                         }
> @@ -2552,7 +2563,11 @@ char *ceph_mdsc_build_path(struct dentry *dentry, int
> *plen, u64 *pbase, int for
>                         break;
>
>                 path[pos] = '/';
> +               iput(pinode);
> +               pinode = NULL;
>         }
> +       if (pinode)
> +               iput(pinode);
>         inode = d_inode(cur);
>         base = inode ? ceph_ino(inode) : 0;
>         dput(cur);
>
>
> Are you doing the same thing too ? If so I will leave this to you, or I can send
> one patch to support it.
>
> Thanks
>
> - Xiubo
>
>
>
>
>> Cheers,
>