[PATCH] IB/core: Fix different types mix in ib_device_cap_flags structure values

[PATCH] IB/core: Fix different types mix in ib_device_cap_flags structure values

[Max Gurtovoy]

ib_device_cap_flags 64-bit expansion caused a possible caps overlapping
(depending on machine endianness) 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. Fix this by
casting ib_device_cap_flags enumerations to ULL.

Fixes: 45686f2d6535 ('IB/core: Add Raw Scatter FCS device capability')
Fixes: 50174a7f2c24 ('IB/core: Add interfaces to control VF attributes')
Fixes: f5aa9159a418 ('IB/core: Add arbitrary sg_list support')
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 |   66 +++++++++++++++++++++++-----------------------
 1 files changed, 33 insertions(+), 33 deletions(-)

diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 432bed5..8bb97dc 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -156,21 +156,21 @@ enum rdma_link_layer {
 };
 
 enum ib_device_cap_flags {
-	IB_DEVICE_RESIZE_MAX_WR			= (1 << 0),
-	IB_DEVICE_BAD_PKEY_CNTR			= (1 << 1),
-	IB_DEVICE_BAD_QKEY_CNTR			= (1 << 2),
-	IB_DEVICE_RAW_MULTI			= (1 << 3),
-	IB_DEVICE_AUTO_PATH_MIG			= (1 << 4),
-	IB_DEVICE_CHANGE_PHY_PORT		= (1 << 5),
-	IB_DEVICE_UD_AV_PORT_ENFORCE		= (1 << 6),
-	IB_DEVICE_CURR_QP_STATE_MOD		= (1 << 7),
-	IB_DEVICE_SHUTDOWN_PORT			= (1 << 8),
-	IB_DEVICE_INIT_TYPE			= (1 << 9),
-	IB_DEVICE_PORT_ACTIVE_EVENT		= (1 << 10),
-	IB_DEVICE_SYS_IMAGE_GUID		= (1 << 11),
-	IB_DEVICE_RC_RNR_NAK_GEN		= (1 << 12),
-	IB_DEVICE_SRQ_RESIZE			= (1 << 13),
-	IB_DEVICE_N_NOTIFY_CQ			= (1 << 14),
+	IB_DEVICE_RESIZE_MAX_WR			= (1ULL << 0),
+	IB_DEVICE_BAD_PKEY_CNTR			= (1ULL << 1),
+	IB_DEVICE_BAD_QKEY_CNTR			= (1ULL << 2),
+	IB_DEVICE_RAW_MULTI			= (1ULL << 3),
+	IB_DEVICE_AUTO_PATH_MIG			= (1ULL << 4),
+	IB_DEVICE_CHANGE_PHY_PORT		= (1ULL << 5),
+	IB_DEVICE_UD_AV_PORT_ENFORCE		= (1ULL << 6),
+	IB_DEVICE_CURR_QP_STATE_MOD		= (1ULL << 7),
+	IB_DEVICE_SHUTDOWN_PORT			= (1ULL << 8),
+	IB_DEVICE_INIT_TYPE			= (1ULL << 9),
+	IB_DEVICE_PORT_ACTIVE_EVENT		= (1ULL << 10),
+	IB_DEVICE_SYS_IMAGE_GUID		= (1ULL << 11),
+	IB_DEVICE_RC_RNR_NAK_GEN		= (1ULL << 12),
+	IB_DEVICE_SRQ_RESIZE			= (1ULL << 13),
+	IB_DEVICE_N_NOTIFY_CQ			= (1ULL << 14),
 
 	/*
 	 * This device supports a per-device lkey or stag that can be
@@ -179,9 +179,9 @@ enum ib_device_cap_flags {
 	 * instead of use the local_dma_lkey flag in the ib_pd structure,
 	 * which will always contain a usable lkey.
 	 */
-	IB_DEVICE_LOCAL_DMA_LKEY		= (1 << 15),
-	IB_DEVICE_RESERVED /* old SEND_W_INV */	= (1 << 16),
-	IB_DEVICE_MEM_WINDOW			= (1 << 17),
+	IB_DEVICE_LOCAL_DMA_LKEY		= (1ULL << 15),
+	IB_DEVICE_RESERVED /* old SEND_W_INV */	= (1ULL << 16),
+	IB_DEVICE_MEM_WINDOW			= (1ULL << 17),
 	/*
 	 * Devices should set IB_DEVICE_UD_IP_SUM if they support
 	 * insertion of UDP and TCP checksum on outgoing UD IPoIB
@@ -189,9 +189,9 @@ enum ib_device_cap_flags {
 	 * incoming messages.  Setting this flag implies that the
 	 * IPoIB driver may set NETIF_F_IP_CSUM for datagram mode.
 	 */
-	IB_DEVICE_UD_IP_CSUM			= (1 << 18),
-	IB_DEVICE_UD_TSO			= (1 << 19),
-	IB_DEVICE_XRC				= (1 << 20),
+	IB_DEVICE_UD_IP_CSUM			= (1ULL << 18),
+	IB_DEVICE_UD_TSO			= (1ULL << 19),
+	IB_DEVICE_XRC				= (1ULL << 20),
 
 	/*
 	 * This device supports the IB "base memory management extension",
@@ -202,25 +202,25 @@ enum ib_device_cap_flags {
 	 * IB_WR_RDMA_READ_WITH_INV verb for RDMA READs that invalidate the
 	 * stag.
 	 */
-	IB_DEVICE_MEM_MGT_EXTENSIONS		= (1 << 21),
-	IB_DEVICE_BLOCK_MULTICAST_LOOPBACK	= (1 << 22),
-	IB_DEVICE_MEM_WINDOW_TYPE_2A		= (1 << 23),
-	IB_DEVICE_MEM_WINDOW_TYPE_2B		= (1 << 24),
-	IB_DEVICE_RC_IP_CSUM			= (1 << 25),
-	IB_DEVICE_RAW_IP_CSUM			= (1 << 26),
+	IB_DEVICE_MEM_MGT_EXTENSIONS		= (1ULL << 21),
+	IB_DEVICE_BLOCK_MULTICAST_LOOPBACK	= (1ULL << 22),
+	IB_DEVICE_MEM_WINDOW_TYPE_2A		= (1ULL << 23),
+	IB_DEVICE_MEM_WINDOW_TYPE_2B		= (1ULL << 24),
+	IB_DEVICE_RC_IP_CSUM			= (1ULL << 25),
+	IB_DEVICE_RAW_IP_CSUM			= (1ULL << 26),
 	/*
 	 * Devices should set IB_DEVICE_CROSS_CHANNEL if they
 	 * support execution of WQEs that involve synchronization
 	 * of I/O operations with single completion queue managed
 	 * by hardware.
 	 */
-	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_CROSS_CHANNEL		= (1ULL << 27),
+	IB_DEVICE_MANAGED_FLOW_STEERING		= (1ULL << 29),
+	IB_DEVICE_SIGNATURE_HANDOVER		= (1ULL << 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] IB/core: Fix different types mix in ib_device_cap_flags structure values

[Bart Van Assche]

On 05/30/16 03:09, Max Gurtovoy wrote:
> ib_device_cap_flags 64-bit expansion caused a possible caps overlapping
> (depending on machine endianness) 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. Fix this by
> casting ib_device_cap_flags enumerations to ULL.
>
 > [ ... ]
> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> [ ... ]
>  enum ib_device_cap_flags {
 >       [ ... ]
>  	IB_DEVICE_SG_GAPS_REG			= (1ULL << 32),
>       [ ... ]
>  };

How can this patch make a difference? The presence of any constant in an 
enum that does not fit in a 32-bit integer makes an enum 64 bits wide. 
In other words, all the changes from "1" into "1ULL" in this patch do 
not have any effect. From the C standard: "Each enumerated type shall be 
compatible with char, a signed integer type, or an unsigned integer 
type. The choice of type is implementation-defined but shall be capable 
of representing the values of all the members of the enumeration. [...] 
An implementation may delay the choice of which integer type until all 
enumeration constants have been seen."

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

Re: [PATCH] IB/core: Fix different types mix in ib_device_cap_flags structure values

[Jason Gunthorpe]

On Tue, May 31, 2016 at 08:35:10AM -0700, Bart Van Assche wrote:
> On 05/30/16 03:09, Max Gurtovoy wrote:
> >ib_device_cap_flags 64-bit expansion caused a possible caps overlapping
> >(depending on machine endianness) 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. Fix this by
> >casting ib_device_cap_flags enumerations to ULL.
> >
> > [ ... ]
> >diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> >[ ... ]
> > enum ib_device_cap_flags {
> >       [ ... ]
> > 	IB_DEVICE_SG_GAPS_REG			= (1ULL << 32),
> >      [ ... ]
> > };
> 
> How can this patch make a difference? The presence of any constant
> in an enum that does not fit in a 32-bit integer makes an enum 64
> bits wide. In other words, all the changes from "1" into "1ULL" in
> this patch do not have

The expressions are evaluated before the enum type is decided, the
enum type has no impact on the type of the expressions.

(1<<32) is always undefined behavior because '1' is only a 32 bit type.

I'm confused why we didn't get any static checker hits on the shift
overflow - modern compilers warn on that??

Jason

Re: [PATCH] IB/core: Fix different types mix in ib_device_cap_flags structure values

[Leon Romanovsky]

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

On Tue, May 31, 2016 at 11:13:06AM -0600, Jason Gunthorpe wrote:
> On Tue, May 31, 2016 at 08:35:10AM -0700, Bart Van Assche wrote:
> > On 05/30/16 03:09, Max Gurtovoy wrote:
> > >ib_device_cap_flags 64-bit expansion caused a possible caps overlapping
> > >(depending on machine endianness) 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. Fix this by
> > >casting ib_device_cap_flags enumerations to ULL.
> > >
> > > [ ... ]
> > >diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> > >[ ... ]
> > > enum ib_device_cap_flags {
> > >       [ ... ]
> > > 	IB_DEVICE_SG_GAPS_REG			= (1ULL << 32),
> > >      [ ... ]
> > > };
> > 
> > How can this patch make a difference? The presence of any constant
> > in an enum that does not fit in a 32-bit integer makes an enum 64
> > bits wide. In other words, all the changes from "1" into "1ULL" in
> > this patch do not have
> 
> The expressions are evaluated before the enum type is decided, the
> enum type has no impact on the type of the expressions.

It is machine/compiler dependent.

Bart,
Can you share your source of C-standard?

This link [1] states in chapter "6.7.2.2 Enumeration specifiers"

"Each enumerated type shall be compatible with char, a signed integer type,
or an unsigned integer type. The choice of type is implementation-defined (110),
but shall be capable of representing the values of all the members of the enumeration.
The enumerated type is incomplete until after the } that terminates the list of enumerator
declarations."

