From fbde50e7cdd7e74b5379c4740c1dbf5f76c541eb Mon Sep 17 00:00:00 2001 From: Jeff Squyres Date: Tue, 4 Mar 2014 22:11:47 +0000 Subject: [PATCH] Fix ibv_port_query() usnic extension usage * Older versions of libusnic_verbs actually return 0 when querying for an unknown port. So also check for a magic ID in the returned data to *really* know if the usnic extensions are supported. * Use a union (in the common_verbs area) and memcpy (in the btl) to avoid undefined C type aliasing behavior. * Ensure to memset the function table to 0 if the usnic extensions are not supported. Submitted by Jeff Squyres, reviewed by Dave Goodell. cmr=v1.7.5:reviewer=ompi-rm1.7 This commit was SVN r30935. --- ompi/mca/btl/usnic/btl_usnic_ext.c | 21 ++++++++++--- ompi/mca/btl/usnic/btl_usnic_ext.h | 5 ++++ .../common/verbs/common_verbs_find_ports.c | 30 ++++++++++++++++--- 3 files changed, 48 insertions(+), 8 deletions(-) diff --git a/ompi/mca/btl/usnic/btl_usnic_ext.c b/ompi/mca/btl/usnic/btl_usnic_ext.c index 8e512599d9..cfab4eb392 100644 --- a/ompi/mca/btl/usnic/btl_usnic_ext.c +++ b/ompi/mca/btl/usnic/btl_usnic_ext.c @@ -30,11 +30,21 @@ void ompi_btl_usnic_ext_init(struct ibv_context *context) memset(&ompi_btl_usnic_ext, 0, sizeof(ompi_btl_usnic_ext)); /* See if this context supports the usnic extensions. Do the - magic query port on port number 42 (which is THE ANSWER) */ + magic query port on port number 42 (which is THE ANSWER). If + it works, we'll get rc==0 and the magic number in the struct + will be set. Note, however, that due to a bug in early + versions of libusnic_verbs, we *may* get rc==0 even if it + doesn't work, which is why we also must check for the magic + value, too. */ int rc; - rc = ibv_query_port(context, 42, - (struct ibv_port_attr*) &ompi_btl_usnic_ext.qpt); - if (0 != rc) { + struct ibv_port_attr attr; + rc = ibv_query_port(context, 42, &attr); + assert(sizeof(ompi_btl_usnic_ext) <= sizeof(attr)); + memcpy(&ompi_btl_usnic_ext, &attr, sizeof(ompi_btl_usnic_ext)); + if (0 != rc || USNIC_PORT_QUERY_MAGIC != ompi_btl_usnic_ext.qpt.magic) { + /* If the probe fails, we must re-memset() the function + pointer block */ + memset(&ompi_btl_usnic_ext, 0, sizeof(ompi_btl_usnic_ext)); opal_output_verbose(5, USNIC_OUT, "btl:usnic: verbs plugin does not support extensions"); return; @@ -50,6 +60,9 @@ void ompi_btl_usnic_ext_init(struct ibv_context *context) "btl:usnic: verbs plugin has extension lookup ABI version %d", ompi_btl_usnic_ext.qpt.lookup_version); if (1 != ompi_btl_usnic_ext.qpt.lookup_version) { + /* If the probe fails, we must re-memset() the function + pointer block, because it may/will return junk in the qpt */ + memset(&ompi_btl_usnic_ext, 0, sizeof(ompi_btl_usnic_ext)); opal_output_verbose(5, USNIC_OUT, "btl:usnic: unrecognized lookup ABI version" " (I only recognize version 1) " diff --git a/ompi/mca/btl/usnic/btl_usnic_ext.h b/ompi/mca/btl/usnic/btl_usnic_ext.h index 9dca978f47..19645588dc 100644 --- a/ompi/mca/btl/usnic/btl_usnic_ext.h +++ b/ompi/mca/btl/usnic/btl_usnic_ext.h @@ -14,13 +14,18 @@ #include +#include "opal_stdint.h" + typedef void *(*ompi_btl_usnic_dlsym_fn_t)(const char *name); typedef struct { int lookup_version; + uint64_t magic; ompi_btl_usnic_dlsym_fn_t lookup; } ompi_btl_usnic_query_port_table_t; +#define USNIC_PORT_QUERY_MAGIC (0x43494e7375534355ULL) + /* * Tells libusnic_verbs to enable UDP support. */ diff --git a/ompi/mca/common/verbs/common_verbs_find_ports.c b/ompi/mca/common/verbs/common_verbs_find_ports.c index e141fdd0b1..bc27cdfd6e 100644 --- a/ompi/mca/common/verbs/common_verbs_find_ports.c +++ b/ompi/mca/common/verbs/common_verbs_find_ports.c @@ -232,6 +232,20 @@ enum { USNIC_UNKNOWN }; +/* See comment in btl_usnic_ext.c about why we must check the return + from the usnic verbs extensions probe for a magic number (which + means we must also copy the usnic extension struct and magic number + value down here into common/verbs. Bummer). */ +typedef union { + struct { + int lookup_version; + uint64_t magic; + } qpt; + struct ibv_port_attr attr; +} port_query_u; + +#define USNIC_PORT_QUERY_MAGIC (0x43494e7375534355ULL) + /* * usNIC devices will always return one of three * device->transport_type values: @@ -269,10 +283,18 @@ static int usnic_transport(struct ibv_device *device, here. */ if (IBV_TRANSPORT_IWARP == device->transport_type) { int rc; - struct ibv_port_attr attr; - rc = ibv_query_port(context, 42, &attr); - if (0 == rc) { - return USNIC_UDP; + port_query_u u; + rc = ibv_query_port(context, 42, &u.attr); + /* See comment in btl_usnic_ext.c about why we have to check + for rc==0 *and* the magic number. */ + if (0 == rc && USNIC_PORT_QUERY_MAGIC == u.qpt.magic) { + /* We only support version 1 of the lookup function in + this particular version of Open MPI */ + if (1 == u.qpt.lookup_version) { + return USNIC_UDP; + } else { + return USNIC_UNKNOWN; + } } else { return USNIC_L2; }