[PATCH v3 0/2] fix ib_device_cap_flags bit curruption

[PATCH v3 0/2] fix ib_device_cap_flags bit curruption

[Max Gurtovoy]

This patch set fixes a bit corruption caused by expanding ib_device_cap_flags
enum to 64 bits, that caused a sign extension.
After many discussions regarding the solution, we decided on minimal fix in the
first stage.
Later on, we'll align all the caps enum to BIT/BIT_ULL macros as GKH recommended.
The addition in this vesion is the alignment of the casting method.


Max Gurtovoy (2):
  IB/core: Fix bit curruption in ib_device_cap_flags structure
  IB/core: Align casting method of ib_device_cap_flags enumerations to
    ULL

 include/rdma/ib_verbs.h |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

[PATCH v3 1/2] 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: f5aa9159a418 ('IB/core: Add arbitrary sg_list support')
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 v3 1/2] IB/core: Fix bit curruption in ib_device_cap_flags structure

[Christoph Hellwig]

On Mon, Jun 06, 2016 at 07:34:39PM +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.

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH v3 2/2] IB/core: Align casting method of ib_device_cap_flags enumerations to ULL

[Max Gurtovoy]

Replace u64 casting to ULL.

Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
---
 include/rdma/ib_verbs.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index c97357b..7e440d4 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -219,8 +219,8 @@ enum ib_device_cap_flags {
 	IB_DEVICE_SIGNATURE_HANDOVER		= (1 << 30),
 	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),