And the footnote (110):
"An implementation **may** delay the choice of which integer type until all enumeration
constants have been seen."

[1] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf


> 
> (1<<32) is always undefined behavior because '1' is only a 32 bit type.
> 
> I'm confused why we didn't get any static checker hits on the shift
> overflow - modern compilers warn on that??
> 
> Jason

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

Re: [PATCH] IB/core: Fix different types mix in ib_device_cap_flags structure values

[Bart Van Assche]

On 05/31/2016 10:30 AM, Leon Romanovsky wrote:
> On Tue, May 31, 2016 at 11:13:06AM -0600, Jason Gunthorpe wrote:
>> On Tue, May 31, 2016 at 08:35:10AM -0700, Bart Van Assche wrote:
>>> On 05/30/16 03:09, Max Gurtovoy wrote:
>>>> ib_device_cap_flags 64-bit expansion caused a possible caps overlapping
>>>> (depending on machine endianness) 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. Fix this by
>>>> casting ib_device_cap_flags enumerations to ULL.
>>>>
>>>> [ ... ]
>>>> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
>>>> [ ... ]
>>>> enum ib_device_cap_flags {
>>>>       [ ... ]
>>>> 	IB_DEVICE_SG_GAPS_REG			= (1ULL << 32),
>>>>      [ ... ]
>>>> };
>>>
>>> How can this patch make a difference? The presence of any constant
>>> in an enum that does not fit in a 32-bit integer makes an enum 64
>>> bits wide. In other words, all the changes from "1" into "1ULL" in
>>> this patch do not have
>>
>> The expressions are evaluated before the enum type is decided, the
>> enum type has no impact on the type of the expressions.
>
> It is machine/compiler dependent.
>
> Bart,
> Can you share your source of C-standard?
>
> This link [1] states in chapter "6.7.2.2 Enumeration specifiers"
>
> "Each enumerated type shall be compatible with char, a signed integer type,
> or an unsigned integer type. The choice of type is implementation-defined (110),
> but shall be capable of representing the values of all the members of the enumeration.
> The enumerated type is incomplete until after the } that terminates the list of enumerator
> declarations."
>
> And the footnote (110):
> "An implementation **may** delay the choice of which integer type until all enumeration
> constants have been seen."
>
> [1] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf

