[PATCH] IB/core: Fix bit curruption in ib_device_cap_flags structure

[PATCH] IB/core: Fix bit curruption in ib_device_cap_flags structure

[Max Gurtovoy]

ib_device_cap_flags 64-bit expansion caused caps overlapping
and made consumers read wrong device capabilities. For example
IB_DEVICE_SG_GAPS_REG was falsely read by the iser driver causing
it to use a non-existing capability. This happened because signed
int becomes sign extended when converted it to u64. Fix this by
casting IB_DEVICE_ON_DEMAND_PAGING enumeration to ULL.

Fixes: fb532d6a79b9 ('IB/{core, ulp} Support above 32 possible device capability flags')
Reported-by: Robert LeBlanc <robert@leblancnet.us>
Cc: Stable <stable@vger.kernel.org> #[v4.6+]
Acked-by: Sagi Grimberg <sagi@grimberg.me>
Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
Signed-off-by: Matan Barak <matanb@mellanox.com>
---
 include/rdma/ib_verbs.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 432bed5..c97357b 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -217,7 +217,7 @@ enum ib_device_cap_flags {
 	IB_DEVICE_CROSS_CHANNEL		= (1 << 27),
 	IB_DEVICE_MANAGED_FLOW_STEERING		= (1 << 29),
 	IB_DEVICE_SIGNATURE_HANDOVER		= (1 << 30),
-	IB_DEVICE_ON_DEMAND_PAGING		= (1 << 31),
+	IB_DEVICE_ON_DEMAND_PAGING		= (1ULL << 31),
 	IB_DEVICE_SG_GAPS_REG			= (1ULL << 32),
 	IB_DEVICE_VIRTUAL_FUNCTION		= ((u64)1 << 33),
 	IB_DEVICE_RAW_SCATTER_FCS		= ((u64)1 << 34),
-- 
1.7.1

Re: [PATCH] IB/core: Fix bit curruption in ib_device_cap_flags structure

[Christoph Hellwig]

>  	IB_DEVICE_CROSS_CHANNEL		= (1 << 27),
>  	IB_DEVICE_MANAGED_FLOW_STEERING		= (1 << 29),
>  	IB_DEVICE_SIGNATURE_HANDOVER		= (1 << 30),
> -	IB_DEVICE_ON_DEMAND_PAGING		= (1 << 31),
> +	IB_DEVICE_ON_DEMAND_PAGING		= (1ULL << 31),
>  	IB_DEVICE_SG_GAPS_REG			= (1ULL << 32),
>  	IB_DEVICE_VIRTUAL_FUNCTION		= ((u64)1 << 33),
>  	IB_DEVICE_RAW_SCATTER_FCS		= ((u64)1 << 34),

So now we've got three different styles to define the constants.

I'd really prefer to move all of them over to the 1ULL << offset style.

Re: [PATCH] IB/core: Fix bit curruption in ib_device_cap_flags structure

[Max Gurtovoy]

On 6/2/2016 2:33 PM, Christoph Hellwig wrote:
>>   	IB_DEVICE_CROSS_CHANNEL		= (1 << 27),
>>   	IB_DEVICE_MANAGED_FLOW_STEERING		= (1 << 29),
>>   	IB_DEVICE_SIGNATURE_HANDOVER		= (1 << 30),
>> -	IB_DEVICE_ON_DEMAND_PAGING		= (1 << 31),
>> +	IB_DEVICE_ON_DEMAND_PAGING		= (1ULL << 31),
>>   	IB_DEVICE_SG_GAPS_REG			= (1ULL << 32),
>>   	IB_DEVICE_VIRTUAL_FUNCTION		= ((u64)1 << 33),
>>   	IB_DEVICE_RAW_SCATTER_FCS		= ((u64)1 << 34),
>
> So now we've got three different styles to define the constants.

My next patch will align casting of IB_DEVICE_VIRTUAL_FUNCTION and 
IB_DEVICE_RAW_SCATTER_FCS to ULL.

>
> I'd really prefer to move all of them over to the 1ULL << offset style.

me too :)

>

Re: [PATCH] IB/core: Fix bit curruption in ib_device_cap_flags structure

[Sagi Grimberg]

