Xiubo Li <xiubli@xxxxxxxxxx> writes:
> On 2/25/22 5:48 PM, Luís Henriques wrote:
>> Xiubo Li <xiubli@xxxxxxxxxx> 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@xxxxxxx>
>>>> ---
>>>> 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,
>