Let me rephrase my question. Before and after this patch 
IB_DEVICE_SG_GAPS_REG is defined as 1ULL << 32, so how can this patch 
make a difference? And if the issue is that some compilers choose a 
32-bit integer for ib_device_cap_flags and others a 64-bit integer, 
shouldn't ib_device_cap_flags be converted from an enum into a series of 
#defines? Or is the issue rather that some compilers choose the enum 
size depending on the size of the first enumeration constant? Anyway, I 
think the patch description needs to be clarified.

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

Re: [PATCH] IB/core: Fix different types mix in ib_device_cap_flags structure values

[Leon Romanovsky]

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

On Tue, May 31, 2016 at 11:05:00AM -0700, Bart Van Assche wrote:
> On 05/31/2016 10:30 AM, Leon Romanovsky wrote:
> >On Tue, May 31, 2016 at 11:13:06AM -0600, Jason Gunthorpe wrote:
> >>On Tue, May 31, 2016 at 08:35:10AM -0700, Bart Van Assche wrote:
> >>>On 05/30/16 03:09, Max Gurtovoy wrote:
> >>>>ib_device_cap_flags 64-bit expansion caused a possible caps overlapping
> >>>>(depending on machine endianness) 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. Fix this by
> >>>>casting ib_device_cap_flags enumerations to ULL.
> >>>>
> >>>>[ ... ]
> >>>>diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> >>>>[ ... ]
> >>>>enum ib_device_cap_flags {
> >>>>      [ ... ]
> >>>>	IB_DEVICE_SG_GAPS_REG			= (1ULL << 32),
> >>>>     [ ... ]
> >>>>};
> >>>
> >>>How can this patch make a difference? The presence of any constant
> >>>in an enum that does not fit in a 32-bit integer makes an enum 64
> >>>bits wide. In other words, all the changes from "1" into "1ULL" in
> >>>this patch do not have
> >>
> >>The expressions are evaluated before the enum type is decided, the
> >>enum type has no impact on the type of the expressions.
> >
> >It is machine/compiler dependent.
> >
> >Bart,
> >Can you share your source of C-standard?
> >
> >This link [1] states in chapter "6.7.2.2 Enumeration specifiers"
> >
> >"Each enumerated type shall be compatible with char, a signed integer type,
> >or an unsigned integer type. The choice of type is implementation-defined (110),
> >but shall be capable of representing the values of all the members of the enumeration.
> >The enumerated type is incomplete until after the } that terminates the list of enumerator
> >declarations."
> >
> >And the footnote (110):
> >"An implementation **may** delay the choice of which integer type until all enumeration
> >constants have been seen."
> >
> >[1] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf
> 
> Let me rephrase my question. Before and after this patch
> IB_DEVICE_SG_GAPS_REG is defined as 1ULL << 32, so how can this patch make a
> difference? And if the issue is that some compilers choose a 32-bit integer
> for ib_device_cap_flags and others a 64-bit integer, shouldn't
> ib_device_cap_flags be converted from an enum into a series of #defines? Or
> is the issue rather that some compilers choose the enum size depending on
> the size of the first enumeration constant? Anyway, I think the patch
> description needs to be clarified.

