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

[Xiubo Li <xiubli@redhat.com>]

On 3/1/22 10:14 PM, Jeff Layton wrote:
> On Tue, 2022-03-01 at 22:07 +0800, Xiubo Li wrote:
>> On 3/1/22 9:57 PM, Jeff Layton wrote:
>>> On Tue, 2022-03-01 at 21:51 +0800, Xiubo Li wrote:
>>>> On 3/1/22 9:10 PM, Jeff Layton wrote:
>>>>> On Tue, 2022-03-01 at 18:57 +0800, Xiubo Li wrote:
>>>>>> 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;
>>>>> There may be a bug here,
>>>> Yeah, this will be crash when I use the PR
>>>> https://github.com/ceph/ceph/pull/45208.
>>>>
>>>>
>>>>> but this doesn't look like the right fix. "end"
>>>>> denotes the end of the buffer we're decoding. We don't generally want to
>>>>> go changing it like this. Consider what would happen if the original
>>>>> "end" was shorter than *p + struct_len.
>>>> I missed you have also set the struct_len in the else branch.
>>>>>>             } 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;
>>>>> I think we just have to do the math here. Maybe this should be something
>>>>> like this?
>>>>>
>>>>>        *p += struct_len - sizeof(**lease) - *altname_len;
>>>> This is correct, but in future if we are adding tens of new fields we
>>>> must minus them all here.
>>>>
>>>> How about this one:
>>>>
>>>>
>>>> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
>>>> index 94b4c6508044..608d077f2eeb 100644
>>>> --- a/fs/ceph/mds_client.c
>>>> +++ b/fs/ceph/mds_client.c
>>>> @@ -313,6 +313,7 @@ static int parse_reply_info_lease(void **p, void *end,
>>>>     {
>>>>            u8 struct_v;
>>>>            u32 struct_len;
>>>> +       void *lend;
>>>>
>>>>            if (features == (u64)-1) {
>>>>                    u8 struct_compat;
>>>> @@ -332,6 +333,7 @@ static int parse_reply_info_lease(void **p, void *end,
>>>>                    *altname = NULL;
>>>>            }
>>>>
>>>> +       lend = *p + struct_len;
>>> Looks reasonable. Maybe also add a check like this?
>>>
>>>       if (lend > end)
>>> 	    return -EIO;
>> I don't think this is needed because the:
>>
>>     ceph_decode_need(p, end, struct_len, bad);
>>
>> before it will help check it ?
>>
>>
> Oh, right....good point. That patch looks fine then.

Cool, I will send out one separate patch to fix it in wip-fscrypt branch.

- Xiubo

>
>>>
>>>>            ceph_decode_need(p, end, struct_len, bad);
>>>>            *lease = *p;
>>>>            *p += sizeof(**lease);
>>>> @@ -347,6 +349,7 @@ static int parse_reply_info_lease(void **p, void *end,
>>>>                            *altname_len = 0;
>>>>                    }
>>>>            }
>>>> +       *p = lend;
>>>>            return 0;
>>>>     bad:
>>>>            return -EIO;
>>>>
>>>>
>>>>>>             }
>>>>>>             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 */