[PATCH V2 0/5] Mkey support in infiniband-diags

[PATCH V2 0/5] Mkey support in infiniband-diags

[Jim Foraker]

     I am about to send version 2 of the mkey patches for
infiniband-diags to the list.  Notable changes are:

. both current mkey and new mkey (for ibportstate) may now be prompted
for in addition to specified on the command line (ie, identical to
saquery smkey support)

. mkey support touches (sadly, just a few) fewer files, due to SMP
handling cleanups committed earlier

. saquery's smkey handling has been fixed to allow the return of
authenticated SA data (this worked for only a few record types before)

. config file chmod and mkey option patches have been split out

     I think this covers most/all of the comments received from V1, with
the exception of the discussions surrounding multiple-mkey policies.
Given that the diags likely should support operations with a
single/specified mkey regardless of subnet policy, I think these patches
get us closer to that goal without standing in the way of future
discussions on how to get there.

     Jim

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

[PATCH V2 1/5] infiniband-diags/ibportstate.c: Support MKey, lease, and protect bits

[Jim Foraker]

Allow user to both display and change mkey-related values.

Signed-off-by: Jim Foraker <foraker1-i2BcT+NCU+M@public.gmane.org>
---
 src/ibportstate.c |   98 ++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 79 insertions(+), 19 deletions(-)

diff --git a/src/ibportstate.c b/src/ibportstate.c
index 5559d18..21cd431 100644
--- a/src/ibportstate.c
+++ b/src/ibportstate.c
@@ -41,6 +41,7 @@
 #include <unistd.h>
 #include <string.h>
 #include <getopt.h>
+#include <errno.h>
 
 #include <infiniband/umad.h>
 #include <infiniband/mad.h>
@@ -64,22 +65,28 @@ enum port_ops {
 	LID,
 	SMLID,
 	LMC,
+	MKEY,
+	MKEYLEASE,
+	MKEYPROT,
 };
 
 struct ibmad_port *srcport;
-int speed = 0; /* no state change */
-int espeed = 0; /* no state change */
-int fdr10 = 0; /* no state change */
-int width = 0; /* no state change */
-int lid;
-int smlid;
-int lmc;
-int mtu;
-int vls = 0; /* no state change */
+uint64_t speed = 0; /* no state change */
+uint64_t espeed = 0; /* no state change */
+uint64_t fdr10 = 0; /* no state change */
+uint64_t width = 0; /* no state change */
+uint64_t lid;
+uint64_t smlid;
+uint64_t lmc;
+uint64_t mtu;
+uint64_t vls = 0; /* no state change */
+uint64_t mkey;
+uint64_t mkeylease;
+uint64_t mkeyprot;
 
 struct {
 	const char *name;
-	int *val;
+	uint64_t *val;
 	int set;
 } port_args[] = {
 	{"query", NULL, 0},	/* QUERY */
@@ -98,6 +105,9 @@ struct {
 	{"lid", &lid, 0},	/* LID */
 	{"smlid", &smlid, 0},	/* SMLID */
 	{"lmc", &lmc, 0},	/* LMC */
+	{"mkey", &mkey, 0},	/* MKEY */
+	{"mkeylease", &mkeylease, 0},	/* MKEY LEASE */
+	{"mkeyprot", &mkeyprot, 0},	/* MKEY PROTECT BITS */
 };
 
 #define NPORT_ARGS (sizeof(port_args) / sizeof(port_args[0]))
@@ -142,7 +152,7 @@ static int get_port_info(ib_portid_t * dest, uint8_t * data, int portnum,
 }
 
 static void show_port_info(ib_portid_t * dest, uint8_t * data, int portnum,
-			   int espeed_cap)
+			   int espeed_cap, int is_switch)
 {
 	char buf[2300];
 	char val[64];
@@ -201,18 +211,32 @@ static void show_port_info(ib_portid_t * dest, uint8_t * data, int portnum,
 			       val);
 		sprintf(buf + strlen(buf), "%s", "\n");
 	}
+	if (!is_switch || portnum == 0) {
+		mad_decode_field(data, IB_PORT_MKEY_F, val);
+		mad_dump_field(IB_PORT_MKEY_F, buf + strlen(buf),
+			       sizeof buf - strlen(buf), val);
+		sprintf(buf+strlen(buf), "%s", "\n");
+		mad_decode_field(data, IB_PORT_MKEY_LEASE_F, val);
+		mad_dump_field(IB_PORT_MKEY_LEASE_F, buf + strlen(buf),
+			       sizeof buf - strlen(buf), val);
+		sprintf(buf+strlen(buf), "%s", "\n");
+		mad_decode_field(data, IB_PORT_MKEY_PROT_BITS_F, val);
+		mad_dump_field(IB_PORT_MKEY_PROT_BITS_F, buf + strlen(buf),
+			       sizeof buf - strlen(buf), val);
+		sprintf(buf+strlen(buf), "%s", "\n");
+	}
 
 	printf("# Port info: %s port %d\n%s", portid2str(dest), portnum, buf);
 }
 
 static void set_port_info(ib_portid_t * dest, uint8_t * data, int portnum,
-			  int espeed_cap)
+			  int espeed_cap, int is_switch)
 {
 	if (!smp_set_via(data, dest, IB_ATTR_PORT_INFO, portnum, 0, srcport))
 		IBERROR("smp set portinfo failed");
 
 	printf("\nAfter PortInfo set:\n");
-	show_port_info(dest, data, portnum, espeed_cap);
+	show_port_info(dest, data, portnum, espeed_cap, is_switch);
 }
 
 static void get_ext_port_info(ib_portid_t * dest, uint8_t * data, int portnum)
@@ -349,9 +373,10 @@ int main(int argc, char **argv)
 	int i;
 	uint16_t devid, rem_devid;
 	long val;
+	char *endp;
 	char usage_args[] = "<dest dr_path|lid|guid> <portnum> [<op>]\n"
 	    "\nSupported ops: enable, disable, reset, speed, width, query,\n"
-	    "\tdown, arm, active, vls, mtu, lid, smlid, lmc\n";
+	    "\tdown, arm, active, vls, mtu, lid, smlid, lmc, mkey, mkeylease, mkeyprot\n";
 	const char *usage_examples[] = {
 		"3 1 disable\t\t\t# by lid",
 		"-G 0x2C9000100D051 1 enable\t# by guid",
@@ -403,7 +428,7 @@ int main(int argc, char **argv)
 			if (++i >= argc)
 				IBERROR("%s requires an additional parameter",
 					port_args[j].name);
-			val = strtol(argv[i], 0, 0);
+			val = strtoull(argv[i], 0, 0);
 			switch (j) {
 			case SPEED:
 				if (val < 0 || val > 15)
@@ -441,8 +466,29 @@ int main(int argc, char **argv)
 			case LMC:
 				if (val < 0 || val > 7)
 					IBERROR("invalid lmc value %ld", val);
+				break;
+			case MKEY:
+				errno = 0;
+				val = strtoull(argv[i], &endp, 0);
+				if (errno || *endp != '\0') {
+					errno = 0;
+					val = strtoull(getpass("New M_Key: "),
+						       &endp, 0);
+					if (errno || *endp != '\0') {
+						IBERROR("Bad new M_Key\n");
+					}
+				}
+				/* All 64-bit values are legal */
+				break;
+			case MKEYLEASE:
+				if (val < 0 || val > 0xFFFF)
+					IBERROR("invalid mkey lease time %ld", val);
+				break;
+			case MKEYPROT:
+				if (val < 0 || val > 3)
+					IBERROR("invalid mkey protection bit setting %ld", val);
 			}
-			*port_args[j].val = (int)val;
+			*port_args[j].val = val;
 			changed = 1;
 			break;
 		}
@@ -455,12 +501,16 @@ int main(int argc, char **argv)
 	is_switch = get_node_info(&portid, data);
 	devid = (uint16_t) mad_get_field(data, 0, IB_NODE_DEVID_F);
 
+	if ((port_args[MKEY].set || port_args[MKEYLEASE].set ||
+	     port_args[MKEYPROT].set) && is_switch && portnum != 0)
+		IBERROR("Can't set M_Key fields on switch port != 0");
+
 	if (port_op != QUERY || changed)
 		printf("Initial %s PortInfo:\n", is_switch ? "Switch" : "CA");
 	else
 		printf("%s PortInfo:\n", is_switch ? "Switch" : "CA");
 	espeed_cap = get_port_info(&portid, data, portnum, is_switch);
-	show_port_info(&portid, data, portnum, espeed_cap);
+	show_port_info(&portid, data, portnum, espeed_cap, is_switch);
 	if (is_mlnx_ext_port_info_supported(devid)) {
 		get_ext_port_info(&portid, data2, portnum);
 		show_ext_port_info(&portid, data2, portnum);
@@ -527,7 +577,17 @@ int main(int argc, char **argv)
 				      fdr10);
 			set_ext_port_info(&portid, data2, portnum);
 		}
-		set_port_info(&portid, data, portnum, is_switch);
+
+		if (port_args[MKEY].set)
+			mad_set_field64(data, 0, IB_PORT_MKEY_F, mkey);
+		if (port_args[MKEYLEASE].set)
+			mad_set_field(data, 0, IB_PORT_MKEY_LEASE_F,
+				      mkeylease);
+		if (port_args[MKEYPROT].set)
+			mad_set_field(data, 0, IB_PORT_MKEY_PROT_BITS_F,
+				      mkeyprot);
+
+		set_port_info(&portid, data, portnum, espeed_cap, is_switch);
 
 	} else if (is_switch && portnum) {
 		/* Now, make sure PortState is Active */
@@ -596,7 +656,7 @@ int main(int argc, char **argv)
 				get_ext_port_info(&peerportid, data2,
 						  peerlocalportnum);
 			show_port_info(&peerportid, data, peerlocalportnum,
-				       peer_espeed_cap);
+				       peer_espeed_cap, is_peer_switch);
 			if (is_mlnx_ext_port_info_supported(rem_devid))
 				show_ext_port_info(&peerportid, data2,
 						   peerlocalportnum);
-- 
1.7.9.2

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

[PATCH V2 2/5] infiniband-diags: Allow specification of an mkey to use on the command line

[Jim Foraker]

Signed-off-by: Jim Foraker <foraker1-i2BcT+NCU+M@public.gmane.org>
---
 include/ibdiag_common.h                     |    1 +
 libibnetdisc/include/infiniband/ibnetdisc.h |    3 ++-
 libibnetdisc/src/ibnetdisc.c                |    1 +
 man/ibaddr.8                                |    2 ++
 src/ibaddr.c                                |    2 ++
 src/ibccconfig.c                            |    2 ++
 src/ibccquery.c                             |    2 ++
 src/ibdiag_common.c                         |   13 +++++++++++++
 src/iblinkinfo.c                            |    3 +++
 src/ibportstate.c                           |    2 ++
 src/ibqueryerrors.c                         |    3 +++
 src/ibroute.c                               |    2 ++
 src/ibsendtrap.c                            |    2 ++
 src/ibtracert.c                             |    2 ++
 src/perfquery.c                             |    2 ++
 src/sminfo.c                                |    2 ++
 src/smpquery.c                              |    2 ++
 17 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/include/ibdiag_common.h b/include/ibdiag_common.h