+	IB_DEVICE_VIRTUAL_FUNCTION		= (1ULL << 33),
+	IB_DEVICE_RAW_SCATTER_FCS		= (1ULL << 34),
 };
 
 enum ib_signature_prot_cap {
-- 
1.7.1

Re: [PATCH v3 2/2] IB/core: Align casting method of ib_device_cap_flags enumerations to ULL

[Christoph Hellwig]

On Mon, Jun 06, 2016 at 07:34:40PM +0300, Max Gurtovoy wrote:
> Replace u64 casting to ULL.
> 
> Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
> ---
>  include/rdma/ib_verbs.h |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> index c97357b..7e440d4 100644
> --- a/include/rdma/ib_verbs.h
> +++ b/include/rdma/ib_verbs.h
> @@ -219,8 +219,8 @@ enum ib_device_cap_flags {
>  	IB_DEVICE_SIGNATURE_HANDOVER		= (1 << 30),
>  	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),
> +	IB_DEVICE_VIRTUAL_FUNCTION		= (1ULL << 33),
> +	IB_DEVICE_RAW_SCATTER_FCS		= (1ULL << 34),

Looks good, but can you also convert the whole enum to this
style?

Re: [PATCH v3 2/2] IB/core: Align casting method of ib_device_cap_flags enumerations to ULL

[Greg KH]

On Mon, Jun 06, 2016 at 10:04:13AM -0700, Christoph Hellwig wrote:
> On Mon, Jun 06, 2016 at 07:34:40PM +0300, Max Gurtovoy wrote:
> > Replace u64 casting to ULL.
> > 
> > Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
> > ---
> >  include/rdma/ib_verbs.h |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> > index c97357b..7e440d4 100644
> > --- a/include/rdma/ib_verbs.h
> > +++ b/include/rdma/ib_verbs.h
> > @@ -219,8 +219,8 @@ enum ib_device_cap_flags {
> >  	IB_DEVICE_SIGNATURE_HANDOVER		= (1 << 30),
> >  	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),
> > +	IB_DEVICE_VIRTUAL_FUNCTION		= (1ULL << 33),
> > +	IB_DEVICE_RAW_SCATTER_FCS		= (1ULL << 34),
> 
> Looks good, but can you also convert the whole enum to this
> style?

If this "convert the whole thing" is going to happen, can you use the
BIT() for this please?  I asked that last time this patch came by...

thanks,

greg k-h

Re: [PATCH v3 2/2] IB/core: Align casting method of ib_device_cap_flags enumerations to ULL

[Christoph Hellwig]

On Mon, Jun 06, 2016 at 10:15:43AM -0700, Greg KH wrote:
> If this "convert the whole thing" is going to happen, can you use the
> BIT() for this please?  I asked that last time this patch came by...

There is absolutely no reason for that bullshit obsfucation.

Re: [PATCH v3 2/2] IB/core: Align casting method of ib_device_cap_flags enumerations to ULL

[Max Gurtovoy]

On 6/6/2016 8:15 PM, Greg KH wrote:
> On Mon, Jun 06, 2016 at 10:04:13AM -0700, Christoph Hellwig wrote:
>> On Mon, Jun 06, 2016 at 07:34:40PM +0300, Max Gurtovoy wrote:
>>> Replace u64 casting to ULL.
>>>
>>> Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
>>> ---
>>>   include/rdma/ib_verbs.h |    4 ++--
>>>   1 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
>>> index c97357b..7e440d4 100644
>>> --- a/include/rdma/ib_verbs.h
>>> +++ b/include/rdma/ib_verbs.h
>>> @@ -219,8 +219,8 @@ enum ib_device_cap_flags {
>>>   	IB_DEVICE_SIGNATURE_HANDOVER		= (1 << 30),
>>>   	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),
>>> +	IB_DEVICE_VIRTUAL_FUNCTION		= (1ULL << 33),
>>> +	IB_DEVICE_RAW_SCATTER_FCS		= (1ULL << 34),
>>
>> Looks good, but can you also convert the whole enum to this
>> style?

This was my v1 patch but many peaple didn't like it so much.
I coordinated with Doug that I'll push small patch for now and later we 
can cast the whole enum to preferable style.

>
> If this "convert the whole thing" is going to happen, can you use the
> BIT() for this please?  I asked that last time this patch came by...

It's a maintainers desicion :)

>
> thanks,
>
> greg k-h
>

Re: [PATCH v3 2/2] IB/core: Align casting method of ib_device_cap_flags enumerations to ULL

[hch]

> Later on, we will wrap all these shifts in BIT() macros as Greg suggested.

Big NAK to any use of that macro.

Re: [PATCH v3 2/2] IB/core: Align casting method of ib_device_cap_flags enumerations to ULL

[Greg KH]

On Mon, Jun 06, 2016 at 10:22:46AM -0700, hch wrote:
> > Later on, we will wrap all these shifts in BIT() macros as Greg suggested.
> 
> Big NAK to any use of that macro.

That's odd, given that most of the kernel has already converted to use
it.  You can try to keep it out of this subsystem, but you will get
patches to convert it from random people and it will wear you down over
time...

sorry,

greg k-h

Re: [PATCH v3 2/2] IB/core: Align casting method of ib_device_cap_flags enumerations to ULL

[Bart Van Assche]

On 06/06/2016 10:40 AM, Greg KH wrote:
> On Mon, Jun 06, 2016 at 10:22:46AM -0700, hch wrote:
>>> Later on, we will wrap all these shifts in BIT() macros as Greg suggested.
>>
>> Big NAK to any use of that macro.
>
> That's odd, given that most of the kernel has already converted to use
> it.  You can try to keep it out of this subsystem, but you will get
> patches to convert it from random people and it will wear you down over
> time...

Hello Greg,

It is considered a bad programming practice to introduce macros that 
obfuscate C operations. All the BIT() macro does is a shift operation. 
Writing the shift explicitly instead of using the BIT() macro makes 
source code easier to read. So why has the BIT() macro ever been introduced?

Thanks,

Bart.

Re: [PATCH v3 0/2] fix ib_device_cap_flags bit curruption

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 707 bytes --]

On 6/6/2016 12:34 PM, Max Gurtovoy wrote:
> This patch set fixes a bit corruption caused by expanding ib_device_cap_flags
> enum to 64 bits, that caused a sign extension.
> After many discussions regarding the solution, we decided on minimal fix in the
> first stage.
> Later on, we'll align all the caps enum to BIT/BIT_ULL macros as GKH recommended.
> The addition in this vesion is the alignment of the casting method.
> 
> 
> Max Gurtovoy (2):
>   IB/core: Fix bit curruption in ib_device_cap_flags structure
>   IB/core: Align casting method of ib_device_cap_flags enumerations to
>     ULL

Series applied (albeit with a fixed up subject and message in the second
patch).  Thanks.



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]