[PATCH] ceph: bound encrypted snapshot suffix formatting

[PATCH] ceph: bound encrypted snapshot suffix formatting

[Pengpeng Hou]

`ceph_encode_encrypted_dname()` base64-encodes the encrypted snapshot
name into the caller buffer and then, for long snapshot names, appends
`_<ino>` with `sprintf(p + elen, ...)`.

Some callers only provide `NAME_MAX` bytes. For long snapshot names, the
returned length also includes the leading underscore that stays in place
ahead of the encoded text. On 64-bit kernels, a long inode suffix can
push the final encoded name past `NAME_MAX` even though the encrypted
prefix itself stayed within the documented 240-byte budget.

Format the suffix into a small local buffer first and reject names whose
suffix would exceed the caller's `NAME_MAX` output buffer.

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 fs/ceph/crypto.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
index f3de43ccb470..eeba8ffb0554 100644
--- a/fs/ceph/crypto.c
+++ b/fs/ceph/crypto.c
@@ -208,6 +208,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
 	struct ceph_client *cl = ceph_inode_to_client(parent);
 	struct inode *dir = parent;
 	char *p = buf;
+	char suffix[1 + 20 + 1];
 	u32 len;
 	int name_len = elen;
 	int ret;
@@ -271,8 +272,20 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
 
 	/* To understand the 240 limit, see CEPH_NOHASH_NAME_MAX comments */
 	WARN_ON(elen > 240);
-	if (dir != parent) // leading _ is already there; append _<inum>
-		elen += 1 + sprintf(p + elen, "_%ld", dir->i_ino);
+	if (dir != parent) { // leading _ is already there; append _<inum>
+		ret = snprintf(suffix, sizeof(suffix), "_%lu", dir->i_ino);
+		if (ret < 0) {
+			elen = ret;
+			goto out;
+		}
+		if (ret >= NAME_MAX - elen) {
+			elen = -ENAMETOOLONG;
+			goto out;
+		}
+
+		memcpy(p + elen, suffix, ret);
+		elen += ret + 1;
+	}
 
 out:
 	kfree(cryptbuf);
-- 
2.50.1 (Apple Git-155)

Re: [PATCH] ceph: bound encrypted snapshot suffix formatting

[Viacheslav Dubeyko]

On Fri, 2026-04-03 at 16:56 +0800, Pengpeng Hou wrote:
> `ceph_encode_encrypted_dname()` base64-encodes the encrypted snapshot
> name into the caller buffer and then, for long snapshot names, appends
> `_<ino>` with `sprintf(p + elen, ...)`.
> 
> Some callers only provide `NAME_MAX` bytes. For long snapshot names, the
> returned length also includes the leading underscore that stays in place
> ahead of the encoded text. On 64-bit kernels, a long inode suffix can
> push the final encoded name past `NAME_MAX` even though the encrypted
> prefix itself stayed within the documented 240-byte budget.
> 
> Format the suffix into a small local buffer first and reject names whose
> suffix would exceed the caller's `NAME_MAX` output buffer.
> 
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> ---
>  fs/ceph/crypto.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
> index f3de43ccb470..eeba8ffb0554 100644
> --- a/fs/ceph/crypto.c
> +++ b/fs/ceph/crypto.c
> @@ -208,6 +208,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
>  	struct ceph_client *cl = ceph_inode_to_client(parent);
>  	struct inode *dir = parent;
>  	char *p = buf;
> +	char suffix[1 + 20 + 1];

I really dislike these hardcoded constants. And I don't quite follow what 1, 20,
and 1 means. Why these numbers? Please, introduce the named constants.

>  	u32 len;
>  	int name_len = elen;
>  	int ret;
> @@ -271,8 +272,20 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
>  
>  	/* To understand the 240 limit, see CEPH_NOHASH_NAME_MAX comments */
>  	WARN_ON(elen > 240);
> -	if (dir != parent) // leading _ is already there; append _<inum>
> -		elen += 1 + sprintf(p + elen, "_%ld", dir->i_ino);
> +	if (dir != parent) { // leading _ is already there; append _<inum>
> +		ret = snprintf(suffix, sizeof(suffix), "_%lu", dir->i_ino);
> +		if (ret < 0) {

The snprintf never returns negative. Are you sure that this check makes sense?

/**
 * sprintf - Format a string and place it in a buffer
 * @buf: The buffer to place the result into
 * @fmt: The format string to use
 * @...: Arguments for the format string
 *
 * The return value is the number of characters written into @buf not including
 * the trailing '\0'. Use snprintf() or scnprintf() in order to avoid
 * buffer overflows.
 *
 * See the vsnprintf() documentation for format string extensions over C99.
 */

> +			elen = ret;
> +			goto out;
> +		}
> +		if (ret >= NAME_MAX - elen) {

Technically speaking, we have only WARN_ON(elen > 240) check before. But it
doesn't prevent elen to be bigger than elen. So, potentially, this logic could
be dangerous enough because we could have overflow here.

Thanks,
Slava.

> +			elen = -ENAMETOOLONG;
> +			goto out;
> +		}
> +
> +		memcpy(p + elen, suffix, ret);
> +		elen += ret + 1;
> +	}
>  
>  out:
>  	kfree(cryptbuf);

[PATCH v2] ceph: bound encrypted snapshot suffix formatting

[Pengpeng Hou]

ceph_encode_encrypted_dname() base64-encodes the encrypted snapshot
name into the caller buffer and then, for long snapshot names, appends
_<ino> with sprintf(p + elen, ...).

Some callers only provide NAME_MAX bytes. For long snapshot names, a
large inode suffix can push the final encoded name past NAME_MAX even
though the encrypted prefix stayed within the documented 240-byte
budget.

Format the suffix into a small local buffer first and reject names
whose suffix would exceed the caller's NAME_MAX output buffer.

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
Changes since v1:
- replace the raw suffix-size constants with a named maximum
- drop the impossible negative snprintf() check
- keep the NAME_MAX bound check local to the formatted suffix length

fs/ceph/crypto.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
index f3de43ccb470..7712557660c3 100644
--- a/fs/ceph/crypto.c
+++ b/fs/ceph/crypto.c
@@ -15,6 +15,8 @@
 #include "mds_client.h"
 #include "crypto.h"
 
+#define CEPH_ENCRYPTED_SNAP_INO_SUFFIX_MAX	sizeof("_18446744073709551615")
+
 static int ceph_crypt_get_context(struct inode *inode, void *ctx, size_t len)
 {
 	struct ceph_inode_info *ci = ceph_inode(inode);
@@ -271,8 +273,19 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
 
 	/* To understand the 240 limit, see CEPH_NOHASH_NAME_MAX comments */
 	WARN_ON(elen > 240);
-	if (dir != parent) // leading _ is already there; append _<inum>
-		elen += 1 + sprintf(p + elen, "_%ld", dir->i_ino);
+	if (dir != parent) {
+		/* leading '_' is already there; append _<inum> */
+		char suffix[CEPH_ENCRYPTED_SNAP_INO_SUFFIX_MAX];
+
+		ret = snprintf(suffix, sizeof(suffix), "_%lu", dir->i_ino);
+		if (ret >= sizeof(suffix) || ret >= NAME_MAX - elen) {
+			elen = -ENAMETOOLONG;
+			goto out;
+		}
+
+		memcpy(p + elen, suffix, ret);
+		elen += ret + 1;
+	}
 
 out:
 	kfree(cryptbuf);
-- 
2.50.1 (Apple Git-155)

Re: [PATCH] ceph: bound encrypted snapshot suffix formatting

[Pengpeng Hou]

Hi Slava,

Thanks, you're right.

I have updated this to replace the raw suffix-size constants with a
named maximum, drop the impossible negative snprintf() check, and keep
the NAME_MAX test local to the formatted suffix length. I'll resend it
that way.

Thanks,
Pengpeng

Re: [PATCH v2] ceph: bound encrypted snapshot suffix formatting

[Viacheslav Dubeyko]

On Tue, 2026-04-07 at 09:57 +0800, Pengpeng Hou wrote:
> ceph_encode_encrypted_dname() base64-encodes the encrypted snapshot
> name into the caller buffer and then, for long snapshot names, appends
> _<ino> with sprintf(p + elen, ...).
> 
> Some callers only provide NAME_MAX bytes. For long snapshot names, a
> large inode suffix can push the final encoded name past NAME_MAX even
> though the encrypted prefix stayed within the documented 240-byte
> budget.
> 
> Format the suffix into a small local buffer first and reject names
> whose suffix would exceed the caller's NAME_MAX output buffer.
> 
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> ---
> Changes since v1:
> - replace the raw suffix-size constants with a named maximum
> - drop the impossible negative snprintf() check
> - keep the NAME_MAX bound check local to the formatted suffix length
> 
> fs/ceph/crypto.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
> index f3de43ccb470..7712557660c3 100644
> --- a/fs/ceph/crypto.c
> +++ b/fs/ceph/crypto.c
> @@ -15,6 +15,8 @@
>  #include "mds_client.h"
>  #include "crypto.h"
>  
> +#define CEPH_ENCRYPTED_SNAP_INO_SUFFIX_MAX	sizeof("_18446744073709551615")

These define is much better. But I am still thinking could we have a more
elegant solution here? :) Do you have any ideas? Maybe, do we need to introduce
some macro DECIMAL_DIGITS_MAX()? At minimum, we need to have a comment here.