I understand from the description/standards that first enum declares
type.

> 
> Bart.

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

Re: [PATCH] IB/core: Fix different types mix in ib_device_cap_flags structure values

[Jason Gunthorpe]

On Tue, May 31, 2016 at 09:12:23PM +0300, Leon Romanovsky wrote:

> I understand from the description/standards that first enum declares
> type.

No, you quoted the relevant standard:

.. but shall be capable of representing the values of all the members
  of the enumeration. ..

Truncation is not allowed.

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

Re: [PATCH] IB/core: Fix different types mix in ib_device_cap_flags structure values

[Leon Romanovsky]

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

On Tue, May 31, 2016 at 12:21:00PM -0600, Jason Gunthorpe wrote:
> On Tue, May 31, 2016 at 09:12:23PM +0300, Leon Romanovsky wrote:
> 
> > I understand from the description/standards that first enum declares
> > type.
> 
> No, you quoted the relevant standard:
> 
> .. but shall be capable of representing the values of all the members
>   of the enumeration. ..
> 
> Truncation is not allowed.

I'm not convinced that it talks about truncation.

From the same document:
"The expression that defines the value of an enumeration constant shall
be an integer constant expression that has a value representable as an
int."

> 
> Jason

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

Re: [PATCH] IB/core: Fix different types mix in ib_device_cap_flags structure values

