[RFC] RDMA/core: Handle ARPHRD_NONE devices

Commit Message

Chuck Lever June 2, 2023, 7:24 p.m. UTC

From: Chuck Lever <[e-mail]>

We would like to enable the use of siw on top of a VPN that is
constructed and managed via a tun device. That hasn't worked up
until now because ARPHRD_NONE devices (such as tun devices) have
no GID for the RDMA/core to look up.

But it turns out that the egress device has already been picked for
us. addr_handler() just has to do the right thing with it.

Suggested-by: Jason Gunthorpe <[e-mail]>
Signed-off-by: Chuck Lever <[e-mail]>
---
 drivers/infiniband/core/cma.c |    4 ++++
 1 file changed, 4 insertions(+)

Comments

Tom Talpey June 2, 2023, 10:18 p.m. UTC | #1

On 6/2/2023 3:24 PM, Chuck Lever wrote:
> From: Chuck Lever <[e-mail]>
> 
> We would like to enable the use of siw on top of a VPN that is
> constructed and managed via a tun device. That hasn't worked up
> until now because ARPHRD_NONE devices (such as tun devices) have
> no GID for the RDMA/core to look up.
> 
> But it turns out that the egress device has already been picked for
> us. addr_handler() just has to do the right thing with it.
> 
> Suggested-by: Jason Gunthorpe <[e-mail]>
> Signed-off-by: Chuck Lever <[e-mail]>
> ---
>   drivers/infiniband/core/cma.c |    4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> index 56e568fcd32b..3351dc5afa17 100644
> --- a/drivers/infiniband/core/cma.c
> +++ b/drivers/infiniband/core/cma.c
> @@ -704,11 +704,15 @@ cma_validate_port(struct ib_device *device, u32 port,
>   		ndev = dev_get_by_index(dev_addr->net, bound_if_index);
>   		if (!ndev)
>   			return ERR_PTR(-ENODEV);
> +	} else if (dev_type == ARPHRD_NONE) {
> +		sgid_attr = rdma_get_gid_attr(device, port, 0);
> +		goto out;
>   	} else {
>   		gid_type = IB_GID_TYPE_IB;
>   	}
>   
>   	sgid_attr = rdma_find_gid_by_port(device, gid, gid_type, port, ndev);
> +out:
>   	dev_put(ndev);
>   	return sgid_attr;
>   }

I like it, but doesn't this test in siw_main.c also need to change?

static struct siw_device *siw_device_create(struct net_device *netdev)
{
...
-->	if (netdev->type != ARPHRD_LOOPBACK && netdev->type != ARPHRD_NONE) {
		addrconf_addr_eui48((unsigned char *)&base_dev->node_guid,
				    netdev->dev_addr);
	} else {
		/*
		 * This device does not have a HW address,
		 * but connection mangagement lib expects gid != 0
		 */
		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);
	}


Tom.

Chuck Lever June 3, 2023, 12:33 a.m. UTC | #2

> On Jun 2, 2023, at 6:18 PM, Tom Talpey <[e-mail]> wrote:
> 
> On 6/2/2023 3:24 PM, Chuck Lever wrote:
>> From: Chuck Lever <[e-mail]>
>> We would like to enable the use of siw on top of a VPN that is
>> constructed and managed via a tun device. That hasn't worked up
>> until now because ARPHRD_NONE devices (such as tun devices) have
>> no GID for the RDMA/core to look up.
>> But it turns out that the egress device has already been picked for
>> us. addr_handler() just has to do the right thing with it.
>> Suggested-by: Jason Gunthorpe <[e-mail]>
>> Signed-off-by: Chuck Lever <[e-mail]>
>> ---
>>  drivers/infiniband/core/cma.c |    4 ++++
>>  1 file changed, 4 insertions(+)
>> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
>> index 56e568fcd32b..3351dc5afa17 100644
>> --- a/drivers/infiniband/core/cma.c
>> +++ b/drivers/infiniband/core/cma.c
>> @@ -704,11 +704,15 @@ cma_validate_port(struct ib_device *device, u32 port,
>>   ndev = dev_get_by_index(dev_addr->net, bound_if_index);
>>   if (!ndev)
>>   return ERR_PTR(-ENODEV);
>> + } else if (dev_type == ARPHRD_NONE) {
>> + sgid_attr = rdma_get_gid_attr(device, port, 0);
>> + goto out;
>>   } else {
>>   gid_type = IB_GID_TYPE_IB;
>>   }
>>     sgid_attr = rdma_find_gid_by_port(device, gid, gid_type, port, ndev);
>> +out:
>>   dev_put(ndev);
>>   return sgid_attr;
>>  }
> 
> I like it, but doesn't this test in siw_main.c also need to change?
> 
> static struct siw_device *siw_device_create(struct net_device *netdev)
> {
> ...
> --> if (netdev->type != ARPHRD_LOOPBACK && netdev->type != ARPHRD_NONE) {
> addrconf_addr_eui48((unsigned char *)&base_dev->node_guid,
>    netdev->dev_addr);
> } else {
> /*
> * This device does not have a HW address,
> * but connection mangagement lib expects gid != 0
> */
> 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);
> }