index 09dc319..409abdb 100644
--- a/include/ibdiag_common.h
+++ b/include/ibdiag_common.h
@@ -50,6 +50,7 @@ extern enum MAD_DEST ibd_dest_type;
 extern ib_portid_t *ibd_sm_id;
 extern int ibd_timeout;
 extern uint32_t ibd_ibnetdisc_flags;
+extern uint64_t ibd_mkey;
 
 /*========================================================*/
 /*                External interface                      */
diff --git a/libibnetdisc/include/infiniband/ibnetdisc.h b/libibnetdisc/include/infiniband/ibnetdisc.h
index 2ae2b06..335ad83 100644
--- a/libibnetdisc/include/infiniband/ibnetdisc.h
+++ b/libibnetdisc/include/infiniband/ibnetdisc.h
@@ -151,7 +151,8 @@ typedef struct ibnd_config {
 	unsigned timeout_ms;
 	unsigned retries;
 	uint32_t flags;
-	uint8_t pad[52];
+	uint64_t mkey;
+	uint8_t pad[44];
 } ibnd_config_t;
 
 /** =========================================================================
diff --git a/libibnetdisc/src/ibnetdisc.c b/libibnetdisc/src/ibnetdisc.c
index 8d38ab7..a0cbe3e 100644
--- a/libibnetdisc/src/ibnetdisc.c
+++ b/libibnetdisc/src/ibnetdisc.c
@@ -711,6 +711,7 @@ ibnd_fabric_t *ibnd_discover_fabric(char * ca_name, int ca_port,
 	}
 	mad_rpc_set_timeout(scan.ibmad_port, cfg->timeout_ms);
 	mad_rpc_set_retries(scan.ibmad_port, cfg->retries);
+	smp_mkey_set(scan.ibmad_port, cfg->mkey);
 
 	IBND_DEBUG("from %s\n", portid2str(from));
 
diff --git a/man/ibaddr.8 b/man/ibaddr.8
index 51f5a1c..86ede0a 100644
--- a/man/ibaddr.8
+++ b/man/ibaddr.8
@@ -73,6 +73,8 @@ using the util_name -h syntax.
 \-P <ca_port>    use the specified ca_port.
 .PP
 \-t <timeout_ms> override the default timeout for the solicited mads.
+.PP
+\-m <M_Key> Use specified M_Key (default 0)
 
 Multiple CA/Multiple Port Support
 
diff --git a/src/ibaddr.c b/src/ibaddr.c
index 455d941..dc50046 100644
--- a/src/ibaddr.c
+++ b/src/ibaddr.c
@@ -149,6 +149,8 @@ int main(int argc, char **argv)
 	if (!srcport)
 		IBERROR("Failed to open '%s' port '%d'", ibd_ca, ibd_ca_port);
 
+	smp_mkey_set(srcport, ibd_mkey);
+
 	if (argc) {
 		if (resolve_portid_str(ibd_ca, ibd_ca_port, &portid, argv[0],
 				       ibd_dest_type, ibd_sm_id, srcport) < 0)
diff --git a/src/ibccconfig.c b/src/ibccconfig.c
index 71d408a..e3a10ae 100644
--- a/src/ibccconfig.c
+++ b/src/ibccconfig.c
@@ -636,6 +636,8 @@ int main(int argc, char **argv)
 	if (!srcport)
 		IBERROR("Failed to open '%s' port '%d'", ibd_ca, ibd_ca_port);
 
+	smp_mkey_set(srcport, ibd_mkey);
+
 	if (resolve_portid_str(ibd_ca, ibd_ca_port, &portid, argv[1],
 			       ibd_dest_type, ibd_sm_id, srcport) < 0)
 		IBERROR("can't resolve destination %s", argv[1]);
diff --git a/src/ibccquery.c b/src/ibccquery.c
index 2bf62fa..e7cae5a 100644
--- a/src/ibccquery.c
+++ b/src/ibccquery.c
@@ -416,6 +416,8 @@ int main(int argc, char **argv)
 	if (!srcport)
 		IBERROR("Failed to open '%s' port '%d'", ibd_ca, ibd_ca_port);
 
+	smp_mkey_set(srcport, ibd_mkey);
+
 	if (resolve_portid_str(ibd_ca, ibd_ca_port, &portid, argv[1],
 			       ibd_dest_type, ibd_sm_id, srcport) < 0)
 		IBERROR("can't resolve destination %s", argv[1]);
diff --git a/src/ibdiag_common.c b/src/ibdiag_common.c
index 5752f95..2288ab3 100644
--- a/src/ibdiag_common.c
+++ b/src/ibdiag_common.c
@@ -70,6 +70,7 @@ char *ibd_ca = NULL;
 int ibd_ca_port = 0;
 int ibd_timeout = 0;
 uint32_t ibd_ibnetdisc_flags = IBND_CONFIG_MLX_EPI;
+uint64_t ibd_mkey;
 
 static const char *prog_name;
 static const char *prog_args;
@@ -261,6 +262,17 @@ static int process_opt(int ch, char *optarg)
 				optarg);
 		ibd_sm_id = &sm_portid;
 		break;
+	case 'm':
+		errno = 0;
+		ibd_mkey = strtoull(optarg, &endp, 0);
+		if (errno || *endp != '\0') {
+			errno = 0;
+			ibd_mkey = strtoull(getpass("M_Key: "), &endp, 0);
+			if (errno || *endp != '\0') {
+				IBERROR("Bad M_Key");
+			}
+                }
+                break;
 	default:
 		return -1;
 	}
@@ -277,6 +289,7 @@ static const struct ibdiag_opt common_opts[] = {
 	{"Guid", 'G', 0, NULL, "use GUID address argument"},
 	{"timeout", 't', 1, "<ms>", "timeout in ms"},
 	{"sm_port", 's', 1, "<lid>", "SM port lid"},
+	{"m_key", 'm', 1, "<key>", "M_Key to use in request"},
 	{"errors", 'e', 0, NULL, "show send and receive errors"},
 	{"verbose", 'v', 0, NULL, "increase verbosity level"},
 	{"debug", 'd', 0, NULL, "raise debug level"},
diff --git a/src/iblinkinfo.c b/src/iblinkinfo.c
index 36aa6d1..acaab54 100644
--- a/src/iblinkinfo.c
+++ b/src/iblinkinfo.c
@@ -632,12 +632,15 @@ int main(int argc, char **argv)
 		exit(1);
 	}
 
+	smp_mkey_set(ibmad_port, ibd_mkey);
+
 	if (ibd_timeout) {
 		mad_rpc_set_timeout(ibmad_port, ibd_timeout);
 		config.timeout_ms = ibd_timeout;
 	}
 
 	config.flags = ibd_ibnetdisc_flags;
+	config.mkey = ibd_mkey;
 
 	node_name_map = open_node_name_map(node_name_map_file);
 
diff --git a/src/ibportstate.c b/src/ibportstate.c
index 21cd431..c7d89f3 100644
--- a/src/ibportstate.c
+++ b/src/ibportstate.c
@@ -401,6 +401,8 @@ int main(int argc, char **argv)
 	if (!srcport)
 		IBERROR("Failed to open '%s' port '%d'", ibd_ca, ibd_ca_port);
 
+	smp_mkey_set(srcport, ibd_mkey);
+
 	if (resolve_portid_str(ibd_ca, ibd_ca_port, &portid, argv[0],
 			       ibd_dest_type, ibd_sm_id, srcport) < 0)
 		IBERROR("can't resolve destination port %s", argv[0]);
diff --git a/src/ibqueryerrors.c b/src/ibqueryerrors.c
index 4fec240..77c57fc 100644
--- a/src/ibqueryerrors.c
+++ b/src/ibqueryerrors.c
@@ -914,12 +914,15 @@ int main(int argc, char **argv)
 	if (!ibmad_port)
 		IBERROR("Failed to open port; %s:%d\n", ibd_ca, ibd_ca_port);
 
+	smp_mkey_set(ibmad_port, ibd_mkey);
+
 	if (ibd_timeout) {
 		mad_rpc_set_timeout(ibmad_port, ibd_timeout);
 		config.timeout_ms = ibd_timeout;
 	}
 
 	config.flags = ibd_ibnetdisc_flags;
+	config.mkey = ibd_mkey;
 
 	node_name_map = open_node_name_map(node_name_map_file);
 
diff --git a/src/ibroute.c b/src/ibroute.c
index faff34d..b8dcc54 100644
--- a/src/ibroute.c
+++ b/src/ibroute.c
@@ -432,6 +432,8 @@ int main(int argc, char **argv)
 	if (!srcport)
 		IBERROR("Failed to open '%s' port '%d'", ibd_ca, ibd_ca_port);
 
+	smp_mkey_set(srcport, ibd_mkey);
+
 	if (resolve_portid_str(ibd_ca, ibd_ca_port, &portid, argv[0],
 			       ibd_dest_type, ibd_sm_id, srcport) < 0)
 		IBERROR("can't resolve destination port %s", argv[1]);
diff --git a/src/ibsendtrap.c b/src/ibsendtrap.c
index 2d63e35..b9bf5c0 100644
--- a/src/ibsendtrap.c
+++ b/src/ibsendtrap.c
@@ -209,6 +209,8 @@ int main(int argc, char **argv)
 	if (!srcport)
 		IBERROR("Failed to open '%s' port '%d'", ibd_ca, ibd_ca_port);
 
+	smp_mkey_set(srcport, ibd_mkey);
+
 	rc = process_send_trap(trap_name);
 	mad_rpc_close_port(srcport);
 	return rc;
diff --git a/src/ibtracert.c b/src/ibtracert.c
index e8fedf3..32aba19 100644
--- a/src/ibtracert.c
+++ b/src/ibtracert.c
@@ -774,6 +774,8 @@ int main(int argc, char **argv)
 	if (!srcport)
 		IBERROR("Failed to open '%s' port '%d'", ibd_ca, ibd_ca_port);
 
+	smp_mkey_set(srcport, ibd_mkey);
+
 	node_name_map = open_node_name_map(node_name_map_file);
 
 	if (resolve_portid_str(ibd_ca, ibd_ca_port, &src_portid, argv[0],
diff --git a/src/perfquery.c b/src/perfquery.c
index 8835d3d..388d04a 100644
--- a/src/perfquery.c
+++ b/src/perfquery.c
@@ -729,6 +729,8 @@ int main(int argc, char **argv)
 	if (!srcport)
 		IBERROR("Failed to open '%s' port '%d'", ibd_ca, ibd_ca_port);
 
+	smp_mkey_set(srcport, ibd_mkey);
+
 	if (argc) {
 		if (resolve_portid_str(ibd_ca, ibd_ca_port, &portid, argv[0],
 				       ibd_dest_type, ibd_sm_id, srcport) < 0)
diff --git a/src/sminfo.c b/src/sminfo.c
index f1abc6a..0f723d0 100644
--- a/src/sminfo.c
+++ b/src/sminfo.c
@@ -122,6 +122,8 @@ int main(int argc, char **argv)
 	if (!srcport)
 		IBERROR("Failed to open '%s' port '%d'", ibd_ca, ibd_ca_port);
 
+	smp_mkey_set(srcport, ibd_mkey);
+
 	if (argc) {
 		if (resolve_portid_str(ibd_ca, ibd_ca_port, &portid, argv[0],
 				       ibd_dest_type, 0, srcport) < 0)
diff --git a/src/smpquery.c b/src/smpquery.c
index 533b2c3..2ef2086 100644
--- a/src/smpquery.c
+++ b/src/smpquery.c
@@ -480,6 +480,8 @@ int main(int argc, char **argv)
 	if (!srcport)
 		IBERROR("Failed to open '%s' port '%d'", ibd_ca, ibd_ca_port);
 
+	smp_mkey_set(srcport, ibd_mkey);
+
 	node_name_map = open_node_name_map(node_name_map_file);
 
 	if (ibd_dest_type != IB_DEST_DRSLID) {
-- 
1.7.9.2

--
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 V2 2/5] infiniband-diags: Allow specification of an mkey to use on the command line

[Hal Rosenstock]

On 4/23/2012 8:56 PM, Jim Foraker wrote:
> Signed-off-by: Jim Foraker <foraker1-i2BcT+NCU+M@public.gmane.org>
> ---
>  include/ibdiag_common.h                     |    1 +
>  libibnetdisc/include/infiniband/ibnetdisc.h |    3 ++-
>  libibnetdisc/src/ibnetdisc.c                |    1 +
>  man/ibaddr.8                                |    2 ++
>  src/ibaddr.c                                |    2 ++
>  src/ibccconfig.c                            |    2 ++
>  src/ibccquery.c                             |    2 ++
>  src/ibdiag_common.c                         |   13 +++++++++++++
>  src/iblinkinfo.c                            |    3 +++
>  src/ibportstate.c                           |    2 ++
>  src/ibqueryerrors.c                         |    3 +++
>  src/ibroute.c                               |    2 ++
>  src/ibsendtrap.c                            |    2 ++
>  src/ibtracert.c                             |    2 ++
>  src/perfquery.c                             |    2 ++
>  src/sminfo.c                                |    2 ++
>  src/smpquery.c                              |    2 ++
>  17 files changed, 45 insertions(+), 1 deletion(-)

Why is only the ibaddr man page changed ? What about the man pages for
all the other diags included here ? Shouldn't they be changed similarly
? Or maybe there should be an ibdiag_common man page and all the man
pages should reference that ?

-- Hal

<snip...>
--
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 V2 2/5] infiniband-diags: Allow specification of an mkey to use on the command line

[Ira Weiny]

On Tue, 24 Apr 2012 10:17:21 -0400
Hal Rosenstock <hal-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org> wrote:

> On 4/23/2012 8:56 PM, Jim Foraker wrote:
> > Signed-off-by: Jim Foraker <foraker1-i2BcT+NCU+M@public.gmane.org>
> > ---
> >  include/ibdiag_common.h                     |    1 +
> >  libibnetdisc/include/infiniband/ibnetdisc.h |    3 ++-
> >  libibnetdisc/src/ibnetdisc.c                |    1 +
> >  man/ibaddr.8                                |    2 ++
> >  src/ibaddr.c                                |    2 ++
> >  src/ibccconfig.c                            |    2 ++
> >  src/ibccquery.c                             |    2 ++
> >  src/ibdiag_common.c                         |   13 +++++++++++++
> >  src/iblinkinfo.c                            |    3 +++
> >  src/ibportstate.c                           |    2 ++
> >  src/ibqueryerrors.c                         |    3 +++
> >  src/ibroute.c                               |    2 ++
> >  src/ibsendtrap.c                            |    2 ++
> >  src/ibtracert.c                             |    2 ++
> >  src/perfquery.c                             |    2 ++
> >  src/sminfo.c                                |    2 ++
> >  src/smpquery.c                              |    2 ++
> >  17 files changed, 45 insertions(+), 1 deletion(-)
> 
> Why is only the ibaddr man page changed ? What about the man pages for
> all the other diags included here ? Shouldn't they be changed similarly
> ? Or maybe there should be an ibdiag_common man page and all the man
> pages should reference that ?

Before the next release it is on my list to refactor all the man pages and make sure they are up to date.

That said yes we should update the commands which support mkey for now.

Ira

> 
> -- Hal
> 
> <snip...>


-- 
Ira Weiny
Member of Technical Staff
Lawrence Livermore National Lab
925-423-8008
weiny2-i2BcT+NCU+M@public.gmane.org
--
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

[PATCH V2 3/5] ib-diags/saquery: Fix smkey handling

[Jim Foraker]

smkey is already defined as a global inside saquery.c, so remove
broken support for passing it around as a function parameter

Signed-off-by: Jim Foraker <foraker1-i2BcT+NCU+M@public.gmane.org>
---
 src/saquery.c |   59 ++++++++++++++++++++++++++++-----------------------------
 1 file changed, 29 insertions(+), 30 deletions(-)

diff --git a/src/saquery.c b/src/saquery.c
index e5fdb25..029228c 100644
--- a/src/saquery.c
+++ b/src/saquery.c
@@ -85,7 +85,7 @@ struct query_cmd {
 
 static char *node_name_map_file = NULL;
 static nn_map_t *node_name_map = NULL;
-static uint64_t smkey = 1;
+static uint64_t smkey = 0;
 
 /**
  * Declare some globals because I don't want this to be too complex.
@@ -726,11 +726,11 @@ static void dump_results(struct sa_query_result *r, void (*dump_func) (void *))
  */
 static int get_any_records(bind_handle_t h,
 			   uint16_t attr_id, uint32_t attr_mod,
-			   ib_net64_t comp_mask, void *attr, uint64_t sm_key,
+			   ib_net64_t comp_mask, void *attr,
 			   struct sa_query_result *result)
 {
 	int ret = sa_query(h, IB_MAD_METHOD_GET_TABLE, attr_id, attr_mod,
-			   cl_ntoh64(comp_mask), sm_key, attr, result);
+			   cl_ntoh64(comp_mask), smkey, attr, result);
 	if (ret) {
 		fprintf(stderr, "Query SA failed: %s\n", strerror(ret));
 		return ret;
@@ -746,12 +746,12 @@ static int get_any_records(bind_handle_t h,
 
 static int get_and_dump_any_records(bind_handle_t h, uint16_t attr_id,
 				    uint32_t attr_mod, ib_net64_t comp_mask,
-				    void *attr, uint64_t sm_key,
+				    void *attr,
 				    void (*dump_func) (void *))
 {
 	struct sa_query_result result;
 	int ret = get_any_records(h, attr_id, attr_mod, comp_mask, attr,
-				  sm_key, &result);
+				  &result);
 	if (ret)
 		return ret;
 
@@ -763,18 +763,17 @@ static int get_and_dump_any_records(bind_handle_t h, uint16_t attr_id,
 /**
  * Get all the records available for requested query type.
  */
-static int get_all_records(bind_handle_t h, uint16_t attr_id, int trusted,
+static int get_all_records(bind_handle_t h, uint16_t attr_id,
 			   struct sa_query_result *result)
 {
-	return get_any_records(h, attr_id, 0, 0, NULL, trusted ? smkey : 0,
-			       result);
+	return get_any_records(h, attr_id, 0, 0, NULL, result);
 }
 
 static int get_and_dump_all_records(bind_handle_t h, uint16_t attr_id,
-				    int trusted, void (*dump_func) (void *))
+				    void (*dump_func) (void *))
 {
 	struct sa_query_result result;
-	int ret = get_all_records(h, attr_id, 0, &result);
+	int ret = get_all_records(h, attr_id, &result);
 	if (ret)
 		return ret;
 
@@ -794,7 +793,7 @@ static int get_lid_from_name(bind_handle_t h, const char *name, uint16_t * lid)
 	int ret;
 	struct sa_query_result result;
 
-	ret = get_all_records(h, IB_SA_ATTR_NODERECORD, 0, &result);
+	ret = get_all_records(h, IB_SA_ATTR_NODERECORD, &result);
 	if (ret)
 		return ret;
 
@@ -894,7 +893,7 @@ static int get_issm_records(bind_handle_t h, ib_net32_t capability_mask,
 	attr.port_info.capability_mask = capability_mask;
 
 	return get_any_records(h, IB_SA_ATTR_PORTINFORECORD, 1 << 31,
-			       IB_PIR_COMPMASK_CAPMASK, &attr, 0, result);
+			       IB_PIR_COMPMASK_CAPMASK, &attr, result);
 }
 
 static int print_node_records(bind_handle_t h)
@@ -903,7 +902,7 @@ static int print_node_records(bind_handle_t h)
 	int ret;
 	struct sa_query_result result;
 
-	ret = get_all_records(h, IB_SA_ATTR_NODERECORD, 0, &result);
+	ret = get_all_records(h, IB_SA_ATTR_NODERECORD, &result);
 	if (ret)
 		return ret;
 
@@ -946,7 +945,7 @@ static int get_print_class_port_info(bind_handle_t h)
 {
 	struct sa_query_result result;
 	int ret = sa_query(h, IB_MAD_METHOD_GET, CLASS_PORT_INFO, 0, 0,
-			   0, NULL, &result);
+			   smkey, NULL, &result);
 	if (ret) {
 		fprintf(stderr, "ERROR: Query SA failed: %s\n", strerror(ret));
 		return ret;
@@ -993,7 +992,7 @@ static int query_path_records(const struct query_cmd *q, bind_handle_t h,
 				  SELEC);
 
 	return get_and_dump_any_records(h, IB_SA_ATTR_PATHRECORD, 0, comp_mask,
-					&pr, 0, dump_path_record);
+					&pr, dump_path_record);
 }
 
 static int print_issm_records(bind_handle_t h)
@@ -1029,11 +1028,11 @@ static int print_multicast_member_records(bind_handle_t h)
 	int ret;
 	unsigned i;
 
-	ret = get_all_records(h, IB_SA_ATTR_MCRECORD, 1, &mc_group_result);
+	ret = get_all_records(h, IB_SA_ATTR_MCRECORD, &mc_group_result);
 	if (ret)
 		return ret;
 
-	ret = get_all_records(h, IB_SA_ATTR_NODERECORD, 0, &nr_result);
+	ret = get_all_records(h, IB_SA_ATTR_NODERECORD, &nr_result);
 	if (ret)
 		goto return_mc;
 
@@ -1054,7 +1053,7 @@ return_mc:
 
 static int print_multicast_group_records(bind_handle_t h)
 {
-	return get_and_dump_all_records(h, IB_SA_ATTR_MCRECORD, 0,
+	return get_and_dump_all_records(h, IB_SA_ATTR_MCRECORD,
 					dump_multicast_group_record);
 }
 
@@ -1078,7 +1077,7 @@ static int query_node_records(const struct query_cmd *q, bind_handle_t h,
 	CHECK_AND_SET_VAL(lid, 16, 0, nr.lid, NR, LID);
 
 	return get_and_dump_any_records(h, IB_SA_ATTR_NODERECORD, 0, comp_mask,
-					&nr, 0, dump_node_record);
+					&nr, dump_node_record);
 }
 
 static int query_portinfo_records(const struct query_cmd *q,
@@ -1098,7 +1097,7 @@ static int query_portinfo_records(const struct query_cmd *q,
 	CHECK_AND_SET_VAL(options, 8, -1, pir.options, PIR, OPTIONS);
 
 	return get_and_dump_any_records(h, IB_SA_ATTR_PORTINFORECORD, 0,
-					comp_mask, &pir, 0,
+					comp_mask, &pir,
 					dump_one_portinfo_record);
 }
 
@@ -1131,13 +1130,13 @@ static int query_mcmember_records(const struct query_cmd *q,
 	CHECK_AND_SET_VAL(p->proxy_join, 8, -1, mr.proxy_join, MCR, PROXY);
 
 	return get_and_dump_any_records(h, IB_SA_ATTR_MCRECORD, 0, comp_mask,
-					&mr, smkey, dump_one_mcmember_record);
+					&mr, dump_one_mcmember_record);
 }
 
 static int query_service_records(const struct query_cmd *q, bind_handle_t h,
 				 struct query_params *p, int argc, char *argv[])
 {
-	return get_and_dump_all_records(h, IB_SA_ATTR_SERVICERECORD, 0,
+	return get_and_dump_all_records(h, IB_SA_ATTR_SERVICERECORD,
 					dump_service_record);
 }
 
@@ -1145,7 +1144,7 @@ static int query_informinfo_records(const struct query_cmd *q,
 				    bind_handle_t h, struct query_params *p,
 				    int argc, char *argv[])
 {
-	return get_and_dump_all_records(h, IB_SA_ATTR_INFORMINFORECORD, 0,
+	return get_and_dump_all_records(h, IB_SA_ATTR_INFORMINFORECORD,
 					dump_inform_info_record);
 }
 
@@ -1169,7 +1168,7 @@ static int query_link_records(const struct query_cmd *q, bind_handle_t h,
 	CHECK_AND_SET_VAL(to_port, 8, -1, lr.to_port_num, LR, TO_PORT);
 
 	return get_and_dump_any_records(h, IB_SA_ATTR_LINKRECORD, 0, comp_mask,
-					&lr, 0, dump_one_link_record);
+					&lr, dump_one_link_record);
 }
 
 static int query_sl2vl_records(const struct query_cmd *q, bind_handle_t h,
@@ -1188,7 +1187,7 @@ static int query_sl2vl_records(const struct query_cmd *q, bind_handle_t h,
 	CHECK_AND_SET_VAL(out_port, 8, -1, slvl.out_port_num, SLVL, OUT_PORT);
 
 	return get_and_dump_any_records(h, IB_SA_ATTR_SL2VLTABLERECORD, 0,
-					comp_mask, &slvl, 0,
+					comp_mask, &slvl,
 					dump_one_slvl_record);
 }
 
@@ -1208,7 +1207,7 @@ static int query_vlarb_records(const struct query_cmd *q, bind_handle_t h,
 	CHECK_AND_SET_VAL(block, 8, -1, vlarb.block_num, VLA, BLOCK);
 
 	return get_and_dump_any_records(h, IB_SA_ATTR_VLARBTABLERECORD, 0,
-					comp_mask, &vlarb, 0,
+					comp_mask, &vlarb,
 					dump_one_vlarb_record);
 }
 
@@ -1229,7 +1228,7 @@ static int query_pkey_tbl_records(const struct query_cmd *q,
 	CHECK_AND_SET_VAL(block, 16, -1, pktr.block_num, PKEY, BLOCK);
 
 	return get_and_dump_any_records(h, IB_SA_ATTR_PKEYTABLERECORD, 0,
-					comp_mask, &pktr, smkey,
+					comp_mask, &pktr,
 					dump_one_pkey_tbl_record);
 }
 
@@ -1248,7 +1247,7 @@ static int query_lft_records(const struct query_cmd *q, bind_handle_t h,
 	CHECK_AND_SET_VAL(block, 16, -1, lftr.block_num, LFTR, BLOCK);
 
 	return get_and_dump_any_records(h, IB_SA_ATTR_LFTRECORD, 0, comp_mask,
-					&lftr, 0, dump_one_lft_record);
+					&lftr, dump_one_lft_record);
 }
 
 static int query_guidinfo_records(const struct query_cmd *q, bind_handle_t h,
@@ -1266,7 +1265,7 @@ static int query_guidinfo_records(const struct query_cmd *q, bind_handle_t h,
 	CHECK_AND_SET_VAL(block, 8, -1, gir.block_num, GIR, BLOCKNUM);
 
 	return get_and_dump_any_records(h, IB_SA_ATTR_GUIDINFORECORD, 0,
-					comp_mask, &gir, 0,
+					comp_mask, &gir,
 					dump_one_guidinfo_record);
 }
 
@@ -1289,7 +1288,7 @@ static int query_mft_records(const struct query_cmd *q, bind_handle_t h,
 	mftr.position_block_num |= cl_hton16(pos << 12);
 
 	return get_and_dump_any_records(h, IB_SA_ATTR_MFTRECORD, 0, comp_mask,
-					&mftr, 0, dump_one_mft_record);
+					&mftr, dump_one_mft_record);
 }
 
 static const struct query_cmd query_cmds[] = {
-- 
1.7.9.2

--
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 V2 3/5] ib-diags/saquery: Fix smkey handling

[Hal Rosenstock]

On 4/23/2012 8:56 PM, Jim Foraker wrote:
> smkey is already defined as a global inside saquery.c, so remove
> broken support for passing it around as a function parameter
> 
> Signed-off-by: Jim Foraker <foraker1-i2BcT+NCU+M@public.gmane.org>
> ---
>  src/saquery.c |   59 ++++++++++++++++++++++++++++-----------------------------
>  1 file changed, 29 insertions(+), 30 deletions(-)
> 
> diff --git a/src/saquery.c b/src/saquery.c
> index e5fdb25..029228c 100644
> --- a/src/saquery.c
> +++ b/src/saquery.c
> @@ -85,7 +85,7 @@ struct query_cmd {
>  
>  static char *node_name_map_file = NULL;
>  static nn_map_t *node_name_map = NULL;
> -static uint64_t smkey = 1;
> +static uint64_t smkey = 0;

Why is the default for smkey being changed from 1 to 0 ? Note that even
though the name is smkey (due to the spec), it is really the default SA key.

-- Hal

<snip...>
--
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 V2 3/5] ib-diags/saquery: Fix smkey handling

[Jim Foraker]

On Tue, 2012-04-24 at 07:17 -0700, Hal Rosenstock wrote:
> On 4/23/2012 8:56 PM, Jim Foraker wrote:
> > smkey is already defined as a global inside saquery.c, so remove
> > broken support for passing it around as a function parameter
> > 
> > Signed-off-by: Jim Foraker <foraker1-i2BcT+NCU+M@public.gmane.org>
> > ---
> >  src/saquery.c |   59 ++++++++++++++++++++++++++++-----------------------------
> >  1 file changed, 29 insertions(+), 30 deletions(-)
> > 
> > diff --git a/src/saquery.c b/src/saquery.c
> > index e5fdb25..029228c 100644
> > --- a/src/saquery.c
> > +++ b/src/saquery.c
> > @@ -85,7 +85,7 @@ struct query_cmd {
> >  
> >  static char *node_name_map_file = NULL;
> >  static nn_map_t *node_name_map = NULL;
> > -static uint64_t smkey = 1;
> > +static uint64_t smkey = 0;
> 
> Why is the default for smkey being changed from 1 to 0 ? Note that even
> though the name is smkey (due to the spec), it is really the default SA key.
     Previous to the patch, smkey was defined as 1, but rarely passed
thru to functions.  In particular, the only SA requests that were using
the default value of 1 were MCMember records, via either the -m option
or the MCMR query.  All other types were hard coded to smkey values of
0, and hence executing untrusted SA requests, regardless of either the
smkey variable defaulting to 1 or of any "--smkey" being passed on the
command line.
     Changing the variable default to 0 causes the patch to effect the
least change in observable behavior.  In particular, for commands not
specifying a smkey on the command line, the visible changes are limited
to MCMember records.  For subnets not using partitioning, the change in
MCMember records is limited to the specifics for that record type
covered in C15-0.2-1.16.
     Setting the default to 1 may provide better behavior, in terms of
making the diags "just work" for an out-of-the-box OpenSM config, but it
seems to me that the continued existence of this bug shows that
authenticated requests might not be particularly important for simple
configs.  Plus, it extracts a penalty -- post-patch, if the default is
set to 1, a user who chooses to change their SA smkey will be penalized
in the sense that they will always need to pass an smkey on the command
line, either the correct one or "--smkey 0" to execute an untrusted
request (packets with incorrect smkeys are dropped, not considered
untrusted).  With a default of 0, we are not providing users
encouragement to leave their SA smkey (which in turn protects other
authentication keys on the fabric) at a well-known, insecure value.
     A compromise would be for someone to write a patch that adds
support for a default SA smkey to the diags config file.  In that case,
I think the right behavior would be for the compiled utility to still
default to 0 so that saquery works on hosts without an smkey set in the
conf (the default config file might set the value to 1), which means
this patch as written does not get in the way.

     Jim

> 
> -- Hal
> 
> <snip...>

--
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 V2 3/5] ib-diags/saquery: Fix smkey handling

[Hal Rosenstock]

On 4/24/2012 7:42 PM, Jim Foraker wrote:
> 
> On Tue, 2012-04-24 at 07:17 -0700, Hal Rosenstock wrote:
>> On 4/23/2012 8:56 PM, Jim Foraker wrote:
>>> smkey is already defined as a global inside saquery.c, so remove
>>> broken support for passing it around as a function parameter
>>>
>>> Signed-off-by: Jim Foraker <foraker1-i2BcT+NCU+M@public.gmane.org>
>>> ---
>>>  src/saquery.c |   59 ++++++++++++++++++++++++++++-----------------------------
>>>  1 file changed, 29 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/src/saquery.c b/src/saquery.c
>>> index e5fdb25..029228c 100644
>>> --- a/src/saquery.c
>>> +++ b/src/saquery.c
>>> @@ -85,7 +85,7 @@ struct query_cmd {
>>>  
>>>  static char *node_name_map_file = NULL;
>>>  static nn_map_t *node_name_map = NULL;
>>> -static uint64_t smkey = 1;
>>> +static uint64_t smkey = 0;
>>
>> Why is the default for smkey being changed from 1 to 0 ? Note that even
>> though the name is smkey (due to the spec), it is really the default SA key.
>      Previous to the patch, smkey was defined as 1, but rarely passed
> thru to functions.  In particular, the only SA requests that were using
> the default value of 1 were MCMember records, via either the -m option
> or the MCMR query.  All other types were hard coded to smkey values of
> 0, and hence executing untrusted SA requests, regardless of either the
> smkey variable defaulting to 1 or of any "--smkey" being passed on the
> command line.

In addition to MCMemberRecords, trust is supported for
P_KeyTableRecords, PortInfoRecords, and ServiceRecords AFAIT. Patch
shortly for InformInfoRecords and InformInfo.

>      Changing the variable default to 0 causes the patch to effect the
> least change in observable behavior.  

Not sure what you mean by that.

> In particular, for commands not
> specifying a smkey on the command line, the visible changes are limited
> to MCMember records.  For subnets not using partitioning, the change in
> MCMember records is limited to the specifics for that record type
> covered in C15-0.2-1.16.

Even without partitioning, this is important for validating all members
in MC group (for IPoIB debug).

It also affects the previously aforementioned SA records as well.

>      Setting the default to 1 may provide better behavior, in terms of
> making the diags "just work" for an out-of-the-box OpenSM config, 

Yes, that was the original intention.

> but it
> seems to me that the continued existence of this bug shows that
> authenticated requests might not be particularly important for simple
> configs.  Plus, it extracts a penalty -- post-patch, if the default is
> set to 1, a user who chooses to change their SA smkey will be penalized
> in the sense that they will always need to pass an smkey on the command
> line, either the correct one or "--smkey 0" to execute an untrusted
> request (packets with incorrect smkeys are dropped, not considered
> untrusted).  

That is the tradeoff and it was the decision made. I can dig out the
threads on the list if need be.

> With a default of 0, we are not providing users
> encouragement to leave their SA smkey (which in turn protects other
> authentication keys on the fabric) at a well-known, insecure value.

Yes, it comes down ease of use v. "security". Also, changing this now
becomes a flag day/backward compatibility issue (at least in terms of
support).

>      A compromise would be for someone to write a patch that adds
> support for a default SA smkey to the diags config file.

Makes sense.

> In that case, I think the right behavior would be for the compiled utility to still
> default to 0 so that saquery works on hosts without an smkey set in the
> conf (the default config file might set the value to 1), which means
> this patch as written does not get in the way.

OK but IMO we shouldn't change the smkey value here until this occurs.

-- Hal

>      Jim
> 
>>
>> -- Hal
>>
>> <snip...>
> 
> 
--
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 V2 3/5] ib-diags/saquery: Fix smkey handling

[Jim Foraker]

On Wed, 2012-04-25 at 05:57 -0700, Hal Rosenstock wrote:
> On 4/24/2012 7:42 PM, Jim Foraker wrote:
> > 
> > On Tue, 2012-04-24 at 07:17 -0700, Hal Rosenstock wrote:
> >> On 4/23/2012 8:56 PM, Jim Foraker wrote:
> >>> smkey is already defined as a global inside saquery.c, so remove
> >>> broken support for passing it around as a function parameter
> >>>
> >>> Signed-off-by: Jim Foraker <foraker1-i2BcT+NCU+M@public.gmane.org>
> >>> ---
> >>>  src/saquery.c |   59 ++++++++++++++++++++++++++++-----------------------------
> >>>  1 file changed, 29 insertions(+), 30 deletions(-)
> >>>
> >>> diff --git a/src/saquery.c b/src/saquery.c
> >>> index e5fdb25..029228c 100644
> >>> --- a/src/saquery.c
> >>> +++ b/src/saquery.c
> >>> @@ -85,7 +85,7 @@ struct query_cmd {
> >>>  
> >>>  static char *node_name_map_file = NULL;
> >>>  static nn_map_t *node_name_map = NULL;
> >>> -static uint64_t smkey = 1;
> >>> +static uint64_t smkey = 0;
> >>
> >> Why is the default for smkey being changed from 1 to 0 ? Note that even
> >> though the name is smkey (due to the spec), it is really the default SA key.
> >      Previous to the patch, smkey was defined as 1, but rarely passed
> > thru to functions.  In particular, the only SA requests that were using
> > the default value of 1 were MCMember records, via either the -m option
> > or the MCMR query.  All other types were hard coded to smkey values of
> > 0, and hence executing untrusted SA requests, regardless of either the
> > smkey variable defaulting to 1 or of any "--smkey" being passed on the
> > command line.
> 
> In addition to MCMemberRecords, trust is supported for
> P_KeyTableRecords, PortInfoRecords, and ServiceRecords AFAIT. Patch
> shortly for InformInfoRecords and InformInfo.
     Trust may be implemented in OpenSM, but it is not being used by the
diags.  The vast majority of the patch deals with removing the smkey or
trusted parameters to calls to get_{all,any}_records() and
get_and_dump_{all,any}_records().  In almost every one of those cases, a
value of "0" was being passed in the trusted/smkey field, either of
which results in sa_query() being called with the smkey set to 0 --
hence, an untrusted request.
     In addition, get_and_dump_all_records() did not pass the trusted
parameter on when it called get_all_records(), so it _always_ generates
untrusted requests.

> 
> >      Changing the variable default to 0 causes the patch to effect the
> > least change in observable behavior.  
> 
> Not sure what you mean by that.
     What I mean is that if we leave smkey at 0 in the patch, the output
that users of saquery will see (without passing a smkey on the command
line) will change very little post-patch -- only MCMember records will
change.
     If we apply the patch but with smkey changed to 1, users will see
far more new behavior.  If the subnet is partitioned, they will see more
results for every record type other than MCMember than they did
previously.  They will also begin to see valid Mkeys, ServiceKeys, and
QPNs in their results.  Conversely, if they have changed their SA smkey
away from 1, they will get no results whatsoever, from any request,
until they pass an smkey on the command line.

> > In particular, for commands not
> > specifying a smkey on the command line, the visible changes are limited
> > to MCMember records.  For subnets not using partitioning, the change in
> > MCMember records is limited to the specifics for that record type
> > covered in C15-0.2-1.16.
> 
> Even without partitioning, this is important for validating all members
> in MC group (for IPoIB debug).
     That information would still be available, it just would require
configuring/passing the right smkey.

> 
> It also affects the previously aforementioned SA records as well.
     It only affects them if the user passes an smkey.  Currently, all
record types other than MCMember are queried for with an smkey of 0,
even if smkey is passed on the command line.  Fixing the handling and
then changing the default to 0 means most SA records are still queried
with an smkey of 0, resulting in identical behavior as before, but now
the user can actually usefully change what smkey is used.

> 
> >      Setting the default to 1 may provide better behavior, in terms of
> > making the diags "just work" for an out-of-the-box OpenSM config, 
> 
> Yes, that was the original intention.
> 
> > but it
> > seems to me that the continued existence of this bug shows that
> > authenticated requests might not be particularly important for simple
> > configs.  Plus, it extracts a penalty -- post-patch, if the default is
> > set to 1, a user who chooses to change their SA smkey will be penalized
> > in the sense that they will always need to pass an smkey on the command
> > line, either the correct one or "--smkey 0" to execute an untrusted
> > request (packets with incorrect smkeys are dropped, not considered
> > untrusted).  
> 
> That is the tradeoff and it was the decision made. I can dig out the
> threads on the list if need be.
> 
> > With a default of 0, we are not providing users
> > encouragement to leave their SA smkey (which in turn protects other
> > authentication keys on the fabric) at a well-known, insecure value.
> 
> Yes, it comes down ease of use v. "security". Also, changing this now
> becomes a flag day/backward compatibility issue (at least in terms of
> support).
     How does this warrant a flag day?  It's a bug fix to a query
utility, which as currently implemented, preserves most current
behavior, and in the two cases it doesn't, the current behavior can be
recaptured by passing one command line parameter.

     Jim

> 
> >      A compromise would be for someone to write a patch that adds
> > support for a default SA smkey to the diags config file.
> 
> Makes sense.
> 
> > In that case, I think the right behavior would be for the compiled utility to still
> > default to 0 so that saquery works on hosts without an smkey set in the
> > conf (the default config file might set the value to 1), which means
> > this patch as written does not get in the way.
> 
> OK but IMO we shouldn't change the smkey value here until this occurs.
> 
> -- Hal
> 
> >      Jim
> > 
> >>
> >> -- Hal
> >>
> >> <snip...>
> > 
> > 
> --
> 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 V2 3/5] ib-diags/saquery: Fix smkey handling

[Hal Rosenstock]

On 4/25/2012 7:24 PM, Jim Foraker wrote:
> 
> On Wed, 2012-04-25 at 05:57 -0700, Hal Rosenstock wrote:
>> On 4/24/2012 7:42 PM, Jim Foraker wrote:
>>>
>>> On Tue, 2012-04-24 at 07:17 -0700, Hal Rosenstock wrote:
>>>> On 4/23/2012 8:56 PM, Jim Foraker wrote:
>>>>> smkey is already defined as a global inside saquery.c, so remove
>>>>> broken support for passing it around as a function parameter
>>>>>
>>>>> Signed-off-by: Jim Foraker <foraker1-i2BcT+NCU+M@public.gmane.org>
>>>>> ---
>>>>>  src/saquery.c |   59 ++++++++++++++++++++++++++++-----------------------------
>>>>>  1 file changed, 29 insertions(+), 30 deletions(-)
>>>>>
>>>>> diff --git a/src/saquery.c b/src/saquery.c
>>>>> index e5fdb25..029228c 100644
>>>>> --- a/src/saquery.c
>>>>> +++ b/src/saquery.c
>>>>> @@ -85,7 +85,7 @@ struct query_cmd {
>>>>>  
>>>>>  static char *node_name_map_file = NULL;
>>>>>  static nn_map_t *node_name_map = NULL;
>>>>> -static uint64_t smkey = 1;
>>>>> +static uint64_t smkey = 0;
>>>>
>>>> Why is the default for smkey being changed from 1 to 0 ? Note that even
>>>> though the name is smkey (due to the spec), it is really the default SA key.
>>>      Previous to the patch, smkey was defined as 1, but rarely passed
>>> thru to functions.  In particular, the only SA requests that were using
>>> the default value of 1 were MCMember records, via either the -m option
>>> or the MCMR query.  All other types were hard coded to smkey values of
>>> 0, and hence executing untrusted SA requests, regardless of either the
>>> smkey variable defaulting to 1 or of any "--smkey" being passed on the
>>> command line.
>>
>> In addition to MCMemberRecords, trust is supported for
>> P_KeyTableRecords, PortInfoRecords, and ServiceRecords AFAIT. Patch
>> shortly for InformInfoRecords and InformInfo.
>      Trust may be implemented in OpenSM, but it is not being used by the
> diags.  The vast majority of the patch deals with removing the smkey or
> trusted parameters to calls to get_{all,any}_records() and
> get_and_dump_{all,any}_records().  In almost every one of those cases, a
> value of "0" was being passed in the trusted/smkey field, either of
> which results in sa_query() being called with the smkey set to 0 --
> hence, an untrusted request.
>      In addition, get_and_dump_all_records() did not pass the trusted
> parameter on when it called get_all_records(), so it _always_ generates
> untrusted requests.
> 
>>
>>>      Changing the variable default to 0 causes the patch to effect the
>>> least change in observable behavior.  
>>
>> Not sure what you mean by that.
>      What I mean is that if we leave smkey at 0 in the patch, the output
> that users of saquery will see (without passing a smkey on the command
> line) will change very little post-patch -- only MCMember records will
> change.
>      If we apply the patch but with smkey changed to 1, users will see
> far more new behavior.  If the subnet is partitioned, they will see more
> results for every record type other than MCMember than they did
> previously.  They will also begin to see valid Mkeys, ServiceKeys, and
> QPNs in their results.  Conversely, if they have changed their SA smkey
> away from 1, they will get no results whatsoever, from any request,
> until they pass an smkey on the command line.
> 
>>> In particular, for commands not
>>> specifying a smkey on the command line, the visible changes are limited
>>> to MCMember records.  For subnets not using partitioning, the change in
>>> MCMember records is limited to the specifics for that record type
>>> covered in C15-0.2-1.16.
>>
>> Even without partitioning, this is important for validating all members
>> in MC group (for IPoIB debug).
>      That information would still be available, it just would require
> configuring/passing the right smkey.
> 
>>
>> It also affects the previously aforementioned SA records as well.
>      It only affects them if the user passes an smkey.  Currently, all
> record types other than MCMember are queried for with an smkey of 0,
> even if smkey is passed on the command line.  Fixing the handling and
> then changing the default to 0 means most SA records are still queried
> with an smkey of 0, resulting in identical behavior as before, but now
> the user can actually usefully change what smkey is used.
> 
>>
>>>      Setting the default to 1 may provide better behavior, in terms of
>>> making the diags "just work" for an out-of-the-box OpenSM config, 
>>
>> Yes, that was the original intention.
>>
>>> but it
>>> seems to me that the continued existence of this bug shows that
>>> authenticated requests might not be particularly important for simple
>>> configs.  Plus, it extracts a penalty -- post-patch, if the default is
>>> set to 1, a user who chooses to change their SA smkey will be penalized
>>> in the sense that they will always need to pass an smkey on the command
>>> line, either the correct one or "--smkey 0" to execute an untrusted
>>> request (packets with incorrect smkeys are dropped, not considered
>>> untrusted).  
>>
>> That is the tradeoff and it was the decision made. I can dig out the
>> threads on the list if need be.
>>
>>> With a default of 0, we are not providing users
>>> encouragement to leave their SA smkey (which in turn protects other
>>> authentication keys on the fabric) at a well-known, insecure value.
>>
>> Yes, it comes down ease of use v. "security". Also, changing this now
>> becomes a flag day/backward compatibility issue (at least in terms of
>> support).
>      How does this warrant a flag day?  It's a bug fix to a query
> utility, which as currently implemented, preserves most current
> behavior, and in the two cases it doesn't, the current behavior can be
> recaptured by passing one command line parameter.

I was referring to the user/admin expectation of seeing all
MCMemberRecords without supplying smkey and now to get that he will need
a different command. Changes like these cause confusion (and support).

I didn't realize about the other lacking saquery support for trust in
the other SA records. That definitely changes the picture.

So the question seems to be whether or not to preserve the original user
experience in terms of MCMemberRecord behavior and if so, how to
accomplish that. I think that might mean an additional policy (like
smkey_mcmr).

-- Hal

> 
>      Jim
> 
>>
>>>      A compromise would be for someone to write a patch that adds
>>> support for a default SA smkey to the diags config file.
>>
>> Makes sense.
>>
>>> In that case, I think the right behavior would be for the compiled utility to still
>>> default to 0 so that saquery works on hosts without an smkey set in the
>>> conf (the default config file might set the value to 1), which means
>>> this patch as written does not get in the way.
>>
>> OK but IMO we shouldn't change the smkey value here until this occurs.
>>
>> -- Hal
>>
>>>      Jim
>>>
>>>>
>>>> -- Hal
>>>>
>>>> <snip...>
>>>
>>>
>> --
>> 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 V2 3/5] ib-diags/saquery: Fix smkey handling

[Jim Foraker]

On Thu, 2012-04-26 at 05:04 -0700, Hal Rosenstock wrote:
> On 4/25/2012 7:24 PM, Jim Foraker wrote:
> > 
> > On Wed, 2012-04-25 at 05:57 -0700, Hal Rosenstock wrote:
> >> On 4/24/2012 7:42 PM, Jim Foraker wrote:
> >>>
> >>> On Tue, 2012-04-24 at 07:17 -0700, Hal Rosenstock wrote:
> >>>> On 4/23/2012 8:56 PM, Jim Foraker wrote:
> >>>>> smkey is already defined as a global inside saquery.c, so remove
> >>>>> broken support for passing it around as a function parameter
> >>>>>
> >>>>> Signed-off-by: Jim Foraker <foraker1-i2BcT+NCU+M@public.gmane.org>
> >>>>> ---
> >>>>>  src/saquery.c |   59 ++++++++++++++++++++++++++++-----------------------------
> >>>>>  1 file changed, 29 insertions(+), 30 deletions(-)
> >>>>>
> >>>>> diff --git a/src/saquery.c b/src/saquery.c
> >>>>> index e5fdb25..029228c 100644
> >>>>> --- a/src/saquery.c
> >>>>> +++ b/src/saquery.c
> >>>>> @@ -85,7 +85,7 @@ struct query_cmd {
> >>>>>  
> >>>>>  static char *node_name_map_file = NULL;
> >>>>>  static nn_map_t *node_name_map = NULL;
> >>>>> -static uint64_t smkey = 1;
> >>>>> +static uint64_t smkey = 0;
> >>>>
> >>>> Why is the default for smkey being changed from 1 to 0 ? Note that even
> >>>> though the name is smkey (due to the spec), it is really the default SA key.
> >>>      Previous to the patch, smkey was defined as 1, but rarely passed
> >>> thru to functions.  In particular, the only SA requests that were using
> >>> the default value of 1 were MCMember records, via either the -m option
> >>> or the MCMR query.  All other types were hard coded to smkey values of
     I need to correct myself.  Looking thru the code again today, I
realized that I had forgotten that ClassPortInfo calls sa_query
directly, so it has been using an smkey of 1 as well.

> >>> 0, and hence executing untrusted SA requests, regardless of either the
> >>> smkey variable defaulting to 1 or of any "--smkey" being passed on the
> >>> command line.
> >>
> >> In addition to MCMemberRecords, trust is supported for
> >> P_KeyTableRecords, PortInfoRecords, and ServiceRecords AFAIT. Patch
> >> shortly for InformInfoRecords and InformInfo.
> >      Trust may be implemented in OpenSM, but it is not being used by the
> > diags.  The vast majority of the patch deals with removing the smkey or
> > trusted parameters to calls to get_{all,any}_records() and
> > get_and_dump_{all,any}_records().  In almost every one of those cases, a
> > value of "0" was being passed in the trusted/smkey field, either of
> > which results in sa_query() being called with the smkey set to 0 --
> > hence, an untrusted request.
> >      In addition, get_and_dump_all_records() did not pass the trusted
> > parameter on when it called get_all_records(), so it _always_ generates
> > untrusted requests.
> > 
> >>
> >>>      Changing the variable default to 0 causes the patch to effect the
> >>> least change in observable behavior.  
> >>
> >> Not sure what you mean by that.
> >      What I mean is that if we leave smkey at 0 in the patch, the output
> > that users of saquery will see (without passing a smkey on the command
> > line) will change very little post-patch -- only MCMember records will
> > change.
> >      If we apply the patch but with smkey changed to 1, users will see
> > far more new behavior.  If the subnet is partitioned, they will see more
> > results for every record type other than MCMember than they did
> > previously.  They will also begin to see valid Mkeys, ServiceKeys, and
> > QPNs in their results.  Conversely, if they have changed their SA smkey
> > away from 1, they will get no results whatsoever, from any request,
> > until they pass an smkey on the command line.
> > 
> >>> In particular, for commands not
> >>> specifying a smkey on the command line, the visible changes are limited
> >>> to MCMember records.  For subnets not using partitioning, the change in
> >>> MCMember records is limited to the specifics for that record type
> >>> covered in C15-0.2-1.16.
> >>
> >> Even without partitioning, this is important for validating all members
> >> in MC group (for IPoIB debug).
> >      That information would still be available, it just would require
> > configuring/passing the right smkey.
> > 
> >>
> >> It also affects the previously aforementioned SA records as well.
> >      It only affects them if the user passes an smkey.  Currently, all
> > record types other than MCMember are queried for with an smkey of 0,
> > even if smkey is passed on the command line.  Fixing the handling and
> > then changing the default to 0 means most SA records are still queried
> > with an smkey of 0, resulting in identical behavior as before, but now
> > the user can actually usefully change what smkey is used.
> > 
> >>
> >>>      Setting the default to 1 may provide better behavior, in terms of
> >>> making the diags "just work" for an out-of-the-box OpenSM config, 
> >>
> >> Yes, that was the original intention.
> >>
> >>> but it
> >>> seems to me that the continued existence of this bug shows that
> >>> authenticated requests might not be particularly important for simple
> >>> configs.  Plus, it extracts a penalty -- post-patch, if the default is
> >>> set to 1, a user who chooses to change their SA smkey will be penalized
> >>> in the sense that they will always need to pass an smkey on the command
> >>> line, either the correct one or "--smkey 0" to execute an untrusted
> >>> request (packets with incorrect smkeys are dropped, not considered
> >>> untrusted).  
> >>
> >> That is the tradeoff and it was the decision made. I can dig out the
> >> threads on the list if need be.
> >>
> >>> With a default of 0, we are not providing users
> >>> encouragement to leave their SA smkey (which in turn protects other
> >>> authentication keys on the fabric) at a well-known, insecure value.
> >>
> >> Yes, it comes down ease of use v. "security". Also, changing this now
> >> becomes a flag day/backward compatibility issue (at least in terms of
> >> support).
> >      How does this warrant a flag day?  It's a bug fix to a query
> > utility, which as currently implemented, preserves most current
> > behavior, and in the two cases it doesn't, the current behavior can be
> > recaptured by passing one command line parameter.
> 
> I was referring to the user/admin expectation of seeing all
> MCMemberRecords without supplying smkey and now to get that he will need
> a different command. Changes like these cause confusion (and support).
> 
> I didn't realize about the other lacking saquery support for trust in
> the other SA records. That definitely changes the picture.
> 
> So the question seems to be whether or not to preserve the original user
> experience in terms of MCMemberRecord behavior and if so, how to
> accomplish that. I think that might mean an additional policy (like
> smkey_mcmr).
     Yes, saquery would need to be aware of two different smkeys -- one
for the previously-trusted requests, and one for the
previously-untrusted requests.  There would then need to be some logic
that works out how passed-in smkeys are used -- do we need to create a
2nd command line option plus 2 config file options?  In order to not
break scripts, do we need to make "--smkey" change only the MCMR and CPI
keys (as was previously the case) and then have something akin to
"--smkey_everything_else" (but hopefully better named)?
     It can all be done, but it exacts a cost in complexity (and hence
future support/maintenance) that seems hard to justify to me.  I'm just
not convinced that it will be less confusing for users to deal with
configuring a split-brained dual-smkey world than it will be to deal
with what amounts to a fairly minor interface change after explicitly
upgrading their software.  They can, after all, resurrect the old
behavior by passing one command line option.

     Jim

> 
> -- Hal
> 
> > 
> >      Jim
> > 
> >>
> >>>      A compromise would be for someone to write a patch that adds
> >>> support for a default SA smkey to the diags config file.
> >>
> >> Makes sense.
> >>
> >>> In that case, I think the right behavior would be for the compiled utility to still
> >>> default to 0 so that saquery works on hosts without an smkey set in the
> >>> conf (the default config file might set the value to 1), which means
> >>> this patch as written does not get in the way.
> >>
> >> OK but IMO we shouldn't change the smkey value here until this occurs.
> >>
> >> -- Hal
> >>
> >>>      Jim
> >>>
> >>>>
> >>>> -- Hal
> >>>>
> >>>> <snip...>
> >>>
> >>>
> >> --
> >> 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 V2 3/5] ib-diags/saquery: Fix smkey handling

[Hal Rosenstock]

On 4/26/2012 8:45 PM, Jim Foraker wrote:
> 
> On Thu, 2012-04-26 at 05:04 -0700, Hal Rosenstock wrote:
>> On 4/25/2012 7:24 PM, Jim Foraker wrote:
>>>
>>> On Wed, 2012-04-25 at 05:57 -0700, Hal Rosenstock wrote:
>>>> On 4/24/2012 7:42 PM, Jim Foraker wrote:
>>>>>
>>>>> On Tue, 2012-04-24 at 07:17 -0700, Hal Rosenstock wrote:
>>>>>> On 4/23/2012 8:56 PM, Jim Foraker wrote:
>>>>>>> smkey is already defined as a global inside saquery.c, so remove
>>>>>>> broken support for passing it around as a function parameter
>>>>>>>
>>>>>>> Signed-off-by: Jim Foraker <foraker1-i2BcT+NCU+M@public.gmane.org>
>>>>>>> ---
>>>>>>>  src/saquery.c |   59 ++++++++++++++++++++++++++++-----------------------------
>>>>>>>  1 file changed, 29 insertions(+), 30 deletions(-)
>>>>>>>
>>>>>>> diff --git a/src/saquery.c b/src/saquery.c
>>>>>>> index e5fdb25..029228c 100644
>>>>>>> --- a/src/saquery.c
>>>>>>> +++ b/src/saquery.c
>>>>>>> @@ -85,7 +85,7 @@ struct query_cmd {
>>>>>>>  
>>>>>>>  static char *node_name_map_file = NULL;
>>>>>>>  static nn_map_t *node_name_map = NULL;
>>>>>>> -static uint64_t smkey = 1;
>>>>>>> +static uint64_t smkey = 0;
>>>>>>
>>>>>> Why is the default for smkey being changed from 1 to 0 ? Note that even
>>>>>> though the name is smkey (due to the spec), it is really the default SA key.
>>>>>      Previous to the patch, smkey was defined as 1, but rarely passed
>>>>> thru to functions.  In particular, the only SA requests that were using
>>>>> the default value of 1 were MCMember records, via either the -m option
>>>>> or the MCMR query.  All other types were hard coded to smkey values of
>      I need to correct myself.  Looking thru the code again today, I
> realized that I had forgotten that ClassPortInfo calls sa_query
> directly, so it has been using an smkey of 1 as well.
> 
>>>>> 0, and hence executing untrusted SA requests, regardless of either the
>>>>> smkey variable defaulting to 1 or of any "--smkey" being passed on the
>>>>> command line.
>>>>
>>>> In addition to MCMemberRecords, trust is supported for
>>>> P_KeyTableRecords, PortInfoRecords, and ServiceRecords AFAIT. Patch
>>>> shortly for InformInfoRecords and InformInfo.
>>>      Trust may be implemented in OpenSM, but it is not being used by the
>>> diags.  The vast majority of the patch deals with removing the smkey or
>>> trusted parameters to calls to get_{all,any}_records() and
>>> get_and_dump_{all,any}_records().  In almost every one of those cases, a
>>> value of "0" was being passed in the trusted/smkey field, either of
>>> which results in sa_query() being called with the smkey set to 0 --
>>> hence, an untrusted request.
>>>      In addition, get_and_dump_all_records() did not pass the trusted
>>> parameter on when it called get_all_records(), so it _always_ generates
>>> untrusted requests.
>>>
>>>>
>>>>>      Changing the variable default to 0 causes the patch to effect the
>>>>> least change in observable behavior.  
>>>>
>>>> Not sure what you mean by that.
>>>      What I mean is that if we leave smkey at 0 in the patch, the output
>>> that users of saquery will see (without passing a smkey on the command
>>> line) will change very little post-patch -- only MCMember records will
>>> change.
>>>      If we apply the patch but with smkey changed to 1, users will see
>>> far more new behavior.  If the subnet is partitioned, they will see more
>>> results for every record type other than MCMember than they did
>>> previously.  They will also begin to see valid Mkeys, ServiceKeys, and
>>> QPNs in their results.  Conversely, if they have changed their SA smkey
>>> away from 1, they will get no results whatsoever, from any request,
>>> until they pass an smkey on the command line.
>>>
>>>>> In particular, for commands not
>>>>> specifying a smkey on the command line, the visible changes are limited
>>>>> to MCMember records.  For subnets not using partitioning, the change in
>>>>> MCMember records is limited to the specifics for that record type
>>>>> covered in C15-0.2-1.16.
>>>>
>>>> Even without partitioning, this is important for validating all members
>>>> in MC group (for IPoIB debug).
>>>      That information would still be available, it just would require
>>> configuring/passing the right smkey.
>>>
>>>>
>>>> It also affects the previously aforementioned SA records as well.
>>>      It only affects them if the user passes an smkey.  Currently, all
>>> record types other than MCMember are queried for with an smkey of 0,
>>> even if smkey is passed on the command line.  Fixing the handling and
>>> then changing the default to 0 means most SA records are still queried
>>> with an smkey of 0, resulting in identical behavior as before, but now
>>> the user can actually usefully change what smkey is used.
>>>
>>>>
>>>>>      Setting the default to 1 may provide better behavior, in terms of
>>>>> making the diags "just work" for an out-of-the-box OpenSM config, 
>>>>
>>>> Yes, that was the original intention.
>>>>
>>>>> but it
>>>>> seems to me that the continued existence of this bug shows that
>>>>> authenticated requests might not be particularly important for simple
>>>>> configs.  Plus, it extracts a penalty -- post-patch, if the default is
>>>>> set to 1, a user who chooses to change their SA smkey will be penalized
>>>>> in the sense that they will always need to pass an smkey on the command
>>>>> line, either the correct one or "--smkey 0" to execute an untrusted
>>>>> request (packets with incorrect smkeys are dropped, not considered
>>>>> untrusted).  
>>>>
>>>> That is the tradeoff and it was the decision made. I can dig out the
>>>> threads on the list if need be.
>>>>
>>>>> With a default of 0, we are not providing users
>>>>> encouragement to leave their SA smkey (which in turn protects other
>>>>> authentication keys on the fabric) at a well-known, insecure value.
>>>>
>>>> Yes, it comes down ease of use v. "security". Also, changing this now
>>>> becomes a flag day/backward compatibility issue (at least in terms of
>>>> support).
>>>      How does this warrant a flag day?  It's a bug fix to a query
>>> utility, which as currently implemented, preserves most current
>>> behavior, and in the two cases it doesn't, the current behavior can be
>>> recaptured by passing one command line parameter.
>>
>> I was referring to the user/admin expectation of seeing all
>> MCMemberRecords without supplying smkey and now to get that he will need
>> a different command. Changes like these cause confusion (and support).
>>
>> I didn't realize about the other lacking saquery support for trust in
>> the other SA records. That definitely changes the picture.
>>
>> So the question seems to be whether or not to preserve the original user
>> experience in terms of MCMemberRecord behavior and if so, how to
>> accomplish that. I think that might mean an additional policy (like
>> smkey_mcmr).
>      Yes, saquery would need to be aware of two different smkeys -- one
> for the previously-trusted requests, and one for the
> previously-untrusted requests.  There would then need to be some logic
> that works out how passed-in smkeys are used -- do we need to create a
> 2nd command line option plus 2 config file options?  In order to not
> break scripts, do we need to make "--smkey" change only the MCMR and CPI
> keys (as was previously the case) and then have something akin to
> "--smkey_everything_else" (but hopefully better named)?
>      It can all be done, but it exacts a cost in complexity (and hence
> future support/maintenance) that seems hard to justify to me.  I'm just
> not convinced that it will be less confusing for users to deal with
> configuring a split-brained dual-smkey world than it will be to deal
> with what amounts to a fairly minor interface change after explicitly
> upgrading their software.  They can, after all, resurrect the old
> behavior by passing one command line option.

At this point, it's Ira's call.

-- Hal

>      Jim
> 
>>
>> -- Hal
>>
>>>
>>>      Jim
>>>
>>>>
>>>>>      A compromise would be for someone to write a patch that adds
>>>>> support for a default SA smkey to the diags config file.
>>>>
>>>> Makes sense.
>>>>
>>>>> In that case, I think the right behavior would be for the compiled utility to still
>>>>> default to 0 so that saquery works on hosts without an smkey set in the
>>>>> conf (the default config file might set the value to 1), which means
>>>>> this patch as written does not get in the way.
>>>>
>>>> OK but IMO we shouldn't change the smkey value here until this occurs.
>>>>
>>>> -- Hal
>>>>
>>>>>      Jim
>>>>>
>>>>>>
>>>>>> -- Hal
>>>>>>
>>>>>> <snip...>
>>>>>
>>>>>
>>>> --
>>>> 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
> 

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

[PATCH V2 4/5] infiniband-diags: install config file mode 400

[Jim Foraker]

Necessary so that we may add credentials (ie, mkey) to it

Signed-off-by: Jim Foraker <foraker1-i2BcT+NCU+M@public.gmane.org>
---
 Makefile.am |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile.am b/Makefile.am
index 950f95b..ef59bd2 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -112,4 +112,4 @@ install-data-hook:
 	fi
 	$(top_srcdir)/config/install-sh -c -m 444 $(top_srcdir)/scripts/IBswcountlimits.pm $(DESTDIR)/$(PERL_INSTALLDIR)/IBswcountlimits.pm
 	$(top_srcdir)/config/install-sh -c -m 444 $(top_srcdir)/etc/error_thresholds $(DESTDIR)/$(sysconfdir)/infiniband-diags
-	$(top_srcdir)/config/install-sh -c -m 444 $(top_srcdir)/etc/ibdiag.conf $(DESTDIR)/$(sysconfdir)/infiniband-diags
+	$(top_srcdir)/config/install-sh -c -m 400 $(top_srcdir)/etc/ibdiag.conf $(DESTDIR)/$(sysconfdir)/infiniband-diags
-- 
1.7.9.2

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

[PATCH V2 5/5] infiniband-diags: Add m_key option to config file

[Jim Foraker]

Signed-off-by: Jim Foraker <foraker1-i2BcT+NCU+M@public.gmane.org>
---
 etc/ibdiag.conf     |    2 ++
 src/ibdiag_common.c |    2 ++
 2 files changed, 4 insertions(+)

diff --git a/etc/ibdiag.conf b/etc/ibdiag.conf
index 77f3ce9..2a2334f 100644
--- a/etc/ibdiag.conf
+++ b/etc/ibdiag.conf
@@ -15,3 +15,5 @@
 # Default = true
 #MLX_EPI=false
 
+# define a default m_key
+#m_key=0x00
diff --git a/src/ibdiag_common.c b/src/ibdiag_common.c
index 2288ab3..5aa43e8 100644
--- a/src/ibdiag_common.c
+++ b/src/ibdiag_common.c
@@ -155,6 +155,8 @@ void read_ibdiag_config(const char *file)
 			} else {
 				ibd_ibnetdisc_flags &= ~IBND_CONFIG_MLX_EPI;
 			}
+		} else if (strncmp(name, "m_key", strlen("m_key")) == 0) {
+			ibd_mkey = strtoull(val_str, 0, 0);
 		}
 	}
 
-- 
1.7.9.2

--
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 V2 1/5] infiniband-diags/ibportstate.c: Support MKey, lease, and protect bits

