[RFC PATCH 0/2] Add support for snapshot names encryption

[RFC PATCH 0/2] Add support for snapshot names encryption

[Luís Henriques]

Hi!

So, I've changed this code back into and RFC as I'm not sure yet if this
is it's final form.  I think the 2 patches in this series should probably
be squashed into a single patch.  I decided to keep them separate as the
1st one is simple (it's the same patch I had already sent), and the 2nd
patch adds a lot more complexity to the whole thing.

So, I've looked at Xiubo initial patch for handling snapshots long names.
It was complex, of course, and it required extra MDS changes.  I *think*
my approach is slightly simpler, but I'm not entirely sure yet that I'm
handling every case.

In order to test this code the following PRs are required:

  mds: add protection from clients without fscrypt support #45073
  mds: use the whole string as the snapshot long name #45192
  mds: support alternate names for snapshots #45224
  mds: limit the snapshot names to 240 characters #45312

Comments are welcome, I'm still testing these patches and I do expect to
find that something is still missing.  And I do expect to find bugs.
These strings parsing scares me a lot, but I couldn't see a simpler
approach.

Luís Henriques (2):
  ceph: add support for encrypted snapshot names
  ceph: add support for handling encrypted snapshot names in subtree

 fs/ceph/crypto.c | 146 +++++++++++++++++++++++++++++++++++++++++------
 fs/ceph/crypto.h |   9 ++-
 fs/ceph/dir.c    |   9 +++
 fs/ceph/inode.c  |  13 +++++
 4 files changed, 156 insertions(+), 21 deletions(-)

[RFC PATCH 1/2] ceph: add support for encrypted snapshot names

[Luís Henriques]

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;
+			spin_unlock(&dentry->d_lock);
+		}
 	} 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;
diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index b573a0f33450..81d3d554d261 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -182,6 +182,19 @@ struct inode *ceph_get_snapdir(struct inode *parent)
 	ci->i_rbytes = 0;
 	ci->i_btime = ceph_inode(parent)->i_btime;
 
