| Message ID | 1464602994-21226-1-git-send-email-maxg@mellanox.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@mellanox.com> 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@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 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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 > -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 {
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(-)