Xiubo Li <xiubli@xxxxxxxxxx> writes:
> On 5/25/22 12:06 AM, Luís Henriques wrote:
>> Feature bits have to be encoded into the correct locations. This hasn't
>> been an issue so far because the only hole in the feature bits was in bit
>> 10 (CEPHFS_FEATURE_RECLAIM_CLIENT), which is located in the 2nd byte. When
>> adding more bits that go beyond the this 2nd byte, the bug will show up.
>>
>> Fixes: 9ba1e224538a ("ceph: allocate the correct amount of extra bytes for the session features")
>> Signed-off-by: Luís Henriques <lhenriques@xxxxxxx>
>> ---
>> fs/ceph/mds_client.c | 7 +++++--
>> fs/ceph/mds_client.h | 2 --
>> 2 files changed, 5 insertions(+), 4 deletions(-)
>>
>> I hope I got this code right. I found this issue when trying to add an
>> extra feature bit that would go beyond bit 15 and that wasn't showing up
>> on the MDS side.
>>
>> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
>> index 1bd3e1bb0fdf..77e742b6fd30 100644
>> --- a/fs/ceph/mds_client.c
>> +++ b/fs/ceph/mds_client.c
>> @@ -1220,14 +1220,17 @@ static int encode_supported_features(void **p, void *end)
>> if (count > 0) {
>> size_t i;
>> size_t size = FEATURE_BYTES(count);
>> + unsigned long bit;
>>
>> if (WARN_ON_ONCE(*p + 4 + size > end))
>> return -ERANGE;
>>
>> ceph_encode_32(p, size);
>> memset(*p, 0, size);
>> - for (i = 0; i < count; i++)
>> - ((unsigned char*)(*p))[i / 8] |= BIT(feature_bits[i] % 8);
>> + for (i = 0; i < count; i++) {
>> + bit = feature_bits[i];
>> + ((unsigned char *)(*p))[bit / 8] |= BIT(bit % 8);
>> + }
>> *p += size;
>> } else {
>> if (WARN_ON_ONCE(*p + 4 > end))
>> diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
>> index 33497846e47e..12901e3a6823 100644
>> --- a/fs/ceph/mds_client.h
>> +++ b/fs/ceph/mds_client.h
>> @@ -45,8 +45,6 @@ enum ceph_feature_type {
>> CEPHFS_FEATURE_MULTI_RECONNECT, \
>> CEPHFS_FEATURE_DELEG_INO, \
>> CEPHFS_FEATURE_METRIC_COLLECT, \
>> - \
>> - CEPHFS_FEATURE_MAX, \
>> }
>> #define CEPHFS_FEATURES_CLIENT_REQUIRED {}
>>
>
> LGTM. Just one small fix in the comment, since you have removed the
> 'CEPHFS_FEATURE_MAX', which makes no sense any more:
Ah, yeah. I missed that. Thanks!
Cheers,
--
Luís
> diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
> index 33497846e47e..ac49344ea79e 100644
> --- a/fs/ceph/mds_client.h
> +++ b/fs/ceph/mds_client.h
> @@ -33,10 +33,6 @@ enum ceph_feature_type {
> CEPHFS_FEATURE_MAX = CEPHFS_FEATURE_METRIC_COLLECT,
> };
>
> -/*
> - * This will always have the highest feature bit value
> - * as the last element of the array.
> - */
> #define CEPHFS_FEATURES_CLIENT_SUPPORTED { \
> 0, 1, 2, 3, 4, 5, 6, 7, \
> CEPHFS_FEATURE_MIMIC, \
>
> Merged into the testing branch.
>
> Thanks Luis.
>
> -- Xiubo
>