+	/* if encrypted, just borrow fscrypt_auth from parent */
+	if (IS_ENCRYPTED(parent)) {
+		struct ceph_inode_info *pci = ceph_inode(parent);
+
+		ci->fscrypt_auth = kmemdup(pci->fscrypt_auth,
+					   pci->fscrypt_auth_len,
+					   GFP_KERNEL);
+		if (ci->fscrypt_auth) {
+			inode->i_flags |= S_ENCRYPTED;
+			ci->fscrypt_auth_len = pci->fscrypt_auth_len;
+		} else
+			dout("Failed to alloc memory for fscrypt_auth in snapdir\n");
+	}
 	if (inode->i_state & I_NEW) {
 		inode->i_op = &ceph_snapdir_iops;
 		inode->i_fop = &ceph_snapdir_fops;

[RFC PATCH 2/2] ceph: add support for handling encrypted snapshot names in subtree

[Luís Henriques]

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 | 146 +++++++++++++++++++++++++++++++++++++++++------
 fs/ceph/crypto.h |   9 ++-
 2 files changed, 134 insertions(+), 21 deletions(-)

diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
index 5a87e7385d3f..e315e3650ea7 100644
--- a/fs/ceph/crypto.c
+++ b/fs/ceph/crypto.c
@@ -128,15 +128,89 @@ 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_fname(const struct inode *parent, struct dentry *dentry, 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_get_inode(parent->i_sb, vino, NULL);
+	if (IS_ERR(dir))
+		dout("Can't find inode %s (%s)\n", inode_number, name);
+
+out:
+	kfree(inode_number);
+	return dir;
+}
+
+int ceph_encode_encrypted_fname(struct inode *parent, struct dentry *dentry, char *buf)
 {
+	struct inode *dir = parent;
+	struct qstr iname;
+	int name_len = dentry->d_name.len;
 	u32 len;
 	int elen;
 	int ret;
-	u8 *cryptbuf;
+	u8 *cryptbuf = NULL;
 
 	WARN_ON_ONCE(!fscrypt_has_encryption_key(parent));
 
+	iname.name = dentry->d_name.name;
+	iname.len = dentry->d_name.len;
+
+	/* Handle the special case of snapshot names that start with '_' */
+	if ((ceph_snap(dir) == CEPH_SNAPDIR) && (iname.name[0] == '_')) {
+		dir = parse_longname(parent, iname.name, &name_len);
+		if (IS_ERR(dir))
+			return PTR_ERR(dir);
+		iname.name++; /* skip initial '_' */
+		iname.len = name_len;
+	}
+
 	/*
 	 * convert cleartext dentry name to ciphertext
 	 * if result is longer than CEPH_NOKEY_NAME_MAX,
@@ -144,18 +218,22 @@ int ceph_encode_encrypted_fname(const struct inode *parent, struct dentry *dentr
 	 *
 	 * See: fscrypt_setup_filename
 	 */
-	if (!fscrypt_fname_encrypted_size(parent, dentry->d_name.len, NAME_MAX, &len))
-		return -ENAMETOOLONG;
+	if (!fscrypt_fname_encrypted_size(dir, iname.len, NAME_MAX, &len)) {
+		elen = -ENAMETOOLONG;
+		goto out;
+	}
 
 	/* Allocate a buffer appropriate to hold the result */
 	cryptbuf = kmalloc(len > CEPH_NOHASH_NAME_MAX ? NAME_MAX : len, GFP_KERNEL);
-	if (!cryptbuf)
-		return -ENOMEM;
+	if (!cryptbuf) {
+		elen = -ENOMEM;
+		goto out;
+	}
 
-	ret = fscrypt_fname_encrypt(parent, &dentry->d_name, cryptbuf, len);
+	ret = fscrypt_fname_encrypt(dir, &iname, cryptbuf, len);
 	if (ret) {
-		kfree(cryptbuf);
-		return ret;
+		elen = ret;
+		goto out;
 	}
 
 	/* hash the end if the name is long enough */
@@ -171,8 +249,18 @@ int ceph_encode_encrypted_fname(const struct inode *parent, struct dentry *dentr
 
 	/* base64 encode the encrypted name */
 	elen = fscrypt_base64url_encode(cryptbuf, len, buf);
-	kfree(cryptbuf);
 	dout("base64-encoded ciphertext name = %.*s\n", elen, buf);
+	if ((elen > 0) && (dir != parent)) {
+		char tmp_buf[FSCRYPT_BASE64URL_CHARS(NAME_MAX)];
+
+		elen = sprintf(tmp_buf, "_%.*s_%ld", elen, buf, dir->i_ino);
+		memcpy(buf, tmp_buf, elen);
+	}
+out:
+	kfree(cryptbuf);
+	if (dir != parent)
+		iput(dir);
+
 	return elen;
 }
 
@@ -197,8 +285,11 @@ int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
 	int ret;
 	struct fscrypt_str _tname = FSTR_INIT(NULL, 0);
 	struct fscrypt_str iname;
+	struct inode *dir = fname->dir;
+	char *name = fname->name;
+	int name_len = fname->name_len;
 
-	if (!IS_ENCRYPTED(fname->dir)) {
+	if (!IS_ENCRYPTED(dir)) {
 		oname->name = fname->name;
 		oname->len = fname->name_len;
 		return 0;
@@ -208,20 +299,29 @@ int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
 	if (fname->name_len > FSCRYPT_BASE64URL_CHARS(NAME_MAX))
 		return -EIO;
 
-	ret = __fscrypt_prepare_readdir(fname->dir);
+	/* Handle the special case of snapshot names that start with '_' */
+	if ((ceph_snap(dir) == CEPH_SNAPDIR) && (name[0] == '_')) {
+		dir = parse_longname(dir, name, &name_len);
+		if (IS_ERR(dir))
+			return PTR_ERR(dir);
+		name++; /* skip '_' */
+	}
+
+	ret = __fscrypt_prepare_readdir(dir);
 	if (ret)
-		return ret;
+		goto out_inode;
 
 	/*
 	 * Use the raw dentry name as sent by the MDS instead of
 	 * generating a nokey name via fscrypt.
 	 */
-	if (!fscrypt_has_encryption_key(fname->dir)) {
+	if (!fscrypt_has_encryption_key(dir)) {
 		memcpy(oname->name, fname->name, fname->name_len);
 		oname->len = fname->name_len;
 		if (is_nokey)
 			*is_nokey = true;
-		return 0;
+		ret = 0;
+		goto out_inode;
 	}
 
 	if (fname->ctext_len == 0) {
@@ -230,11 +330,11 @@ int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
 		if (!tname) {
 			ret = fscrypt_fname_alloc_buffer(NAME_MAX, &_tname);
 			if (ret)
-				return ret;
+				goto out_inode;
 			tname = &_tname;
 		}
 
-		declen = fscrypt_base64url_decode(fname->name, fname->name_len, tname->name);
+		declen = fscrypt_base64url_decode(name, name_len, tname->name);
 		if (declen <= 0) {
 			ret = -EIO;
 			goto out;
@@ -246,9 +346,19 @@ int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
 		iname.len = fname->ctext_len;
 	}
 
-	ret = fscrypt_fname_disk_to_usr(fname->dir, 0, 0, &iname, oname);
+	ret = fscrypt_fname_disk_to_usr(dir, 0, 0, &iname, oname);
+	if (!ret && (dir != fname->dir)) {
+		name_len = snprintf(tname->name, tname->len, "_%.*s_%ld",
+				    oname->len, oname->name,
+				    dir->i_ino);
+		memcpy(oname->name, tname->name, name_len);
+		oname->len = name_len;
+	}
 out:
 	fscrypt_fname_free_buffer(&_tname);
+out_inode:
+	if ((dir != fname->dir) && !IS_ERR(dir))
+		iput(dir);
 	return ret;
 }
 
diff --git a/fs/ceph/crypto.h b/fs/ceph/crypto.h
index 1e08f8a64ad6..189af2404165 100644
--- a/fs/ceph/crypto.h
+++ b/fs/ceph/crypto.h
@@ -75,13 +75,16 @@ static inline u32 ceph_fscrypt_auth_len(struct ceph_fscrypt_auth *fa)
  * smaller size. If the ciphertext name is longer than the value below, then
  * sha256 hash the remaining bytes.
  *
- * 189 bytes => 252 bytes base64-encoded, which is <= NAME_MAX (255)
+ * 180 bytes => 240 bytes base64-encoded, which is <= NAME_MAX (255)
+ *
+ * (Note: 240 bytes is the maximum size allowed for snapshot names to take into
+ *  account the format: '_<SNAPSHOT-NAME>_<INODE-NUMBER>')
  *
  * Note that for long names that end up having their tail portion hashed, we
  * must also store the full encrypted name (in the dentry's alternate_name
  * field).
  */
-#define CEPH_NOHASH_NAME_MAX (189 - SHA256_DIGEST_SIZE)
+#define CEPH_NOHASH_NAME_MAX (180 - SHA256_DIGEST_SIZE)
 
 void ceph_fscrypt_set_ops(struct super_block *sb);
 
@@ -90,7 +93,7 @@ void ceph_fscrypt_free_dummy_policy(struct ceph_fs_client *fsc);
 int ceph_fscrypt_prepare_context(struct inode *dir, struct inode *inode,
 				 struct ceph_acl_sec_ctx *as);
 void ceph_fscrypt_as_ctx_to_req(struct ceph_mds_request *req, struct ceph_acl_sec_ctx *as);
-int ceph_encode_encrypted_fname(const struct inode *parent, struct dentry *dentry, char *buf);
+int ceph_encode_encrypted_fname(struct inode *parent, struct dentry *dentry, char *buf);
 
 static inline int ceph_fname_alloc_buffer(struct inode *parent, struct fscrypt_str *fname)
 {

Re: [RFC PATCH 0/2] Add support for snapshot names encryption

[Luís Henriques]

Luís Henriques <lhenriques@suse.de> writes:

> Hi!
>
> So, I've changed this code back into and RFC as I'm not sure yet if this
> is it's final form.  I think the 2 patches in this series should probably
> be squashed into a single patch.  I decided to keep them separate as the
> 1st one is simple (it's the same patch I had already sent), and the 2nd
> patch adds a lot more complexity to the whole thing.
>
> So, I've looked at Xiubo initial patch for handling snapshots long names.
> It was complex, of course, and it required extra MDS changes.  I *think*
> my approach is slightly simpler, but I'm not entirely sure yet that I'm
> handling every case.
>
> In order to test this code the following PRs are required:
>
>   mds: add protection from clients without fscrypt support #45073
>   mds: use the whole string as the snapshot long name #45192
>   mds: support alternate names for snapshots #45224
>   mds: limit the snapshot names to 240 characters #45312
>
> Comments are welcome, I'm still testing these patches and I do expect to
> find that something is still missing.  And I do expect to find bugs.
> These strings parsing scares me a lot, but I couldn't see a simpler
> approach.

Again, I forgot to mention in the cover-letter that handling
base64-encoded snapshots that start with '_' is still missing.  That's
next on my list.

Cheers,
-- 
Luís

>
> Luís Henriques (2):
>   ceph: add support for encrypted snapshot names
>   ceph: add support for handling encrypted snapshot names in subtree
>
>  fs/ceph/crypto.c | 146 +++++++++++++++++++++++++++++++++++++++++------
>  fs/ceph/crypto.h |   9 ++-
>  fs/ceph/dir.c    |   9 +++
>  fs/ceph/inode.c  |  13 +++++
>  4 files changed, 156 insertions(+), 21 deletions(-)
>

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

[Xiubo Li]

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/.


> +			spin_unlock(&dentry->d_lock);
> +		}
>   	} 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;
> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> index b573a0f33450..81d3d554d261 100644
> --- a/fs/ceph/inode.c
> +++ b/fs/ceph/inode.c
> @@ -182,6 +182,19 @@ struct inode *ceph_get_snapdir(struct inode *parent)
>   	ci->i_rbytes = 0;
>   	ci->i_btime = ceph_inode(parent)->i_btime;
>   
> +	/* if encrypted, just borrow fscrypt_auth from parent */
> +	if (IS_ENCRYPTED(parent)) {
> +		struct ceph_inode_info *pci = ceph_inode(parent);
> +
> +		ci->fscrypt_auth = kmemdup(pci->fscrypt_auth,
> +					   pci->fscrypt_auth_len,
> +					   GFP_KERNEL);
> +		if (ci->fscrypt_auth) {
> +			inode->i_flags |= S_ENCRYPTED;
> +			ci->fscrypt_auth_len = pci->fscrypt_auth_len;
> +		} else
> +			dout("Failed to alloc memory for fscrypt_auth in snapdir\n");
> +	}

Here I think Jeff has already commented it in your last version, it 
should fail by returning NULL ?

- Xiubo

>   	if (inode->i_state & I_NEW) {
>   		inode->i_op = &ceph_snapdir_iops;
>   		inode->i_fop = &ceph_snapdir_fops;
>

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

[Xiubo Li]

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 ?

- Xiubo

>
>> + spin_unlock(&dentry->d_lock);
>> +        }
>>       } 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;
>> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
>> index b573a0f33450..81d3d554d261 100644
>> --- a/fs/ceph/inode.c
>> +++ b/fs/ceph/inode.c
>> @@ -182,6 +182,19 @@ struct inode *ceph_get_snapdir(struct inode 
>> *parent)
>>       ci->i_rbytes = 0;
>>       ci->i_btime = ceph_inode(parent)->i_btime;
>>   +    /* if encrypted, just borrow fscrypt_auth from parent */
>> +    if (IS_ENCRYPTED(parent)) {
>> +        struct ceph_inode_info *pci = ceph_inode(parent);
>> +
>> +        ci->fscrypt_auth = kmemdup(pci->fscrypt_auth,
>> +                       pci->fscrypt_auth_len,
>> +                       GFP_KERNEL);
>> +        if (ci->fscrypt_auth) {
>> +            inode->i_flags |= S_ENCRYPTED;
>> +            ci->fscrypt_auth_len = pci->fscrypt_auth_len;
>> +        } else
>> +            dout("Failed to alloc memory for fscrypt_auth in 
>> snapdir\n");
>> +    }
>
> Here I think Jeff has already commented it in your last version, it 
> should fail by returning NULL ?
>
> - Xiubo
>
>>       if (inode->i_state & I_NEW) {
>>           inode->i_op = &ceph_snapdir_iops;
>>           inode->i_fop = &ceph_snapdir_fops;
>>

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

[Xiubo Li]

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.

In the ceph_mkdir()->ceph_new_inode() it will generate a new random 
nonce and then setup the fscrypt context for the inode of .snap/snapXXX. 
But this context is not correct, because the context of .snap/snapXXX 
should always be inherit from .snap's parent, which will be sent from 
the MDS in the request reply.


> - Xiubo
>
>>
>>> + spin_unlock(&dentry->d_lock);
>>> +        }
>>>       } 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;
>>> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
>>> index b573a0f33450..81d3d554d261 100644
>>> --- a/fs/ceph/inode.c
>>> +++ b/fs/ceph/inode.c
>>> @@ -182,6 +182,19 @@ struct inode *ceph_get_snapdir(struct inode 
>>> *parent)
>>>       ci->i_rbytes = 0;
>>>       ci->i_btime = ceph_inode(parent)->i_btime;
>>>   +    /* if encrypted, just borrow fscrypt_auth from parent */
>>> +    if (IS_ENCRYPTED(parent)) {
>>> +        struct ceph_inode_info *pci = ceph_inode(parent);
>>> +
>>> +        ci->fscrypt_auth = kmemdup(pci->fscrypt_auth,
>>> +                       pci->fscrypt_auth_len,
>>> +                       GFP_KERNEL);
>>> +        if (ci->fscrypt_auth) {
>>> +            inode->i_flags |= S_ENCRYPTED;
>>> +            ci->fscrypt_auth_len = pci->fscrypt_auth_len;
>>> +        } else
>>> +            dout("Failed to alloc memory for fscrypt_auth in 
>>> snapdir\n");
>>> +    }
>>
>> Here I think Jeff has already commented it in your last version, it 
>> should fail by returning NULL ?
>>
>> - Xiubo
>>
>>>       if (inode->i_state & I_NEW) {
>>>           inode->i_op = &ceph_snapdir_iops;
>>>           inode->i_fop = &ceph_snapdir_fops;
>>>

Re: [RFC PATCH 2/2] ceph: add support for handling encrypted snapshot names in subtree

[Xiubo Li]

On 3/11/22 1:26 AM, 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.

Maybe we should update this info in Documentation/filesystems/ceph.rst.

- Xiubo


>
> Signed-off-by: Luís Henriques <lhenriques@suse.de>
> ---
>   fs/ceph/crypto.c | 146 +++++++++++++++++++++++++++++++++++++++++------
>   fs/ceph/crypto.h |   9 ++-
>   2 files changed, 134 insertions(+), 21 deletions(-)
>
> diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
> index 5a87e7385d3f..e315e3650ea7 100644
> --- a/fs/ceph/crypto.c
> +++ b/fs/ceph/crypto.c
> @@ -128,15 +128,89 @@ 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_fname(const struct inode *parent, struct dentry *dentry, 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_get_inode(parent->i_sb, vino, NULL);
> +	if (IS_ERR(dir))
> +		dout("Can't find inode %s (%s)\n", inode_number, name);
> +
> +out:
> +	kfree(inode_number);
> +	return dir;
> +}
> +
> +int ceph_encode_encrypted_fname(struct inode *parent, struct dentry *dentry, char *buf)
>   {
> +	struct inode *dir = parent;
> +	struct qstr iname;
> +	int name_len = dentry->d_name.len;
>   	u32 len;
>   	int elen;
>   	int ret;
> -	u8 *cryptbuf;
> +	u8 *cryptbuf = NULL;
>   
>   	WARN_ON_ONCE(!fscrypt_has_encryption_key(parent));
>   
> +	iname.name = dentry->d_name.name;
> +	iname.len = dentry->d_name.len;
> +
> +	/* Handle the special case of snapshot names that start with '_' */
> +	if ((ceph_snap(dir) == CEPH_SNAPDIR) && (iname.name[0] == '_')) {
> +		dir = parse_longname(parent, iname.name, &name_len);
> +		if (IS_ERR(dir))
> +			return PTR_ERR(dir);
> +		iname.name++; /* skip initial '_' */
> +		iname.len = name_len;
> +	}
> +
>   	/*
>   	 * convert cleartext dentry name to ciphertext
>   	 * if result is longer than CEPH_NOKEY_NAME_MAX,
> @@ -144,18 +218,22 @@ int ceph_encode_encrypted_fname(const struct inode *parent, struct dentry *dentr
>   	 *
>   	 * See: fscrypt_setup_filename
>   	 */
> -	if (!fscrypt_fname_encrypted_size(parent, dentry->d_name.len, NAME_MAX, &len))
> -		return -ENAMETOOLONG;
> +	if (!fscrypt_fname_encrypted_size(dir, iname.len, NAME_MAX, &len)) {
> +		elen = -ENAMETOOLONG;
> +		goto out;
> +	}
>   
>   	/* Allocate a buffer appropriate to hold the result */
>   	cryptbuf = kmalloc(len > CEPH_NOHASH_NAME_MAX ? NAME_MAX : len, GFP_KERNEL);
> -	if (!cryptbuf)
> -		return -ENOMEM;
> +	if (!cryptbuf) {
> +		elen = -ENOMEM;
> +		goto out;
> +	}
>   
> -	ret = fscrypt_fname_encrypt(parent, &dentry->d_name, cryptbuf, len);
> +	ret = fscrypt_fname_encrypt(dir, &iname, cryptbuf, len);
>   	if (ret) {
> -		kfree(cryptbuf);
> -		return ret;
> +		elen = ret;
> +		goto out;
>   	}
>   
>   	/* hash the end if the name is long enough */
> @@ -171,8 +249,18 @@ int ceph_encode_encrypted_fname(const struct inode *parent, struct dentry *dentr
>   
>   	/* base64 encode the encrypted name */
>   	elen = fscrypt_base64url_encode(cryptbuf, len, buf);
> -	kfree(cryptbuf);
>   	dout("base64-encoded ciphertext name = %.*s\n", elen, buf);
> +	if ((elen > 0) && (dir != parent)) {
> +		char tmp_buf[FSCRYPT_BASE64URL_CHARS(NAME_MAX)];
> +
> +		elen = sprintf(tmp_buf, "_%.*s_%ld", elen, buf, dir->i_ino);
> +		memcpy(buf, tmp_buf, elen);
> +	}
> +out:
> +	kfree(cryptbuf);
> +	if (dir != parent)
> +		iput(dir);
> +
>   	return elen;
>   }
>   
> @@ -197,8 +285,11 @@ int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
>   	int ret;
>   	struct fscrypt_str _tname = FSTR_INIT(NULL, 0);
>   	struct fscrypt_str iname;
> +	struct inode *dir = fname->dir;
> +	char *name = fname->name;
> +	int name_len = fname->name_len;
>   
> -	if (!IS_ENCRYPTED(fname->dir)) {
> +	if (!IS_ENCRYPTED(dir)) {
>   		oname->name = fname->name;
>   		oname->len = fname->name_len;
>   		return 0;
> @@ -208,20 +299,29 @@ int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
>   	if (fname->name_len > FSCRYPT_BASE64URL_CHARS(NAME_MAX))
>   		return -EIO;
>   
> -	ret = __fscrypt_prepare_readdir(fname->dir);
> +	/* Handle the special case of snapshot names that start with '_' */
> +	if ((ceph_snap(dir) == CEPH_SNAPDIR) && (name[0] == '_')) {
> +		dir = parse_longname(dir, name, &name_len);
> +		if (IS_ERR(dir))
> +			return PTR_ERR(dir);
> +		name++; /* skip '_' */
> +	}
> +
> +	ret = __fscrypt_prepare_readdir(dir);
>   	if (ret)
> -		return ret;
> +		goto out_inode;
>   
>   	/*
>   	 * Use the raw dentry name as sent by the MDS instead of
>   	 * generating a nokey name via fscrypt.
>   	 */
> -	if (!fscrypt_has_encryption_key(fname->dir)) {
> +	if (!fscrypt_has_encryption_key(dir)) {
>   		memcpy(oname->name, fname->name, fname->name_len);
>   		oname->len = fname->name_len;
>   		if (is_nokey)
>   			*is_nokey = true;
> -		return 0;
> +		ret = 0;
> +		goto out_inode;
>   	}
>   
>   	if (fname->ctext_len == 0) {
> @@ -230,11 +330,11 @@ int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
>   		if (!tname) {
>   			ret = fscrypt_fname_alloc_buffer(NAME_MAX, &_tname);
>   			if (ret)
> -				return ret;
> +				goto out_inode;
>   			tname = &_tname;
>   		}
>   
> -		declen = fscrypt_base64url_decode(fname->name, fname->name_len, tname->name);
> +		declen = fscrypt_base64url_decode(name, name_len, tname->name);
>   		if (declen <= 0) {
>   			ret = -EIO;
>   			goto out;
> @@ -246,9 +346,19 @@ int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
>   		iname.len = fname->ctext_len;
>   	}
>   
> -	ret = fscrypt_fname_disk_to_usr(fname->dir, 0, 0, &iname, oname);
> +	ret = fscrypt_fname_disk_to_usr(dir, 0, 0, &iname, oname);
> +	if (!ret && (dir != fname->dir)) {
> +		name_len = snprintf(tname->name, tname->len, "_%.*s_%ld",
> +				    oname->len, oname->name,
> +				    dir->i_ino);
> +		memcpy(oname->name, tname->name, name_len);
> +		oname->len = name_len;
> +	}
>   out:
>   	fscrypt_fname_free_buffer(&_tname);
> +out_inode:
> +	if ((dir != fname->dir) && !IS_ERR(dir))
> +		iput(dir);
>   	return ret;
>   }
>   
> diff --git a/fs/ceph/crypto.h b/fs/ceph/crypto.h
> index 1e08f8a64ad6..189af2404165 100644
> --- a/fs/ceph/crypto.h
> +++ b/fs/ceph/crypto.h
> @@ -75,13 +75,16 @@ static inline u32 ceph_fscrypt_auth_len(struct ceph_fscrypt_auth *fa)
>    * smaller size. If the ciphertext name is longer than the value below, then
>    * sha256 hash the remaining bytes.
>    *
> - * 189 bytes => 252 bytes base64-encoded, which is <= NAME_MAX (255)
> + * 180 bytes => 240 bytes base64-encoded, which is <= NAME_MAX (255)
> + *
> + * (Note: 240 bytes is the maximum size allowed for snapshot names to take into
> + *  account the format: '_<SNAPSHOT-NAME>_<INODE-NUMBER>')
>    *
>    * Note that for long names that end up having their tail portion hashed, we
>    * must also store the full encrypted name (in the dentry's alternate_name
>    * field).
>    */
> -#define CEPH_NOHASH_NAME_MAX (189 - SHA256_DIGEST_SIZE)
> +#define CEPH_NOHASH_NAME_MAX (180 - SHA256_DIGEST_SIZE)
>   
>   void ceph_fscrypt_set_ops(struct super_block *sb);
>   
> @@ -90,7 +93,7 @@ void ceph_fscrypt_free_dummy_policy(struct ceph_fs_client *fsc);
>   int ceph_fscrypt_prepare_context(struct inode *dir, struct inode *inode,
>   				 struct ceph_acl_sec_ctx *as);
>   void ceph_fscrypt_as_ctx_to_req(struct ceph_mds_request *req, struct ceph_acl_sec_ctx *as);
> -int ceph_encode_encrypted_fname(const struct inode *parent, struct dentry *dentry, char *buf);
> +int ceph_encode_encrypted_fname(struct inode *parent, struct dentry *dentry, char *buf);
>   
>   static inline int ceph_fname_alloc_buffer(struct inode *parent, struct fscrypt_str *fname)
>   {
>

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

[Luís Henriques]

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.
>
> In the ceph_mkdir()->ceph_new_inode() it will generate a new random nonce and
> then setup the fscrypt context for the inode of .snap/snapXXX. But this context
> is not correct, because the context of .snap/snapXXX should always be inherit
> from .snap's parent, which will be sent from the MDS in the request reply.

Hmm, OK, let me look closer into this.  What you're saying makes sense and
you're probably right.  Thank you for the hints.

Cheers,
-- 
Luís

>
>
>> - Xiubo
>>
>>>
>>>> + spin_unlock(&dentry->d_lock);
>>>> +        }
>>>>       } 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;
>>>> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
>>>> index b573a0f33450..81d3d554d261 100644
>>>> --- a/fs/ceph/inode.c
>>>> +++ b/fs/ceph/inode.c
>>>> @@ -182,6 +182,19 @@ struct inode *ceph_get_snapdir(struct inode *parent)
>>>>       ci->i_rbytes = 0;
>>>>       ci->i_btime = ceph_inode(parent)->i_btime;
>>>>   +    /* if encrypted, just borrow fscrypt_auth from parent */
>>>> +    if (IS_ENCRYPTED(parent)) {
>>>> +        struct ceph_inode_info *pci = ceph_inode(parent);
>>>> +
>>>> +        ci->fscrypt_auth = kmemdup(pci->fscrypt_auth,
>>>> +                       pci->fscrypt_auth_len,
>>>> +                       GFP_KERNEL);
>>>> +        if (ci->fscrypt_auth) {
>>>> +            inode->i_flags |= S_ENCRYPTED;
>>>> +            ci->fscrypt_auth_len = pci->fscrypt_auth_len;
>>>> +        } else
>>>> +            dout("Failed to alloc memory for fscrypt_auth in snapdir\n");
>>>> +    }
>>>
>>> Here I think Jeff has already commented it in your last version, it should
>>> fail by returning NULL ?
>>>
>>> - Xiubo
>>>
>>>>       if (inode->i_state & I_NEW) {
>>>>           inode->i_op = &ceph_snapdir_iops;
>>>>           inode->i_fop = &ceph_snapdir_fops;
>>>>
>

Re: [RFC PATCH 2/2] ceph: add support for handling encrypted snapshot names in subtree

[Luís Henriques]

Xiubo Li <xiubli@redhat.com> writes:

> On 3/11/22 1:26 AM, 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.
>
> Maybe we should update this info in Documentation/filesystems/ceph.rst.

Yep, sure.  This definitely needs to be documented somewhere.

Cheers,
-- 
Luís

>
> - Xiubo
>
>
>>
>> Signed-off-by: Luís Henriques <lhenriques@suse.de>
>> ---
>>   fs/ceph/crypto.c | 146 +++++++++++++++++++++++++++++++++++++++++------
>>   fs/ceph/crypto.h |   9 ++-
>>   2 files changed, 134 insertions(+), 21 deletions(-)
>>
>> diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
>> index 5a87e7385d3f..e315e3650ea7 100644
>> --- a/fs/ceph/crypto.c
>> +++ b/fs/ceph/crypto.c
>> @@ -128,15 +128,89 @@ 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_fname(const struct inode *parent, struct dentry
>> *dentry, 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_get_inode(parent->i_sb, vino, NULL);
>> +	if (IS_ERR(dir))
>> +		dout("Can't find inode %s (%s)\n", inode_number, name);
>> +
>> +out:
>> +	kfree(inode_number);
>> +	return dir;
>> +}
>> +
>> +int ceph_encode_encrypted_fname(struct inode *parent, struct dentry *dentry, char *buf)
>>   {
>> +	struct inode *dir = parent;
>> +	struct qstr iname;
>> +	int name_len = dentry->d_name.len;
>>   	u32 len;
>>   	int elen;
>>   	int ret;
>> -	u8 *cryptbuf;
>> +	u8 *cryptbuf = NULL;
>>     	WARN_ON_ONCE(!fscrypt_has_encryption_key(parent));
>>   +	iname.name = dentry->d_name.name;
>> +	iname.len = dentry->d_name.len;
>> +
>> +	/* Handle the special case of snapshot names that start with '_' */
>> +	if ((ceph_snap(dir) == CEPH_SNAPDIR) && (iname.name[0] == '_')) {
>> +		dir = parse_longname(parent, iname.name, &name_len);
>> +		if (IS_ERR(dir))
>> +			return PTR_ERR(dir);
>> +		iname.name++; /* skip initial '_' */
>> +		iname.len = name_len;
>> +	}
>> +
>>   	/*
>>   	 * convert cleartext dentry name to ciphertext
>>   	 * if result is longer than CEPH_NOKEY_NAME_MAX,
>> @@ -144,18 +218,22 @@ int ceph_encode_encrypted_fname(const struct inode *parent, struct dentry *dentr
>>   	 *
>>   	 * See: fscrypt_setup_filename
>>   	 */
>> -	if (!fscrypt_fname_encrypted_size(parent, dentry->d_name.len, NAME_MAX, &len))
>> -		return -ENAMETOOLONG;
>> +	if (!fscrypt_fname_encrypted_size(dir, iname.len, NAME_MAX, &len)) {
>> +		elen = -ENAMETOOLONG;
>> +		goto out;
>> +	}
>>     	/* Allocate a buffer appropriate to hold the result */
>>   	cryptbuf = kmalloc(len > CEPH_NOHASH_NAME_MAX ? NAME_MAX : len, GFP_KERNEL);
>> -	if (!cryptbuf)
>> -		return -ENOMEM;
>> +	if (!cryptbuf) {
>> +		elen = -ENOMEM;
>> +		goto out;
>> +	}
>>   -	ret = fscrypt_fname_encrypt(parent, &dentry->d_name, cryptbuf, len);
>> +	ret = fscrypt_fname_encrypt(dir, &iname, cryptbuf, len);
>>   	if (ret) {
>> -		kfree(cryptbuf);
>> -		return ret;
>> +		elen = ret;
>> +		goto out;
>>   	}
>>     	/* hash the end if the name is long enough */
>> @@ -171,8 +249,18 @@ int ceph_encode_encrypted_fname(const struct inode *parent, struct dentry *dentr
>>     	/* base64 encode the encrypted name */
>>   	elen = fscrypt_base64url_encode(cryptbuf, len, buf);
>> -	kfree(cryptbuf);
>>   	dout("base64-encoded ciphertext name = %.*s\n", elen, buf);
>> +	if ((elen > 0) && (dir != parent)) {
>> +		char tmp_buf[FSCRYPT_BASE64URL_CHARS(NAME_MAX)];
>> +
>> +		elen = sprintf(tmp_buf, "_%.*s_%ld", elen, buf, dir->i_ino);
>> +		memcpy(buf, tmp_buf, elen);
>> +	}
>> +out:
>> +	kfree(cryptbuf);
>> +	if (dir != parent)
>> +		iput(dir);
>> +
>>   	return elen;
>>   }
>>   @@ -197,8 +285,11 @@ int ceph_fname_to_usr(const struct ceph_fname *fname,
>> struct fscrypt_str *tname,
>>   	int ret;
>>   	struct fscrypt_str _tname = FSTR_INIT(NULL, 0);
>>   	struct fscrypt_str iname;
>> +	struct inode *dir = fname->dir;
>> +	char *name = fname->name;
>> +	int name_len = fname->name_len;
>>   -	if (!IS_ENCRYPTED(fname->dir)) {
>> +	if (!IS_ENCRYPTED(dir)) {
>>   		oname->name = fname->name;
>>   		oname->len = fname->name_len;
>>   		return 0;
>> @@ -208,20 +299,29 @@ int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
>>   	if (fname->name_len > FSCRYPT_BASE64URL_CHARS(NAME_MAX))
>>   		return -EIO;
>>   -	ret = __fscrypt_prepare_readdir(fname->dir);
>> +	/* Handle the special case of snapshot names that start with '_' */
>> +	if ((ceph_snap(dir) == CEPH_SNAPDIR) && (name[0] == '_')) {
>> +		dir = parse_longname(dir, name, &name_len);
>> +		if (IS_ERR(dir))
>> +			return PTR_ERR(dir);
>> +		name++; /* skip '_' */
>> +	}
>> +
>> +	ret = __fscrypt_prepare_readdir(dir);
>>   	if (ret)
>> -		return ret;
>> +		goto out_inode;
>>     	/*
>>   	 * Use the raw dentry name as sent by the MDS instead of
>>   	 * generating a nokey name via fscrypt.
>>   	 */
>> -	if (!fscrypt_has_encryption_key(fname->dir)) {
>> +	if (!fscrypt_has_encryption_key(dir)) {
>>   		memcpy(oname->name, fname->name, fname->name_len);
>>   		oname->len = fname->name_len;
>>   		if (is_nokey)
>>   			*is_nokey = true;
>> -		return 0;
>> +		ret = 0;
>> +		goto out_inode;
>>   	}
>>     	if (fname->ctext_len == 0) {
>> @@ -230,11 +330,11 @@ int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
>>   		if (!tname) {
>>   			ret = fscrypt_fname_alloc_buffer(NAME_MAX, &_tname);
>>   			if (ret)
>> -				return ret;
>> +				goto out_inode;
>>   			tname = &_tname;
>>   		}
>>   -		declen = fscrypt_base64url_decode(fname->name, fname->name_len,
>> tname->name);
>> +		declen = fscrypt_base64url_decode(name, name_len, tname->name);
>>   		if (declen <= 0) {
>>   			ret = -EIO;
>>   			goto out;
>> @@ -246,9 +346,19 @@ int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
>>   		iname.len = fname->ctext_len;
>>   	}
>>   -	ret = fscrypt_fname_disk_to_usr(fname->dir, 0, 0, &iname, oname);
>> +	ret = fscrypt_fname_disk_to_usr(dir, 0, 0, &iname, oname);
>> +	if (!ret && (dir != fname->dir)) {
>> +		name_len = snprintf(tname->name, tname->len, "_%.*s_%ld",
>> +				    oname->len, oname->name,
>> +				    dir->i_ino);
>> +		memcpy(oname->name, tname->name, name_len);
>> +		oname->len = name_len;
>> +	}
>>   out:
>>   	fscrypt_fname_free_buffer(&_tname);
>> +out_inode:
>> +	if ((dir != fname->dir) && !IS_ERR(dir))
>> +		iput(dir);
>>   	return ret;
>>   }
>>   diff --git a/fs/ceph/crypto.h b/fs/ceph/crypto.h
>> index 1e08f8a64ad6..189af2404165 100644
>> --- a/fs/ceph/crypto.h
>> +++ b/fs/ceph/crypto.h
>> @@ -75,13 +75,16 @@ static inline u32 ceph_fscrypt_auth_len(struct ceph_fscrypt_auth *fa)
>>    * smaller size. If the ciphertext name is longer than the value below, then
>>    * sha256 hash the remaining bytes.
>>    *
>> - * 189 bytes => 252 bytes base64-encoded, which is <= NAME_MAX (255)
>> + * 180 bytes => 240 bytes base64-encoded, which is <= NAME_MAX (255)
>> + *
>> + * (Note: 240 bytes is the maximum size allowed for snapshot names to take into
>> + *  account the format: '_<SNAPSHOT-NAME>_<INODE-NUMBER>')
>>    *
>>    * Note that for long names that end up having their tail portion hashed, we
>>    * must also store the full encrypted name (in the dentry's alternate_name
>>    * field).
>>    */
>> -#define CEPH_NOHASH_NAME_MAX (189 - SHA256_DIGEST_SIZE)
>> +#define CEPH_NOHASH_NAME_MAX (180 - SHA256_DIGEST_SIZE)
>>     void ceph_fscrypt_set_ops(struct super_block *sb);
>>   @@ -90,7 +93,7 @@ void ceph_fscrypt_free_dummy_policy(struct ceph_fs_client
>> *fsc);
>>   int ceph_fscrypt_prepare_context(struct inode *dir, struct inode *inode,
>>   				 struct ceph_acl_sec_ctx *as);
>>   void ceph_fscrypt_as_ctx_to_req(struct ceph_mds_request *req, struct ceph_acl_sec_ctx *as);
>> -int ceph_encode_encrypted_fname(const struct inode *parent, struct dentry *dentry, char *buf);
>> +int ceph_encode_encrypted_fname(struct inode *parent, struct dentry *dentry, char *buf);
>>     static inline int ceph_fname_alloc_buffer(struct inode *parent, struct
>> fscrypt_str *fname)
>>   {
>>
>

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

[Luís Henriques]

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?

Cheers,
-- 
Luís


> In the ceph_mkdir()->ceph_new_inode() it will generate a new random nonce and
> then setup the fscrypt context for the inode of .snap/snapXXX. But this context
> is not correct, because the context of .snap/snapXXX should always be inherit
> from .snap's parent, which will be sent from the MDS in the request reply.
>
>
>> - Xiubo
>>
>>>
>>>> + spin_unlock(&dentry->d_lock);
>>>> +        }
>>>>       } 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;
>>>> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
>>>> index b573a0f33450..81d3d554d261 100644
>>>> --- a/fs/ceph/inode.c
>>>> +++ b/fs/ceph/inode.c
>>>> @@ -182,6 +182,19 @@ struct inode *ceph_get_snapdir(struct inode *parent)
>>>>       ci->i_rbytes = 0;
>>>>       ci->i_btime = ceph_inode(parent)->i_btime;
>>>>   +    /* if encrypted, just borrow fscrypt_auth from parent */
>>>> +    if (IS_ENCRYPTED(parent)) {
>>>> +        struct ceph_inode_info *pci = ceph_inode(parent);
>>>> +
>>>> +        ci->fscrypt_auth = kmemdup(pci->fscrypt_auth,
>>>> +                       pci->fscrypt_auth_len,
>>>> +                       GFP_KERNEL);
>>>> +        if (ci->fscrypt_auth) {
>>>> +            inode->i_flags |= S_ENCRYPTED;
>>>> +            ci->fscrypt_auth_len = pci->fscrypt_auth_len;
>>>> +        } else
>>>> +            dout("Failed to alloc memory for fscrypt_auth in snapdir\n");
>>>> +    }
>>>
>>> Here I think Jeff has already commented it in your last version, it should
>>> fail by returning NULL ?
>>>
>>> - Xiubo
>>>
>>>>       if (inode->i_state & I_NEW) {
>>>>           inode->i_op = &ceph_snapdir_iops;
>>>>           inode->i_fop = &ceph_snapdir_fops;
>>>>
>

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

[Xiubo Li]

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,

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

[Luís Henriques]

Xiubo Li <xiubli@redhat.com> writes:
<...>
> 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().

Ah! That's it!  Great suggestion, I'll go test this and send out a new
version later.  (And I think I'll need to rebase my patches on top of the
latest changes too.)

Cheers,
-- 
Luís