| Message ID | 168675125998.2279.7297073638926155456.stgit@manet.1015granger.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Handle ARPHRD_NONE devices for siw | expand |
On Wed, Jun 14, 2023 at 10:00:59AM -0400, Chuck Lever wrote: > From: Chuck Lever <chuck.lever@oracle.com> > > 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 -- no GID is necessary. addr_handler() just has to do the right > thing with it. > > Suggested-by: Jason Gunthorpe <jgg@nvidia.com> > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > --- > drivers/infiniband/core/cma.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c > index a1756ed1faa1..50b8da2c4720 100644 > --- a/drivers/infiniband/core/cma.c > +++ b/drivers/infiniband/core/cma.c > @@ -700,6 +700,19 @@ cma_validate_port(struct ib_device *device, u32 port, > if ((dev_type != ARPHRD_INFINIBAND) && rdma_protocol_ib(device, port)) > goto out; > > + if (rdma_protocol_iwarp(device, port)) { > + sgid_attr = rdma_get_gid_attr(device, port, 0); > + if (IS_ERR(sgid_attr)) > + goto out; > + > + /* XXX: I don't think this is RCU-safe, but does it need to be? */ Maybe for subtle reasons related to iwarp it is safe, but it should make a lockdep splat since you are not holding rcu? Remove the comment and do a rcu_lock/unlock around this and the deref Jason
> On Jun 22, 2023, at 11:13 AM, Jason Gunthorpe <jgg@nvidia.com> wrote: > > On Wed, Jun 14, 2023 at 10:00:59AM -0400, Chuck Lever wrote: >> From: Chuck Lever <chuck.lever@oracle.com> >> >> 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 -- no GID is necessary. addr_handler() just has to do the right >> thing with it. >> >> Suggested-by: Jason Gunthorpe <jgg@nvidia.com> >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com> >> --- >> drivers/infiniband/core/cma.c | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c >> index a1756ed1faa1..50b8da2c4720 100644 >> --- a/drivers/infiniband/core/cma.c >> +++ b/drivers/infiniband/core/cma.c >> @@ -700,6 +700,19 @@ cma_validate_port(struct ib_device *device, u32 port, >> if ((dev_type != ARPHRD_INFINIBAND) && rdma_protocol_ib(device, port)) >> goto out; >> >> + if (rdma_protocol_iwarp(device, port)) { >> + sgid_attr = rdma_get_gid_attr(device, port, 0); >> + if (IS_ERR(sgid_attr)) >> + goto out; >> + >> + /* XXX: I don't think this is RCU-safe, but does it need to be? */ > > Maybe for subtle reasons related to iwarp it is safe, but it should > make a lockdep splat since you are not holding rcu? > > Remove the comment and do a rcu_lock/unlock around this and the deref Done, v4 posted. The reason I was unsure: CC drivers/infiniband/core/roce_gid_mgmt.o CHECK /home/cel/src/linux/linux/drivers/infiniband/core/roce_gid_mgmt.c /home/cel/src/linux/linux/drivers/infiniband/core/roce_gid_mgmt.c:292:23: warning: incorrect type in assignment (different address spaces) /home/cel/src/linux/linux/drivers/infiniband/core/roce_gid_mgmt.c:292:23: expected struct net_device [noderef] __rcu *[addressable] ndev /home/cel/src/linux/linux/drivers/infiniband/core/roce_gid_mgmt.c:292:23: got struct net_device *ndev /home/cel/src/linux/linux/drivers/infiniband/core/roce_gid_mgmt.c:386:48: warning: incorrect type in initializer (different address spaces) /home/cel/src/linux/linux/drivers/infiniband/core/roce_gid_mgmt.c:386:48: expected struct net_device [noderef] __rcu *ndev /home/cel/src/linux/linux/drivers/infiniband/core/roce_gid_mgmt.c:386:48: got struct net_device *ndev /home/cel/src/linux/linux/drivers/infiniband/core/roce_gid_mgmt.c:811:48: warning: incorrect type in argument 2 (different address spaces) /home/cel/src/linux/linux/drivers/infiniband/core/roce_gid_mgmt.c:811:48: expected void *filter_cookie /home/cel/src/linux/linux/drivers/infiniband/core/roce_gid_mgmt.c:811:48: got struct net_device [noderef] __rcu *ndev /home/cel/src/linux/linux/drivers/infiniband/core/roce_gid_mgmt.c:814:31: warning: incorrect type in argument 1 (different address spaces) /home/cel/src/linux/linux/drivers/infiniband/core/roce_gid_mgmt.c:814:31: expected struct net_device *dev /home/cel/src/linux/linux/drivers/infiniband/core/roce_gid_mgmt.c:814:31: got struct net_device [noderef] __rcu *ndev /home/cel/src/linux/linux/drivers/infiniband/core/roce_gid_mgmt.c:851:31: warning: incorrect type in assignment (different address spaces) /home/cel/src/linux/linux/drivers/infiniband/core/roce_gid_mgmt.c:851:31: expected struct net_device [noderef] __rcu *ndev /home/cel/src/linux/linux/drivers/infiniband/core/roce_gid_mgmt.c:851:31: got struct net_device *ndev -- Chuck Lever
On Fri, Jun 23, 2023 at 05:02:23PM +0000, Chuck Lever III wrote: > > > > On Jun 22, 2023, at 11:13 AM, Jason Gunthorpe <jgg@nvidia.com> wrote: > > > > On Wed, Jun 14, 2023 at 10:00:59AM -0400, Chuck Lever wrote: > >> From: Chuck Lever <chuck.lever@oracle.com> > >> > >> 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 -- no GID is necessary. addr_handler() just has to do the right > >> thing with it. > >> > >> Suggested-by: Jason Gunthorpe <jgg@nvidia.com> > >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > >> --- > >> drivers/infiniband/core/cma.c | 13 +++++++++++++ > >> 1 file changed, 13 insertions(+) > >> > >> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c > >> index a1756ed1faa1..50b8da2c4720 100644 > >> --- a/drivers/infiniband/core/cma.c > >> +++ b/drivers/infiniband/core/cma.c > >> @@ -700,6 +700,19 @@ cma_validate_port(struct ib_device *device, u32 port, > >> if ((dev_type != ARPHRD_INFINIBAND) && rdma_protocol_ib(device, port)) > >> goto out; > >> > >> + if (rdma_protocol_iwarp(device, port)) { > >> + sgid_attr = rdma_get_gid_attr(device, port, 0); > >> + if (IS_ERR(sgid_attr)) > >> + goto out; > >> + > >> + /* XXX: I don't think this is RCU-safe, but does it need to be? */ > > > > Maybe for subtle reasons related to iwarp it is safe, but it should > > make a lockdep splat since you are not holding rcu? > > > > Remove the comment and do a rcu_lock/unlock around this and the deref > > Done, v4 posted. > > The reason I was unsure: > > CC drivers/infiniband/core/roce_gid_mgmt.o > CHECK /home/cel/src/linux/linux/drivers/infiniband/core/roce_gid_mgmt.c > /home/cel/src/linux/linux/drivers/infiniband/core/roce_gid_mgmt.c:292:23: warning: incorrect type in assignment (different address spaces) > /home/cel/src/linux/linux/drivers/infiniband/core/roce_gid_mgmt.c:292:23: expected struct net_device [noderef] __rcu *[addressable] ndev > /home/cel/src/linux/linux/drivers/infiniband/core/roce_gid_mgmt.c:292:23: got struct net_device *ndev > /home/cel/src/linux/linux/drivers/infiniband/core/roce_gid_mgmt.c:386:48: warning: incorrect type in initializer (different address spaces) > /home/cel/src/linux/linux/drivers/infiniband/core/roce_gid_mgmt.c:386:48: expected struct net_device [noderef] __rcu *ndev > /home/cel/src/linux/linux/drivers/infiniband/core/roce_gid_mgmt.c:386:48: got struct net_device *ndev > /home/cel/src/linux/linux/drivers/infiniband/core/roce_gid_mgmt.c:811:48: warning: incorrect type in argument 2 (different address spaces) > /home/cel/src/linux/linux/drivers/infiniband/core/roce_gid_mgmt.c:811:48: expected void *filter_cookie > /home/cel/src/linux/linux/drivers/infiniband/core/roce_gid_mgmt.c:811:48: got struct net_device [noderef] __rcu *ndev > /home/cel/src/linux/linux/drivers/infiniband/core/roce_gid_mgmt.c:814:31: warning: incorrect type in argument 1 (different address spaces) > /home/cel/src/linux/linux/drivers/infiniband/core/roce_gid_mgmt.c:814:31: expected struct net_device *dev > /home/cel/src/linux/linux/drivers/infiniband/core/roce_gid_mgmt.c:814:31: got struct net_device [noderef] __rcu *ndev > /home/cel/src/linux/linux/drivers/infiniband/core/roce_gid_mgmt.c:851:31: warning: incorrect type in assignment (different address spaces) > /home/cel/src/linux/linux/drivers/infiniband/core/roce_gid_mgmt.c:851:31: expected struct net_device [noderef] __rcu *ndev > /home/cel/src/linux/linux/drivers/infiniband/core/roce_gid_mgmt.c:851:31: got struct net_device *ndev Ah, nobody has cleaned that stuff :\ Jason
diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c index a1756ed1faa1..50b8da2c4720 100644 --- a/drivers/infiniband/core/cma.c +++ b/drivers/infiniband/core/cma.c @@ -700,6 +700,19 @@ cma_validate_port(struct ib_device *device, u32 port, if ((dev_type != ARPHRD_INFINIBAND) && rdma_protocol_ib(device, port)) goto out; + if (rdma_protocol_iwarp(device, port)) { + sgid_attr = rdma_get_gid_attr(device, port, 0); + if (IS_ERR(sgid_attr)) + goto out; + + /* XXX: I don't think this is RCU-safe, but does it need to be? */ + ndev = rcu_dereference(sgid_attr->ndev); + if (!net_eq(dev_net(ndev), dev_addr->net) || + ndev->ifindex != bound_if_index) + sgid_attr = ERR_PTR(-ENODEV); + goto out; + } + if (dev_type == ARPHRD_ETHER && rdma_protocol_roce(device, port)) { ndev = dev_get_by_index(dev_addr->net, bound_if_index); if (!ndev)