On Wed, Mar 07, 2018 at 11:52:28PM +0000, Hefty, Sean wrote:
> > > 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?
>
> At the time rdma_bind_addr() is called, the destination address of
> the rdma_cm_id is unset (sa_family = UNSPEC). The daddr->sa_family
> isn't set until the end of bind.
This is surprising to me. You can't bind without also specifying a
family, so why should be family be left as UNSPEC for so long?
Doesn't that just risk bugs where we mix and match AF's incorrectly?
Could we get to the same place without the above case by just
adjusting the bind flow to set the family earlier?
> > It seems really wierd to call AF_UNSPEC 'zero'..
>
> The check for UNSPEC can move out of cma_zero_addr() into
> cma_any_addr(), but that doesn't address the next concern...
>
>
> > 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.
>
> The behavior of cma_any_addr() and cma_any_port() is different for
> UNSPEC. cma_any_addr() returns false, but cma_any_port() returns
> true. The latter results from cma_port() returning 0 in the case of
> UNSPEC (or any unhandled address format). For consistency,
> cma_zero_addr() was modified to align with cma_port(), which also
> makes any_addr/port consistent.
>
> In most of the cases I checked, passing in an address with sa_family
> set to UNSPEC to cma_any_addr() should not occur. The port
> available check is one (valid) case where it can occur. The
> cma_match_private_data() call referenced above _should_ always have
> a valid sa_family set for the source address, for example.
>
> The only place that it looks like cma_any_addr() might be called
> with sa_family = UNSPEC is cma_set_mgid()/cma_iboe_set_mgid(). And
> I'm not sure the existing code through those paths is correct. If
> the address is UNSPEC, then the existing code will default to the
> AF_INET case.
>
> The librdmacm checks for UNSPEC in multicast join operations, so we
> may never be hitting this case in practice. But I think a user
> space app could invoke this code path. I think the result of that
> would just be the formation of a gid with 4 bytes of 0's in it. In
> any case, the proposed changes would modify this path and convert
> UNSPEC to an mgid = 0.
Okay, so basically I think some summary of this email needs to be
inserted as a comment above the case AF_UNSPEC, assuming it isn't
simplified as above..
The fact you had to audit existing paths to prove we don't use
AF_UNSPEC in them says this is just way to subtle and tricky...
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