[Jason Gunthorpe]

On Tue, May 31, 2016 at 09:54:32PM +0300, Leon Romanovsky wrote:

> > No, you quoted the relevant standard:
> > 
> > .. but shall be capable of representing the values of all the members
> >   of the enumeration. ..
> > 
> > Truncation is not allowed.
> 
> I'm not convinced that it talks about truncation.
> 
> From the same document:
> "The expression that defines the value of an enumeration constant shall
> be an integer constant expression that has a value representable as an
> int."

Hmm.

There are two things going on here.

The quote above is refereing to the type of the constants. In C99 the
type of the constants is not the type of the enum. (I did not know
that, what a goofy obscure thing. C++11 gets it right) It would appear
the constants are fixed at int by the standard.

However, gcc/clang both will upgrade the type of the constant values,
on a value by value basis, to something larger:

This illustrates the concept:

enum Foo
{
	FOO_VALUE1 = 1ULL,
	FOO_VALUE2 = (uint32_t)UINT32_MAX,
	FOO_VALUE3 = ((uint64_t)2) << 32,
	FOO_VALUE4 = 1<<31,
};

static void check_int(int *v)  { }
static void check_long(long *v)  { }
// Both work:
//static void check_ull(uint64_t *v)  { }
static void check_ull(enum Foo *v)  { }

int main(int argc,const char *argv[])
{
	check_int((__typeof__(FOO_VALUE1) *)0);
	check_long((__typeof__(FOO_VALUE2) *)0);
	check_ull((__typeof__(FOO_VALUE3) *)0);
	check_int((__typeof__(FOO_VALUE4) *)0);
}				  

The four constants have different types and the types are not
necessarily what you'd expect (eg FOO_VALUE3 has been promoted to a
64 bit signed long, FOO_VALUE1 has been demoted to an int)

So the enum is safe, but having different types of the values is
certainly unexpected.

I still think the actual bug is only 1<<31 I pointed out earlier:

enum Foo
{
	FOO_VALUE1 = 1,
	FOO_VALUE2 = ((uint64_t)2) << 32,
	FOO_VALUE3 = 1<<31,
};
uint64_t res = FOO_VALUE1 | FOO_VALUE2 | FOO_VALUE3;
printf("%lx\n",res);
==
ffffffff80000001

Which is clearly wrong. This is because signed int becomes sign
extended when converted to uint64_t, corrupting the upper bits.

Since adding the ULL's doesn't make the enum constants have uniform
type I would not bother with the churn.

Jason

Re: [PATCH] IB/core: Fix different types mix in ib_device_cap_flags structure values

[Max Gurtovoy]