I'm not sure that code does anything. The base_dev's name field
is actually not initialized at that point, so nothing is copied
here.

If you're asking whether siw needs to build a non-zero GID to
make the posted patch work, more testing is needed; but I don't
believe the GID has any relevance -- the egress ib_device is
selected based entirely on the source IP address in this case.


--
Chuck Lever

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

> -----Original Message-----
> From: Chuck Lever III <[e-mail]>
> Sent: Saturday, 3 June 2023 02:33
> To: Tom Talpey <[e-mail]>
> Cc: Chuck Lever <[e-mail]>; Jason Gunthorpe <[e-mail]>; linux-
> rdma <[e-mail]>; Bernard Metzler <[e-mail]>;
> [e-mail]
> Subject: [EXTERNAL] Re: [PATCH RFC] RDMA/core: Handle ARPHRD_NONE devices
> 
> 
> 
> > On Jun 2, 2023, at 6:18 PM, Tom Talpey <[e-mail]> wrote:
> >
> > On 6/2/2023 3:24 PM, Chuck Lever wrote:
> >> From: Chuck Lever <[e-mail]>
> >> We would like to enable the use of siw on top of a VPN that is
> >> constructed and managed via a tun device. That hasn't worked up
> >> until now because ARPHRD_NONE devices (such as tun devices) have
> >> no GID for the RDMA/core to look up.
> >> But it turns out that the egress device has already been picked for
> >> us. addr_handler() just has to do the right thing with it.
> >> Suggested-by: Jason Gunthorpe <[e-mail]>
> >> Signed-off-by: Chuck Lever <[e-mail]>
> >> ---
> >>  drivers/infiniband/core/cma.c |    4 ++++
> >>  1 file changed, 4 insertions(+)
> >> diff --git a/drivers/infiniband/core/cma.c
> b/drivers/infiniband/core/cma.c
> >> index 56e568fcd32b..3351dc5afa17 100644
> >> --- a/drivers/infiniband/core/cma.c
> >> +++ b/drivers/infiniband/core/cma.c
> >> @@ -704,11 +704,15 @@ cma_validate_port(struct ib_device *device, u32
> port,
> >>   ndev = dev_get_by_index(dev_addr->net, bound_if_index);
> >>   if (!ndev)
> >>   return ERR_PTR(-ENODEV);
> >> + } else if (dev_type == ARPHRD_NONE) {
> >> + sgid_attr = rdma_get_gid_attr(device, port, 0);
> >> + goto out;
> >>   } else {
> >>   gid_type = IB_GID_TYPE_IB;
> >>   }
> >>     sgid_attr = rdma_find_gid_by_port(device, gid, gid_type, port,
> ndev);
> >> +out:
> >>   dev_put(ndev);
> >>   return sgid_attr;
> >>  }
> >
> > I like it, but doesn't this test in siw_main.c also need to change?
> >
> > static struct siw_device *siw_device_create(struct net_device *netdev)
> > {
> > ...
> > --> if (netdev->type != ARPHRD_LOOPBACK && netdev->type != ARPHRD_NONE) {
> > addrconf_addr_eui48((unsigned char *)&base_dev->node_guid,
> >    netdev->dev_addr);
> > } else {
> > /*
> > * This device does not have a HW address,
> > * but connection mangagement lib expects gid != 0
> > */
> > 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);
> > }
> 
> I'm not sure that code does anything. The base_dev's name field
> is actually not initialized at that point, so nothing is copied
> here.
> 
Oh in that case it’s an issue here.

