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

[Xiubo Li <xiubli@redhat.com>]

On 3/1/22 6:57 PM, 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.
>
>
Hi Jeff,

I will send out a patch series to fix this later.

And we could merge the new patch to this one.

- XIubo



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