>>>       IB_DEVICE_CROSS_CHANNEL        = (1 << 27),
>>>       IB_DEVICE_MANAGED_FLOW_STEERING        = (1 << 29),
>>>       IB_DEVICE_SIGNATURE_HANDOVER        = (1 << 30),
>>> -    IB_DEVICE_ON_DEMAND_PAGING        = (1 << 31),
>>> +    IB_DEVICE_ON_DEMAND_PAGING        = (1ULL << 31),
>>>       IB_DEVICE_SG_GAPS_REG            = (1ULL << 32),
>>>       IB_DEVICE_VIRTUAL_FUNCTION        = ((u64)1 << 33),
>>>       IB_DEVICE_RAW_SCATTER_FCS        = ((u64)1 << 34),
>>
>> So now we've got three different styles to define the constants.
>
> My next patch will align casting of IB_DEVICE_VIRTUAL_FUNCTION and
> IB_DEVICE_RAW_SCATTER_FCS to ULL.
>
>>
>> I'd really prefer to move all of them over to the 1ULL << offset style.
>
> me too :)

Sounds good to me too...

Max, give it a day or two before resending (trying to save you the
multi-respin overhead).

Also, please next time when you submit a patch make sure to document
the patch version and what was changed from the last version.

Cheers,
Sagi.

Re: [PATCH] IB/core: Fix bit curruption in ib_device_cap_flags structure

[Jason Gunthorpe]

On Thu, Jun 02, 2016 at 04:33:53AM -0700, Christoph Hellwig wrote:
> >  	IB_DEVICE_CROSS_CHANNEL		= (1 << 27),
> >  	IB_DEVICE_MANAGED_FLOW_STEERING		= (1 << 29),
> >  	IB_DEVICE_SIGNATURE_HANDOVER		= (1 << 30),
> > -	IB_DEVICE_ON_DEMAND_PAGING		= (1 << 31),
> > +	IB_DEVICE_ON_DEMAND_PAGING		= (1ULL << 31),
> >  	IB_DEVICE_SG_GAPS_REG			= (1ULL << 32),
> >  	IB_DEVICE_VIRTUAL_FUNCTION		= ((u64)1 << 33),
> >  	IB_DEVICE_RAW_SCATTER_FCS		= ((u64)1 << 34),
> 
> So now we've got three different styles to define the constants.
> 
> I'd really prefer to move all of them over to the 1ULL << offset style.

If people can agree on the style that would be a nice followup cleanup
patch, but since this fixes a bug and needs a pass through stable
shouldn't it stay succinct like this?

Jason

Re: [PATCH] IB/core: Fix bit curruption in ib_device_cap_flags structure

[Greg KH]

