| Message ID | 20220524160627.20893-1-lhenriques@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ceph: use correct index when encoding client supported features | expand |
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>
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
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,
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 {}
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.