[RFC] RDMA/siw: Fabricate a GID on tun and loopback devices

Commit Message

Chuck Lever June 3, 2023, 1:55 p.m. UTC

From: Chuck Lever <[e-mail]>

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 <[e-mail]>
Fixes: a2d36b02c15d ("RDMA/siw: Enable siw on tunnel devices")
Signed-off-by: Chuck Lever <[e-mail]>
---
 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(-)

Comments

Bernard Metzler June 3, 2023, 2:01 p.m. UTC | #1

> -----Original Message-----
> From: Chuck Lever <[e-mail]>
> Sent: Saturday, 3 June 2023 15:56
> To: Bernard Metzler <[e-mail]>
> Cc: Tom Talpey <[e-mail]>; Chuck Lever <[e-mail]>;
> [e-mail]; [e-mail]
> Subject: [EXTERNAL] [PATCH RFC] RDMA/siw: Fabricate a GID on tun and
> loopback devices
> 
> From: Chuck Lever <[e-mail]>
> 
> 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 <[e-mail]>
> Fixes: a2d36b02c15d ("RDMA/siw: Enable siw on tunnel devices")
> Signed-off-by: Chuck Lever <[e-mail]>
> ---
>  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:
>  		/*
> -		 * 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 June 3, 2023, 2:04 p.m. UTC | #2

> On Jun 3, 2023, at 10:01 AM, Bernard Metzler <[e-mail]> wrote:
> 
> 
> 
>> -----Original Message-----
>> From: Chuck Lever <[e-mail]>
>> Sent: Saturday, 3 June 2023 15:56
>> To: Bernard Metzler <[e-mail]>
>> Cc: Tom Talpey <[e-mail]>; Chuck Lever <[e-mail]>;
>> [e-mail]; [e-mail]
>> Subject: [EXTERNAL] [PATCH RFC] RDMA/siw: Fabricate a GID on tun and
>> loopback devices
>> 
>> From: Chuck Lever <[e-mail]>
>> 
>> 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 <[e-mail]>
>> Fixes: a2d36b02c15d ("RDMA/siw: Enable siw on tunnel devices")
>> Signed-off-by: Chuck Lever <[e-mail]>
>> ---
>> 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?


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

Bernard Metzler June 3, 2023, 2:39 p.m. UTC | #3

> -----Original Message-----
> From: Chuck Lever III <[e-mail]>
> Sent: Saturday, 3 June 2023 16:04
> To: Bernard Metzler <[e-mail]>
> Cc: Chuck Lever <[e-mail]>; Tom Talpey <[e-mail]>; linux-
> [e-mail]
> 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 <[e-mail]> wrote:
> >
> >
> >
> >> -----Original Message-----
> >> From: Chuck Lever <[e-mail]>
> >> Sent: Saturday, 3 June 2023 15:56
> >> To: Bernard Metzler <[e-mail]>
> >> Cc: Tom Talpey <[e-mail]>; Chuck Lever <[e-mail]>;
> >> [e-mail]; [e-mail]
> >> Subject: [EXTERNAL] [PATCH RFC] RDMA/siw: Fabricate a GID on tun and
> >> loopback devices
> >>
> >> From: Chuck Lever <[e-mail]>
> >>
> >> 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 <[e-mail]>
> >> Fixes: a2d36b02c15d ("RDMA/siw: Enable siw on tunnel devices")
> >> Signed-off-by: Chuck Lever <[e-mail]>
> >> ---
> >> 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
>

Tom Talpey June 3, 2023, 2:52 p.m. UTC | #4

On 6/3/2023 10:39 AM, Bernard Metzler wrote:
> 
> 
>> -----Original Message-----
>> From: Chuck Lever III <[e-mail]>
>> Sent: Saturday, 3 June 2023 16:04
>> To: Bernard Metzler <[e-mail]>
>> Cc: Chuck Lever <[e-mail]>; Tom Talpey <[e-mail]>; linux-
>> [e-mail]
>> 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 <[e-mail]> wrote:
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Chuck Lever <[e-mail]>
>>>> Sent: Saturday, 3 June 2023 15:56
>>>> To: Bernard Metzler <[e-mail]>
>>>> Cc: Tom Talpey <[e-mail]>; Chuck Lever <[e-mail]>;
>>>> [e-mail]; [e-mail]
>>>> Subject: [EXTERNAL] [PATCH RFC] RDMA/siw: Fabricate a GID on tun and
>>>> loopback devices
>>>>
>>>> From: Chuck Lever <[e-mail]>
>>>>
>>>> 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 <[e-mail]>
>>>> Fixes: a2d36b02c15d ("RDMA/siw: Enable siw on tunnel devices")
>>>> Signed-off-by: Chuck Lever <[e-mail]>
>>>> ---
>>>> 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
>>
>

Chuck Lever June 3, 2023, 3:12 p.m. UTC | #5

> On Jun 3, 2023, at 10:52 AM, Tom Talpey <[e-mail]> wrote:
> 
> On 6/3/2023 10:39 AM, Bernard Metzler wrote:
>>> -----Original Message-----
>>> From: Chuck Lever III <[e-mail]>
>>> Sent: Saturday, 3 June 2023 16:04
>>> To: Bernard Metzler <[e-mail]>
>>> Cc: Chuck Lever <[e-mail]>; Tom Talpey <[e-mail]>; linux-
>>> [e-mail]
>>> 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 <[e-mail]> wrote:
>>>> 
>>>> 
>>>> 
>>>>> -----Original Message-----
>>>>> From: Chuck Lever <[e-mail]>
>>>>> Sent: Saturday, 3 June 2023 15:56
>>>>> To: Bernard Metzler <[e-mail]>
>>>>> Cc: Tom Talpey <[e-mail]>; Chuck Lever <[e-mail]>;
>>>>> [e-mail]; [e-mail]
>>>>> Subject: [EXTERNAL] [PATCH RFC] RDMA/siw: Fabricate a GID on tun and
>>>>> loopback devices
>>>>> 
>>>>> From: Chuck Lever <[e-mail]>
>>>>> 
>>>>> 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 <[e-mail]>
>>>>> Fixes: a2d36b02c15d ("RDMA/siw: Enable siw on tunnel devices")
>>>>> Signed-off-by: Chuck Lever <[e-mail]>
>>>>> ---
>>>>> 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

Patch

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:
 		/*
-		 * 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;
 }