Re: [PATCH rdma-rc] RDMA/cma: Clear all multicast request fields

[Jason Gunthorpe <jgg@nvidia.com>]

On Mon, Jan 17, 2022 at 08:20:40PM +0200, Leon Romanovsky wrote:
> On Mon, Jan 17, 2022 at 12:16:21PM -0400, Jason Gunthorpe wrote:
> > On Mon, Jan 10, 2022 at 08:05:40PM +0200, Leon Romanovsky wrote:
> > 
> > > > We should probably check the PS even earlier to prevent the IB side
> > > > from having the same issue.
> > > 
> > > What do you think about this?
> > 
> > IB is a bit different, it has a bunch of PS's that are UD compatible..
> > 
> > Probably what we really want here is to check/restrict the CM ID to
> > SIDR mode, which does have the qkey and is the only mode that makes
> > sense to be mixed with multicast, and then forget about port space
> > entirely.
> > 
> > It may be that port space indirectly restricts the CM ID to SIDR mode,
> > but the language here should be 'is in sidr mode', not some confusing
> > open coded port space check.
> > 
> > I'm also not sure of the lifecycle of the qkey, qkeys only exist in
> > SIDR mode so obviously anything that sets/gets a qkey should be
> > restriced to SIDR CM IDs..
> > 
> > > diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> > > index 835ac54d4a24..0a1f008ca929 100644
> > > +++ b/drivers/infiniband/core/cma.c
> > > @@ -4669,12 +4669,8 @@ static int cma_join_ib_multicast(struct rdma_id_private *id_priv,
> > >         if (ret)
> > >                 return ret;
> > > 
> > > -       ret = cma_set_qkey(id_priv, 0);
> > > -       if (ret)
> > > -               return ret;
> > > -
> > >         cma_set_mgid(id_priv, (struct sockaddr *) &mc->addr, &rec.mgid);
> > > -       rec.qkey = cpu_to_be32(id_priv->qkey);
> > > +       rec.qkey = cpu_to_be32(RDMA_UDP_QKEY);
> > 
> > And I'm not sure this makes sense? The UD qkey should still be
> > negotiated right?
> 
> Yes, I think so, it will be changed in SIDR phase.
> 
> The original code has "cma_set_qkey(id_priv, 0)" call, that in IB case will
> execute this switch anyway:
>    515         switch (id_priv->id.ps) {
>    516         case RDMA_PS_UDP:
>    517         case RDMA_PS_IB:
>    518                 id_priv->qkey = RDMA_UDP_QKEY;
> 
> The difference is that we won't store RDMA_UDP_QKEY in id_priv->qkey,
> but I'm unsure that this is right.

Well the whoele cma_set_qkey() function appears to be complete
jumblied nonsense as if qkey is zero then it doesn't do anything if
the qkey was already set.

When called with 0 it is really some sort of 'make a default qkey if
the user hasn't set one already' and in that case defaulting to
RDMA_UDP_QKEY does makes some kind of sense.

The functions purposes should be split into two functions really.

So, we end up with 'make sure the cm id is in SDIR mode' then 'if the
qkey is not set, set it to a default', so that the net result is the
qkey is always set once the function returns.

Though, I'm not sure what the semantics are for qkey during SIDR
negotiation, that should be checked in the spec.

Jason