> If you're asking whether siw needs to build a non-zero GID to
> make the posted patch work, more testing is needed; but I don't
> believe the GID has any relevance -- the egress ib_device is
> selected based entirely on the source IP address in this case.
> 

The whole GID based address resolution I think is an
artefact of IB/RoCE address handling. iWarp is supposed to
run on TCP streams, which endpoints are well defined by L3
addresses. IP routing shall define the outgoing interface...
siw tries to play well and invents GIDs to satisfy
the RDMA core concepts. But a GID is not part of the iWarp
concept. I am not sure for 'real' HW iWarp devices, but to
me it looks like the iwcm code could be done more
independently, if no application expects valid GIDs.

Chuck Lever June 3, 2023, 1:53 p.m. UTC | #4

> On Jun 3, 2023, at 9:51 AM, Bernard Metzler <[e-mail]> wrote:
> 
> 
> 
>> -----Original Message-----
>> From: Chuck Lever III <[e-mail]>
>> Sent: Saturday, 3 June 2023 02:33
>> To: Tom Talpey <[e-mail]>
>> Cc: Chuck Lever <[e-mail]>; Jason Gunthorpe <[e-mail]>; linux-
>> rdma <[e-mail]>; Bernard Metzler <[e-mail]>;
>> [e-mail]
>> Subject: [EXTERNAL] Re: [PATCH RFC] RDMA/core: Handle ARPHRD_NONE devices
>> 
>> 
>> 
>>> On Jun 2, 2023, at 6:18 PM, Tom Talpey <[e-mail]> wrote:
>>> 
>>> On 6/2/2023 3:24 PM, Chuck Lever wrote:
>>>> From: Chuck Lever <[e-mail]>
>>>> We would like to enable the use of siw on top of a VPN that is
>>>> constructed and managed via a tun device. That hasn't worked up
>>>> until now because ARPHRD_NONE devices (such as tun devices) have
>>>> no GID for the RDMA/core to look up.
>>>> But it turns out that the egress device has already been picked for
>>>> us. addr_handler() just has to do the right thing with it.
>>>> Suggested-by: Jason Gunthorpe <[e-mail]>
>>>> Signed-off-by: Chuck Lever <[e-mail]>
>>>> ---
>>>> drivers/infiniband/core/cma.c |    4 ++++
>>>> 1 file changed, 4 insertions(+)
>>>> diff --git a/drivers/infiniband/core/cma.c
>> b/drivers/infiniband/core/cma.c
>>>> index 56e568fcd32b..3351dc5afa17 100644
>>>> --- a/drivers/infiniband/core/cma.c
>>>> +++ b/drivers/infiniband/core/cma.c
>>>> @@ -704,11 +704,15 @@ cma_validate_port(struct ib_device *device, u32
>> port,
>>>>  ndev = dev_get_by_index(dev_addr->net, bound_if_index);
>>>>  if (!ndev)
>>>>  return ERR_PTR(-ENODEV);
>>>> + } else if (dev_type == ARPHRD_NONE) {
>>>> + sgid_attr = rdma_get_gid_attr(device, port, 0);
>>>> + goto out;
>>>>  } else {
>>>>  gid_type = IB_GID_TYPE_IB;
>>>>  }
>>>>    sgid_attr = rdma_find_gid_by_port(device, gid, gid_type, port,
>> ndev);
>>>> +out:
>>>>  dev_put(ndev);
>>>>  return sgid_attr;
>>>> }
>>> 
>>> I like it, but doesn't this test in siw_main.c also need to change?
>>> 
>>> static struct siw_device *siw_device_create(struct net_device *netdev)
>>> {
>>> ...
>>> --> if (netdev->type != ARPHRD_LOOPBACK && netdev->type != ARPHRD_NONE) {
>>> addrconf_addr_eui48((unsigned char *)&base_dev->node_guid,
>>>   netdev->dev_addr);
>>> } else {
>>> /*
>>> * This device does not have a HW address,
>>> * but connection mangagement lib expects gid != 0
>>> */
>>> 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);
>>> }
>> 
>> I'm not sure that code does anything. The base_dev's name field
>> is actually not initialized at that point, so nothing is copied
>> here.
>> 
> Oh in that case it’s an issue here.

