On 6/3/2023 10:39 AM, Bernard Metzler wrote:
-----Original Message-----
From: Chuck Lever III <chuck.lever@xxxxxxxxxx>
Sent: Saturday, 3 June 2023 16:04
To: Bernard Metzler <BMT@xxxxxxxxxxxxxx>
Cc: Chuck Lever <cel@xxxxxxxxxx>; Tom Talpey <tom@xxxxxxxxxx>; linux-
rdma@xxxxxxxxxxxxxxx
Subject: [EXTERNAL] Re: [PATCH RFC] RDMA/siw: Fabricate a GID on tun and
loopback devices
On Jun 3, 2023, at 10:01 AM, Bernard Metzler <BMT@xxxxxxxxxxxxxx> wrote:
-----Original Message-----
From: Chuck Lever <cel@xxxxxxxxxx>
Sent: Saturday, 3 June 2023 15:56
To: Bernard Metzler <BMT@xxxxxxxxxxxxxx>
Cc: Tom Talpey <tom@xxxxxxxxxx>; Chuck Lever <chuck.lever@xxxxxxxxxx>;
linux-rdma@xxxxxxxxxxxxxxx; tom@xxxxxxxxxx
Subject: [EXTERNAL] [PATCH RFC] RDMA/siw: Fabricate a GID on tun and
loopback devices
From: Chuck Lever <chuck.lever@xxxxxxxxxx>
LOOPBACK and NONE (tunnel) devices have all-zero MAC addresses.
Currently, siw_device_create() falls back to copying the IB device's
name in those cases, because an all-zero MAC address breaks the RDMA
core address resolution mechanism.
However, at the point when siw_device_create() constructs a GID, the
ib_device::name field is uninitialized, leaving the MAC address to
remain in an all-zero state.
Fabricate a random artificial GID for such devices, and ensure that
artificial GID is returned for all device query operations.
Reported-by: Tom Talpey <tom@xxxxxxxxxx>
Fixes: a2d36b02c15d ("RDMA/siw: Enable siw on tunnel devices")
Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
---
drivers/infiniband/sw/siw/siw.h | 1 +
drivers/infiniband/sw/siw/siw_main.c | 26 +++++++++++---------------
drivers/infiniband/sw/siw/siw_verbs.c | 4 ++--
3 files changed, 14 insertions(+), 17 deletions(-)
diff --git a/drivers/infiniband/sw/siw/siw.h
b/drivers/infiniband/sw/siw/siw.h
index d7f5b2a8669d..41fb8976abc6 100644
--- a/drivers/infiniband/sw/siw/siw.h
+++ b/drivers/infiniband/sw/siw/siw.h
@@ -74,6 +74,7 @@ struct siw_device {
u32 vendor_part_id;
int numa_node;
+ char raw_gid[ETH_ALEN];
/* physical port state (only one port per device) */
enum ib_port_state state;
diff --git a/drivers/infiniband/sw/siw/siw_main.c
b/drivers/infiniband/sw/siw/siw_main.c
index 1225ca613f50..efc86565ac5d 100644
--- a/drivers/infiniband/sw/siw/siw_main.c
+++ b/drivers/infiniband/sw/siw/siw_main.c
@@ -75,8 +75,7 @@ static int siw_device_register(struct siw_device
*sdev,
const char *name)
return rv;
}
- siw_dbg(base_dev, "HWaddr=%pM\n", sdev->netdev->dev_addr);
-
+ siw_dbg(base_dev, "HWaddr=%pM\n", sdev->raw_gid);
return 0;
}
@@ -314,24 +313,21 @@ static struct siw_device *siw_device_create(struct
net_device *netdev)
return NULL;
base_dev = &sdev->base_dev;
-
sdev->netdev = netdev;
- if (netdev->type != ARPHRD_LOOPBACK && netdev->type != ARPHRD_NONE) {
- addrconf_addr_eui48((unsigned char *)&base_dev->node_guid,
- netdev->dev_addr);
- } else {
+ switch (netdev->type) {
+ case ARPHRD_LOOPBACK:
+ case ARPHRD_NONE:
One thing I'm wondering is if there are other cases where
there is no L2 address besides ARPHRD_NONE and LOOPBACK.
Should we instead check netdev's addrlen instead of checking
the ARP type?
I think so. In fact it is a potential incomplete
collection of cases where no L2 address is available.
Let's make it explicit.
Yes, absolutely. IFF_POINTTOPOINT devices (e.g. ppp) come to mind.
There are a bunch of others.
I'm not sure the ARPHRD types are the best indicator. Support for
ARP is only part of the hardware address picture. In any case there
are dozens of ARPHRD types, which seems a fragile thing to depend
on here.
I'd say dev->addr_len == 0 is a definite indicator. I bet there are
others though. Volatile addresses? Sounds like netdev may need to
weigh in here.
Tom.
/*
- * This device does not have a HW address,
- * but connection mangagement lib expects gid != 0
+ * This device does not have a HW address, but
+ * connection mangagement requires a unique gid.
*/
- size_t len = min_t(size_t, strlen(base_dev->name), 6);
- char addr[6] = { };
-
- memcpy(addr, base_dev->name, len);
- addrconf_addr_eui48((unsigned char *)&base_dev->node_guid,
- addr);
+ eth_random_addr(sdev->raw_gid);
+ break;
+ default:
+ memcpy(sdev->raw_gid, netdev->dev_addr, ETH_ALEN);
}
+ addrconf_addr_eui48((u8 *)&base_dev->node_guid, sdev->raw_gid);
base_dev->uverbs_cmd_mask |= BIT_ULL(IB_USER_VERBS_CMD_POST_SEND);
diff --git a/drivers/infiniband/sw/siw/siw_verbs.c
b/drivers/infiniband/sw/siw/siw_verbs.c
index 398ec13db624..32b0befd25e2 100644
--- a/drivers/infiniband/sw/siw/siw_verbs.c
+++ b/drivers/infiniband/sw/siw/siw_verbs.c
@@ -157,7 +157,7 @@ int siw_query_device(struct ib_device *base_dev,
struct
ib_device_attr *attr,
attr->vendor_part_id = sdev->vendor_part_id;
addrconf_addr_eui48((u8 *)&attr->sys_image_guid,
- sdev->netdev->dev_addr);
+ sdev->raw_gid);
return 0;
}
@@ -218,7 +218,7 @@ int siw_query_gid(struct ib_device *base_dev, u32
port,
int idx,
/* subnet_prefix == interface_id == 0; */
memset(gid, 0, sizeof(*gid));
- memcpy(&gid->raw[0], sdev->netdev->dev_addr, 6);
+ memcpy(gid->raw, sdev->raw_gid, ETH_ALEN);
return 0;
}
That looks good to me, thanks!
--
Chuck Lever
