On Mon, Mar 05, 2018 at 11:59:06AM -0600, Shiraz Saleem wrote:
> From: Tatyana Nikolova <tatyana.e.nikolova@xxxxxxxxx>
>
> cma_port_is_unique() allows local port reuse if the quad (source
> address and port, destination address and port) for this connection
> is unique. However, if the destination info is zero or unspecified, it
> can't make a correct decision but still allows port reuse. For example,
> sometimes rdma_bind_addr() is called with unspecified destination and
> reusing the port can lead to creating a connection with a duplicate quad,
> after the destination is resolved. The issue manifests when MPI scale-up
> tests hang after the duplicate quad is used.
>
> Add checks for unspecified or zero destination address and port to
> prevent source port reuse based on invalid destination.
>
> Fixes: 19b752a19dce ("IB/cma: Allow port reuse for rdma_id")
> Reviewed-by: Sean Hefty <sean.hefty@xxxxxxxxx>
> Signed-off-by: Tatyana Nikolova <tatyana.e.nikolova@xxxxxxxxx>
> Signed-off-by: Shiraz Saleem <shiraz.saleem@xxxxxxxxx>
> drivers/infiniband/core/cma.c | 16 ++++++++++------
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> index 6294a70..bb8f0e2 100644
> +++ b/drivers/infiniband/core/cma.c
> @@ -1047,6 +1047,8 @@ EXPORT_SYMBOL(rdma_init_qp_attr);
> static inline int cma_zero_addr(struct sockaddr *addr)
> {
> switch (addr->sa_family) {
> + case AF_UNSPEC:
> + return 1;
I can't figure out why this is here?
I thought an 'unspecified destination' shouldn't be AF_UNSPEC. It is
AF_INETxx and a zero address in CMA? Where can AF_UNSPEC be set that
trips this up?
It seems really wierd to call AF_UNSPEC 'zero'..
And if you change this then it means cma_any_addr() becomes true for
AF_UNSPEC.. And that seems like it would have impact beyond just
cma_port_is_unique,
eg here:
static bool cma_match_private_data(struct rdma_id_private *id_priv,
const struct cma_hdr *hdr)
{
if (cma_any_addr(addr) && !id_priv->afonly)
return true;
Now AF_UNSPEC returns true, where before it was false.. Seems
concerning.
This really needs more explanation please.
> /* different dest port -> unique */
> - if (!cma_any_port(cur_daddr) &&
> + if (!cma_any_port(daddr) &&
> + !cma_any_port(cur_daddr) &&
> (dport != cur_dport))
> continue;
> + /* different dst address -> unique */
> + if (!cma_any_addr(daddr) &&
> + !cma_any_addr(cur_daddr) &&
> + cma_addr_cmp(daddr, cur_daddr))
> + continue;
Why re-order so much stuff? The diff could be much smaller.
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