I have a patch that fabricates a proper GID here that I can
post separately.


>> If you're asking whether siw needs to build a non-zero GID to
>> make the posted patch work, more testing is needed; but I don't
>> believe the GID has any relevance -- the egress ib_device is
>> selected based entirely on the source IP address in this case.
>> 
> 
> The whole GID based address resolution I think is an
> artefact of IB/RoCE address handling. iWarp is supposed to
> run on TCP streams, which endpoints are well defined by L3
> addresses. IP routing shall define the outgoing interface...
> siw tries to play well and invents GIDs to satisfy
> the RDMA core concepts. But a GID is not part of the iWarp
> concept. I am not sure for 'real' HW iWarp devices, but to
> me it looks like the iwcm code could be done more
> independently, if no application expects valid GIDs.


--
Chuck Lever

Bernard Metzler June 3, 2023, 1:55 p.m. UTC | #5

> -----Original Message-----
> From: Chuck Lever III <[e-mail]>
> Sent: Saturday, 3 June 2023 15:53
> To: Bernard Metzler <[e-mail]>
> Cc: Tom Talpey <[e-mail]>; Chuck Lever <[e-mail]>; Jason
> Gunthorpe <[e-mail]>; linux-rdma <[e-mail]>;
> [e-mail]
> Subject: [EXTERNAL] Re: [PATCH RFC] RDMA/core: Handle ARPHRD_NONE devices
> 
> 
> 
> > On Jun 3, 2023, at 9:51 AM, Bernard Metzler <[e-mail]> wrote:
> >
> >
> >
> >> -----Original Message-----
> >> From: Chuck Lever III <[e-mail]>
> >> Sent: Saturday, 3 June 2023 02:33
> >> To: Tom Talpey <[e-mail]>
> >> Cc: Chuck Lever <[e-mail]>; Jason Gunthorpe <[e-mail]>;
> linux-
> >> rdma <[e-mail]>; Bernard Metzler <[e-mail]>;
> >> [e-mail]
> >> Subject: [EXTERNAL] Re: [PATCH RFC] RDMA/core: Handle ARPHRD_NONE
> devices
> >>
> >>
> >>
> >>> On Jun 2, 2023, at 6:18 PM, Tom Talpey <[e-mail]> wrote:
> >>>
> >>> On 6/2/2023 3:24 PM, Chuck Lever wrote:
> >>>> From: Chuck Lever <[e-mail]>
> >>>> We would like to enable the use of siw on top of a VPN that is
> >>>> constructed and managed via a tun device. That hasn't worked up
> >>>> until now because ARPHRD_NONE devices (such as tun devices) have
> >>>> no GID for the RDMA/core to look up.
> >>>> But it turns out that the egress device has already been picked for
> >>>> us. addr_handler() just has to do the right thing with it.
> >>>> Suggested-by: Jason Gunthorpe <[e-mail]>
> >>>> Signed-off-by: Chuck Lever <[e-mail]>
> >>>> ---
> >>>> drivers/infiniband/core/cma.c |    4 ++++
> >>>> 1 file changed, 4 insertions(+)
> >>>> diff --git a/drivers/infiniband/core/cma.c
> >> b/drivers/infiniband/core/cma.c
> >>>> index 56e568fcd32b..3351dc5afa17 100644
> >>>> --- a/drivers/infiniband/core/cma.c
> >>>> +++ b/drivers/infiniband/core/cma.c
> >>>> @@ -704,11 +704,15 @@ cma_validate_port(struct ib_device *device, u32
> >> port,
> >>>>  ndev = dev_get_by_index(dev_addr->net, bound_if_index);
> >>>>  if (!ndev)
> >>>>  return ERR_PTR(-ENODEV);
> >>>> + } else if (dev_type == ARPHRD_NONE) {
> >>>> + sgid_attr = rdma_get_gid_attr(device, port, 0);
> >>>> + goto out;
> >>>>  } else {
> >>>>  gid_type = IB_GID_TYPE_IB;
> >>>>  }
> >>>>    sgid_attr = rdma_find_gid_by_port(device, gid, gid_type, port,
> >> ndev);
> >>>> +out:
> >>>>  dev_put(ndev);
> >>>>  return sgid_attr;
> >>>> }
> >>>
> >>> I like it, but doesn't this test in siw_main.c also need to change?
> >>>
> >>> static struct siw_device *siw_device_create(struct net_device *netdev)
> >>> {
> >>> ...
> >>> --> if (netdev->type != ARPHRD_LOOPBACK && netdev->type != ARPHRD_NONE)
> {
> >>> addrconf_addr_eui48((unsigned char *)&base_dev->node_guid,
> >>>   netdev->dev_addr);
> >>> } else {
> >>> /*
> >>> * This device does not have a HW address,
> >>> * but connection mangagement lib expects gid != 0
> >>> */
> >>> 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);
> >>> }
> >>
> >> I'm not sure that code does anything. The base_dev's name field
> >> is actually not initialized at that point, so nothing is copied
> >> here.
> >>
> > Oh in that case it’s an issue here.
> 
> I have a patch that fabricates a proper GID here that I can
> post separately.
> 
Sounds good!
> 
> >> If you're asking whether siw needs to build a non-zero GID to
> >> make the posted patch work, more testing is needed; but I don't
> >> believe the GID has any relevance -- the egress ib_device is
> >> selected based entirely on the source IP address in this case.
> >>
> >
> > The whole GID based address resolution I think is an
> > artefact of IB/RoCE address handling. iWarp is supposed to
> > run on TCP streams, which endpoints are well defined by L3
> > addresses. IP routing shall define the outgoing interface...
> > siw tries to play well and invents GIDs to satisfy
> > the RDMA core concepts. But a GID is not part of the iWarp
> > concept. I am not sure for 'real' HW iWarp devices, but to
> > me it looks like the iwcm code could be done more
> > independently, if no application expects valid GIDs.
> 
> 
> --
> Chuck Lever
>

