> -----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.
>
> >> /*
> >> - * 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
>