> The four constants have different types and the types are not
> necessarily what you'd expect (eg FOO_VALUE3 has been promoted to a
> 64 bit signed long, FOO_VALUE1 has been demoted to an int)
>
> So the enum is safe, but having different types of the values is
> certainly unexpected.
>
> I still think the actual bug is only 1<<31 I pointed out earlier:
>
> enum Foo
> {
> 	FOO_VALUE1 = 1,
> 	FOO_VALUE2 = ((uint64_t)2) << 32,
> 	FOO_VALUE3 = 1<<31,
> };
> uint64_t res = FOO_VALUE1 | FOO_VALUE2 | FOO_VALUE3;
> printf("%lx\n",res);
> ==
> ffffffff80000001
>
> Which is clearly wrong. This is because signed int becomes sign
> extended when converted to uint64_t, corrupting the upper bits.
>
> Since adding the ULL's doesn't make the enum constants have uniform
> type I would not bother with the churn.

tested this with single casting to IB_DEVICE_ON_DEMAND_PAGING (1 << 31) 
to (1ULL << 31). works fine.
I'll send another patch today.

Thanks,
Max.

>
> Jason
>

Re: [PATCH] IB/core: Fix different types mix in ib_device_cap_flags structure values

[Sagi Grimberg]

>> Which is clearly wrong. This is because signed int becomes sign
>> extended when converted to uint64_t, corrupting the upper bits.
>>
>> Since adding the ULL's doesn't make the enum constants have uniform
>> type I would not bother with the churn.
>
> tested this with single casting to IB_DEVICE_ON_DEMAND_PAGING (1 << 31)
> to (1ULL << 31). works fine.
> I'll send another patch today.

Good to know, thanks Max.
--
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

Re: [PATCH] IB/core: Fix different types mix in ib_device_cap_flags structure values

[Bart Van Assche]

On 05/31/2016 11:12 AM, Leon Romanovsky wrote:
> I understand from the description/standards that first enum declares
> type.

 From http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf: "The 
expression that defines the value of an enumeration constant shall be an 
integer constant expression that has a value representable as an int."

Since int is in bold in that phrase I assume these three letters refer 
to the int data type. And since sizeof(int) == 4 for many Linux 
architectures, including x86-64, shouldn't enum ib_device_cap_flags be 
converted into something else than an enum?

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

Re: [PATCH] IB/core: Fix different types mix in ib_device_cap_flags structure values

[Jason Gunthorpe]

On Tue, May 31, 2016 at 11:05:00AM -0700, Bart Van Assche wrote:

> Let me rephrase my question. Before and after this patch
> IB_DEVICE_SG_GAPS_REG is defined as 1ULL << 32, so how can this patch make a
> difference?

Excellent question.

I thought the issue was this:

        IB_DEVICE_ON_DEMAND_PAGING              = (1 << 31),

Because (1<<31) is also technically undefined if 1 is signed, which,
IIRC, is a compiler choice.. But you are right, all the other ones
look OK.

Mellanox, so what does this *actually* do??

> And if the issue is that some compilers choose a 32-bit integer for
> ib_device_cap_flags and others a 64-bit integer, shouldn't

That isn't allowed by the standard. The compiler cannot truncate enum
values.

Jason

Re: [PATCH] IB/core: Fix different types mix in ib_device_cap_flags structure values

[Robert LeBlanc]

I know that there has been quite a discussion about this. I just want
to report back that this patch does resolve the issue for me. I had to
remove the IB_DEVICE_RAW_SCATTER_FCS lines from the patch for it to
apply to the 4.6.0 tarball from kernel.org.

We now return to our regularly scheduled discussion...
----------------
Robert LeBlanc
PGP Fingerprint 79A2 9CA4 6CC4 45DD A904  C70E E654 3BB2 FA62 B9F1


