[rdma-rc,3/9] Revert "RDMA/cma: Simplify rdma_resolve_addr() error flow"
diff mbox series

Message ID 20200212072635.682689-4-leon@kernel.org
State Accepted
Delegated to: Jason Gunthorpe
Headers show
Series
  • Fixes for v5.6
Related show

Commit Message

Leon Romanovsky Feb. 12, 2020, 7:26 a.m. UTC
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(-)

Comments

Jason Gunthorpe Feb. 13, 2020, 1:30 p.m. UTC | #1
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
Parav Pandit Feb. 14, 2020, 3:11 a.m. UTC | #2
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.
Jason Gunthorpe Feb. 14, 2020, 2:08 p.m. UTC | #3
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
Parav Pandit Feb. 14, 2020, 2:48 p.m. UTC | #4
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
>
Jason Gunthorpe Feb. 19, 2020, 8:40 p.m. UTC | #5
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

Patch
diff mbox series

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 {