public inbox for linux-rdma@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] IB/core: Fix bit curruption in ib_device_cap_flags structure
@ 2016-06-02  9:28 Max Gurtovoy
  2016-06-02 11:33 ` Christoph Hellwig
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Max Gurtovoy @ 2016-06-02  9:28 UTC (permalink / raw)
  To: matanb-VPRAkNaXOzVWk0Htik3J/w, leon-2ukJVAZIZ/Y,
	sagi-NQWnxTmZq1alnMjI0IkVqw, linux-rdma-u79uwXL29TY76Z2rM5mHXA
  Cc: stable-u79uwXL29TY76Z2rM5mHXA, robert-4JaGZRWAfWbajFs6igw21g,
	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-4JaGZRWAfWbajFs6igw21g@public.gmane.org>
Cc: Stable <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> #[v4.6+]
Acked-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
Signed-off-by: Max Gurtovoy <maxg-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] IB/core: Fix bit curruption in ib_device_cap_flags structure
  2016-06-02  9:28 [PATCH] IB/core: Fix bit curruption in ib_device_cap_flags structure Max Gurtovoy
@ 2016-06-02 11:33 ` Christoph Hellwig
       [not found]   ` <20160602113353.GA18494-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
  2016-06-02 16:51   ` Jason Gunthorpe
  2016-06-02 16:52 ` Jason Gunthorpe
       [not found] ` <1464859685-18666-1-git-send-email-maxg-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
  2 siblings, 2 replies; 13+ messages in thread
From: Christoph Hellwig @ 2016-06-02 11:33 UTC (permalink / raw)
  To: Max Gurtovoy; +Cc: matanb, leon, sagi, linux-rdma, stable, robert

>  	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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] IB/core: Fix bit curruption in ib_device_cap_flags structure
       [not found]   ` <20160602113353.GA18494-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
@ 2016-06-02 11:48     ` Max Gurtovoy
  2016-06-02 11:56       ` Sagi Grimberg
  0 siblings, 1 reply; 13+ messages in thread
From: Max Gurtovoy @ 2016-06-02 11:48 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: matanb-VPRAkNaXOzVWk0Htik3J/w, leon-2ukJVAZIZ/Y,
	sagi-NQWnxTmZq1alnMjI0IkVqw, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
	stable-u79uwXL29TY76Z2rM5mHXA, robert-4JaGZRWAfWbajFs6igw21g



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 :)

>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] IB/core: Fix bit curruption in ib_device_cap_flags structure
  2016-06-02 11:48     ` Max Gurtovoy
@ 2016-06-02 11:56       ` Sagi Grimberg
  0 siblings, 0 replies; 13+ messages in thread
From: Sagi Grimberg @ 2016-06-02 11:56 UTC (permalink / raw)
  To: Max Gurtovoy, Christoph Hellwig; +Cc: matanb, leon, linux-rdma, stable, robert


>>>       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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] IB/core: Fix bit curruption in ib_device_cap_flags structure
       [not found] ` <1464859685-18666-1-git-send-email-maxg-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
@ 2016-06-02 16:24   ` Greg KH
  2016-06-02 16:41     ` Max Gurtovoy
       [not found]     ` <20160602162426.GC26699-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
  2016-06-03 10:47   ` Leon Romanovsky
  1 sibling, 2 replies; 13+ messages in thread
From: Greg KH @ 2016-06-02 16:24 UTC (permalink / raw)
  To: Max Gurtovoy
  Cc: matanb-VPRAkNaXOzVWk0Htik3J/w, leon-2ukJVAZIZ/Y,
	sagi-NQWnxTmZq1alnMjI0IkVqw, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
	stable-u79uwXL29TY76Z2rM5mHXA, robert-4JaGZRWAfWbajFs6igw21g

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-4JaGZRWAfWbajFs6igw21g@public.gmane.org>
> Cc: Stable <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> #[v4.6+]
> Acked-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
> Signed-off-by: Max Gurtovoy <maxg-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> ---
>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] IB/core: Fix bit curruption in ib_device_cap_flags structure
       [not found]     ` <20160602162426.GC26699-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
@ 2016-06-02 16:41       ` Bart Van Assche
       [not found]         ` <c72fac21-0a3d-fc40-ec82-e156f29c2fab-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
  2016-06-02 16:48       ` Christoph Lameter
  1 sibling, 1 reply; 13+ messages in thread
From: Bart Van Assche @ 2016-06-02 16:41 UTC (permalink / raw)
  To: Greg KH, Max Gurtovoy
  Cc: matanb-VPRAkNaXOzVWk0Htik3J/w, leon-2ukJVAZIZ/Y,
	sagi-NQWnxTmZq1alnMjI0IkVqw, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
	stable-u79uwXL29TY76Z2rM5mHXA, robert-4JaGZRWAfWbajFs6igw21g

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] IB/core: Fix bit curruption in ib_device_cap_flags structure
  2016-06-02 16:24   ` Greg KH
@ 2016-06-02 16:41     ` Max Gurtovoy
       [not found]       ` <575061A6.1060302-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
       [not found]     ` <20160602162426.GC26699-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
  1 sibling, 1 reply; 13+ messages in thread
From: Max Gurtovoy @ 2016-06-02 16:41 UTC (permalink / raw)
  To: Greg KH; +Cc: matanb, leon, sagi, linux-rdma, stable, robert



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
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] IB/core: Fix bit curruption in ib_device_cap_flags structure
       [not found]     ` <20160602162426.GC26699-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
  2016-06-02 16:41       ` Bart Van Assche
