Re: [RFC PATCH v10 11/48] ceph: decode alternate_name in lease info

[Xiubo Li <xiubli@redhat.com>]

On 1/12/22 3:15 AM, Jeff Layton wrote:
> Ceph is a bit different from local filesystems, in that we don't want
> to store filenames as raw binary data, since we may also be dealing
> with clients that don't support fscrypt.
>
> We could just base64-encode the encrypted filenames, but that could
> leave us with filenames longer than NAME_MAX. It turns out that the
> MDS doesn't care much about filename length, but the clients do.
>
> To manage this, we've added a new "alternate name" field that can be
> optionally added to any dentry that we'll use to store the binary
> crypttext of the filename if its base64-encoded value will be longer
> than NAME_MAX. When a dentry has one of these names attached, the MDS
> will send it along in the lease info, which we can then store for
> later usage.
>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>   fs/ceph/mds_client.c | 40 ++++++++++++++++++++++++++++++----------
>   fs/ceph/mds_client.h | 11 +++++++----
>   2 files changed, 37 insertions(+), 14 deletions(-)
>
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index 34a4f6dbac9d..709f3f654555 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -306,27 +306,44 @@ static int parse_reply_info_dir(void **p, void *end,
>   
>   static int parse_reply_info_lease(void **p, void *end,
>   				  struct ceph_mds_reply_lease **lease,
> -				  u64 features)
> +				  u64 features, u32 *altname_len, u8 **altname)
>   {
> +	u8 struct_v;
> +	u32 struct_len;
> +
>   	if (features == (u64)-1) {
> -		u8 struct_v, struct_compat;
> -		u32 struct_len;
> +		u8 struct_compat;
> +
>   		ceph_decode_8_safe(p, end, struct_v, bad);
>   		ceph_decode_8_safe(p, end, struct_compat, bad);
> +
>   		/* struct_v is expected to be >= 1. we only understand
>   		 * encoding whose struct_compat == 1. */
>   		if (!struct_v || struct_compat != 1)
>   			goto bad;
> +
>   		ceph_decode_32_safe(p, end, struct_len, bad);
> -		ceph_decode_need(p, end, struct_len, bad);
> -		end = *p + struct_len;

Hi Jeff,

This is buggy, more detail please see https://tracker.ceph.com/issues/54430.

The following patch will fix it. We should skip the extra memories anyway.


diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 94b4c6508044..3dea96df4769 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -326,6 +326,7 @@ static int parse_reply_info_lease(void **p, void *end,
                         goto bad;

                 ceph_decode_32_safe(p, end, struct_len, bad);
+               end = *p + struct_len;
         } else {
                 struct_len = sizeof(**lease);
                 *altname_len = 0;
@@ -346,6 +347,7 @@ static int parse_reply_info_lease(void **p, void *end,
                         *altname = NULL;
                         *altname_len = 0;
                 }
+               *p = end;
         }
         return 0;
  bad:



> +	} else {
> +		struct_len = sizeof(**lease);
> +		*altname_len = 0;
> +		*altname = NULL;
>   	}
>   
> -	ceph_decode_need(p, end, sizeof(**lease), bad);
> +	ceph_decode_need(p, end, struct_len, bad);
>   	*lease = *p;
>   	*p += sizeof(**lease);
> -	if (features == (u64)-1)
> -		*p = end;
> +
> +	if (features == (u64)-1) {
> +		if (struct_v >= 2) {
> +			ceph_decode_32_safe(p, end, *altname_len, bad);
> +			ceph_decode_need(p, end, *altname_len, bad);
> +			*altname = *p;
> +			*p += *altname_len;
> +		} else {
> +			*altname = NULL;
> +			*altname_len = 0;
> +		}
> +	}
>   	return 0;
>   bad:
>   	return -EIO;
> @@ -356,7 +373,8 @@ static int parse_reply_info_trace(void **p, void *end,
>   		info->dname = *p;
>   		*p += info->dname_len;
>   
> -		err = parse_reply_info_lease(p, end, &info->dlease, features);
> +		err = parse_reply_info_lease(p, end, &info->dlease, features,
> +					     &info->altname_len, &info->altname);
>   		if (err < 0)
>   			goto out_bad;
>   	}
> @@ -423,9 +441,11 @@ static int parse_reply_info_readdir(void **p, void *end,
>   		dout("parsed dir dname '%.*s'\n", rde->name_len, rde->name);
>   
>   		/* dentry lease */
> -		err = parse_reply_info_lease(p, end, &rde->lease, features);
> +		err = parse_reply_info_lease(p, end, &rde->lease, features,
> +					     &rde->altname_len, &rde->altname);
>   		if (err)
>   			goto out_bad;
> +
>   		/* inode */
>   		err = parse_reply_info_in(p, end, &rde->inode, features);
>   		if (err < 0)
> diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
> index e7d2c8a1b9c1..128901a847af 100644
> --- a/fs/ceph/mds_client.h
> +++ b/fs/ceph/mds_client.h
> @@ -29,8 +29,8 @@ enum ceph_feature_type {
>   	CEPHFS_FEATURE_MULTI_RECONNECT,
>   	CEPHFS_FEATURE_DELEG_INO,
>   	CEPHFS_FEATURE_METRIC_COLLECT,
> -
> -	CEPHFS_FEATURE_MAX = CEPHFS_FEATURE_METRIC_COLLECT,
> +	CEPHFS_FEATURE_ALTERNATE_NAME,
> +	CEPHFS_FEATURE_MAX = CEPHFS_FEATURE_ALTERNATE_NAME,
>   };
>   
>   /*
> @@ -45,8 +45,7 @@ enum ceph_feature_type {
>   	CEPHFS_FEATURE_MULTI_RECONNECT,		\
>   	CEPHFS_FEATURE_DELEG_INO,		\
>   	CEPHFS_FEATURE_METRIC_COLLECT,		\
> -						\
> -	CEPHFS_FEATURE_MAX,			\
> +	CEPHFS_FEATURE_ALTERNATE_NAME,		\
>   }
>   #define CEPHFS_FEATURES_CLIENT_REQUIRED {}
>   
> @@ -98,7 +97,9 @@ struct ceph_mds_reply_info_in {
>   
>   struct ceph_mds_reply_dir_entry {
>   	char                          *name;
> +	u8			      *altname;
>   	u32                           name_len;
> +	u32			      altname_len;
>   	struct ceph_mds_reply_lease   *lease;
>   	struct ceph_mds_reply_info_in inode;
>   	loff_t			      offset;
> @@ -117,7 +118,9 @@ struct ceph_mds_reply_info_parsed {
>   	struct ceph_mds_reply_info_in diri, targeti;
>   	struct ceph_mds_reply_dirfrag *dirfrag;
>   	char                          *dname;
> +	u8			      *altname;
>   	u32                           dname_len;
> +	u32                           altname_len;
>   	struct ceph_mds_reply_lease   *dlease;
>   
>   	/* extra */