> +
>  static int ceph_crypt_get_context(struct inode *inode, void *ctx, size_t len)
>  {
>  	struct ceph_inode_info *ci = ceph_inode(inode);
> @@ -271,8 +273,19 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
>  
>  	/* To understand the 240 limit, see CEPH_NOHASH_NAME_MAX comments */
>  	WARN_ON(elen > 240);
> -	if (dir != parent) // leading _ is already there; append _<inum>
> -		elen += 1 + sprintf(p + elen, "_%ld", dir->i_ino);
> +	if (dir != parent) {
> +		/* leading '_' is already there; append _<inum> */
> +		char suffix[CEPH_ENCRYPTED_SNAP_INO_SUFFIX_MAX];
> +
> +		ret = snprintf(suffix, sizeof(suffix), "_%lu", dir->i_ino);
> +		if (ret >= sizeof(suffix) || ret >= NAME_MAX - elen) {

It looks like that ret >= sizeof(suffix) is dead code. The sizeof(suffix) = 22.
The maximum possible suffix is _18446744073709551615 = 21 chars → snprintf
returns 21, never ≥ 22.

> +			elen = -ENAMETOOLONG;
> +			goto out;
> +		}
> +
> +		memcpy(p + elen, suffix, ret);
> +		elen += ret + 1;

I believe we need to have a comment here. The +1 is not for the NUL — it
accounts for the leading _ at buf[0]. Without a comment it could be confusing.

Thanks,
Slava.

> +	}
>  
>  out:
>  	kfree(cryptbuf);

[PATCH v3] ceph: bound encrypted snapshot suffix formatting

[Pengpeng Hou]

ceph_encode_encrypted_dname() base64-encodes the encrypted snapshot
name into the caller buffer and then, for long snapshot names, appends
_<ino> with sprintf(p + elen, ...).

Some callers only provide NAME_MAX bytes. For long snapshot names, a
large inode suffix can push the final encoded name past NAME_MAX even
though the encrypted prefix stayed within the documented 240-byte
budget.

Format the suffix into a small local buffer first and reject names
whose suffix would exceed the caller's NAME_MAX output buffer.

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>

---
Changes since v2:
- document the suffix buffer size with a comment
- drop the dead ret >= sizeof(suffix) check
- track the skipped leading '_' explicitly via prefix_len
---
 fs/ceph/crypto.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
index f3de43ccb470..7989056a463c 100644
--- a/fs/ceph/crypto.c
+++ b/fs/ceph/crypto.c
@@ -15,6 +15,12 @@
 #include "mds_client.h"
 #include "crypto.h"
 
+/*
+ * Reserve room for '_' + decimal 64-bit inode number + trailing NUL.
+ * ceph_encode_encrypted_dname() copies only the visible suffix bytes.
+ */
+#define CEPH_ENCRYPTED_SNAP_INO_SUFFIX_MAX	sizeof("_18446744073709551615")
+
 static int ceph_crypt_get_context(struct inode *inode, void *ctx, size_t len)
 {
 	struct ceph_inode_info *ci = ceph_inode(inode);
@@ -209,6 +215,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
 	struct inode *dir = parent;
 	char *p = buf;
 	u32 len;
+	int prefix_len = 0;
 	int name_len = elen;
 	int ret;
 	u8 *cryptbuf = NULL;
@@ -219,6 +226,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
 		if (IS_ERR(dir))
 			return PTR_ERR(dir);
 		p++; /* skip initial '_' */
+		prefix_len = 1;
 	}
 
 	if (!fscrypt_has_encryption_key(dir))
@@ -271,8 +279,20 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
 
 	/* To understand the 240 limit, see CEPH_NOHASH_NAME_MAX comments */
 	WARN_ON(elen > 240);
-	if (dir != parent) // leading _ is already there; append _<inum>
-		elen += 1 + sprintf(p + elen, "_%ld", dir->i_ino);
+	if (dir != parent) {
+		/* leading '_' is already there; append _<inum> */
+		char suffix[CEPH_ENCRYPTED_SNAP_INO_SUFFIX_MAX];
+
+		ret = snprintf(suffix, sizeof(suffix), "_%lu", dir->i_ino);
+		if (ret > NAME_MAX - prefix_len - elen) {
+			elen = -ENAMETOOLONG;
+			goto out;
+		}
+
+		memcpy(p + elen, suffix, ret);
+		/* Include the leading '_' skipped by p. */
+		elen += ret + prefix_len;
+	}
 
 out:
 	kfree(cryptbuf);
-- 
2.50.1 (Apple Git-155)

Re: [PATCH v3] ceph: bound encrypted snapshot suffix formatting

[Viacheslav Dubeyko]

On Wed, 2026-04-08 at 08:57 +0800, Pengpeng Hou wrote:
> ceph_encode_encrypted_dname() base64-encodes the encrypted snapshot
> name into the caller buffer and then, for long snapshot names, appends
> _<ino> with sprintf(p + elen, ...).
> 
> Some callers only provide NAME_MAX bytes. For long snapshot names, a
> large inode suffix can push the final encoded name past NAME_MAX even
> though the encrypted prefix stayed within the documented 240-byte
> budget.
> 
> Format the suffix into a small local buffer first and reject names
> whose suffix would exceed the caller's NAME_MAX output buffer.
> 
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> 
> ---
> Changes since v2:
> - document the suffix buffer size with a comment
> - drop the dead ret >= sizeof(suffix) check
> - track the skipped leading '_' explicitly via prefix_len
> ---
>  fs/ceph/crypto.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
> index f3de43ccb470..7989056a463c 100644
> --- a/fs/ceph/crypto.c
> +++ b/fs/ceph/crypto.c
> @@ -15,6 +15,12 @@
>  #include "mds_client.h"
>  #include "crypto.h"
>  
> +/*
> + * Reserve room for '_' + decimal 64-bit inode number + trailing NUL.
> + * ceph_encode_encrypted_dname() copies only the visible suffix bytes.
> + */
> +#define CEPH_ENCRYPTED_SNAP_INO_SUFFIX_MAX	sizeof("_18446744073709551615")
> +
>  static int ceph_crypt_get_context(struct inode *inode, void *ctx, size_t len)
>  {
>  	struct ceph_inode_info *ci = ceph_inode(inode);
> @@ -209,6 +215,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
>  	struct inode *dir = parent;
>  	char *p = buf;
>  	u32 len;
> +	int prefix_len = 0;
>  	int name_len = elen;
>  	int ret;
>  	u8 *cryptbuf = NULL;
> @@ -219,6 +226,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
>  		if (IS_ERR(dir))
>  			return PTR_ERR(dir);
>  		p++; /* skip initial '_' */
> +		prefix_len = 1;
>  	}
>  
>  	if (!fscrypt_has_encryption_key(dir))
> @@ -271,8 +279,20 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
>  
>  	/* To understand the 240 limit, see CEPH_NOHASH_NAME_MAX comments */
>  	WARN_ON(elen > 240);
> -	if (dir != parent) // leading _ is already there; append _<inum>
> -		elen += 1 + sprintf(p + elen, "_%ld", dir->i_ino);
> +	if (dir != parent) {
> +		/* leading '_' is already there; append _<inum> */
> +		char suffix[CEPH_ENCRYPTED_SNAP_INO_SUFFIX_MAX];
> +
> +		ret = snprintf(suffix, sizeof(suffix), "_%lu", dir->i_ino);
> +		if (ret > NAME_MAX - prefix_len - elen) {

The patch looks much cleaner now. But I am still slightly worried about this
calculation. Yes, we have WARN_ON(elen > 240) but it doesn't stop the execution
flow. And, finally, NAME_MAX - prefix_len - elen could have overflow and it
could behave incorrectly. Maybe, we need to add the check if (elen > 240) here?
What do you think?

Thanks,
Slava.

> +			elen = -ENAMETOOLONG;
> +			goto out;
> +		}
> +
> +		memcpy(p + elen, suffix, ret);
> +		/* Include the leading '_' skipped by p. */
> +		elen += ret + prefix_len;
> +	}
>  
>  out:
>  	kfree(cryptbuf);

[PATCH v4] ceph: bound encrypted snapshot suffix formatting

[Pengpeng Hou]

ceph_encode_encrypted_dname() base64-encodes the encrypted snapshot
name into the caller buffer and then, for long snapshot names, appends
_<ino> with sprintf(p + elen, ...).

Some callers only provide NAME_MAX bytes. For long snapshot names, a
large inode suffix can push the final encoded name past NAME_MAX even
though the encrypted prefix stayed within the documented 240-byte
budget.

Format the suffix into a small local buffer first and reject names
whose suffix would exceed the caller's NAME_MAX output buffer.

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
Changes since v3:
- reject `elen > 240` explicitly instead of relying only on the earlier
  `WARN_ON()`
- rewrite the NAME_MAX bound check in terms of the final total length
  instead of `NAME_MAX - prefix_len - elen`

 fs/ceph/crypto.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
index f3de43ccb470..42e3fff34697 100644
--- a/fs/ceph/crypto.c
+++ b/fs/ceph/crypto.c
@@ -15,6 +15,12 @@
 #include "mds_client.h"
 #include "crypto.h"
 
+/*
+ * Reserve room for '_' + decimal 64-bit inode number + trailing NUL.
+ * ceph_encode_encrypted_dname() copies only the visible suffix bytes.
+ */
+#define CEPH_ENCRYPTED_SNAP_INO_SUFFIX_MAX	sizeof("_18446744073709551615")
+
 static int ceph_crypt_get_context(struct inode *inode, void *ctx, size_t len)
 {
 	struct ceph_inode_info *ci = ceph_inode(inode);
@@ -209,6 +215,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
 	struct inode *dir = parent;
 	char *p = buf;
 	u32 len;
+	int prefix_len = 0;
 	int name_len = elen;
 	int ret;
 	u8 *cryptbuf = NULL;
@@ -219,6 +226,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
 		if (IS_ERR(dir))
 			return PTR_ERR(dir);
 		p++; /* skip initial '_' */
+		prefix_len = 1;
 	}
 
 	if (!fscrypt_has_encryption_key(dir))
@@ -271,8 +279,27 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
 
 	/* To understand the 240 limit, see CEPH_NOHASH_NAME_MAX comments */
 	WARN_ON(elen > 240);
-	if (dir != parent) // leading _ is already there; append _<inum>
-		elen += 1 + sprintf(p + elen, "_%ld", dir->i_ino);
+	if (elen > 240) {
+		elen = -ENAMETOOLONG;
+		goto out;
+	}
+
+	if (dir != parent) {
+		int total_len;
+		/* leading '_' is already there; append _<inum> */
+		char suffix[CEPH_ENCRYPTED_SNAP_INO_SUFFIX_MAX];
+
+		ret = snprintf(suffix, sizeof(suffix), "_%lu", dir->i_ino);
+		total_len = prefix_len + elen + ret;
+		if (total_len > NAME_MAX) {
+			elen = -ENAMETOOLONG;
+			goto out;
+		}
+
+		memcpy(p + elen, suffix, ret);
+		/* Include the leading '_' skipped by p. */
+		elen = total_len;
+	}
 
 out:
 	kfree(cryptbuf);
-- 
2.50.1 (Apple Git-155)

Re: [PATCH v4] ceph: bound encrypted snapshot suffix formatting

[Viacheslav Dubeyko]

On Thu, 2026-04-09 at 10:39 +0800, Pengpeng Hou wrote:
> ceph_encode_encrypted_dname() base64-encodes the encrypted snapshot
> name into the caller buffer and then, for long snapshot names, appends
> _<ino> with sprintf(p + elen, ...).
> 
> Some callers only provide NAME_MAX bytes. For long snapshot names, a
> large inode suffix can push the final encoded name past NAME_MAX even
> though the encrypted prefix stayed within the documented 240-byte
> budget.
> 
> Format the suffix into a small local buffer first and reject names
> whose suffix would exceed the caller's NAME_MAX output buffer.
> 
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> ---
> Changes since v3:
> - reject `elen > 240` explicitly instead of relying only on the earlier
>   `WARN_ON()`
> - rewrite the NAME_MAX bound check in terms of the final total length
>   instead of `NAME_MAX - prefix_len - elen`
> 
>  fs/ceph/crypto.c | 31 +++++++++++++++++++++++++++++--
>  1 file changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
> index f3de43ccb470..42e3fff34697 100644
> --- a/fs/ceph/crypto.c
> +++ b/fs/ceph/crypto.c
> @@ -15,6 +15,12 @@
>  #include "mds_client.h"
>  #include "crypto.h"
>  
> +/*
> + * Reserve room for '_' + decimal 64-bit inode number + trailing NUL.
> + * ceph_encode_encrypted_dname() copies only the visible suffix bytes.
> + */
> +#define CEPH_ENCRYPTED_SNAP_INO_SUFFIX_MAX	sizeof("_18446744073709551615")
> +
>  static int ceph_crypt_get_context(struct inode *inode, void *ctx, size_t len)
>  {
>  	struct ceph_inode_info *ci = ceph_inode(inode);
> @@ -209,6 +215,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
>  	struct inode *dir = parent;
>  	char *p = buf;
>  	u32 len;
> +	int prefix_len = 0;
>  	int name_len = elen;
>  	int ret;
>  	u8 *cryptbuf = NULL;
> @@ -219,6 +226,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
>  		if (IS_ERR(dir))
>  			return PTR_ERR(dir);
>  		p++; /* skip initial '_' */
> +		prefix_len = 1;
>  	}
>  
>  	if (!fscrypt_has_encryption_key(dir))
> @@ -271,8 +279,27 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
>  
>  	/* To understand the 240 limit, see CEPH_NOHASH_NAME_MAX comments */
>  	WARN_ON(elen > 240);
> -	if (dir != parent) // leading _ is already there; append _<inum>
> -		elen += 1 + sprintf(p + elen, "_%ld", dir->i_ino);
> +	if (elen > 240) {
> +		elen = -ENAMETOOLONG;
> +		goto out;
> +	}
> +
> +	if (dir != parent) {
> +		int total_len;
> +		/* leading '_' is already there; append _<inum> */
> +		char suffix[CEPH_ENCRYPTED_SNAP_INO_SUFFIX_MAX];
> +
> +		ret = snprintf(suffix, sizeof(suffix), "_%lu", dir->i_ino);
> +		total_len = prefix_len + elen + ret;
> +		if (total_len > NAME_MAX) {
> +			elen = -ENAMETOOLONG;
> +			goto out;
> +		}
> +
> +		memcpy(p + elen, suffix, ret);
> +		/* Include the leading '_' skipped by p. */
> +		elen = total_len;
> +	}
>  
>  out:
>  	kfree(cryptbuf);

Looks good.

Reviewed-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>

Let me run xfstests for the patch to double check that everything is OK. I'll
share the result ASAP.

Thanks,
Slava.

Re: [PATCH v4] ceph: bound encrypted snapshot suffix formatting

[Viacheslav Dubeyko]

On Thu, 2026-04-09 at 18:09 +0000, Viacheslav Dubeyko wrote:
> On Thu, 2026-04-09 at 10:39 +0800, Pengpeng Hou wrote:
> > ceph_encode_encrypted_dname() base64-encodes the encrypted snapshot
> > name into the caller buffer and then, for long snapshot names, appends
> > _<ino> with sprintf(p + elen, ...).
> > 
> > Some callers only provide NAME_MAX bytes. For long snapshot names, a
> > large inode suffix can push the final encoded name past NAME_MAX even
> > though the encrypted prefix stayed within the documented 240-byte
> > budget.
> > 
> > Format the suffix into a small local buffer first and reject names
> > whose suffix would exceed the caller's NAME_MAX output buffer.
> > 
> > Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> > ---
> > Changes since v3:
> > - reject `elen > 240` explicitly instead of relying only on the earlier
> >   `WARN_ON()`
> > - rewrite the NAME_MAX bound check in terms of the final total length
> >   instead of `NAME_MAX - prefix_len - elen`
> > 
> >  fs/ceph/crypto.c | 31 +++++++++++++++++++++++++++++--
> >  1 file changed, 29 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
> > index f3de43ccb470..42e3fff34697 100644
> > --- a/fs/ceph/crypto.c
> > +++ b/fs/ceph/crypto.c
> > @@ -15,6 +15,12 @@
> >  #include "mds_client.h"
> >  #include "crypto.h"
> >  
> > +/*
> > + * Reserve room for '_' + decimal 64-bit inode number + trailing NUL.
> > + * ceph_encode_encrypted_dname() copies only the visible suffix bytes.
> > + */
> > +#define CEPH_ENCRYPTED_SNAP_INO_SUFFIX_MAX	sizeof("_18446744073709551615")
> > +
> >  static int ceph_crypt_get_context(struct inode *inode, void *ctx, size_t len)
> >  {
> >  	struct ceph_inode_info *ci = ceph_inode(inode);
> > @@ -209,6 +215,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
> >  	struct inode *dir = parent;
> >  	char *p = buf;
> >  	u32 len;
> > +	int prefix_len = 0;
> >  	int name_len = elen;
> >  	int ret;
> >  	u8 *cryptbuf = NULL;
> > @@ -219,6 +226,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
> >  		if (IS_ERR(dir))
> >  			return PTR_ERR(dir);
> >  		p++; /* skip initial '_' */
> > +		prefix_len = 1;
> >  	}
> >  
> >  	if (!fscrypt_has_encryption_key(dir))
> > @@ -271,8 +279,27 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
> >  
> >  	/* To understand the 240 limit, see CEPH_NOHASH_NAME_MAX comments */
> >  	WARN_ON(elen > 240);
> > -	if (dir != parent) // leading _ is already there; append _<inum>
> > -		elen += 1 + sprintf(p + elen, "_%ld", dir->i_ino);
> > +	if (elen > 240) {
> > +		elen = -ENAMETOOLONG;
> > +		goto out;
> > +	}
> > +
> > +	if (dir != parent) {
> > +		int total_len;
> > +		/* leading '_' is already there; append _<inum> */
> > +		char suffix[CEPH_ENCRYPTED_SNAP_INO_SUFFIX_MAX];
> > +
> > +		ret = snprintf(suffix, sizeof(suffix), "_%lu", dir->i_ino);
> > +		total_len = prefix_len + elen + ret;
> > +		if (total_len > NAME_MAX) {
> > +			elen = -ENAMETOOLONG;
> > +			goto out;
> > +		}
> > +
> > +		memcpy(p + elen, suffix, ret);
> > +		/* Include the leading '_' skipped by p. */
> > +		elen = total_len;
> > +	}
> >  
> >  out:
> >  	kfree(cryptbuf);
> 
> Looks good.
> 
> Reviewed-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
> 
> Let me run xfstests for the patch to double check that everything is OK. I'll
> share the result ASAP.
> 

The xfstests run was successful. I don't see any issues with the patch.

Tested-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>

Thanks,
Slava.

RE: [PATCH v4] ceph: bound encrypted snapshot suffix formatting

[Viacheslav Dubeyko]

On Fri, 2026-04-10 at 20:40 +0000, Viacheslav Dubeyko wrote:
> On Thu, 2026-04-09 at 18:09 +0000, Viacheslav Dubeyko wrote:
> > On Thu, 2026-04-09 at 10:39 +0800, Pengpeng Hou wrote:
> > > ceph_encode_encrypted_dname() base64-encodes the encrypted snapshot
> > > name into the caller buffer and then, for long snapshot names, appends
> > > _<ino> with sprintf(p + elen, ...).
> > > 
> > > Some callers only provide NAME_MAX bytes. For long snapshot names, a
> > > large inode suffix can push the final encoded name past NAME_MAX even
> > > though the encrypted prefix stayed within the documented 240-byte
> > > budget.
> > > 
> > > Format the suffix into a small local buffer first and reject names
> > > whose suffix would exceed the caller's NAME_MAX output buffer.
> > > 
> > > Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> > > ---
> > > Changes since v3:
> > > - reject `elen > 240` explicitly instead of relying only on the earlier
> > >   `WARN_ON()`
> > > - rewrite the NAME_MAX bound check in terms of the final total length
> > >   instead of `NAME_MAX - prefix_len - elen`
> > > 
> > >  fs/ceph/crypto.c | 31 +++++++++++++++++++++++++++++--
> > >  1 file changed, 29 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
> > > index f3de43ccb470..42e3fff34697 100644
> > > --- a/fs/ceph/crypto.c
> > > +++ b/fs/ceph/crypto.c
> > > @@ -15,6 +15,12 @@
> > >  #include "mds_client.h"
> > >  #include "crypto.h"
> > >  
> > > +/*
> > > + * Reserve room for '_' + decimal 64-bit inode number + trailing NUL.
> > > + * ceph_encode_encrypted_dname() copies only the visible suffix bytes.
> > > + */
> > > +#define CEPH_ENCRYPTED_SNAP_INO_SUFFIX_MAX	sizeof("_18446744073709551615")
> > > +
> > >  static int ceph_crypt_get_context(struct inode *inode, void *ctx, size_t len)
> > >  {
> > >  	struct ceph_inode_info *ci = ceph_inode(inode);
> > > @@ -209,6 +215,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
> > >  	struct inode *dir = parent;
> > >  	char *p = buf;
> > >  	u32 len;
> > > +	int prefix_len = 0;
> > >  	int name_len = elen;
> > >  	int ret;
> > >  	u8 *cryptbuf = NULL;
> > > @@ -219,6 +226,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
> > >  		if (IS_ERR(dir))
> > >  			return PTR_ERR(dir);
> > >  		p++; /* skip initial '_' */
> > > +		prefix_len = 1;
> > >  	}
> > >  
> > >  	if (!fscrypt_has_encryption_key(dir))
> > > @@ -271,8 +279,27 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
> > >  
> > >  	/* To understand the 240 limit, see CEPH_NOHASH_NAME_MAX comments */
> > >  	WARN_ON(elen > 240);
> > > -	if (dir != parent) // leading _ is already there; append _<inum>
> > > -		elen += 1 + sprintf(p + elen, "_%ld", dir->i_ino);
> > > +	if (elen > 240) {
> > > +		elen = -ENAMETOOLONG;
> > > +		goto out;
> > > +	}
> > > +
> > > +	if (dir != parent) {
> > > +		int total_len;
> > > +		/* leading '_' is already there; append _<inum> */
> > > +		char suffix[CEPH_ENCRYPTED_SNAP_INO_SUFFIX_MAX];
> > > +
> > > +		ret = snprintf(suffix, sizeof(suffix), "_%lu", dir->i_ino);
> > > +		total_len = prefix_len + elen + ret;
> > > +		if (total_len > NAME_MAX) {
> > > +			elen = -ENAMETOOLONG;
> > > +			goto out;
> > > +		}
> > > +
> > > +		memcpy(p + elen, suffix, ret);
> > > +		/* Include the leading '_' skipped by p. */
> > > +		elen = total_len;
> > > +	}
> > >  
> > >  out:
> > >  	kfree(cryptbuf);
> > 
> > Looks good.
> > 
> > Reviewed-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
> > 
> > Let me run xfstests for the patch to double check that everything is OK. I'll
> > share the result ASAP.
> > 
> 
> The xfstests run was successful. I don't see any issues with the patch.
> 
> Tested-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
> 
> 

Applied on testing branch of CephFS kernel client git tree.

Thanks,
Slava.

Re: [PATCH v4] ceph: bound encrypted snapshot suffix formatting

[Ilya Dryomov]

On Fri, Apr 10, 2026 at 10:46 PM Viacheslav Dubeyko
<Slava.Dubeyko@ibm.com> wrote:
>
> On Fri, 2026-04-10 at 20:40 +0000, Viacheslav Dubeyko wrote:
> > On Thu, 2026-04-09 at 18:09 +0000, Viacheslav Dubeyko wrote:
> > > On Thu, 2026-04-09 at 10:39 +0800, Pengpeng Hou wrote:
> > > > ceph_encode_encrypted_dname() base64-encodes the encrypted snapshot
> > > > name into the caller buffer and then, for long snapshot names, appends
> > > > _<ino> with sprintf(p + elen, ...).
> > > >
> > > > Some callers only provide NAME_MAX bytes. For long snapshot names, a
> > > > large inode suffix can push the final encoded name past NAME_MAX even
> > > > though the encrypted prefix stayed within the documented 240-byte
> > > > budget.
> > > >
> > > > Format the suffix into a small local buffer first and reject names
> > > > whose suffix would exceed the caller's NAME_MAX output buffer.
> > > >
> > > > Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> > > > ---
> > > > Changes since v3:
> > > > - reject `elen > 240` explicitly instead of relying only on the earlier
> > > >   `WARN_ON()`
> > > > - rewrite the NAME_MAX bound check in terms of the final total length
> > > >   instead of `NAME_MAX - prefix_len - elen`
> > > >
> > > >  fs/ceph/crypto.c | 31 +++++++++++++++++++++++++++++--
> > > >  1 file changed, 29 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
> > > > index f3de43ccb470..42e3fff34697 100644
> > > > --- a/fs/ceph/crypto.c
> > > > +++ b/fs/ceph/crypto.c
> > > > @@ -15,6 +15,12 @@
> > > >  #include "mds_client.h"
> > > >  #include "crypto.h"
> > > >
> > > > +/*
> > > > + * Reserve room for '_' + decimal 64-bit inode number + trailing NUL.
> > > > + * ceph_encode_encrypted_dname() copies only the visible suffix bytes.
> > > > + */
> > > > +#define CEPH_ENCRYPTED_SNAP_INO_SUFFIX_MAX       sizeof("_18446744073709551615")
> > > > +
> > > >  static int ceph_crypt_get_context(struct inode *inode, void *ctx, size_t len)
> > > >  {
> > > >   struct ceph_inode_info *ci = ceph_inode(inode);
> > > > @@ -209,6 +215,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
> > > >   struct inode *dir = parent;
> > > >   char *p = buf;
> > > >   u32 len;
> > > > + int prefix_len = 0;
> > > >   int name_len = elen;
> > > >   int ret;
> > > >   u8 *cryptbuf = NULL;
> > > > @@ -219,6 +226,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
> > > >           if (IS_ERR(dir))
> > > >                   return PTR_ERR(dir);
> > > >           p++; /* skip initial '_' */
> > > > +         prefix_len = 1;
> > > >   }
> > > >
> > > >   if (!fscrypt_has_encryption_key(dir))
> > > > @@ -271,8 +279,27 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
> > > >
> > > >   /* To understand the 240 limit, see CEPH_NOHASH_NAME_MAX comments */
> > > >   WARN_ON(elen > 240);
> > > > - if (dir != parent) // leading _ is already there; append _<inum>
> > > > -         elen += 1 + sprintf(p + elen, "_%ld", dir->i_ino);
> > > > + if (elen > 240) {
> > > > +         elen = -ENAMETOOLONG;
> > > > +         goto out;
> > > > + }
> > > > +
> > > > + if (dir != parent) {
> > > > +         int total_len;
> > > > +         /* leading '_' is already there; append _<inum> */
> > > > +         char suffix[CEPH_ENCRYPTED_SNAP_INO_SUFFIX_MAX];
> > > > +
> > > > +         ret = snprintf(suffix, sizeof(suffix), "_%lu", dir->i_ino);
> > > > +         total_len = prefix_len + elen + ret;
> > > > +         if (total_len > NAME_MAX) {
> > > > +                 elen = -ENAMETOOLONG;
> > > > +                 goto out;
> > > > +         }
> > > > +
> > > > +         memcpy(p + elen, suffix, ret);
> > > > +         /* Include the leading '_' skipped by p. */
> > > > +         elen = total_len;
> > > > + }
> > > >
> > > >  out:
> > > >   kfree(cryptbuf);
> > >
> > > Looks good.
> > >
> > > Reviewed-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
> > >
> > > Let me run xfstests for the patch to double check that everything is OK. I'll
> > > share the result ASAP.
> > >
> >
> > The xfstests run was successful. I don't see any issues with the patch.
> >
> > Tested-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
> >
> >
>
> Applied on testing branch of CephFS kernel client git tree.

Hi Pengpeng, Slava,

This patch raised my attention because my understanding was that the
entire CEPH_NOHASH_NAME_MAX + sha256() was put in place precisely to
handle longer names nicely and make them fit into NAME_MAX-sized buffer.
Simply rejecting longer names seemed to be in direct contradiction with
that and yet the patch on its own was clearly merited given

 * (240 bytes is the maximum size allowed for snapshot names to take into
 *  account the format: '_<SNAPSHOT-NAME>_<INODE-NUMBER>'.)

comment on CEPH_NOHASH_NAME_MAX definition.

I dug a bit deeper and started a discussion in [1].  The preliminary
conclusion is that the 240 bytes assumption was a mistake -- somehow
the minimum number of characters needed for <inum> ended up being used
instead of the maximum.  CEPH_NOHASH_NAME_MAX value is likely incorrect
and should have been smaller -- something along the lines of 174 -
SHA256_DIGEST_SIZE instead of 180 - SHA256_DIGEST_SIZE.

I kept this patch in the testing branch but not including it for 7.1
pending further investigation.

[1] https://github.com/ceph/ceph/pull/45312

Thanks,

                Ilya

Re: [PATCH v4] ceph: bound encrypted snapshot suffix formatting

[Viacheslav Dubeyko]

On Wed, 2026-04-22 at 11:53 +0200, Ilya Dryomov wrote:
> On Fri, Apr 10, 2026 at 10:46 PM Viacheslav Dubeyko
> <Slava.Dubeyko@ibm.com> wrote:
> > 
> > On Fri, 2026-04-10 at 20:40 +0000, Viacheslav Dubeyko wrote:
> > > On Thu, 2026-04-09 at 18:09 +0000, Viacheslav Dubeyko wrote:
> > > > On Thu, 2026-04-09 at 10:39 +0800, Pengpeng Hou wrote:
> > > > > ceph_encode_encrypted_dname() base64-encodes the encrypted snapshot
> > > > > name into the caller buffer and then, for long snapshot names, appends
> > > > > _<ino> with sprintf(p + elen, ...).
> > > > > 
> > > > > Some callers only provide NAME_MAX bytes. For long snapshot names, a
> > > > > large inode suffix can push the final encoded name past NAME_MAX even
> > > > > though the encrypted prefix stayed within the documented 240-byte
> > > > > budget.
> > > > > 
> > > > > Format the suffix into a small local buffer first and reject names
> > > > > whose suffix would exceed the caller's NAME_MAX output buffer.
> > > > > 
> > > > > Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> > > > > ---
> > > > > Changes since v3:
> > > > > - reject `elen > 240` explicitly instead of relying only on the earlier
> > > > >   `WARN_ON()`
> > > > > - rewrite the NAME_MAX bound check in terms of the final total length
> > > > >   instead of `NAME_MAX - prefix_len - elen`
> > > > > 
> > > > >  fs/ceph/crypto.c | 31 +++++++++++++++++++++++++++++--
> > > > >  1 file changed, 29 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
> > > > > index f3de43ccb470..42e3fff34697 100644
> > > > > --- a/fs/ceph/crypto.c
> > > > > +++ b/fs/ceph/crypto.c
> > > > > @@ -15,6 +15,12 @@
> > > > >  #include "mds_client.h"
> > > > >  #include "crypto.h"
> > > > > 
> > > > > +/*
> > > > > + * Reserve room for '_' + decimal 64-bit inode number + trailing NUL.
> > > > > + * ceph_encode_encrypted_dname() copies only the visible suffix bytes.
> > > > > + */
> > > > > +#define CEPH_ENCRYPTED_SNAP_INO_SUFFIX_MAX       sizeof("_18446744073709551615")
> > > > > +
> > > > >  static int ceph_crypt_get_context(struct inode *inode, void *ctx, size_t len)
> > > > >  {
> > > > >   struct ceph_inode_info *ci = ceph_inode(inode);
> > > > > @@ -209,6 +215,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
> > > > >   struct inode *dir = parent;
> > > > >   char *p = buf;
> > > > >   u32 len;
> > > > > + int prefix_len = 0;
> > > > >   int name_len = elen;
> > > > >   int ret;
> > > > >   u8 *cryptbuf = NULL;
> > > > > @@ -219,6 +226,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
> > > > >           if (IS_ERR(dir))
> > > > >                   return PTR_ERR(dir);
> > > > >           p++; /* skip initial '_' */
> > > > > +         prefix_len = 1;
> > > > >   }
> > > > > 
> > > > >   if (!fscrypt_has_encryption_key(dir))
> > > > > @@ -271,8 +279,27 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
> > > > > 
> > > > >   /* To understand the 240 limit, see CEPH_NOHASH_NAME_MAX comments */
> > > > >   WARN_ON(elen > 240);
> > > > > - if (dir != parent) // leading _ is already there; append _<inum>
> > > > > -         elen += 1 + sprintf(p + elen, "_%ld", dir->i_ino);
> > > > > + if (elen > 240) {
> > > > > +         elen = -ENAMETOOLONG;
> > > > > +         goto out;
> > > > > + }
> > > > > +
> > > > > + if (dir != parent) {
> > > > > +         int total_len;
> > > > > +         /* leading '_' is already there; append _<inum> */
> > > > > +         char suffix[CEPH_ENCRYPTED_SNAP_INO_SUFFIX_MAX];
> > > > > +
> > > > > +         ret = snprintf(suffix, sizeof(suffix), "_%lu", dir->i_ino);
> > > > > +         total_len = prefix_len + elen + ret;
> > > > > +         if (total_len > NAME_MAX) {
> > > > > +                 elen = -ENAMETOOLONG;
> > > > > +                 goto out;
> > > > > +         }
> > > > > +
> > > > > +         memcpy(p + elen, suffix, ret);
> > > > > +         /* Include the leading '_' skipped by p. */
> > > > > +         elen = total_len;
> > > > > + }
> > > > > 
> > > > >  out:
> > > > >   kfree(cryptbuf);
> > > > 
> > > > Looks good.
> > > > 
> > > > Reviewed-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
> > > > 
> > > > Let me run xfstests for the patch to double check that everything is OK. I'll
> > > > share the result ASAP.
> > > > 
> > > 
> > > The xfstests run was successful. I don't see any issues with the patch.
> > > 
> > > Tested-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
> > > 
> > > 
> > 
> > Applied on testing branch of CephFS kernel client git tree.
> 
> Hi Pengpeng, Slava,
> 
> This patch raised my attention because my understanding was that the
> entire CEPH_NOHASH_NAME_MAX + sha256() was put in place precisely to
> handle longer names nicely and make them fit into NAME_MAX-sized buffer.
> Simply rejecting longer names seemed to be in direct contradiction with
> that and yet the patch on its own was clearly merited given
> 
>  * (240 bytes is the maximum size allowed for snapshot names to take into
>  *  account the format: '_<SNAPSHOT-NAME>_<INODE-NUMBER>'.)
> 
> comment on CEPH_NOHASH_NAME_MAX definition.
> 
> I dug a bit deeper and started a discussion in [1].  The preliminary
> conclusion is that the 240 bytes assumption was a mistake -- somehow
> the minimum number of characters needed for <inum> ended up being used
> instead of the maximum.  CEPH_NOHASH_NAME_MAX value is likely incorrect
> and should have been smaller -- something along the lines of 174 -
> SHA256_DIGEST_SIZE instead of 180 - SHA256_DIGEST_SIZE.
> 

The limitation could be 240 or bigger one, but anyway we need to process this
limitation in proper way. And this patch has exactly this goal. Is 240 bytes
limitation your concern? As far as I can see, this patch doesn't introduce this
limitation. It was there before this modification. We can extend this limitation
anytime.

Thanks,
Slava.

Re: [PATCH v4] ceph: bound encrypted snapshot suffix formatting

[Luis Henriques]

On Wed, Apr 22 2026, Ilya Dryomov wrote:

> On Fri, Apr 10, 2026 at 10:46 PM Viacheslav Dubeyko
> <Slava.Dubeyko@ibm.com> wrote:
>>
>> On Fri, 2026-04-10 at 20:40 +0000, Viacheslav Dubeyko wrote:
>> > On Thu, 2026-04-09 at 18:09 +0000, Viacheslav Dubeyko wrote:
>> > > On Thu, 2026-04-09 at 10:39 +0800, Pengpeng Hou wrote:
>> > > > ceph_encode_encrypted_dname() base64-encodes the encrypted snapshot
>> > > > name into the caller buffer and then, for long snapshot names, appends
>> > > > _<ino> with sprintf(p + elen, ...).
>> > > >
>> > > > Some callers only provide NAME_MAX bytes. For long snapshot names, a
>> > > > large inode suffix can push the final encoded name past NAME_MAX even
>> > > > though the encrypted prefix stayed within the documented 240-byte
>> > > > budget.
>> > > >
>> > > > Format the suffix into a small local buffer first and reject names
>> > > > whose suffix would exceed the caller's NAME_MAX output buffer.
>> > > >
>> > > > Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
>> > > > ---
>> > > > Changes since v3:
>> > > > - reject `elen > 240` explicitly instead of relying only on the earlier
>> > > >   `WARN_ON()`
>> > > > - rewrite the NAME_MAX bound check in terms of the final total length
>> > > >   instead of `NAME_MAX - prefix_len - elen`
>> > > >
>> > > >  fs/ceph/crypto.c | 31 +++++++++++++++++++++++++++++--
>> > > >  1 file changed, 29 insertions(+), 2 deletions(-)
>> > > >
>> > > > diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
>> > > > index f3de43ccb470..42e3fff34697 100644
>> > > > --- a/fs/ceph/crypto.c
>> > > > +++ b/fs/ceph/crypto.c
>> > > > @@ -15,6 +15,12 @@
>> > > >  #include "mds_client.h"
>> > > >  #include "crypto.h"
>> > > >
>> > > > +/*
>> > > > + * Reserve room for '_' + decimal 64-bit inode number + trailing NUL.
>> > > > + * ceph_encode_encrypted_dname() copies only the visible suffix bytes.
>> > > > + */
>> > > > +#define CEPH_ENCRYPTED_SNAP_INO_SUFFIX_MAX       sizeof("_18446744073709551615")
>> > > > +
>> > > >  static int ceph_crypt_get_context(struct inode *inode, void *ctx, size_t len)
>> > > >  {
>> > > >   struct ceph_inode_info *ci = ceph_inode(inode);
>> > > > @@ -209,6 +215,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
>> > > >   struct inode *dir = parent;
>> > > >   char *p = buf;
>> > > >   u32 len;
>> > > > + int prefix_len = 0;
>> > > >   int name_len = elen;
>> > > >   int ret;
>> > > >   u8 *cryptbuf = NULL;
>> > > > @@ -219,6 +226,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
>> > > >           if (IS_ERR(dir))
>> > > >                   return PTR_ERR(dir);
>> > > >           p++; /* skip initial '_' */
>> > > > +         prefix_len = 1;
>> > > >   }
>> > > >
>> > > >   if (!fscrypt_has_encryption_key(dir))
>> > > > @@ -271,8 +279,27 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
>> > > >
>> > > >   /* To understand the 240 limit, see CEPH_NOHASH_NAME_MAX comments */
>> > > >   WARN_ON(elen > 240);
>> > > > - if (dir != parent) // leading _ is already there; append _<inum>
>> > > > -         elen += 1 + sprintf(p + elen, "_%ld", dir->i_ino);
>> > > > + if (elen > 240) {
>> > > > +         elen = -ENAMETOOLONG;
>> > > > +         goto out;
>> > > > + }
>> > > > +
>> > > > + if (dir != parent) {
>> > > > +         int total_len;
>> > > > +         /* leading '_' is already there; append _<inum> */
>> > > > +         char suffix[CEPH_ENCRYPTED_SNAP_INO_SUFFIX_MAX];
>> > > > +
>> > > > +         ret = snprintf(suffix, sizeof(suffix), "_%lu", dir->i_ino);
>> > > > +         total_len = prefix_len + elen + ret;
>> > > > +         if (total_len > NAME_MAX) {
>> > > > +                 elen = -ENAMETOOLONG;
>> > > > +                 goto out;
>> > > > +         }
>> > > > +
>> > > > +         memcpy(p + elen, suffix, ret);
>> > > > +         /* Include the leading '_' skipped by p. */
>> > > > +         elen = total_len;
>> > > > + }
>> > > >
>> > > >  out:
>> > > >   kfree(cryptbuf);
>> > >
>> > > Looks good.
>> > >
>> > > Reviewed-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
>> > >
>> > > Let me run xfstests for the patch to double check that everything is OK. I'll
>> > > share the result ASAP.
>> > >
>> >
>> > The xfstests run was successful. I don't see any issues with the patch.
>> >
>> > Tested-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
>> >
>> >
>>
>> Applied on testing branch of CephFS kernel client git tree.
>
> Hi Pengpeng, Slava,
>
> This patch raised my attention because my understanding was that the
> entire CEPH_NOHASH_NAME_MAX + sha256() was put in place precisely to
> handle longer names nicely and make them fit into NAME_MAX-sized buffer.
> Simply rejecting longer names seemed to be in direct contradiction with
> that and yet the patch on its own was clearly merited given
>
>  * (240 bytes is the maximum size allowed for snapshot names to take into
>  *  account the format: '_<SNAPSHOT-NAME>_<INODE-NUMBER>'.)
>
> comment on CEPH_NOHASH_NAME_MAX definition.
>
> I dug a bit deeper and started a discussion in [1].  The preliminary
> conclusion is that the 240 bytes assumption was a mistake -- somehow
> the minimum number of characters needed for <inum> ended up being used
> instead of the maximum.  CEPH_NOHASH_NAME_MAX value is likely incorrect
> and should have been smaller -- something along the lines of 174 -
> SHA256_DIGEST_SIZE instead of 180 - SHA256_DIGEST_SIZE.

FWIW I _think_ Ilya may be correct, and there's actually an issue when
these constants were defined.  I spent quite some time last evening trying
to dig into the details on why the inode length was assumed to be 13.
Apparently, it was just a mistake that no one spotted at the time :-(

Unfortunately, my bandwidth to look into this is quite limited.  But I
believe it should be easy to verify that this is indeed a bug by simply
creating snapshots on an encrypted directory that has an inode number
bigger than 2^40 and then checking the large snapshot name in the '.snap'
directory of a subdirectory.

Cheers,
-- 
Luís

> I kept this patch in the testing branch but not including it for 7.1
> pending further investigation.
>
> [1] https://github.com/ceph/ceph/pull/45312
>
> Thanks,
>
>                 Ilya

Re: [PATCH v4] ceph: bound encrypted snapshot suffix formatting

[Ilya Dryomov]

On Thu, Apr 23, 2026 at 8:04 PM Viacheslav Dubeyko
<Slava.Dubeyko@ibm.com> wrote:
>
> On Wed, 2026-04-22 at 11:53 +0200, Ilya Dryomov wrote:
> > On Fri, Apr 10, 2026 at 10:46 PM Viacheslav Dubeyko
> > <Slava.Dubeyko@ibm.com> wrote:
> > >
> > > On Fri, 2026-04-10 at 20:40 +0000, Viacheslav Dubeyko wrote:
> > > > On Thu, 2026-04-09 at 18:09 +0000, Viacheslav Dubeyko wrote:
> > > > > On Thu, 2026-04-09 at 10:39 +0800, Pengpeng Hou wrote:
> > > > > > ceph_encode_encrypted_dname() base64-encodes the encrypted snapshot
> > > > > > name into the caller buffer and then, for long snapshot names, appends
> > > > > > _<ino> with sprintf(p + elen, ...).
> > > > > >
> > > > > > Some callers only provide NAME_MAX bytes. For long snapshot names, a
> > > > > > large inode suffix can push the final encoded name past NAME_MAX even
> > > > > > though the encrypted prefix stayed within the documented 240-byte
> > > > > > budget.
> > > > > >
> > > > > > Format the suffix into a small local buffer first and reject names
> > > > > > whose suffix would exceed the caller's NAME_MAX output buffer.
> > > > > >
> > > > > > Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> > > > > > ---
> > > > > > Changes since v3:
> > > > > > - reject `elen > 240` explicitly instead of relying only on the earlier
> > > > > >   `WARN_ON()`
> > > > > > - rewrite the NAME_MAX bound check in terms of the final total length
> > > > > >   instead of `NAME_MAX - prefix_len - elen`
> > > > > >
> > > > > >  fs/ceph/crypto.c | 31 +++++++++++++++++++++++++++++--
> > > > > >  1 file changed, 29 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
> > > > > > index f3de43ccb470..42e3fff34697 100644
> > > > > > --- a/fs/ceph/crypto.c
> > > > > > +++ b/fs/ceph/crypto.c
> > > > > > @@ -15,6 +15,12 @@
> > > > > >  #include "mds_client.h"
> > > > > >  #include "crypto.h"
> > > > > >
> > > > > > +/*
> > > > > > + * Reserve room for '_' + decimal 64-bit inode number + trailing NUL.
> > > > > > + * ceph_encode_encrypted_dname() copies only the visible suffix bytes.
> > > > > > + */
> > > > > > +#define CEPH_ENCRYPTED_SNAP_INO_SUFFIX_MAX       sizeof("_18446744073709551615")
> > > > > > +
> > > > > >  static int ceph_crypt_get_context(struct inode *inode, void *ctx, size_t len)
> > > > > >  {
> > > > > >   struct ceph_inode_info *ci = ceph_inode(inode);
> > > > > > @@ -209,6 +215,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
> > > > > >   struct inode *dir = parent;
> > > > > >   char *p = buf;
> > > > > >   u32 len;
> > > > > > + int prefix_len = 0;
> > > > > >   int name_len = elen;
> > > > > >   int ret;
> > > > > >   u8 *cryptbuf = NULL;
> > > > > > @@ -219,6 +226,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
> > > > > >           if (IS_ERR(dir))
> > > > > >                   return PTR_ERR(dir);
> > > > > >           p++; /* skip initial '_' */
> > > > > > +         prefix_len = 1;
> > > > > >   }
> > > > > >
> > > > > >   if (!fscrypt_has_encryption_key(dir))
> > > > > > @@ -271,8 +279,27 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
> > > > > >
> > > > > >   /* To understand the 240 limit, see CEPH_NOHASH_NAME_MAX comments */
> > > > > >   WARN_ON(elen > 240);
> > > > > > - if (dir != parent) // leading _ is already there; append _<inum>
> > > > > > -         elen += 1 + sprintf(p + elen, "_%ld", dir->i_ino);
> > > > > > + if (elen > 240) {
> > > > > > +         elen = -ENAMETOOLONG;
> > > > > > +         goto out;
> > > > > > + }
> > > > > > +
> > > > > > + if (dir != parent) {
> > > > > > +         int total_len;
> > > > > > +         /* leading '_' is already there; append _<inum> */
> > > > > > +         char suffix[CEPH_ENCRYPTED_SNAP_INO_SUFFIX_MAX];
> > > > > > +
> > > > > > +         ret = snprintf(suffix, sizeof(suffix), "_%lu", dir->i_ino);
> > > > > > +         total_len = prefix_len + elen + ret;
> > > > > > +         if (total_len > NAME_MAX) {
> > > > > > +                 elen = -ENAMETOOLONG;
> > > > > > +                 goto out;
> > > > > > +         }
> > > > > > +
> > > > > > +         memcpy(p + elen, suffix, ret);
> > > > > > +         /* Include the leading '_' skipped by p. */
> > > > > > +         elen = total_len;
> > > > > > + }
> > > > > >
> > > > > >  out:
> > > > > >   kfree(cryptbuf);
> > > > >
> > > > > Looks good.
> > > > >
> > > > > Reviewed-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
> > > > >
> > > > > Let me run xfstests for the patch to double check that everything is OK. I'll
> > > > > share the result ASAP.
> > > > >
> > > >
> > > > The xfstests run was successful. I don't see any issues with the patch.
> > > >
> > > > Tested-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
> > > >
> > > >
> > >
> > > Applied on testing branch of CephFS kernel client git tree.
> >
> > Hi Pengpeng, Slava,
> >
> > This patch raised my attention because my understanding was that the
> > entire CEPH_NOHASH_NAME_MAX + sha256() was put in place precisely to
> > handle longer names nicely and make them fit into NAME_MAX-sized buffer.
> > Simply rejecting longer names seemed to be in direct contradiction with
> > that and yet the patch on its own was clearly merited given
> >
> >  * (240 bytes is the maximum size allowed for snapshot names to take into
> >  *  account the format: '_<SNAPSHOT-NAME>_<INODE-NUMBER>'.)
> >
> > comment on CEPH_NOHASH_NAME_MAX definition.
> >
> > I dug a bit deeper and started a discussion in [1].  The preliminary
> > conclusion is that the 240 bytes assumption was a mistake -- somehow
> > the minimum number of characters needed for <inum> ended up being used
> > instead of the maximum.  CEPH_NOHASH_NAME_MAX value is likely incorrect
> > and should have been smaller -- something along the lines of 174 -
> > SHA256_DIGEST_SIZE instead of 180 - SHA256_DIGEST_SIZE.
> >
>
> The limitation could be 240 or bigger one, but anyway we need to process this
> limitation in proper way. And this patch has exactly this goal. Is 240 bytes
> limitation your concern? As far as I can see, this patch doesn't introduce this
> limitation. It was there before this modification. We can extend this limitation
> anytime.

Hi Slava,

My take on this is that there shouldn't be a limitation to begin with
(other than NAME_MAX which is natural and applies universally, not just
to snapshot names).  This function has a bunch of code that is there
specifically to handle longer names and avoid any artificial limits:
the part of the name that spills over CEPH_NOHASH_NAME_MAX is hashed
and the whole thing is set up in such a way that the end result is
never bigger than 240 bytes.  240 isn't a random number -- it was
picked on purpose to leave room for _ prefix and _<INODE-NUMBER> suffix
(255 1 - 1 - 13) but a mistake appears to have crept in.  Instead of
accounting for the maximum possible <INODE-NUMBER> length (which is 20
in decimal encoding), it accounted only for 13 (likely because it
happens to be the maximum possible length in a single-MDS setup?).

IMO the right course of action here would be to see if the hashing
parameters can be adjusted, not introduce new ENAMETOOLONG errors.

Thanks,

                Ilya

RE: [PATCH v4] ceph: bound encrypted snapshot suffix formatting

[Viacheslav Dubeyko]

On Fri, 2026-04-24 at 11:27 +0200, Ilya Dryomov wrote:
> On Thu, Apr 23, 2026 at 8:04 PM Viacheslav Dubeyko
> <Slava.Dubeyko@ibm.com> wrote:
> > 
> > On Wed, 2026-04-22 at 11:53 +0200, Ilya Dryomov wrote:
> > > On Fri, Apr 10, 2026 at 10:46 PM Viacheslav Dubeyko
> > > <Slava.Dubeyko@ibm.com> wrote:
> > > > 
> > > > On Fri, 2026-04-10 at 20:40 +0000, Viacheslav Dubeyko wrote:
> > > > > On Thu, 2026-04-09 at 18:09 +0000, Viacheslav Dubeyko wrote:
> > > > > > On Thu, 2026-04-09 at 10:39 +0800, Pengpeng Hou wrote:
> > > > > > > ceph_encode_encrypted_dname() base64-encodes the encrypted snapshot
> > > > > > > name into the caller buffer and then, for long snapshot names, appends
> > > > > > > _<ino> with sprintf(p + elen, ...).
> > > > > > > 
> > > > > > > Some callers only provide NAME_MAX bytes. For long snapshot names, a
> > > > > > > large inode suffix can push the final encoded name past NAME_MAX even
> > > > > > > though the encrypted prefix stayed within the documented 240-byte
> > > > > > > budget.
> > > > > > > 
> > > > > > > Format the suffix into a small local buffer first and reject names
> > > > > > > whose suffix would exceed the caller's NAME_MAX output buffer.
> > > > > > > 
> > > > > > > Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> > > > > > > ---
> > > > > > > Changes since v3:
> > > > > > > - reject `elen > 240` explicitly instead of relying only on the earlier
> > > > > > >   `WARN_ON()`
> > > > > > > - rewrite the NAME_MAX bound check in terms of the final total length
> > > > > > >   instead of `NAME_MAX - prefix_len - elen`
> > > > > > > 
> > > > > > >  fs/ceph/crypto.c | 31 +++++++++++++++++++++++++++++--
> > > > > > >  1 file changed, 29 insertions(+), 2 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
> > > > > > > index f3de43ccb470..42e3fff34697 100644
> > > > > > > --- a/fs/ceph/crypto.c
> > > > > > > +++ b/fs/ceph/crypto.c
> > > > > > > @@ -15,6 +15,12 @@
> > > > > > >  #include "mds_client.h"
> > > > > > >  #include "crypto.h"
> > > > > > > 
> > > > > > > +/*
> > > > > > > + * Reserve room for '_' + decimal 64-bit inode number + trailing NUL.
> > > > > > > + * ceph_encode_encrypted_dname() copies only the visible suffix bytes.
> > > > > > > + */
> > > > > > > +#define CEPH_ENCRYPTED_SNAP_INO_SUFFIX_MAX       sizeof("_18446744073709551615")
> > > > > > > +
> > > > > > >  static int ceph_crypt_get_context(struct inode *inode, void *ctx, size_t len)
> > > > > > >  {
> > > > > > >   struct ceph_inode_info *ci = ceph_inode(inode);
> > > > > > > @@ -209,6 +215,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
> > > > > > >   struct inode *dir = parent;
> > > > > > >   char *p = buf;
> > > > > > >   u32 len;
> > > > > > > + int prefix_len = 0;
> > > > > > >   int name_len = elen;
> > > > > > >   int ret;
> > > > > > >   u8 *cryptbuf = NULL;
> > > > > > > @@ -219,6 +226,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
> > > > > > >           if (IS_ERR(dir))
> > > > > > >                   return PTR_ERR(dir);
> > > > > > >           p++; /* skip initial '_' */
> > > > > > > +         prefix_len = 1;
> > > > > > >   }
> > > > > > > 
> > > > > > >   if (!fscrypt_has_encryption_key(dir))
> > > > > > > @@ -271,8 +279,27 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
> > > > > > > 
> > > > > > >   /* To understand the 240 limit, see CEPH_NOHASH_NAME_MAX comments */
> > > > > > >   WARN_ON(elen > 240);
> > > > > > > - if (dir != parent) // leading _ is already there; append _<inum>
> > > > > > > -         elen += 1 + sprintf(p + elen, "_%ld", dir->i_ino);
> > > > > > > + if (elen > 240) {
> > > > > > > +         elen = -ENAMETOOLONG;
> > > > > > > +         goto out;
> > > > > > > + }
> > > > > > > +
> > > > > > > + if (dir != parent) {
> > > > > > > +         int total_len;
> > > > > > > +         /* leading '_' is already there; append _<inum> */
> > > > > > > +         char suffix[CEPH_ENCRYPTED_SNAP_INO_SUFFIX_MAX];
> > > > > > > +
> > > > > > > +         ret = snprintf(suffix, sizeof(suffix), "_%lu", dir->i_ino);
> > > > > > > +         total_len = prefix_len + elen + ret;
> > > > > > > +         if (total_len > NAME_MAX) {
> > > > > > > +                 elen = -ENAMETOOLONG;
> > > > > > > +                 goto out;
> > > > > > > +         }
> > > > > > > +
> > > > > > > +         memcpy(p + elen, suffix, ret);
> > > > > > > +         /* Include the leading '_' skipped by p. */
> > > > > > > +         elen = total_len;
> > > > > > > + }
> > > > > > > 
> > > > > > >  out:
> > > > > > >   kfree(cryptbuf);
> > > > > > 
> > > > > > Looks good.
> > > > > > 
> > > > > > Reviewed-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
> > > > > > 
> > > > > > Let me run xfstests for the patch to double check that everything is OK. I'll
> > > > > > share the result ASAP.
> > > > > > 
> > > > > 
> > > > > The xfstests run was successful. I don't see any issues with the patch.
> > > > > 
> > > > > Tested-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
> > > > > 
> > > > > 
> > > > 
> > > > Applied on testing branch of CephFS kernel client git tree.
> > > 
> > > Hi Pengpeng, Slava,
> > > 
> > > This patch raised my attention because my understanding was that the
> > > entire CEPH_NOHASH_NAME_MAX + sha256() was put in place precisely to
> > > handle longer names nicely and make them fit into NAME_MAX-sized buffer.
> > > Simply rejecting longer names seemed to be in direct contradiction with
> > > that and yet the patch on its own was clearly merited given
> > > 
> > >  * (240 bytes is the maximum size allowed for snapshot names to take into
> > >  *  account the format: '_<SNAPSHOT-NAME>_<INODE-NUMBER>'.)
> > > 
> > > comment on CEPH_NOHASH_NAME_MAX definition.
> > > 
> > > I dug a bit deeper and started a discussion in [1].  The preliminary
> > > conclusion is that the 240 bytes assumption was a mistake -- somehow
> > > the minimum number of characters needed for <inum> ended up being used
> > > instead of the maximum.  CEPH_NOHASH_NAME_MAX value is likely incorrect
> > > and should have been smaller -- something along the lines of 174 -
> > > SHA256_DIGEST_SIZE instead of 180 - SHA256_DIGEST_SIZE.
> > > 
> > 
> > The limitation could be 240 or bigger one, but anyway we need to process this
> > limitation in proper way. And this patch has exactly this goal. Is 240 bytes
> > limitation your concern? As far as I can see, this patch doesn't introduce this
> > limitation. It was there before this modification. We can extend this limitation
> > anytime.
> 
> Hi Slava,
> 
> My take on this is that there shouldn't be a limitation to begin with
> (other than NAME_MAX which is natural and applies universally, not just
> to snapshot names).  This function has a bunch of code that is there
> specifically to handle longer names and avoid any artificial limits:
> the part of the name that spills over CEPH_NOHASH_NAME_MAX is hashed
> and the whole thing is set up in such a way that the end result is
> never bigger than 240 bytes.  240 isn't a random number -- it was
> picked on purpose to leave room for _ prefix and _<INODE-NUMBER> suffix
> (255 1 - 1 - 13) but a mistake appears to have crept in.  Instead of
> accounting for the maximum possible <INODE-NUMBER> length (which is 20
> in decimal encoding), it accounted only for 13 (likely because it
> happens to be the maximum possible length in a single-MDS setup?).
> 
> IMO the right course of action here would be to see if the hashing
> parameters can be adjusted, not introduce new ENAMETOOLONG errors.
> 
> 

Hi Pengpeng,

Could you please rework the patch taking into account the shared remarks?

Thanks,
Slava.

Re: [PATCH v4] ceph: bound encrypted snapshot suffix formatting

[Ilya Dryomov]

On Fri, Apr 24, 2026 at 8:31 PM Viacheslav Dubeyko
<Slava.Dubeyko@ibm.com> wrote:
>
> On Fri, 2026-04-24 at 11:27 +0200, Ilya Dryomov wrote:
> > On Thu, Apr 23, 2026 at 8:04 PM Viacheslav Dubeyko
> > <Slava.Dubeyko@ibm.com> wrote:
> > >
> > > On Wed, 2026-04-22 at 11:53 +0200, Ilya Dryomov wrote:
> > > > On Fri, Apr 10, 2026 at 10:46 PM Viacheslav Dubeyko
> > > > <Slava.Dubeyko@ibm.com> wrote:
> > > > >
> > > > > On Fri, 2026-04-10 at 20:40 +0000, Viacheslav Dubeyko wrote:
> > > > > > On Thu, 2026-04-09 at 18:09 +0000, Viacheslav Dubeyko wrote:
> > > > > > > On Thu, 2026-04-09 at 10:39 +0800, Pengpeng Hou wrote:
> > > > > > > > ceph_encode_encrypted_dname() base64-encodes the encrypted snapshot
> > > > > > > > name into the caller buffer and then, for long snapshot names, appends
> > > > > > > > _<ino> with sprintf(p + elen, ...).
> > > > > > > >
> > > > > > > > Some callers only provide NAME_MAX bytes. For long snapshot names, a
> > > > > > > > large inode suffix can push the final encoded name past NAME_MAX even
> > > > > > > > though the encrypted prefix stayed within the documented 240-byte
> > > > > > > > budget.
> > > > > > > >
> > > > > > > > Format the suffix into a small local buffer first and reject names
> > > > > > > > whose suffix would exceed the caller's NAME_MAX output buffer.
> > > > > > > >
> > > > > > > > Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> > > > > > > > ---
> > > > > > > > Changes since v3:
> > > > > > > > - reject `elen > 240` explicitly instead of relying only on the earlier
> > > > > > > >   `WARN_ON()`
> > > > > > > > - rewrite the NAME_MAX bound check in terms of the final total length
> > > > > > > >   instead of `NAME_MAX - prefix_len - elen`
> > > > > > > >
> > > > > > > >  fs/ceph/crypto.c | 31 +++++++++++++++++++++++++++++--
> > > > > > > >  1 file changed, 29 insertions(+), 2 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
> > > > > > > > index f3de43ccb470..42e3fff34697 100644
> > > > > > > > --- a/fs/ceph/crypto.c
> > > > > > > > +++ b/fs/ceph/crypto.c
> > > > > > > > @@ -15,6 +15,12 @@
> > > > > > > >  #include "mds_client.h"
> > > > > > > >  #include "crypto.h"
> > > > > > > >
> > > > > > > > +/*
> > > > > > > > + * Reserve room for '_' + decimal 64-bit inode number + trailing NUL.
> > > > > > > > + * ceph_encode_encrypted_dname() copies only the visible suffix bytes.
> > > > > > > > + */
> > > > > > > > +#define CEPH_ENCRYPTED_SNAP_INO_SUFFIX_MAX       sizeof("_18446744073709551615")
> > > > > > > > +
> > > > > > > >  static int ceph_crypt_get_context(struct inode *inode, void *ctx, size_t len)
> > > > > > > >  {
> > > > > > > >   struct ceph_inode_info *ci = ceph_inode(inode);
> > > > > > > > @@ -209,6 +215,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
> > > > > > > >   struct inode *dir = parent;
> > > > > > > >   char *p = buf;
> > > > > > > >   u32 len;
> > > > > > > > + int prefix_len = 0;
> > > > > > > >   int name_len = elen;
> > > > > > > >   int ret;
> > > > > > > >   u8 *cryptbuf = NULL;
> > > > > > > > @@ -219,6 +226,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
> > > > > > > >           if (IS_ERR(dir))
> > > > > > > >                   return PTR_ERR(dir);
> > > > > > > >           p++; /* skip initial '_' */
> > > > > > > > +         prefix_len = 1;
> > > > > > > >   }
> > > > > > > >
> > > > > > > >   if (!fscrypt_has_encryption_key(dir))
> > > > > > > > @@ -271,8 +279,27 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
> > > > > > > >
> > > > > > > >   /* To understand the 240 limit, see CEPH_NOHASH_NAME_MAX comments */
> > > > > > > >   WARN_ON(elen > 240);
> > > > > > > > - if (dir != parent) // leading _ is already there; append _<inum>
> > > > > > > > -         elen += 1 + sprintf(p + elen, "_%ld", dir->i_ino);
> > > > > > > > + if (elen > 240) {
> > > > > > > > +         elen = -ENAMETOOLONG;
> > > > > > > > +         goto out;
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + if (dir != parent) {
> > > > > > > > +         int total_len;
> > > > > > > > +         /* leading '_' is already there; append _<inum> */
> > > > > > > > +         char suffix[CEPH_ENCRYPTED_SNAP_INO_SUFFIX_MAX];
> > > > > > > > +
> > > > > > > > +         ret = snprintf(suffix, sizeof(suffix), "_%lu", dir->i_ino);
> > > > > > > > +         total_len = prefix_len + elen + ret;
> > > > > > > > +         if (total_len > NAME_MAX) {
> > > > > > > > +                 elen = -ENAMETOOLONG;
> > > > > > > > +                 goto out;
> > > > > > > > +         }
> > > > > > > > +
> > > > > > > > +         memcpy(p + elen, suffix, ret);
> > > > > > > > +         /* Include the leading '_' skipped by p. */
> > > > > > > > +         elen = total_len;
> > > > > > > > + }
> > > > > > > >
> > > > > > > >  out:
> > > > > > > >   kfree(cryptbuf);
> > > > > > >
> > > > > > > Looks good.
> > > > > > >
> > > > > > > Reviewed-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
> > > > > > >
> > > > > > > Let me run xfstests for the patch to double check that everything is OK. I'll
> > > > > > > share the result ASAP.
> > > > > > >
> > > > > >
> > > > > > The xfstests run was successful. I don't see any issues with the patch.
> > > > > >
> > > > > > Tested-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
> > > > > >
> > > > > >
> > > > >
> > > > > Applied on testing branch of CephFS kernel client git tree.
> > > >
> > > > Hi Pengpeng, Slava,
> > > >
> > > > This patch raised my attention because my understanding was that the
> > > > entire CEPH_NOHASH_NAME_MAX + sha256() was put in place precisely to
> > > > handle longer names nicely and make them fit into NAME_MAX-sized buffer.
> > > > Simply rejecting longer names seemed to be in direct contradiction with
> > > > that and yet the patch on its own was clearly merited given
> > > >
> > > >  * (240 bytes is the maximum size allowed for snapshot names to take into
> > > >  *  account the format: '_<SNAPSHOT-NAME>_<INODE-NUMBER>'.)
> > > >
> > > > comment on CEPH_NOHASH_NAME_MAX definition.
> > > >
> > > > I dug a bit deeper and started a discussion in [1].  The preliminary
> > > > conclusion is that the 240 bytes assumption was a mistake -- somehow
> > > > the minimum number of characters needed for <inum> ended up being used
> > > > instead of the maximum.  CEPH_NOHASH_NAME_MAX value is likely incorrect
> > > > and should have been smaller -- something along the lines of 174 -
> > > > SHA256_DIGEST_SIZE instead of 180 - SHA256_DIGEST_SIZE.
> > > >
> > >
> > > The limitation could be 240 or bigger one, but anyway we need to process this
> > > limitation in proper way. And this patch has exactly this goal. Is 240 bytes
> > > limitation your concern? As far as I can see, this patch doesn't introduce this
> > > limitation. It was there before this modification. We can extend this limitation
> > > anytime.
> >
> > Hi Slava,
> >
> > My take on this is that there shouldn't be a limitation to begin with
> > (other than NAME_MAX which is natural and applies universally, not just
> > to snapshot names).  This function has a bunch of code that is there
> > specifically to handle longer names and avoid any artificial limits:
> > the part of the name that spills over CEPH_NOHASH_NAME_MAX is hashed
> > and the whole thing is set up in such a way that the end result is
> > never bigger than 240 bytes.  240 isn't a random number -- it was
> > picked on purpose to leave room for _ prefix and _<INODE-NUMBER> suffix
> > (255 1 - 1 - 13) but a mistake appears to have crept in.  Instead of
> > accounting for the maximum possible <INODE-NUMBER> length (which is 20
> > in decimal encoding), it accounted only for 13 (likely because it
> > happens to be the maximum possible length in a single-MDS setup?).
> >
> > IMO the right course of action here would be to see if the hashing
> > parameters can be adjusted, not introduce new ENAMETOOLONG errors.
> >
> >
>
> Hi Pengpeng,
>
> Could you please rework the patch taking into account the shared remarks?

I would hold on until the discussion in [1] comes to conclusion.  It
turns out that the userspace client doesn't encrypt snapshot names
anymore, so another (much worse, at least IMO) route would be to drop
this bit of functionality from the kernel client as well [2].

[1] https://github.com/ceph/ceph/pull/45312
[2] https://tracker.ceph.com/issues/76257

Thanks,

                Ilya

Re: [PATCH v4] ceph: bound encrypted snapshot suffix formatting

[Luis Henriques]

On Fri, Apr 24 2026, Ilya Dryomov wrote:

> On Fri, Apr 24, 2026 at 8:31 PM Viacheslav Dubeyko
> <Slava.Dubeyko@ibm.com> wrote:
>>
>> On Fri, 2026-04-24 at 11:27 +0200, Ilya Dryomov wrote:
>> > On Thu, Apr 23, 2026 at 8:04 PM Viacheslav Dubeyko
>> > <Slava.Dubeyko@ibm.com> wrote:
>> > >
>> > > On Wed, 2026-04-22 at 11:53 +0200, Ilya Dryomov wrote:
>> > > > On Fri, Apr 10, 2026 at 10:46 PM Viacheslav Dubeyko
>> > > > <Slava.Dubeyko@ibm.com> wrote:
>> > > > >
>> > > > > On Fri, 2026-04-10 at 20:40 +0000, Viacheslav Dubeyko wrote:
>> > > > > > On Thu, 2026-04-09 at 18:09 +0000, Viacheslav Dubeyko wrote:
>> > > > > > > On Thu, 2026-04-09 at 10:39 +0800, Pengpeng Hou wrote:
>> > > > > > > > ceph_encode_encrypted_dname() base64-encodes the encrypted snapshot
>> > > > > > > > name into the caller buffer and then, for long snapshot names, appends
>> > > > > > > > _<ino> with sprintf(p + elen, ...).
>> > > > > > > >
>> > > > > > > > Some callers only provide NAME_MAX bytes. For long snapshot names, a
>> > > > > > > > large inode suffix can push the final encoded name past NAME_MAX even
>> > > > > > > > though the encrypted prefix stayed within the documented 240-byte
>> > > > > > > > budget.
>> > > > > > > >
>> > > > > > > > Format the suffix into a small local buffer first and reject names
>> > > > > > > > whose suffix would exceed the caller's NAME_MAX output buffer.
>> > > > > > > >
>> > > > > > > > Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
>> > > > > > > > ---
>> > > > > > > > Changes since v3:
>> > > > > > > > - reject `elen > 240` explicitly instead of relying only on the earlier
>> > > > > > > >   `WARN_ON()`
>> > > > > > > > - rewrite the NAME_MAX bound check in terms of the final total length
>> > > > > > > >   instead of `NAME_MAX - prefix_len - elen`
>> > > > > > > >
>> > > > > > > >  fs/ceph/crypto.c | 31 +++++++++++++++++++++++++++++--
>> > > > > > > >  1 file changed, 29 insertions(+), 2 deletions(-)
>> > > > > > > >
>> > > > > > > > diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
>> > > > > > > > index f3de43ccb470..42e3fff34697 100644
>> > > > > > > > --- a/fs/ceph/crypto.c
>> > > > > > > > +++ b/fs/ceph/crypto.c
>> > > > > > > > @@ -15,6 +15,12 @@
>> > > > > > > >  #include "mds_client.h"
>> > > > > > > >  #include "crypto.h"
>> > > > > > > >
>> > > > > > > > +/*
>> > > > > > > > + * Reserve room for '_' + decimal 64-bit inode number + trailing NUL.
>> > > > > > > > + * ceph_encode_encrypted_dname() copies only the visible suffix bytes.
>> > > > > > > > + */
>> > > > > > > > +#define CEPH_ENCRYPTED_SNAP_INO_SUFFIX_MAX       sizeof("_18446744073709551615")
>> > > > > > > > +
>> > > > > > > >  static int ceph_crypt_get_context(struct inode *inode, void *ctx, size_t len)
>> > > > > > > >  {
>> > > > > > > >   struct ceph_inode_info *ci = ceph_inode(inode);
>> > > > > > > > @@ -209,6 +215,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
>> > > > > > > >   struct inode *dir = parent;
>> > > > > > > >   char *p = buf;
>> > > > > > > >   u32 len;
>> > > > > > > > + int prefix_len = 0;
>> > > > > > > >   int name_len = elen;
>> > > > > > > >   int ret;
>> > > > > > > >   u8 *cryptbuf = NULL;
>> > > > > > > > @@ -219,6 +226,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
>> > > > > > > >           if (IS_ERR(dir))
>> > > > > > > >                   return PTR_ERR(dir);
>> > > > > > > >           p++; /* skip initial '_' */
>> > > > > > > > +         prefix_len = 1;
>> > > > > > > >   }
>> > > > > > > >
>> > > > > > > >   if (!fscrypt_has_encryption_key(dir))
>> > > > > > > > @@ -271,8 +279,27 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
>> > > > > > > >
>> > > > > > > >   /* To understand the 240 limit, see CEPH_NOHASH_NAME_MAX comments */
>> > > > > > > >   WARN_ON(elen > 240);
>> > > > > > > > - if (dir != parent) // leading _ is already there; append _<inum>
>> > > > > > > > -         elen += 1 + sprintf(p + elen, "_%ld", dir->i_ino);
>> > > > > > > > + if (elen > 240) {
>> > > > > > > > +         elen = -ENAMETOOLONG;
>> > > > > > > > +         goto out;
>> > > > > > > > + }
>> > > > > > > > +
>> > > > > > > > + if (dir != parent) {
>> > > > > > > > +         int total_len;
>> > > > > > > > +         /* leading '_' is already there; append _<inum> */
>> > > > > > > > +         char suffix[CEPH_ENCRYPTED_SNAP_INO_SUFFIX_MAX];
>> > > > > > > > +
>> > > > > > > > +         ret = snprintf(suffix, sizeof(suffix), "_%lu", dir->i_ino);
>> > > > > > > > +         total_len = prefix_len + elen + ret;
>> > > > > > > > +         if (total_len > NAME_MAX) {
>> > > > > > > > +                 elen = -ENAMETOOLONG;
>> > > > > > > > +                 goto out;
>> > > > > > > > +         }
>> > > > > > > > +
>> > > > > > > > +         memcpy(p + elen, suffix, ret);
>> > > > > > > > +         /* Include the leading '_' skipped by p. */
>> > > > > > > > +         elen = total_len;
>> > > > > > > > + }
>> > > > > > > >
>> > > > > > > >  out:
>> > > > > > > >   kfree(cryptbuf);
>> > > > > > >
>> > > > > > > Looks good.
>> > > > > > >
>> > > > > > > Reviewed-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
>> > > > > > >
>> > > > > > > Let me run xfstests for the patch to double check that everything is OK. I'll
>> > > > > > > share the result ASAP.
>> > > > > > >
>> > > > > >
>> > > > > > The xfstests run was successful. I don't see any issues with the patch.
>> > > > > >
>> > > > > > Tested-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
>> > > > > >
>> > > > > >
>> > > > >
>> > > > > Applied on testing branch of CephFS kernel client git tree.
>> > > >
>> > > > Hi Pengpeng, Slava,
>> > > >
>> > > > This patch raised my attention because my understanding was that the
>> > > > entire CEPH_NOHASH_NAME_MAX + sha256() was put in place precisely to
>> > > > handle longer names nicely and make them fit into NAME_MAX-sized buffer.
>> > > > Simply rejecting longer names seemed to be in direct contradiction with
>> > > > that and yet the patch on its own was clearly merited given
>> > > >
>> > > >  * (240 bytes is the maximum size allowed for snapshot names to take into
>> > > >  *  account the format: '_<SNAPSHOT-NAME>_<INODE-NUMBER>'.)
>> > > >
>> > > > comment on CEPH_NOHASH_NAME_MAX definition.
>> > > >
>> > > > I dug a bit deeper and started a discussion in [1].  The preliminary
>> > > > conclusion is that the 240 bytes assumption was a mistake -- somehow
>> > > > the minimum number of characters needed for <inum> ended up being used
>> > > > instead of the maximum.  CEPH_NOHASH_NAME_MAX value is likely incorrect
>> > > > and should have been smaller -- something along the lines of 174 -
>> > > > SHA256_DIGEST_SIZE instead of 180 - SHA256_DIGEST_SIZE.
>> > > >
>> > >
>> > > The limitation could be 240 or bigger one, but anyway we need to process this
>> > > limitation in proper way. And this patch has exactly this goal. Is 240 bytes
>> > > limitation your concern? As far as I can see, this patch doesn't introduce this
>> > > limitation. It was there before this modification. We can extend this limitation
>> > > anytime.
>> >
>> > Hi Slava,
>> >
>> > My take on this is that there shouldn't be a limitation to begin with
>> > (other than NAME_MAX which is natural and applies universally, not just
>> > to snapshot names).  This function has a bunch of code that is there
>> > specifically to handle longer names and avoid any artificial limits:
>> > the part of the name that spills over CEPH_NOHASH_NAME_MAX is hashed
>> > and the whole thing is set up in such a way that the end result is
>> > never bigger than 240 bytes.  240 isn't a random number -- it was
>> > picked on purpose to leave room for _ prefix and _<INODE-NUMBER> suffix
>> > (255 1 - 1 - 13) but a mistake appears to have crept in.  Instead of
>> > accounting for the maximum possible <INODE-NUMBER> length (which is 20
>> > in decimal encoding), it accounted only for 13 (likely because it
>> > happens to be the maximum possible length in a single-MDS setup?).
>> >
>> > IMO the right course of action here would be to see if the hashing
>> > parameters can be adjusted, not introduce new ENAMETOOLONG errors.
>> >
>> >
>>
>> Hi Pengpeng,
>>
>> Could you please rework the patch taking into account the shared remarks?
>
> I would hold on until the discussion in [1] comes to conclusion.  It
> turns out that the userspace client doesn't encrypt snapshot names
> anymore, so another (much worse, at least IMO) route would be to drop
> this bit of functionality from the kernel client as well [2].

OK, let me see if I understood this correctly:

- The user-space client implementation leaks metadata (the snaphsot names)
- The kernel client doesn't leak the snapshots names, thought there are
  bugs to be fixed (including on the MDS side)
- The proposed solution is to drop the kernel snapshot names encryption.

So, currently snapshots created on the kernel client can't be accessed
from the user-space client, and vice-versa?

Also, won't dropping the encryption from the kernel client effectively
make old snapshots created using a kernel client unusable?

Cheers,
-- 
Luís

>
> [1] https://github.com/ceph/ceph/pull/45312
> [2] https://tracker.ceph.com/issues/76257
>
> Thanks,
>
>                 Ilya

Re: [PATCH v4] ceph: bound encrypted snapshot suffix formatting

[Ilya Dryomov]

On Mon, Apr 27, 2026 at 10:12 AM Luis Henriques <luis@igalia.com> wrote:
>
> On Fri, Apr 24 2026, Ilya Dryomov wrote:
>
> > On Fri, Apr 24, 2026 at 8:31 PM Viacheslav Dubeyko
> > <Slava.Dubeyko@ibm.com> wrote:
> >>
> >> On Fri, 2026-04-24 at 11:27 +0200, Ilya Dryomov wrote:
> >> > On Thu, Apr 23, 2026 at 8:04 PM Viacheslav Dubeyko
> >> > <Slava.Dubeyko@ibm.com> wrote:
> >> > >
> >> > > On Wed, 2026-04-22 at 11:53 +0200, Ilya Dryomov wrote:
> >> > > > On Fri, Apr 10, 2026 at 10:46 PM Viacheslav Dubeyko
> >> > > > <Slava.Dubeyko@ibm.com> wrote:
> >> > > > >
> >> > > > > On Fri, 2026-04-10 at 20:40 +0000, Viacheslav Dubeyko wrote:
> >> > > > > > On Thu, 2026-04-09 at 18:09 +0000, Viacheslav Dubeyko wrote:
> >> > > > > > > On Thu, 2026-04-09 at 10:39 +0800, Pengpeng Hou wrote:
> >> > > > > > > > ceph_encode_encrypted_dname() base64-encodes the encrypted snapshot
> >> > > > > > > > name into the caller buffer and then, for long snapshot names, appends
> >> > > > > > > > _<ino> with sprintf(p + elen, ...).
> >> > > > > > > >
> >> > > > > > > > Some callers only provide NAME_MAX bytes. For long snapshot names, a
> >> > > > > > > > large inode suffix can push the final encoded name past NAME_MAX even
> >> > > > > > > > though the encrypted prefix stayed within the documented 240-byte
> >> > > > > > > > budget.
> >> > > > > > > >
> >> > > > > > > > Format the suffix into a small local buffer first and reject names
> >> > > > > > > > whose suffix would exceed the caller's NAME_MAX output buffer.
> >> > > > > > > >
> >> > > > > > > > Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> >> > > > > > > > ---
> >> > > > > > > > Changes since v3:
> >> > > > > > > > - reject `elen > 240` explicitly instead of relying only on the earlier
> >> > > > > > > >   `WARN_ON()`
> >> > > > > > > > - rewrite the NAME_MAX bound check in terms of the final total length
> >> > > > > > > >   instead of `NAME_MAX - prefix_len - elen`
> >> > > > > > > >
> >> > > > > > > >  fs/ceph/crypto.c | 31 +++++++++++++++++++++++++++++--
> >> > > > > > > >  1 file changed, 29 insertions(+), 2 deletions(-)
> >> > > > > > > >
> >> > > > > > > > diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
> >> > > > > > > > index f3de43ccb470..42e3fff34697 100644
> >> > > > > > > > --- a/fs/ceph/crypto.c
> >> > > > > > > > +++ b/fs/ceph/crypto.c
> >> > > > > > > > @@ -15,6 +15,12 @@
> >> > > > > > > >  #include "mds_client.h"
> >> > > > > > > >  #include "crypto.h"
> >> > > > > > > >
> >> > > > > > > > +/*
> >> > > > > > > > + * Reserve room for '_' + decimal 64-bit inode number + trailing NUL.
> >> > > > > > > > + * ceph_encode_encrypted_dname() copies only the visible suffix bytes.
> >> > > > > > > > + */
> >> > > > > > > > +#define CEPH_ENCRYPTED_SNAP_INO_SUFFIX_MAX       sizeof("_18446744073709551615")
> >> > > > > > > > +
> >> > > > > > > >  static int ceph_crypt_get_context(struct inode *inode, void *ctx, size_t len)
> >> > > > > > > >  {
> >> > > > > > > >   struct ceph_inode_info *ci = ceph_inode(inode);
> >> > > > > > > > @@ -209,6 +215,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
> >> > > > > > > >   struct inode *dir = parent;
> >> > > > > > > >   char *p = buf;
> >> > > > > > > >   u32 len;
> >> > > > > > > > + int prefix_len = 0;
> >> > > > > > > >   int name_len = elen;
> >> > > > > > > >   int ret;
> >> > > > > > > >   u8 *cryptbuf = NULL;
> >> > > > > > > > @@ -219,6 +226,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
> >> > > > > > > >           if (IS_ERR(dir))
> >> > > > > > > >                   return PTR_ERR(dir);
> >> > > > > > > >           p++; /* skip initial '_' */
> >> > > > > > > > +         prefix_len = 1;
> >> > > > > > > >   }
> >> > > > > > > >
> >> > > > > > > >   if (!fscrypt_has_encryption_key(dir))
> >> > > > > > > > @@ -271,8 +279,27 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
> >> > > > > > > >
> >> > > > > > > >   /* To understand the 240 limit, see CEPH_NOHASH_NAME_MAX comments */
> >> > > > > > > >   WARN_ON(elen > 240);
> >> > > > > > > > - if (dir != parent) // leading _ is already there; append _<inum>
> >> > > > > > > > -         elen += 1 + sprintf(p + elen, "_%ld", dir->i_ino);
> >> > > > > > > > + if (elen > 240) {
> >> > > > > > > > +         elen = -ENAMETOOLONG;
> >> > > > > > > > +         goto out;
> >> > > > > > > > + }
> >> > > > > > > > +
> >> > > > > > > > + if (dir != parent) {
> >> > > > > > > > +         int total_len;
> >> > > > > > > > +         /* leading '_' is already there; append _<inum> */
> >> > > > > > > > +         char suffix[CEPH_ENCRYPTED_SNAP_INO_SUFFIX_MAX];
> >> > > > > > > > +
> >> > > > > > > > +         ret = snprintf(suffix, sizeof(suffix), "_%lu", dir->i_ino);
> >> > > > > > > > +         total_len = prefix_len + elen + ret;
> >> > > > > > > > +         if (total_len > NAME_MAX) {
> >> > > > > > > > +                 elen = -ENAMETOOLONG;
> >> > > > > > > > +                 goto out;
> >> > > > > > > > +         }
> >> > > > > > > > +
> >> > > > > > > > +         memcpy(p + elen, suffix, ret);
> >> > > > > > > > +         /* Include the leading '_' skipped by p. */
> >> > > > > > > > +         elen = total_len;
> >> > > > > > > > + }
> >> > > > > > > >
> >> > > > > > > >  out:
> >> > > > > > > >   kfree(cryptbuf);
> >> > > > > > >
> >> > > > > > > Looks good.
> >> > > > > > >
> >> > > > > > > Reviewed-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
> >> > > > > > >
> >> > > > > > > Let me run xfstests for the patch to double check that everything is OK. I'll
> >> > > > > > > share the result ASAP.
> >> > > > > > >
> >> > > > > >
> >> > > > > > The xfstests run was successful. I don't see any issues with the patch.
> >> > > > > >
> >> > > > > > Tested-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
> >> > > > > >
> >> > > > > >
> >> > > > >
> >> > > > > Applied on testing branch of CephFS kernel client git tree.
> >> > > >
> >> > > > Hi Pengpeng, Slava,
> >> > > >
> >> > > > This patch raised my attention because my understanding was that the
> >> > > > entire CEPH_NOHASH_NAME_MAX + sha256() was put in place precisely to
> >> > > > handle longer names nicely and make them fit into NAME_MAX-sized buffer.
> >> > > > Simply rejecting longer names seemed to be in direct contradiction with
> >> > > > that and yet the patch on its own was clearly merited given
> >> > > >
> >> > > >  * (240 bytes is the maximum size allowed for snapshot names to take into
> >> > > >  *  account the format: '_<SNAPSHOT-NAME>_<INODE-NUMBER>'.)
> >> > > >
> >> > > > comment on CEPH_NOHASH_NAME_MAX definition.
> >> > > >
> >> > > > I dug a bit deeper and started a discussion in [1].  The preliminary
> >> > > > conclusion is that the 240 bytes assumption was a mistake -- somehow
> >> > > > the minimum number of characters needed for <inum> ended up being used
> >> > > > instead of the maximum.  CEPH_NOHASH_NAME_MAX value is likely incorrect
> >> > > > and should have been smaller -- something along the lines of 174 -
> >> > > > SHA256_DIGEST_SIZE instead of 180 - SHA256_DIGEST_SIZE.
> >> > > >
> >> > >
> >> > > The limitation could be 240 or bigger one, but anyway we need to process this
> >> > > limitation in proper way. And this patch has exactly this goal. Is 240 bytes
> >> > > limitation your concern? As far as I can see, this patch doesn't introduce this
> >> > > limitation. It was there before this modification. We can extend this limitation
> >> > > anytime.
> >> >
> >> > Hi Slava,
> >> >
> >> > My take on this is that there shouldn't be a limitation to begin with
> >> > (other than NAME_MAX which is natural and applies universally, not just
> >> > to snapshot names).  This function has a bunch of code that is there
> >> > specifically to handle longer names and avoid any artificial limits:
> >> > the part of the name that spills over CEPH_NOHASH_NAME_MAX is hashed
> >> > and the whole thing is set up in such a way that the end result is
> >> > never bigger than 240 bytes.  240 isn't a random number -- it was
> >> > picked on purpose to leave room for _ prefix and _<INODE-NUMBER> suffix
> >> > (255 1 - 1 - 13) but a mistake appears to have crept in.  Instead of
> >> > accounting for the maximum possible <INODE-NUMBER> length (which is 20
> >> > in decimal encoding), it accounted only for 13 (likely because it
> >> > happens to be the maximum possible length in a single-MDS setup?).
> >> >
> >> > IMO the right course of action here would be to see if the hashing
> >> > parameters can be adjusted, not introduce new ENAMETOOLONG errors.
> >> >
> >> >
> >>
> >> Hi Pengpeng,
> >>
> >> Could you please rework the patch taking into account the shared remarks?
> >
> > I would hold on until the discussion in [1] comes to conclusion.  It
> > turns out that the userspace client doesn't encrypt snapshot names
> > anymore, so another (much worse, at least IMO) route would be to drop
> > this bit of functionality from the kernel client as well [2].
>
> OK, let me see if I understood this correctly:
>
> - The user-space client implementation leaks metadata (the snaphsot names)
> - The kernel client doesn't leak the snapshots names, thought there are
>   bugs to be fixed (including on the MDS side)
> - The proposed solution is to drop the kernel snapshot names encryption.
>
> So, currently snapshots created on the kernel client can't be accessed
> from the user-space client, and vice-versa?

Hi Luis,

I _hope_ not -- since the inode that corresponds to the snapshot
created on the kernel client would have the fscrypt context (i.e.
fscrypt_auth_len > 0), the userspace client should be able to process
it generically.

>
> Also, won't dropping the encryption from the kernel client effectively
> make old snapshots created using a kernel client unusable?

I don't think so but it looks like these scenarios haven't been tested
when the change [1] went in.  Personally, I'm not convinced that taking
away the ability to encrypt snapshot names completely (as opposed to
at least allowing snapshot names to be either encrypted or unencrypted
depending on whether the client that created the snapshot had the key)
was the right move.  In fact, I would have seriously considered going
in the opposite direction of disallowing creating snapshots inside of
encrypted directories without a key the same way creating or linking in
files or directories is disallowed in that case.

I'm adding Chris, Venky and Patrick to this thread to avoid split
discussion.

[1] https://github.com/ceph/ceph/commit/73a8b2fda1976f553ec474027a3a73a5f6ceb441

Thanks,

                Ilya

RE: [PATCH v4] ceph: bound encrypted snapshot suffix formatting

[Viacheslav Dubeyko]

On Tue, 2026-04-28 at 13:40 +0200, Ilya Dryomov wrote:
> On Mon, Apr 27, 2026 at 10:12 AM Luis Henriques <luis@igalia.com> wrote:
> > 
> > On Fri, Apr 24 2026, Ilya Dryomov wrote:
> > 
> > > On Fri, Apr 24, 2026 at 8:31 PM Viacheslav Dubeyko
> > > <Slava.Dubeyko@ibm.com> wrote:
> > > > 
> > > > On Fri, 2026-04-24 at 11:27 +0200, Ilya Dryomov wrote:
> > > > > On Thu, Apr 23, 2026 at 8:04 PM Viacheslav Dubeyko
> > > > > <Slava.Dubeyko@ibm.com> wrote:
> > > > > > 
> > > > > > On Wed, 2026-04-22 at 11:53 +0200, Ilya Dryomov wrote:
> > > > > > > On Fri, Apr 10, 2026 at 10:46 PM Viacheslav Dubeyko
> > > > > > > <Slava.Dubeyko@ibm.com> wrote:
> > > > > > > > 
> > > > > > > > On Fri, 2026-04-10 at 20:40 +0000, Viacheslav Dubeyko wrote:
> > > > > > > > > On Thu, 2026-04-09 at 18:09 +0000, Viacheslav Dubeyko wrote:
> > > > > > > > > > On Thu, 2026-04-09 at 10:39 +0800, Pengpeng Hou wrote:
> > > > > > > > > > > ceph_encode_encrypted_dname() base64-encodes the encrypted snapshot
> > > > > > > > > > > name into the caller buffer and then, for long snapshot names, appends
> > > > > > > > > > > _<ino> with sprintf(p + elen, ...).
> > > > > > > > > > > 
> > > > > > > > > > > Some callers only provide NAME_MAX bytes. For long snapshot names, a
> > > > > > > > > > > large inode suffix can push the final encoded name past NAME_MAX even
> > > > > > > > > > > though the encrypted prefix stayed within the documented 240-byte
> > > > > > > > > > > budget.
> > > > > > > > > > > 
> > > > > > > > > > > Format the suffix into a small local buffer first and reject names
> > > > > > > > > > > whose suffix would exceed the caller's NAME_MAX output buffer.
> > > > > > > > > > > 
> > > > > > > > > > > Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> > > > > > > > > > > ---
> > > > > > > > > > > Changes since v3:
> > > > > > > > > > > - reject `elen > 240` explicitly instead of relying only on the earlier
> > > > > > > > > > >   `WARN_ON()`
> > > > > > > > > > > - rewrite the NAME_MAX bound check in terms of the final total length
> > > > > > > > > > >   instead of `NAME_MAX - prefix_len - elen`
> > > > > > > > > > > 
> > > > > > > > > > >  fs/ceph/crypto.c | 31 +++++++++++++++++++++++++++++--
> > > > > > > > > > >  1 file changed, 29 insertions(+), 2 deletions(-)
> > > > > > > > > > > 
> > > > > > > > > > > diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
> > > > > > > > > > > index f3de43ccb470..42e3fff34697 100644
> > > > > > > > > > > --- a/fs/ceph/crypto.c
> > > > > > > > > > > +++ b/fs/ceph/crypto.c
> > > > > > > > > > > @@ -15,6 +15,12 @@
> > > > > > > > > > >  #include "mds_client.h"
> > > > > > > > > > >  #include "crypto.h"
> > > > > > > > > > > 
> > > > > > > > > > > +/*
> > > > > > > > > > > + * Reserve room for '_' + decimal 64-bit inode number + trailing NUL.
> > > > > > > > > > > + * ceph_encode_encrypted_dname() copies only the visible suffix bytes.
> > > > > > > > > > > + */
> > > > > > > > > > > +#define CEPH_ENCRYPTED_SNAP_INO_SUFFIX_MAX       sizeof("_18446744073709551615")
> > > > > > > > > > > +
> > > > > > > > > > >  static int ceph_crypt_get_context(struct inode *inode, void *ctx, size_t len)
> > > > > > > > > > >  {
> > > > > > > > > > >   struct ceph_inode_info *ci = ceph_inode(inode);
> > > > > > > > > > > @@ -209,6 +215,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
> > > > > > > > > > >   struct inode *dir = parent;
> > > > > > > > > > >   char *p = buf;
> > > > > > > > > > >   u32 len;
> > > > > > > > > > > + int prefix_len = 0;
> > > > > > > > > > >   int name_len = elen;
> > > > > > > > > > >   int ret;
> > > > > > > > > > >   u8 *cryptbuf = NULL;
> > > > > > > > > > > @@ -219,6 +226,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
> > > > > > > > > > >           if (IS_ERR(dir))
> > > > > > > > > > >                   return PTR_ERR(dir);
> > > > > > > > > > >           p++; /* skip initial '_' */
> > > > > > > > > > > +         prefix_len = 1;
> > > > > > > > > > >   }
> > > > > > > > > > > 
> > > > > > > > > > >   if (!fscrypt_has_encryption_key(dir))
> > > > > > > > > > > @@ -271,8 +279,27 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
> > > > > > > > > > > 
> > > > > > > > > > >   /* To understand the 240 limit, see CEPH_NOHASH_NAME_MAX comments */
> > > > > > > > > > >   WARN_ON(elen > 240);
> > > > > > > > > > > - if (dir != parent) // leading _ is already there; append _<inum>
> > > > > > > > > > > -         elen += 1 + sprintf(p + elen, "_%ld", dir->i_ino);
> > > > > > > > > > > + if (elen > 240) {
> > > > > > > > > > > +         elen = -ENAMETOOLONG;
> > > > > > > > > > > +         goto out;
> > > > > > > > > > > + }
> > > > > > > > > > > +
> > > > > > > > > > > + if (dir != parent) {
> > > > > > > > > > > +         int total_len;
> > > > > > > > > > > +         /* leading '_' is already there; append _<inum> */
> > > > > > > > > > > +         char suffix[CEPH_ENCRYPTED_SNAP_INO_SUFFIX_MAX];
> > > > > > > > > > > +
> > > > > > > > > > > +         ret = snprintf(suffix, sizeof(suffix), "_%lu", dir->i_ino);
> > > > > > > > > > > +         total_len = prefix_len + elen + ret;
> > > > > > > > > > > +         if (total_len > NAME_MAX) {
> > > > > > > > > > > +                 elen = -ENAMETOOLONG;
> > > > > > > > > > > +                 goto out;
> > > > > > > > > > > +         }
> > > > > > > > > > > +
> > > > > > > > > > > +         memcpy(p + elen, suffix, ret);
> > > > > > > > > > > +         /* Include the leading '_' skipped by p. */
> > > > > > > > > > > +         elen = total_len;
> > > > > > > > > > > + }
> > > > > > > > > > > 
> > > > > > > > > > >  out:
> > > > > > > > > > >   kfree(cryptbuf);
> > > > > > > > > > 
> > > > > > > > > > Looks good.
> > > > > > > > > > 
> > > > > > > > > > Reviewed-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
> > > > > > > > > > 
> > > > > > > > > > Let me run xfstests for the patch to double check that everything is OK. I'll
> > > > > > > > > > share the result ASAP.
> > > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > The xfstests run was successful. I don't see any issues with the patch.
> > > > > > > > > 
> > > > > > > > > Tested-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > Applied on testing branch of CephFS kernel client git tree.
> > > > > > > 
> > > > > > > Hi Pengpeng, Slava,
> > > > > > > 
> > > > > > > This patch raised my attention because my understanding was that the
> > > > > > > entire CEPH_NOHASH_NAME_MAX + sha256() was put in place precisely to
> > > > > > > handle longer names nicely and make them fit into NAME_MAX-sized buffer.
> > > > > > > Simply rejecting longer names seemed to be in direct contradiction with
> > > > > > > that and yet the patch on its own was clearly merited given
> > > > > > > 
> > > > > > >  * (240 bytes is the maximum size allowed for snapshot names to take into
> > > > > > >  *  account the format: '_<SNAPSHOT-NAME>_<INODE-NUMBER>'.)
> > > > > > > 
> > > > > > > comment on CEPH_NOHASH_NAME_MAX definition.
> > > > > > > 
> > > > > > > I dug a bit deeper and started a discussion in [1].  The preliminary
> > > > > > > conclusion is that the 240 bytes assumption was a mistake -- somehow
> > > > > > > the minimum number of characters needed for <inum> ended up being used
> > > > > > > instead of the maximum.  CEPH_NOHASH_NAME_MAX value is likely incorrect
> > > > > > > and should have been smaller -- something along the lines of 174 -
> > > > > > > SHA256_DIGEST_SIZE instead of 180 - SHA256_DIGEST_SIZE.
> > > > > > > 
> > > > > > 
> > > > > > The limitation could be 240 or bigger one, but anyway we need to process this
> > > > > > limitation in proper way. And this patch has exactly this goal. Is 240 bytes
> > > > > > limitation your concern? As far as I can see, this patch doesn't introduce this
> > > > > > limitation. It was there before this modification. We can extend this limitation
> > > > > > anytime.
> > > > > 
> > > > > Hi Slava,
> > > > > 
> > > > > My take on this is that there shouldn't be a limitation to begin with
> > > > > (other than NAME_MAX which is natural and applies universally, not just
> > > > > to snapshot names).  This function has a bunch of code that is there
> > > > > specifically to handle longer names and avoid any artificial limits:
> > > > > the part of the name that spills over CEPH_NOHASH_NAME_MAX is hashed
> > > > > and the whole thing is set up in such a way that the end result is
> > > > > never bigger than 240 bytes.  240 isn't a random number -- it was
> > > > > picked on purpose to leave room for _ prefix and _<INODE-NUMBER> suffix
> > > > > (255 1 - 1 - 13) but a mistake appears to have crept in.  Instead of
> > > > > accounting for the maximum possible <INODE-NUMBER> length (which is 20
> > > > > in decimal encoding), it accounted only for 13 (likely because it
> > > > > happens to be the maximum possible length in a single-MDS setup?).
> > > > > 
> > > > > IMO the right course of action here would be to see if the hashing
> > > > > parameters can be adjusted, not introduce new ENAMETOOLONG errors.
> > > > > 
> > > > > 
> > > > 
> > > > Hi Pengpeng,
> > > > 
> > > > Could you please rework the patch taking into account the shared remarks?
> > > 
> > > I would hold on until the discussion in [1] comes to conclusion.  It
> > > turns out that the userspace client doesn't encrypt snapshot names
> > > anymore, so another (much worse, at least IMO) route would be to drop
> > > this bit of functionality from the kernel client as well [2].
> > 
> > OK, let me see if I understood this correctly:
> > 
> > - The user-space client implementation leaks metadata (the snaphsot names)
> > - The kernel client doesn't leak the snapshots names, thought there are
> >   bugs to be fixed (including on the MDS side)
> > - The proposed solution is to drop the kernel snapshot names encryption.
> > 
> > So, currently snapshots created on the kernel client can't be accessed
> > from the user-space client, and vice-versa?
> 
> Hi Luis,
> 
> I _hope_ not -- since the inode that corresponds to the snapshot
> created on the kernel client would have the fscrypt context (i.e.
> fscrypt_auth_len > 0), the userspace client should be able to process
> it generically.
> 
> > 
> > Also, won't dropping the encryption from the kernel client effectively
> > make old snapshots created using a kernel client unusable?
> 
> I don't think so but it looks like these scenarios haven't been tested
> when the change [1] went in.  Personally, I'm not convinced that taking
> away the ability to encrypt snapshot names completely (as opposed to
> at least allowing snapshot names to be either encrypted or unencrypted
> depending on whether the client that created the snapshot had the key)
> was the right move.  In fact, I would have seriously considered going
> in the opposite direction of disallowing creating snapshots inside of
> encrypted directories without a key the same way creating or linking in
> files or directories is disallowed in that case.
> 

This makes more sense from my point of view that removing the feature at all.


Thanks,
Slava.