On Mon, May 30, 2016 at 4:09 AM, Max Gurtovoy <maxg-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> wrote:
> ib_device_cap_flags 64-bit expansion caused a possible caps overlapping
> (depending on machine endianness) 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. Fix this by
> casting ib_device_cap_flags enumerations to ULL.
>
> Fixes: 45686f2d6535 ('IB/core: Add Raw Scatter FCS device capability')
> Fixes: 50174a7f2c24 ('IB/core: Add interfaces to control VF attributes')
> Fixes: f5aa9159a418 ('IB/core: Add arbitrary sg_list support')
> 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 |   66 +++++++++++++++++++++++-----------------------
>  1 files changed, 33 insertions(+), 33 deletions(-)
>
> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> index 432bed5..8bb97dc 100644
> --- a/include/rdma/ib_verbs.h
> +++ b/include/rdma/ib_verbs.h
> @@ -156,21 +156,21 @@ enum rdma_link_layer {
>  };
>
>  enum ib_device_cap_flags {
> -       IB_DEVICE_RESIZE_MAX_WR                 = (1 << 0),
> -       IB_DEVICE_BAD_PKEY_CNTR                 = (1 << 1),
> -       IB_DEVICE_BAD_QKEY_CNTR                 = (1 << 2),
> -       IB_DEVICE_RAW_MULTI                     = (1 << 3),
> -       IB_DEVICE_AUTO_PATH_MIG                 = (1 << 4),
> -       IB_DEVICE_CHANGE_PHY_PORT               = (1 << 5),
> -       IB_DEVICE_UD_AV_PORT_ENFORCE            = (1 << 6),
> -       IB_DEVICE_CURR_QP_STATE_MOD             = (1 << 7),
> -       IB_DEVICE_SHUTDOWN_PORT                 = (1 << 8),
> -       IB_DEVICE_INIT_TYPE                     = (1 << 9),
> -       IB_DEVICE_PORT_ACTIVE_EVENT             = (1 << 10),
> -       IB_DEVICE_SYS_IMAGE_GUID                = (1 << 11),
> -       IB_DEVICE_RC_RNR_NAK_GEN                = (1 << 12),
> -       IB_DEVICE_SRQ_RESIZE                    = (1 << 13),
> -       IB_DEVICE_N_NOTIFY_CQ                   = (1 << 14),
> +       IB_DEVICE_RESIZE_MAX_WR                 = (1ULL << 0),
> +       IB_DEVICE_BAD_PKEY_CNTR                 = (1ULL << 1),
> +       IB_DEVICE_BAD_QKEY_CNTR                 = (1ULL << 2),
> +       IB_DEVICE_RAW_MULTI                     = (1ULL << 3),
> +       IB_DEVICE_AUTO_PATH_MIG                 = (1ULL << 4),
> +       IB_DEVICE_CHANGE_PHY_PORT               = (1ULL << 5),
> +       IB_DEVICE_UD_AV_PORT_ENFORCE            = (1ULL << 6),
> +       IB_DEVICE_CURR_QP_STATE_MOD             = (1ULL << 7),
> +       IB_DEVICE_SHUTDOWN_PORT                 = (1ULL << 8),
> +       IB_DEVICE_INIT_TYPE                     = (1ULL << 9),
> +       IB_DEVICE_PORT_ACTIVE_EVENT             = (1ULL << 10),
> +       IB_DEVICE_SYS_IMAGE_GUID                = (1ULL << 11),
> +       IB_DEVICE_RC_RNR_NAK_GEN                = (1ULL << 12),
> +       IB_DEVICE_SRQ_RESIZE                    = (1ULL << 13),
> +       IB_DEVICE_N_NOTIFY_CQ                   = (1ULL << 14),
>
>         /*
>          * This device supports a per-device lkey or stag that can be
> @@ -179,9 +179,9 @@ enum ib_device_cap_flags {
>          * instead of use the local_dma_lkey flag in the ib_pd structure,
>          * which will always contain a usable lkey.
>          */
> -       IB_DEVICE_LOCAL_DMA_LKEY                = (1 << 15),
> -       IB_DEVICE_RESERVED /* old SEND_W_INV */ = (1 << 16),
> -       IB_DEVICE_MEM_WINDOW                    = (1 << 17),
> +       IB_DEVICE_LOCAL_DMA_LKEY                = (1ULL << 15),
> +       IB_DEVICE_RESERVED /* old SEND_W_INV */ = (1ULL << 16),
> +       IB_DEVICE_MEM_WINDOW                    = (1ULL << 17),
>         /*
>          * Devices should set IB_DEVICE_UD_IP_SUM if they support
>          * insertion of UDP and TCP checksum on outgoing UD IPoIB
> @@ -189,9 +189,9 @@ enum ib_device_cap_flags {
>          * incoming messages.  Setting this flag implies that the
>          * IPoIB driver may set NETIF_F_IP_CSUM for datagram mode.
>          */
> -       IB_DEVICE_UD_IP_CSUM                    = (1 << 18),
> -       IB_DEVICE_UD_TSO                        = (1 << 19),
> -       IB_DEVICE_XRC                           = (1 << 20),
> +       IB_DEVICE_UD_IP_CSUM                    = (1ULL << 18),
> +       IB_DEVICE_UD_TSO                        = (1ULL << 19),
> +       IB_DEVICE_XRC                           = (1ULL << 20),
>
>         /*
>          * This device supports the IB "base memory management extension",
> @@ -202,25 +202,25 @@ enum ib_device_cap_flags {
>          * IB_WR_RDMA_READ_WITH_INV verb for RDMA READs that invalidate the
>          * stag.
>          */
> -       IB_DEVICE_MEM_MGT_EXTENSIONS            = (1 << 21),
> -       IB_DEVICE_BLOCK_MULTICAST_LOOPBACK      = (1 << 22),
> -       IB_DEVICE_MEM_WINDOW_TYPE_2A            = (1 << 23),
> -       IB_DEVICE_MEM_WINDOW_TYPE_2B            = (1 << 24),
> -       IB_DEVICE_RC_IP_CSUM                    = (1 << 25),
> -       IB_DEVICE_RAW_IP_CSUM                   = (1 << 26),
> +       IB_DEVICE_MEM_MGT_EXTENSIONS            = (1ULL << 21),
> +       IB_DEVICE_BLOCK_MULTICAST_LOOPBACK      = (1ULL << 22),
> +       IB_DEVICE_MEM_WINDOW_TYPE_2A            = (1ULL << 23),
> +       IB_DEVICE_MEM_WINDOW_TYPE_2B            = (1ULL << 24),
> +       IB_DEVICE_RC_IP_CSUM                    = (1ULL << 25),
> +       IB_DEVICE_RAW_IP_CSUM                   = (1ULL << 26),
>         /*
>          * Devices should set IB_DEVICE_CROSS_CHANNEL if they
>          * support execution of WQEs that involve synchronization
>          * of I/O operations with single completion queue managed
>          * by hardware.
>          */
> -       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_CROSS_CHANNEL         = (1ULL << 27),
> +       IB_DEVICE_MANAGED_FLOW_STEERING         = (1ULL << 29),
> +       IB_DEVICE_SIGNATURE_HANDOVER            = (1ULL << 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
>
> --
> 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
--
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

Re: [PATCH] IB/core: Fix different types mix in ib_device_cap_flags structure values

[Bart Van Assche]

On 05/30/16 03:09, Max Gurtovoy wrote:
> ib_device_cap_flags 64-bit expansion caused a possible caps overlapping
> (depending on machine endianness) 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. Fix this by
> casting ib_device_cap_flags enumerations to ULL.
>
  > [ ... ]
> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> [ ... ]
>  enum ib_device_cap_flags {
  >       [ ... ]
>  	IB_DEVICE_SG_GAPS_REG			= (1ULL << 32),
>       [ ... ]
>  };

How can this patch make a difference? The presence of any constant in an 
enum that does not fit in a 32-bit integer makes an enum 64 bits wide. 
In other words, all the changes from "1" into "1ULL" in this patch do 
not have any effect. From the C standard: "Each enumerated type shall be 
compatible with char, a signed integer type, or an unsigned integer 
type. The choice of type is implementation-defined but shall be capable 
of representing the values of all the members of the enumeration. [...] 
An implementation may delay the choice of which integer type until all 
enumeration constants have been seen."

Bart.

Re: [PATCH] IB/core: Fix different types mix in ib_device_cap_flags structure values

[Max Gurtovoy]

Guys,
let me check the patch again from the beginning.

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