[Hal Rosenstock]

On 4/23/2012 8:56 PM, Jim Foraker wrote:
> Allow user to both display and change mkey-related values.
> 
> Signed-off-by: Jim Foraker <foraker1-i2BcT+NCU+M@public.gmane.org>
> ---
>  src/ibportstate.c |   98 ++++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 79 insertions(+), 19 deletions(-)
> 
> diff --git a/src/ibportstate.c b/src/ibportstate.c
> index 5559d18..21cd431 100644
> --- a/src/ibportstate.c
> +++ b/src/ibportstate.c
> @@ -41,6 +41,7 @@
>  #include <unistd.h>
>  #include <string.h>
>  #include <getopt.h>
> +#include <errno.h>
>  
>  #include <infiniband/umad.h>
>  #include <infiniband/mad.h>
> @@ -64,22 +65,28 @@ enum port_ops {
>  	LID,
>  	SMLID,
>  	LMC,
> +	MKEY,
> +	MKEYLEASE,
> +	MKEYPROT,
>  };
>  
>  struct ibmad_port *srcport;
> -int speed = 0; /* no state change */
> -int espeed = 0; /* no state change */
> -int fdr10 = 0; /* no state change */
> -int width = 0; /* no state change */
> -int lid;
> -int smlid;
> -int lmc;
> -int mtu;
> -int vls = 0; /* no state change */
> +uint64_t speed = 0; /* no state change */
> +uint64_t espeed = 0; /* no state change */
> +uint64_t fdr10 = 0; /* no state change */
> +uint64_t width = 0; /* no state change */
> +uint64_t lid;
> +uint64_t smlid;
> +uint64_t lmc;
> +uint64_t mtu;
> +uint64_t vls = 0; /* no state change */
> +uint64_t mkey;
> +uint64_t mkeylease;
> +uint64_t mkeyprot;
>  
>  struct {
>  	const char *name;
> -	int *val;
> +	uint64_t *val;
>  	int set;
>  } port_args[] = {
>  	{"query", NULL, 0},	/* QUERY */
> @@ -98,6 +105,9 @@ struct {
>  	{"lid", &lid, 0},	/* LID */
>  	{"smlid", &smlid, 0},	/* SMLID */
>  	{"lmc", &lmc, 0},	/* LMC */
> +	{"mkey", &mkey, 0},	/* MKEY */
> +	{"mkeylease", &mkeylease, 0},	/* MKEY LEASE */
> +	{"mkeyprot", &mkeyprot, 0},	/* MKEY PROTECT BITS */
>  };
>  
>  #define NPORT_ARGS (sizeof(port_args) / sizeof(port_args[0]))
> @@ -142,7 +152,7 @@ static int get_port_info(ib_portid_t * dest, uint8_t * data, int portnum,
>  }
>  
>  static void show_port_info(ib_portid_t * dest, uint8_t * data, int portnum,
> -			   int espeed_cap)
> +			   int espeed_cap, int is_switch)
>  {
>  	char buf[2300];
>  	char val[64];
> @@ -201,18 +211,32 @@ static void show_port_info(ib_portid_t * dest, uint8_t * data, int portnum,
>  			       val);
>  		sprintf(buf + strlen(buf), "%s", "\n");
>  	}
> +	if (!is_switch || portnum == 0) {
> +		mad_decode_field(data, IB_PORT_MKEY_F, val);
> +		mad_dump_field(IB_PORT_MKEY_F, buf + strlen(buf),
> +			       sizeof buf - strlen(buf), val);
> +		sprintf(buf+strlen(buf), "%s", "\n");
> +		mad_decode_field(data, IB_PORT_MKEY_LEASE_F, val);
> +		mad_dump_field(IB_PORT_MKEY_LEASE_F, buf + strlen(buf),
> +			       sizeof buf - strlen(buf), val);
> +		sprintf(buf+strlen(buf), "%s", "\n");
> +		mad_decode_field(data, IB_PORT_MKEY_PROT_BITS_F, val);
> +		mad_dump_field(IB_PORT_MKEY_PROT_BITS_F, buf + strlen(buf),
> +			       sizeof buf - strlen(buf), val);
> +		sprintf(buf+strlen(buf), "%s", "\n");
> +	}