On Thu, Jun 02, 2016 at 12:28:05PM +0300, Max Gurtovoy wrote:
> ib_device_cap_flags 64-bit expansion caused caps overlapping
> and made consumers read wrong device capabilities. For example
> IB_DEVICE_SG_GAPS_REG was falsely read by the iser driver causing
> it to use a non-existing capability. This happened because signed
> int becomes sign extended when converted it to u64. Fix this by
> casting IB_DEVICE_ON_DEMAND_PAGING enumeration to ULL.
> 
> Fixes: fb532d6a79b9 ('IB/{core, ulp} Support above 32 possible device capability flags')
> Reported-by: Robert LeBlanc <robert@leblancnet.us>
> Cc: Stable <stable@vger.kernel.org> #[v4.6+]
> Acked-by: Sagi Grimberg <sagi@grimberg.me>
> Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
> Signed-off-by: Matan Barak <matanb@mellanox.com>
> ---
>  include/rdma/ib_verbs.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> index 432bed5..c97357b 100644
> --- a/include/rdma/ib_verbs.h
> +++ b/include/rdma/ib_verbs.h
> @@ -217,7 +217,7 @@ enum ib_device_cap_flags {
>  	IB_DEVICE_CROSS_CHANNEL		= (1 << 27),
>  	IB_DEVICE_MANAGED_FLOW_STEERING		= (1 << 29),
>  	IB_DEVICE_SIGNATURE_HANDOVER		= (1 << 30),
> -	IB_DEVICE_ON_DEMAND_PAGING		= (1 << 31),
> +	IB_DEVICE_ON_DEMAND_PAGING		= (1ULL << 31),
>  	IB_DEVICE_SG_GAPS_REG			= (1ULL << 32),
>  	IB_DEVICE_VIRTUAL_FUNCTION		= ((u64)1 << 33),
>  	IB_DEVICE_RAW_SCATTER_FCS		= ((u64)1 << 34),

Why not just use the BIT() and BIT_ULL() macros instead of "open coding"
these?

thanks,

greg k-h

Re: [PATCH] IB/core: Fix bit curruption in ib_device_cap_flags structure

[Bart Van Assche]

On 06/02/2016 09:24 AM, Greg KH wrote:
> On Thu, Jun 02, 2016 at 12:28:05PM +0300, Max Gurtovoy wrote:
>> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
>> index 432bed5..c97357b 100644
>> --- a/include/rdma/ib_verbs.h
>> +++ b/include/rdma/ib_verbs.h
>> @@ -217,7 +217,7 @@ enum ib_device_cap_flags {
>>  	IB_DEVICE_CROSS_CHANNEL		= (1 << 27),
>>  	IB_DEVICE_MANAGED_FLOW_STEERING		= (1 << 29),
>>  	IB_DEVICE_SIGNATURE_HANDOVER		= (1 << 30),
>> -	IB_DEVICE_ON_DEMAND_PAGING		= (1 << 31),
>> +	IB_DEVICE_ON_DEMAND_PAGING		= (1ULL << 31),
>>  	IB_DEVICE_SG_GAPS_REG			= (1ULL << 32),
>>  	IB_DEVICE_VIRTUAL_FUNCTION		= ((u64)1 << 33),
>>  	IB_DEVICE_RAW_SCATTER_FCS		= ((u64)1 << 34),
>
> Why not just use the BIT() and BIT_ULL() macros instead of "open coding"
> these?

Hi Greg,

Are you sure that these macros are useful? My personal opinion is that 
these macros makes code harder to read instead of easier. I'd like to 
see these macros disappear.

Thanks,

Bart.

Re: [PATCH] IB/core: Fix bit curruption in ib_device_cap_flags structure

[Greg KH]

On Thu, Jun 02, 2016 at 09:41:03AM -0700, Bart Van Assche wrote:
> On 06/02/2016 09:24 AM, Greg KH wrote:
> > On Thu, Jun 02, 2016 at 12:28:05PM +0300, Max Gurtovoy wrote:
> > > diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> > > index 432bed5..c97357b 100644
> > > --- a/include/rdma/ib_verbs.h
> > > +++ b/include/rdma/ib_verbs.h
> > > @@ -217,7 +217,7 @@ enum ib_device_cap_flags {
> > >  	IB_DEVICE_CROSS_CHANNEL		= (1 << 27),
> > >  	IB_DEVICE_MANAGED_FLOW_STEERING		= (1 << 29),
> > >  	IB_DEVICE_SIGNATURE_HANDOVER		= (1 << 30),
> > > -	IB_DEVICE_ON_DEMAND_PAGING		= (1 << 31),
> > > +	IB_DEVICE_ON_DEMAND_PAGING		= (1ULL << 31),
> > >  	IB_DEVICE_SG_GAPS_REG			= (1ULL << 32),
> > >  	IB_DEVICE_VIRTUAL_FUNCTION		= ((u64)1 << 33),
> > >  	IB_DEVICE_RAW_SCATTER_FCS		= ((u64)1 << 34),
> > 
> > Why not just use the BIT() and BIT_ULL() macros instead of "open coding"
> > these?
> 
> Hi Greg,
> 
> Are you sure that these macros are useful?

Yes.

> My personal opinion is that these macros makes code harder to read
> instead of easier. I'd like to see these macros disappear.

Sorry but we are converting the whole kernel to use them, I suggest you
get used to them :)

greg k-h

Re: [PATCH] IB/core: Fix bit curruption in ib_device_cap_flags structure

[Max Gurtovoy]

On 6/2/2016 7:24 PM, Greg KH wrote:
> On Thu, Jun 02, 2016 at 12:28:05PM +0300, Max Gurtovoy wrote:
>> ib_device_cap_flags 64-bit expansion caused caps overlapping
>> and made consumers read wrong device capabilities. For example
>> IB_DEVICE_SG_GAPS_REG was falsely read by the iser driver causing
>> it to use a non-existing capability. This happened because signed
>> int becomes sign extended when converted it to u64. Fix this by
>> casting IB_DEVICE_ON_DEMAND_PAGING enumeration to ULL.
>>
>> Fixes: fb532d6a79b9 ('IB/{core, ulp} Support above 32 possible device capability flags')
>> Reported-by: Robert LeBlanc <robert@leblancnet.us>
>> Cc: Stable <stable@vger.kernel.org> #[v4.6+]
>> Acked-by: Sagi Grimberg <sagi@grimberg.me>
>> Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
>> Signed-off-by: Matan Barak <matanb@mellanox.com>
>> ---
>>   include/rdma/ib_verbs.h |    2 +-
>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
>> index 432bed5..c97357b 100644
>> --- a/include/rdma/ib_verbs.h
>> +++ b/include/rdma/ib_verbs.h
>> @@ -217,7 +217,7 @@ enum ib_device_cap_flags {
>>   	IB_DEVICE_CROSS_CHANNEL		= (1 << 27),
>>   	IB_DEVICE_MANAGED_FLOW_STEERING		= (1 << 29),
>>   	IB_DEVICE_SIGNATURE_HANDOVER		= (1 << 30),
>> -	IB_DEVICE_ON_DEMAND_PAGING		= (1 << 31),
>> +	IB_DEVICE_ON_DEMAND_PAGING		= (1ULL << 31),
>>   	IB_DEVICE_SG_GAPS_REG			= (1ULL << 32),
>>   	IB_DEVICE_VIRTUAL_FUNCTION		= ((u64)1 << 33),
>>   	IB_DEVICE_RAW_SCATTER_FCS		= ((u64)1 << 34),
>
> Why not just use the BIT() and BIT_ULL() macros instead of "open coding"
> these?

Good idea. do you think it will be a good idea to set all the elements 
in this ib_device_cap_flags enum to BIT_ULL() in this patch ? to have 1 
style instead of many ?

>
> thanks,
>
> greg k-h
>

Re: [PATCH] IB/core: Fix bit curruption in ib_device_cap_flags structure

[Greg KH]

On Thu, Jun 02, 2016 at 07:41:10PM +0300, Max Gurtovoy wrote:
> 
> 
> On 6/2/2016 7:24 PM, Greg KH wrote:
> > On Thu, Jun 02, 2016 at 12:28:05PM +0300, Max Gurtovoy wrote:
> > > ib_device_cap_flags 64-bit expansion caused caps overlapping
> > > and made consumers read wrong device capabilities. For example
> > > IB_DEVICE_SG_GAPS_REG was falsely read by the iser driver causing
> > > it to use a non-existing capability. This happened because signed
> > > int becomes sign extended when converted it to u64. Fix this by
> > > casting IB_DEVICE_ON_DEMAND_PAGING enumeration to ULL.
> > > 
> > > Fixes: fb532d6a79b9 ('IB/{core, ulp} Support above 32 possible device capability flags')
> > > Reported-by: Robert LeBlanc <robert@leblancnet.us>
> > > Cc: Stable <stable@vger.kernel.org> #[v4.6+]
> > > Acked-by: Sagi Grimberg <sagi@grimberg.me>
> > > Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
> > > Signed-off-by: Matan Barak <matanb@mellanox.com>
> > > ---
> > >   include/rdma/ib_verbs.h |    2 +-
> > >   1 files changed, 1 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> > > index 432bed5..c97357b 100644
> > > --- a/include/rdma/ib_verbs.h
> > > +++ b/include/rdma/ib_verbs.h
> > > @@ -217,7 +217,7 @@ enum ib_device_cap_flags {
> > >   	IB_DEVICE_CROSS_CHANNEL		= (1 << 27),
> > >   	IB_DEVICE_MANAGED_FLOW_STEERING		= (1 << 29),
> > >   	IB_DEVICE_SIGNATURE_HANDOVER		= (1 << 30),
> > > -	IB_DEVICE_ON_DEMAND_PAGING		= (1 << 31),
> > > +	IB_DEVICE_ON_DEMAND_PAGING		= (1ULL << 31),
> > >   	IB_DEVICE_SG_GAPS_REG			= (1ULL << 32),
> > >   	IB_DEVICE_VIRTUAL_FUNCTION		= ((u64)1 << 33),
> > >   	IB_DEVICE_RAW_SCATTER_FCS		= ((u64)1 << 34),
> > 
> > Why not just use the BIT() and BIT_ULL() macros instead of "open coding"
> > these?
> 
> Good idea. do you think it will be a good idea to set all the elements in
> this ib_device_cap_flags enum to BIT_ULL() in this patch ? to have 1 style
> instead of many ?

Yes.

Re: [PATCH] IB/core: Fix bit curruption in ib_device_cap_flags structure

[Christoph Lameter]

On Thu, 2 Jun 2016, Greg KH wrote:

> > diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> > index 432bed5..c97357b 100644
> > --- a/include/rdma/ib_verbs.h
> > +++ b/include/rdma/ib_verbs.h
> > @@ -217,7 +217,7 @@ enum ib_device_cap_flags {
> >  	IB_DEVICE_CROSS_CHANNEL		= (1 << 27),
> >  	IB_DEVICE_MANAGED_FLOW_STEERING		= (1 << 29),
> >  	IB_DEVICE_SIGNATURE_HANDOVER		= (1 << 30),
> > -	IB_DEVICE_ON_DEMAND_PAGING		= (1 << 31),
> > +	IB_DEVICE_ON_DEMAND_PAGING		= (1ULL << 31),
> >  	IB_DEVICE_SG_GAPS_REG			= (1ULL << 32),
> >  	IB_DEVICE_VIRTUAL_FUNCTION		= ((u64)1 << 33),
> >  	IB_DEVICE_RAW_SCATTER_FCS		= ((u64)1 << 34),
>
> Why not just use the BIT() and BIT_ULL() macros instead of "open coding"
> these?

This whole enum approach for powers of two also looks not that good. Do an
unsigned long long or u64 please? Use enum to define the individual
bits and not for the bitmask? Could add _MASK for that if you really want
it.

unsigned long long ib_device_cap_flags;

enum ib_dev_caps = { IB_DEVICE_CROSS_CHANNEL, IB_DEVICE_SG_GAPS_REG, ....
NR_IB_DEV_CAPS };

Then use

BIT_ULL(IB_DEVICE_CROSS_CHANNEL) in the code?


Or just specify the bitmasks using hex constants?

from sched.h:

#define SD_LOAD_BALANCE         0x0001  /* Do load balancing on this domain. */
#define SD_BALANCE_NEWIDLE      0x0002  /* Balance when about to become idle */
#define SD_BALANCE_EXEC         0x0004  /* Balance on exec */
#define SD_BALANCE_FORK         0x0008  /* Balance on fork, clone */
#define SD_BALANCE_WAKE         0x0010  /* Balance on wakeup */
#define SD_WAKE_AFFINE          0x0020  /* Wake task to waking CPU */
#define SD_SHARE_CPUCAPACITY    0x0080  /* Domain members share cpu power */
#define SD_SHARE_POWERDOMAIN    0x0100  /* Domain members share power domain */
#define SD_SHARE_PKG_RESOURCES  0x0200  /* Domain members share cpu pkg resources */
#define SD_SERIALIZE            0x0400  /* Only a single load balancing instance */
#define SD_ASYM_PACKING         0x0800  /* Place busy groups earlier in the domain */
#define SD_PREFER_SIBLING       0x1000  /* Prefer to place tasks in a sibling domain */
#define SD_OVERLAP              0x2000  /* sched_domains of this level overlap */
#define SD_NUMA                 0x4000  /* cross-node balancing */

Re: [PATCH] IB/core: Fix bit curruption in ib_device_cap_flags structure

[Jason Gunthorpe]

On Thu, Jun 02, 2016 at 12:28:05PM +0300, Max Gurtovoy wrote:
> ib_device_cap_flags 64-bit expansion caused caps overlapping
> and made consumers read wrong device capabilities. For example
> IB_DEVICE_SG_GAPS_REG was falsely read by the iser driver causing
> it to use a non-existing capability. This happened because signed
> int becomes sign extended when converted it to u64. Fix this by
> casting IB_DEVICE_ON_DEMAND_PAGING enumeration to ULL.
> 
> Fixes: fb532d6a79b9 ('IB/{core, ulp} Support above 32 possible device capability flags')
> Reported-by: Robert LeBlanc <robert@leblancnet.us>
> Cc: Stable <stable@vger.kernel.org> #[v4.6+]
> Acked-by: Sagi Grimberg <sagi@grimberg.me>
> Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
> Signed-off-by: Matan Barak <matanb@mellanox.com>
>  include/rdma/ib_verbs.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)

Reviewed-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

Re: [PATCH] IB/core: Fix bit curruption in ib_device_cap_flags structure

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 783 bytes --]

On Thu, Jun 02, 2016 at 12:28:05PM +0300, Max Gurtovoy wrote:
> ib_device_cap_flags 64-bit expansion caused caps overlapping
> and made consumers read wrong device capabilities. For example
> IB_DEVICE_SG_GAPS_REG was falsely read by the iser driver causing
> it to use a non-existing capability. This happened because signed
> int becomes sign extended when converted it to u64. Fix this by
> casting IB_DEVICE_ON_DEMAND_PAGING enumeration to ULL.
> 
> Fixes: fb532d6a79b9 ('IB/{core, ulp} Support above 32 possible device capability flags')

Everything fine, except this Fixes line, which is similar to the issue,
but unrelated. The sign extension error was produced after adding
bit(32) to the enum and the commit fb532d6a79b9 didn't do it and didn't
mean to do it.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]