| Message ID | 20200212072635.682689-4-leon@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | Fixes for v5.6 | expand |
On Wed, Feb 12, 2020 at 09:26:29AM +0200, Leon Romanovsky wrote: > From: Parav Pandit <parav@mellanox.com> > > This reverts commit 219d2e9dfda9431b808c28d5efc74b404b95b638. > > Below flow requires cm_id_priv's destination address to be setup > before performing rdma_bind_addr(). > Without which, source port allocation for existing bind list fails > when port range is small, resulting into rdma_resolve_addr() > failure. I don't quite understands this - what is "when port range is small" ? > rdma_resolve_addr() > cma_bind_addr() > rdma_bind_addr() > cma_get_port() > cma_alloc_any_port() > cma_port_is_unique() <- compared with zero daddr Do you understand why cma_port_is_unique is even testing the dst_addr? It seems very strange. Outbound connections should not alias the source port in the first place?? Jason
On 2/13/2020 7:30 AM, Jason Gunthorpe wrote: > On Wed, Feb 12, 2020 at 09:26:29AM +0200, Leon Romanovsky wrote: >> From: Parav Pandit <parav@mellanox.com> >> >> This reverts commit 219d2e9dfda9431b808c28d5efc74b404b95b638. >> >> Below flow requires cm_id_priv's destination address to be setup >> before performing rdma_bind_addr(). >> Without which, source port allocation for existing bind list fails >> when port range is small, resulting into rdma_resolve_addr() >> failure. > > I don't quite understands this - what is "when port range is small" ? > There is sysfs knob to reduce source port range to use for binding. So it is easy to create the issue by reducing port range to handful of them. >> rdma_resolve_addr() >> cma_bind_addr() >> rdma_bind_addr() >> cma_get_port() >> cma_alloc_any_port() >> cma_port_is_unique() <- compared with zero daddr > > Do you understand why cma_port_is_unique is even testing the dst_addr? > It seems very strange. ma_port_unique() reuses the source port as long as destination is different (destination = either different dest.addr or different dest.port between two cm_ids ). This behavior is synonymous to TCP also. > Outbound connections should not alias the source port in the first > place?? > I believe it can because TCP seems to allow that too as long destination is different. I think it allows if it results into different 4-tuple.
On Fri, Feb 14, 2020 at 03:11:48AM +0000, Parav Pandit wrote: > On 2/13/2020 7:30 AM, Jason Gunthorpe wrote: > > On Wed, Feb 12, 2020 at 09:26:29AM +0200, Leon Romanovsky wrote: > >> From: Parav Pandit <parav@mellanox.com> > >> > >> This reverts commit 219d2e9dfda9431b808c28d5efc74b404b95b638. > >> > >> Below flow requires cm_id_priv's destination address to be setup > >> before performing rdma_bind_addr(). > >> Without which, source port allocation for existing bind list fails > >> when port range is small, resulting into rdma_resolve_addr() > >> failure. > > > > I don't quite understands this - what is "when port range is small" ? > > > There is sysfs knob to reduce source port range to use for binding. > So it is easy to create the issue by reducing port range to handful of them. > > >> rdma_resolve_addr() > >> cma_bind_addr() > >> rdma_bind_addr() > >> cma_get_port() > >> cma_alloc_any_port() > >> cma_port_is_unique() <- compared with zero daddr > > > > Do you understand why cma_port_is_unique is even testing the dst_addr? > > It seems very strange. > > ma_port_unique() reuses the source port as long as > destination is different (destination = either different > dest.addr or different dest.port between two cm_ids ). > > This behavior is synonymous to TCP also. > > > Outbound connections should not alias the source port in the first > > place?? > > > I believe it can because TCP seems to allow that too as long destination > is different. > > I think it allows if it results into different 4-tuple. So the issue here is if the dest is left as 0 then the cma_alloc_any_port() isn't able to alias source ports any more? Jason
On 2/14/2020 8:08 AM, Jason Gunthorpe wrote: > On Fri, Feb 14, 2020 at 03:11:48AM +0000, Parav Pandit wrote: >> On 2/13/2020 7:30 AM, Jason Gunthorpe wrote: >>> On Wed, Feb 12, 2020 at 09:26:29AM +0200, Leon Romanovsky wrote: >>>> From: Parav Pandit <parav@mellanox.com> >>>> >>>> This reverts commit 219d2e9dfda9431b808c28d5efc74b404b95b638. >>>> >>>> Below flow requires cm_id_priv's destination address to be setup >>>> before performing rdma_bind_addr(). >>>> Without which, source port allocation for existing bind list fails >>>> when port range is small, resulting into rdma_resolve_addr() >>>> failure. >>> >>> I don't quite understands this - what is "when port range is small" ? >>> >> There is sysfs knob to reduce source port range to use for binding. >> So it is easy to create the issue by reducing port range to handful of them. >> >>>> rdma_resolve_addr() >>>> cma_bind_addr() >>>> rdma_bind_addr() >>>> cma_get_port() >>>> cma_alloc_any_port() >>>> cma_port_is_unique() <- compared with zero daddr >>> >>> Do you understand why cma_port_is_unique is even testing the dst_addr? >>> It seems very strange. >> >> ma_port_unique() reuses the source port as long as >> destination is different (destination = either different >> dest.addr or different dest.port between two cm_ids ). >> >> This behavior is synonymous to TCP also. >> >>> Outbound connections should not alias the source port in the first >>> place?? >>> >> I believe it can because TCP seems to allow that too as long destination >> is different. >> >> I think it allows if it results into different 4-tuple. > > So the issue here is if the dest is left as 0 then the > cma_alloc_any_port() isn't able to alias source ports any more? > Correct. > Jason >
On Wed, Feb 12, 2020 at 09:26:29AM +0200, Leon Romanovsky wrote: > From: Parav Pandit <parav@mellanox.com> > > This reverts commit 219d2e9dfda9431b808c28d5efc74b404b95b638. > > Below flow requires cm_id_priv's destination address to be setup > before performing rdma_bind_addr(). > Without which, source port allocation for existing bind list fails > when port range is small, resulting into rdma_resolve_addr() > failure. > > rdma_resolve_addr() > cma_bind_addr() > rdma_bind_addr() > cma_get_port() > cma_alloc_any_port() > cma_port_is_unique() <- compared with zero daddr > > Fixes: 219d2e9dfda9 ("RDMA/cma: Simplify rdma_resolve_addr() error flow") > Signed-off-by: Parav Pandit <parav@mellanox.com> > Signed-off-by: Leon Romanovsky <leonro@mellanox.com> > --- > drivers/infiniband/core/cma.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) Applied to for-rc Thanks, Jason
diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c index 72f032160c4b..2dec3a02ab9f 100644 --- a/drivers/infiniband/core/cma.c +++ b/drivers/infiniband/core/cma.c @@ -3212,19 +3212,26 @@ int rdma_resolve_addr(struct rdma_cm_id *id, struct sockaddr *src_addr, int ret; id_priv = container_of(id, struct rdma_id_private, id); + memcpy(cma_dst_addr(id_priv), dst_addr, rdma_addr_size(dst_addr)); if (id_priv->state == RDMA_CM_IDLE) { ret = cma_bind_addr(id, src_addr, dst_addr); - if (ret) + if (ret) { + memset(cma_dst_addr(id_priv), 0, + rdma_addr_size(dst_addr)); return ret; + } } - if (cma_family(id_priv) != dst_addr->sa_family) + if (cma_family(id_priv) != dst_addr->sa_family) { + memset(cma_dst_addr(id_priv), 0, rdma_addr_size(dst_addr)); return -EINVAL; + } - if (!cma_comp_exch(id_priv, RDMA_CM_ADDR_BOUND, RDMA_CM_ADDR_QUERY)) + if (!cma_comp_exch(id_priv, RDMA_CM_ADDR_BOUND, RDMA_CM_ADDR_QUERY)) { + memset(cma_dst_addr(id_priv), 0, rdma_addr_size(dst_addr)); return -EINVAL; + } - memcpy(cma_dst_addr(id_priv), dst_addr, rdma_addr_size(dst_addr)); if (cma_any_addr(dst_addr)) { ret = cma_resolve_loopback(id_priv); } else {