Don't we want to be careful about displaying mkey info ? I think by
default it shouldn't be displayed and require an additional
option to make it visible which defaults not to do this.

-- Hal

>  
>  	printf("# Port info: %s port %d\n%s", portid2str(dest), portnum, buf);
>  }
>  
>  static void set_port_info(ib_portid_t * dest, uint8_t * data, int portnum,
> -			  int espeed_cap)
> +			  int espeed_cap, int is_switch)
>  {
>  	if (!smp_set_via(data, dest, IB_ATTR_PORT_INFO, portnum, 0, srcport))
>  		IBERROR("smp set portinfo failed");
>  
>  	printf("\nAfter PortInfo set:\n");
> -	show_port_info(dest, data, portnum, espeed_cap);
> +	show_port_info(dest, data, portnum, espeed_cap, is_switch);
>  }
>  
>  static void get_ext_port_info(ib_portid_t * dest, uint8_t * data, int portnum)
> @@ -349,9 +373,10 @@ int main(int argc, char **argv)
>  	int i;
>  	uint16_t devid, rem_devid;
>  	long val;
> +	char *endp;
>  	char usage_args[] = "<dest dr_path|lid|guid> <portnum> [<op>]\n"
>  	    "\nSupported ops: enable, disable, reset, speed, width, query,\n"
> -	    "\tdown, arm, active, vls, mtu, lid, smlid, lmc\n";
> +	    "\tdown, arm, active, vls, mtu, lid, smlid, lmc, mkey, mkeylease, mkeyprot\n";
>  	const char *usage_examples[] = {
>  		"3 1 disable\t\t\t# by lid",
>  		"-G 0x2C9000100D051 1 enable\t# by guid",
> @@ -403,7 +428,7 @@ int main(int argc, char **argv)
>  			if (++i >= argc)
>  				IBERROR("%s requires an additional parameter",
>  					port_args[j].name);
> -			val = strtol(argv[i], 0, 0);
> +			val = strtoull(argv[i], 0, 0);
>  			switch (j) {
>  			case SPEED:
>  				if (val < 0 || val > 15)
> @@ -441,8 +466,29 @@ int main(int argc, char **argv)
>  			case LMC:
>  				if (val < 0 || val > 7)
>  					IBERROR("invalid lmc value %ld", val);
> +				break;
> +			case MKEY:
> +				errno = 0;
> +				val = strtoull(argv[i], &endp, 0);
> +				if (errno || *endp != '\0') {
> +					errno = 0;
> +					val = strtoull(getpass("New M_Key: "),
> +						       &endp, 0);
> +					if (errno || *endp != '\0') {
> +						IBERROR("Bad new M_Key\n");
> +					}
> +				}
> +				/* All 64-bit values are legal */
> +				break;
> +			case MKEYLEASE:
> +				if (val < 0 || val > 0xFFFF)
> +					IBERROR("invalid mkey lease time %ld", val);
> +				break;
> +			case MKEYPROT:
> +				if (val < 0 || val > 3)
> +					IBERROR("invalid mkey protection bit setting %ld", val);
>  			}
> -			*port_args[j].val = (int)val;
> +			*port_args[j].val = val;
>  			changed = 1;
>  			break;
>  		}
> @@ -455,12 +501,16 @@ int main(int argc, char **argv)
>  	is_switch = get_node_info(&portid, data);
>  	devid = (uint16_t) mad_get_field(data, 0, IB_NODE_DEVID_F);
>  
> +	if ((port_args[MKEY].set || port_args[MKEYLEASE].set ||
> +	     port_args[MKEYPROT].set) && is_switch && portnum != 0)
> +		IBERROR("Can't set M_Key fields on switch port != 0");
> +
>  	if (port_op != QUERY || changed)
>  		printf("Initial %s PortInfo:\n", is_switch ? "Switch" : "CA");
>  	else
>  		printf("%s PortInfo:\n", is_switch ? "Switch" : "CA");
>  	espeed_cap = get_port_info(&portid, data, portnum, is_switch);
> -	show_port_info(&portid, data, portnum, espeed_cap);
> +	show_port_info(&portid, data, portnum, espeed_cap, is_switch);
>  	if (is_mlnx_ext_port_info_supported(devid)) {
>  		get_ext_port_info(&portid, data2, portnum);
>  		show_ext_port_info(&portid, data2, portnum);
> @@ -527,7 +577,17 @@ int main(int argc, char **argv)
>  				      fdr10);
>  			set_ext_port_info(&portid, data2, portnum);
>  		}
> -		set_port_info(&portid, data, portnum, is_switch);
> +
> +		if (port_args[MKEY].set)
> +			mad_set_field64(data, 0, IB_PORT_MKEY_F, mkey);
> +		if (port_args[MKEYLEASE].set)
> +			mad_set_field(data, 0, IB_PORT_MKEY_LEASE_F,
> +				      mkeylease);
> +		if (port_args[MKEYPROT].set)
> +			mad_set_field(data, 0, IB_PORT_MKEY_PROT_BITS_F,
> +				      mkeyprot);
> +
> +		set_port_info(&portid, data, portnum, espeed_cap, is_switch);
>  
>  	} else if (is_switch && portnum) {
>  		/* Now, make sure PortState is Active */
> @@ -596,7 +656,7 @@ int main(int argc, char **argv)
>  				get_ext_port_info(&peerportid, data2,
>  						  peerlocalportnum);
>  			show_port_info(&peerportid, data, peerlocalportnum,
> -				       peer_espeed_cap);
> +				       peer_espeed_cap, is_peer_switch);
>  			if (is_mlnx_ext_port_info_supported(rem_devid))
>  				show_ext_port_info(&peerportid, data2,
>  						   peerlocalportnum);

--
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 V2 0/5] Mkey support in infiniband-diags

[Hal Rosenstock]

On 4/23/2012 8:53 PM, Jim Foraker wrote:
>      I am about to send version 2 of the mkey patches for
> infiniband-diags to the list.  Notable changes are:
> 
> . both current mkey and new mkey (for ibportstate) may now be prompted
> for in addition to specified on the command line (ie, identical to
> saquery smkey support)
> 
> . mkey support touches (sadly, just a few) fewer files, due to SMP
> handling cleanups committed earlier
> 
> . saquery's smkey handling has been fixed to allow the return of
> authenticated SA data (this worked for only a few record types before)
> 
> . config file chmod and mkey option patches have been split out
> 
>      I think this covers most/all of the comments received from V1, with
> the exception of the discussions surrounding multiple-mkey policies.
> Given that the diags likely should support operations with a
> single/specified mkey regardless of subnet policy, I think these patches
> get us closer to that goal without standing in the way of future
> discussions on how to get there.

Guess we'll cross this bridge when we get there. I'm a little concerned
with any command line/config file syntax changing but I don't see a way
around that if we are going to support multiple mkey policies in the future.

-- Hal

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