[PATCH] ceph: use correct index when encoding client supported features

[PATCH] ceph: use correct index when encoding client supported features

[Luís Henriques]

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@suse.de>
---
 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 {}
 

Re: [PATCH] ceph: use correct index when encoding client supported features

[Jeff Layton]

On Tue, 2022-05-24 at 17:06 +0100, 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@suse.de>
> ---
>  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);
> +		}

I wish we could just use __set_bit/__clear_bit here. It would be a lot
easier to reason this out. Your logic looks correct to me though.


>  		*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 {}
>  

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH] ceph: use correct index when encoding client supported features

[Xiubo Li]

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@suse.de>
> ---
>   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:


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

Re: [PATCH] ceph: use correct index when encoding client supported features

[Luís Henriques]

Xiubo Li <xiubli@redhat.com> 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@suse.de>
>> ---
>>   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
>