Re: [PATCH 1/2] ceph: pass fscrypt `tname` buffers directly

[David Laight]

On Tue, 26 May 2026 19:58:27 -0700
Sam Edwards <cfsworks@xxxxxxxxx> wrote:

> ceph_fname_to_usr() needs a temporary buffer for some operations
> (currently only base64-decoding ciphertext) and it is convenient to
> allow the caller to specify this buffer to avoid a heap allocation, so
> it has a (nullable) `tname` argument. Until now, this argument was a
> `struct fscrypt_str`; however, this is unnecessary for two reasons:
> 
> 1. `tname->len` isn't used anywhere: ceph_fname_to_usr() assumes a
>    buffer large enough to hold the ciphertext, and
>    parse_reply_info_readdir() -- the only caller to use tname -- doesn't
>    set it.
> 2. While the `tname` parameter is documented "may be NULL,"
>    parse_reply_info_readdir() always passes it but with `tname->name`
>    sometimes NULL in violation of the contract, indicating that the
>    unnecessary container creates actual confusion.
> 
> Therefore, change the type to `unsigned char *` and pass the buffer
> directly.
> 
> Signed-off-by: Sam Edwards <CFSworks@xxxxxxxxx>
> ---
>  fs/ceph/crypto.c     | 10 +++++-----
>  fs/ceph/crypto.h     |  4 ++--
>  fs/ceph/mds_client.c |  6 +++---
>  3 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
> index 64d240759277..7515cb251226 100644
> --- a/fs/ceph/crypto.c
> +++ b/fs/ceph/crypto.c
> @@ -300,7 +300,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
>   *
>   * Returns 0 on success or negative error code on error.
>   */
> -int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
> +int ceph_fname_to_usr(const struct ceph_fname *fname, unsigned char *tname,

I can't help feeling that the buffer length should also be passed.
Either explicitly or, if constant, implicitly by embedding the array
in a structure.

-- David


>  		      struct fscrypt_str *oname, bool *is_nokey)
>  {
>  	struct inode *dir = fname->dir;
> @@ -357,16 +357,16 @@ int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
>  			ret = fscrypt_fname_alloc_buffer(NAME_MAX, &_tname);
>  			if (ret)
>  				goto out_inode;
> -			tname = &_tname;
> +			tname = _tname.name;
>  		}
>  
> -		declen = base64_decode(name, name_len,
> -				       tname->name, false, BASE64_IMAP);
> +		declen = base64_decode(name, name_len, tname, false,
> +				       BASE64_IMAP);
>  		if (declen <= 0) {
>  			ret = -EIO;
>  			goto out;
>  		}
> -		iname.name = tname->name;
> +		iname.name = tname;
>  		iname.len = declen;
>  	} else {
>  		iname.name = fname->ctext;
> diff --git a/fs/ceph/crypto.h b/fs/ceph/crypto.h
> index b748e2060bc9..79cb563fd887 100644
> --- a/fs/ceph/crypto.h
> +++ b/fs/ceph/crypto.h
> @@ -115,7 +115,7 @@ static inline void ceph_fname_free_buffer(struct inode *parent,
>  		fscrypt_fname_free_buffer(fname);
>  }
>  
> -int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
> +int ceph_fname_to_usr(const struct ceph_fname *fname, unsigned char *tname,
>  		      struct fscrypt_str *oname, bool *is_nokey);
>  int ceph_fscrypt_prepare_readdir(struct inode *dir);
>  
> @@ -204,7 +204,7 @@ static inline void ceph_fname_free_buffer(struct inode *parent,
>  }
>  
>  static inline int ceph_fname_to_usr(const struct ceph_fname *fname,
> -				    struct fscrypt_str *tname,
> +				    unsigned char *tname,
>  				    struct fscrypt_str *oname, bool *is_nokey)
>  {
>  	oname->name = fname->name;
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index ed17e0023705..aa6730b48e97 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -488,11 +488,11 @@ static int parse_reply_info_readdir(void **p, void *end,
>  		struct inode *inode = d_inode(req->r_dentry);
>  		struct ceph_inode_info *ci = ceph_inode(inode);
>  		struct ceph_mds_reply_dir_entry *rde = info->dir_entries + i;
> -		struct fscrypt_str tname = FSTR_INIT(NULL, 0);
>  		struct fscrypt_str oname = FSTR_INIT(NULL, 0);
>  		struct ceph_fname fname;
>  		u32 altname_len, _name_len;
>  		u8 *altname, *_name;
> +		u8 *tname = NULL;
>  
>  		/* dentry */
>  		ceph_decode_32_safe(p, end, _name_len, bad);
> @@ -540,7 +540,7 @@ static int parse_reply_info_readdir(void **p, void *end,
>  			 * always be shorter, which is 3/4 of origin
>  			 * string.
>  			 */
> -			tname.name = _name;
> +			tname = _name;
>  
>  			/*
>  			 * Set oname to _name too, and this will be
> @@ -557,7 +557,7 @@ static int parse_reply_info_readdir(void **p, void *end,
>  			oname.len = altname_len;
>  		}
>  		rde->is_nokey = false;
> -		err = ceph_fname_to_usr(&fname, &tname, &oname, &rde->is_nokey);
> +		err = ceph_fname_to_usr(&fname, tname, &oname, &rde->is_nokey);
>  		if (err) {
>  			pr_err_client(cl, "unable to decode %.*s, got %d\n",
>  				      _name_len, _name, err);