> On Jun 3, 2023, at 10:52 AM, Tom Talpey <tom@xxxxxxxxxx> wrote:
>
> 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.
That would be wise; I can repost the patch and copy netdev@.
> 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
--
Chuck Lever