@ 2016-06-02 16:48       ` Christoph Lameter
  1 sibling, 0 replies; 13+ messages in thread
From: Christoph Lameter @ 2016-06-02 16:48 UTC (permalink / raw)
  To: Greg KH
  Cc: Max Gurtovoy, matanb-VPRAkNaXOzVWk0Htik3J/w, leon-2ukJVAZIZ/Y,
	sagi-NQWnxTmZq1alnMjI0IkVqw, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
	stable-u79uwXL29TY76Z2rM5mHXA, robert-4JaGZRWAfWbajFs6igw21g

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 */

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] IB/core: Fix bit curruption in ib_device_cap_flags structure
  2016-06-02 11:33 ` Christoph Hellwig
       [not found]   ` <20160602113353.GA18494-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
@ 2016-06-02 16:51   ` Jason Gunthorpe
  1 sibling, 0 replies; 13+ messages in thread
From: Jason Gunthorpe @ 2016-06-02 16:51 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Max Gurtovoy, matanb, leon, sagi, linux-rdma, stable, robert

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] IB/core: Fix bit curruption in ib_device_cap_flags structure
  2016-06-02  9:28 [PATCH] IB/core: Fix bit curruption in ib_device_cap_flags structure Max Gurtovoy
  2016-06-02 11:33 ` Christoph Hellwig
@ 2016-06-02 16:52 ` Jason Gunthorpe
       [not found] ` <1464859685-18666-1-git-send-email-maxg-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
  2 siblings, 0 replies; 13+ messages in thread
From: Jason Gunthorpe @ 2016-06-02 16:52 UTC (permalink / raw)
  To: Max Gurtovoy; +Cc: matanb, leon, sagi, linux-rdma, stable, robert

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>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] IB/core: Fix bit curruption in ib_device_cap_flags structure
       [not found]         ` <c72fac21-0a3d-fc40-ec82-e156f29c2fab-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
@ 2016-06-02 16:52           ` Greg KH
  0 siblings, 0 replies; 13+ messages in thread
From: Greg KH @ 2016-06-02 16:52 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: Max Gurtovoy, matanb-VPRAkNaXOzVWk0Htik3J/w, leon-2ukJVAZIZ/Y,
	sagi-NQWnxTmZq1alnMjI0IkVqw, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
	stable-u79uwXL29TY76Z2rM5mHXA, robert-4JaGZRWAfWbajFs6igw21g

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
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] IB/core: Fix bit curruption in ib_device_cap_flags structure
       [not found]       ` <575061A6.1060302-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
@ 2016-06-02 16:52         ` Greg KH
  0 siblings, 0 replies; 13+ messages in thread
From: Greg KH @ 2016-06-02 16:52 UTC (permalink / raw)
  To: Max Gurtovoy
  Cc: matanb-VPRAkNaXOzVWk0Htik3J/w, leon-2ukJVAZIZ/Y,
	sagi-NQWnxTmZq1alnMjI0IkVqw, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
	stable-u79uwXL29TY76Z2rM5mHXA, robert-4JaGZRWAfWbajFs6igw21g

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-4JaGZRWAfWbajFs6igw21g@public.gmane.org>
> > > Cc: Stable <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> #[v4.6+]
> > > Acked-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
> > > Signed-off-by: Max Gurtovoy <maxg-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> > > Signed-off-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> > > ---
> > >   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.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] IB/core: Fix bit curruption in ib_device_cap_flags structure
       [not found] ` <1464859685-18666-1-git-send-email-maxg-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
  2016-06-02 16:24   ` Greg KH
@ 2016-06-03 10:47   ` Leon Romanovsky
  1 sibling, 0 replies; 13+ messages in thread
From: Leon Romanovsky @ 2016-06-03 10:47 UTC (permalink / raw)
  To: Max Gurtovoy
  Cc: matanb-VPRAkNaXOzVWk0Htik3J/w, sagi-NQWnxTmZq1alnMjI0IkVqw,
	linux-rdma-u79uwXL29TY76Z2rM5mHXA, stable-u79uwXL29TY76Z2rM5mHXA,
	robert-4JaGZRWAfWbajFs6igw21g

[-- 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 --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2016-06-03 10:47 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-06-02  9:28 [PATCH] IB/core: Fix bit curruption in ib_device_cap_flags structure Max Gurtovoy
2016-06-02 11:33 ` Christoph Hellwig
     [not found]   ` <20160602113353.GA18494-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
2016-06-02 11:48     ` Max Gurtovoy
2016-06-02 11:56       ` Sagi Grimberg
2016-06-02 16:51   ` Jason Gunthorpe
2016-06-02 16:52 ` Jason Gunthorpe
     [not found] ` <1464859685-18666-1-git-send-email-maxg-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
2016-06-02 16:24   ` Greg KH
2016-06-02 16:41     ` Max Gurtovoy
     [not found]       ` <575061A6.1060302-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
2016-06-02 16:52         ` Greg KH
     [not found]     ` <20160602162426.GC26699-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
2016-06-02 16:41       ` Bart Van Assche
     [not found]         ` <c72fac21-0a3d-fc40-ec82-e156f29c2fab-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
2016-06-02 16:52           ` Greg KH
2016-06-02 16:48       ` Christoph Lameter
2016-06-03 10:47   ` Leon Romanovsky

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox