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

[Xiubo Li <xiubli@redhat.com>]

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;
         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 */