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@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);
> + }
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@xxxxxxxxxx>