Jason Gunthorpe June 5, 2023, 7:05 p.m. UTC | #6

On Sat, Jun 03, 2023 at 01:51:10PM +0000, Bernard Metzler wrote:
> The whole GID based address resolution I think is an
> artefact of IB/RoCE address handling. iWarp is supposed to
> run on TCP streams, which endpoints are well defined by L3
> addresses. IP routing shall define the outgoing interface...
> siw tries to play well and invents GIDs to satisfy
> the RDMA core concepts. But a GID is not part of the iWarp
> concept. I am not sure for 'real' HW iWarp devices, but to
> me it looks like the iwcm code could be done more
> independently, if no application expects valid GIDs.

GIDs are part of verbs, but I'm not really sure how iwarp fit itself
into verbs with the mandatory CM and all. Possibly the only purpose of
the GID is to convey some information from the CM to uverbs layer.

Jason

Jason Gunthorpe June 6, 2023, 3:48 p.m. UTC | #7

On Fri, Jun 02, 2023 at 03:24:30PM -0400, Chuck Lever wrote:
> From: Chuck Lever <[e-mail]>
> 
> We would like to enable the use of siw on top of a VPN that is
> constructed and managed via a tun device. That hasn't worked up
> until now because ARPHRD_NONE devices (such as tun devices) have
> no GID for the RDMA/core to look up.
> 
> But it turns out that the egress device has already been picked for
> us. addr_handler() just has to do the right thing with it.
> 
> Suggested-by: Jason Gunthorpe <[e-mail]>
> Signed-off-by: Chuck Lever <[e-mail]>
> ---
>  drivers/infiniband/core/cma.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> index 56e568fcd32b..3351dc5afa17 100644
> --- a/drivers/infiniband/core/cma.c
> +++ b/drivers/infiniband/core/cma.c
> @@ -704,11 +704,15 @@ cma_validate_port(struct ib_device *device, u32 port,
>  		ndev = dev_get_by_index(dev_addr->net, bound_if_index);
>  		if (!ndev)
>  			return ERR_PTR(-ENODEV);
> +	} else if (dev_type == ARPHRD_NONE) {
> +		sgid_attr = rdma_get_gid_attr(device, port, 0);

It seems believable, should it be locked to iwarp devices?

More broadly, should iwarp devices just always do this and skip all
the rest of it?

I think it also has to check that the returned netdev in the sgid_attr
matches the egress netdev selected?

Jason

Chuck Lever June 6, 2023, 8:15 p.m. UTC | #8

> On Jun 6, 2023, at 11:48 AM, Jason Gunthorpe <[e-mail]> wrote:
> 
> On Fri, Jun 02, 2023 at 03:24:30PM -0400, Chuck Lever wrote:
>> From: Chuck Lever <[e-mail]>
>> 
>> We would like to enable the use of siw on top of a VPN that is
>> constructed and managed via a tun device. That hasn't worked up
>> until now because ARPHRD_NONE devices (such as tun devices) have
>> no GID for the RDMA/core to look up.
>> 
>> But it turns out that the egress device has already been picked for
>> us. addr_handler() just has to do the right thing with it.
>> 
>> Suggested-by: Jason Gunthorpe <[e-mail]>
>> Signed-off-by: Chuck Lever <[e-mail]>
>> ---
>> drivers/infiniband/core/cma.c |    4 ++++
>> 1 file changed, 4 insertions(+)
>> 
>> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
>> index 56e568fcd32b..3351dc5afa17 100644
>> --- a/drivers/infiniband/core/cma.c
>> +++ b/drivers/infiniband/core/cma.c
>> @@ -704,11 +704,15 @@ cma_validate_port(struct ib_device *device, u32 port,
>> ndev = dev_get_by_index(dev_addr->net, bound_if_index);
>> if (!ndev)
>> return ERR_PTR(-ENODEV);
>> + } else if (dev_type == ARPHRD_NONE) {
>> + sgid_attr = rdma_get_gid_attr(device, port, 0);
> 
> It seems believable, should it be locked to iwarp devices?
> 
> More broadly, should iwarp devices just always do this and skip all
> the rest of it?
> 
> I think it also has to check that the returned netdev in the sgid_attr
> matches the egress netdev selected?

Both @ndev and sgid_attr.ndev are NULL here. So I assume you want
the code to check, on return, that sgid_attr.device == device?


--
Chuck Lever

Jason Gunthorpe June 6, 2023, 11:17 p.m. UTC | #9

On Tue, Jun 06, 2023 at 08:15:36PM +0000, Chuck Lever III wrote:
> 
> 
> > On Jun 6, 2023, at 11:48 AM, Jason Gunthorpe <[e-mail]> wrote:
> > 
> > On Fri, Jun 02, 2023 at 03:24:30PM -0400, Chuck Lever wrote:
> >> From: Chuck Lever <[e-mail]>
> >> 
> >> We would like to enable the use of siw on top of a VPN that is
> >> constructed and managed via a tun device. That hasn't worked up
> >> until now because ARPHRD_NONE devices (such as tun devices) have
> >> no GID for the RDMA/core to look up.
> >> 
> >> But it turns out that the egress device has already been picked for
> >> us. addr_handler() just has to do the right thing with it.
> >> 
> >> Suggested-by: Jason Gunthorpe <[e-mail]>
> >> Signed-off-by: Chuck Lever <[e-mail]>
> >> ---
> >> drivers/infiniband/core/cma.c |    4 ++++
> >> 1 file changed, 4 insertions(+)
> >> 
> >> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> >> index 56e568fcd32b..3351dc5afa17 100644
> >> --- a/drivers/infiniband/core/cma.c
> >> +++ b/drivers/infiniband/core/cma.c
> >> @@ -704,11 +704,15 @@ cma_validate_port(struct ib_device *device, u32 port,
> >> ndev = dev_get_by_index(dev_addr->net, bound_if_index);
> >> if (!ndev)
> >> return ERR_PTR(-ENODEV);
> >> + } else if (dev_type == ARPHRD_NONE) {
> >> + sgid_attr = rdma_get_gid_attr(device, port, 0);
> > 
> > It seems believable, should it be locked to iwarp devices?
> > 
> > More broadly, should iwarp devices just always do this and skip all
> > the rest of it?
> > 
> > I think it also has to check that the returned netdev in the sgid_attr
> > matches the egress netdev selected?
> 
> Both @ndev and sgid_attr.ndev are NULL here. 

The nedev to check is the dev_addr->bound_dev_if, that represents the netdev

It is some iwarp mistake that the sgid attr's don't have proper
netdevs, that is newer than iwarp so it never got updated.

So, maybe it is too hard to fix for this, and maybe we can just assume
it all has to be right

Jason

Chuck Lever June 7, 2023, 2:48 p.m. UTC | #10

> On Jun 6, 2023, at 7:17 PM, Jason Gunthorpe <[e-mail]> wrote:
> 
> On Tue, Jun 06, 2023 at 08:15:36PM +0000, Chuck Lever III wrote:
>> 
>> 
>>> On Jun 6, 2023, at 11:48 AM, Jason Gunthorpe <[e-mail]> wrote:
>>> 
>>> On Fri, Jun 02, 2023 at 03:24:30PM -0400, Chuck Lever wrote:
>>>> From: Chuck Lever <[e-mail]>
>>>> 
>>>> We would like to enable the use of siw on top of a VPN that is
>>>> constructed and managed via a tun device. That hasn't worked up
>>>> until now because ARPHRD_NONE devices (such as tun devices) have
>>>> no GID for the RDMA/core to look up.
>>>> 
>>>> But it turns out that the egress device has already been picked for
>>>> us. addr_handler() just has to do the right thing with it.
>>>> 
>>>> Suggested-by: Jason Gunthorpe <[e-mail]>
>>>> Signed-off-by: Chuck Lever <[e-mail]>
>>>> ---
>>>> drivers/infiniband/core/cma.c |    4 ++++
>>>> 1 file changed, 4 insertions(+)
>>>> 
>>>> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
>>>> index 56e568fcd32b..3351dc5afa17 100644
>>>> --- a/drivers/infiniband/core/cma.c
>>>> +++ b/drivers/infiniband/core/cma.c
>>>> @@ -704,11 +704,15 @@ cma_validate_port(struct ib_device *device, u32 port,
>>>> ndev = dev_get_by_index(dev_addr->net, bound_if_index);
>>>> if (!ndev)
>>>> return ERR_PTR(-ENODEV);
>>>> + } else if (dev_type == ARPHRD_NONE) {
>>>> + sgid_attr = rdma_get_gid_attr(device, port, 0);
>>> 
>>> It seems believable, should it be locked to iwarp devices?
>>> 
>>> More broadly, should iwarp devices just always do this and skip all
>>> the rest of it?
>>> 
>>> I think it also has to check that the returned netdev in the sgid_attr
>>> matches the egress netdev selected?
>> 
>> Both @ndev and sgid_attr.ndev are NULL here. 
> 
> The nedev to check is the dev_addr->bound_dev_if, that represents the netdev
> 
> It is some iwarp mistake that the sgid attr's don't have proper
> netdevs, that is newer than iwarp so it never got updated.
> 
> So, maybe it is too hard to fix for this, and maybe we can just assume
> it all has to be right

sgid_attr.index is not set it looks like. I will send you an "official"
version of this patch and we can go from there.


--
Chuck Lever

Patch

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 56e568fcd32b..3351dc5afa17 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -704,11 +704,15 @@  cma_validate_port(struct ib_device *device, u32 port,
 		ndev = dev_get_by_index(dev_addr->net, bound_if_index);
 		if (!ndev)
 			return ERR_PTR(-ENODEV);
+	} else if (dev_type == ARPHRD_NONE) {
+		sgid_attr = rdma_get_gid_attr(device, port, 0);
+		goto out;
 	} else {
 		gid_type = IB_GID_TYPE_IB;
 	}
 
 	sgid_attr = rdma_find_gid_by_port(device, gid, gid_type, port, ndev);
+out:
 	dev_put(ndev);
 	return sgid_attr;
 }