Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   =?utf-8?Q?Lu=C3=ADs_Henriques?= <lhenriques@suse.de>
To:     Xiubo Li <xiubli@redhat.com>
Cc:     Jeff Layton <jlayton@kernel.org>,
        Ilya Dryomov <idryomov@gmail.com>, ceph-devel@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] ceph: use correct index when encoding client supported
 features
References: <20220524160627.20893-1-lhenriques@suse.de>
        <d1f22b3b-7415-0fdb-e702-24f2a7b0b939@redhat.com>
Date:   Fri, 27 May 2022 10:16:27 +0100
In-Reply-To: <d1f22b3b-7415-0fdb-e702-24f2a7b0b939@redhat.com> (Xiubo Li's
        message of "Thu, 26 May 2022 08:55:15 +0800")
Message-ID: <87h75bwd1w.fsf@brahms.olymp>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Xiubo Li <xiubli@redhat.com> writes:

> On 5/25/22 12:06 AM, Lu=C3=ADs 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.  W=
hen
>> 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 f=
or the session features")
>> Signed-off-by: Lu=C3=ADs 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, v=
oid *end)
>>   	if (count > 0) {
>>   		size_t i;
>>   		size_t size =3D FEATURE_BYTES(count);
>> +		unsigned long bit;
>>=20=20=20
>>   		if (WARN_ON_ONCE(*p + 4 + size > end))
>>   			return -ERANGE;
>>=20=20=20
>>   		ceph_encode_32(p, size);
>>   		memset(*p, 0, size);
>> -		for (i =3D 0; i < count; i++)
>> -			((unsigned char*)(*p))[i / 8] |=3D BIT(feature_bits[i] % 8);
>> +		for (i =3D 0; i < count; i++) {
>> +			bit =3D feature_bits[i];
>> +			((unsigned char *)(*p))[bit / 8] |=3D BIT(bit % 8);
>> +		}
>>   		*p +=3D 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 {}
>>=20=20=20
>
> LGTM. Just one small fix in the comment, since you have removed the=20
> 'CEPHFS_FEATURE_MAX', which makes no sense any more:

Ah, yeah.  I missed that.  Thanks!

Cheers,
--=20
Lu=C3=ADs


> 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 {
>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 CEPHFS_FEATURE_MAX =3D CEPHFS=
_FEATURE_METRIC_COLLECT,
>  =C2=A0};
>
> -/*
> - * This will always have the highest feature bit value
> - * as the last element of the array.
> - */
>  =C2=A0#define CEPHFS_FEATURES_CLIENT_SUPPORTED {=C2=A0=C2=A0=C2=A0=C2=A0=
 \
>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 0, 1, 2, 3, 4, 5, 6, 7,=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0 \
>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 CEPHFS_FEATURE_MIMIC,=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0 \
>
> Merged into the testing branch.
>
> Thanks Luis.
>
> -- Xiubo
>