Re: [RFC PATCH] ceph: add support for encrypted snapshot names
From: Xiubo Li
Date: Sat Feb 26 2022 - 01:53:18 EST
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 ?
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,