* [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
[not found] ` <1464859685-18666-1-git-send-email-maxg-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
2016-06-02 16:52 ` Jason Gunthorpe
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
[not found] ` <20160602162426.GC26699-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
2016-06-02 16:41 ` Max Gurtovoy
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
[not found] ` <20160602162426.GC26699-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
@ 2016-06-02 16:41 ` Max Gurtovoy
[not found] ` <575061A6.1060302-VPRAkNaXOzVWk0Htik3J/w@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
[not found] ` <1464859685-18666-1-git-send-email-maxg-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
@ 2016-06-02 16:52 ` Jason Gunthorpe
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
[not found] ` <1464859685-18666-1-git-send-email-maxg-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
2016-06-02 16:24 ` 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-02 16:41 ` Max Gurtovoy
[not found] ` <575061A6.1060302-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
2016-06-02 16:52 ` Greg KH
2016-06-03 10:47 ` Leon Romanovsky
2016-06-02 16:52 ` Jason Gunthorpe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox