Re: [PATCH rdma-rc] RDMA/cma: Limit join multicast to UD QP type only

From: Leon Romanovsky
Date: Wed Apr 13 2022 - 04:07:25 EST


On Tue, Apr 12, 2022 at 11:11:34AM -0300, Jason Gunthorpe wrote:
> On Sun, Apr 10, 2022 at 03:03:24PM +0300, Leon Romanovsky wrote:
> > On Fri, Apr 08, 2022 at 03:24:40PM -0300, Jason Gunthorpe wrote:
> > > On Mon, Apr 04, 2022 at 05:52:18PM +0300, Leon Romanovsky wrote:
> > > > -static int cma_set_qkey(struct rdma_id_private *id_priv, u32 qkey)
> > > > +static int cma_set_default_qkey(struct rdma_id_private *id_priv)
> > > > {
> > > > struct ib_sa_mcmember_rec rec;
> > > > int ret = 0;
> > > >
> > > > - if (id_priv->qkey) {
> > > > - if (qkey && id_priv->qkey != qkey)
> > > > - return -EINVAL;
> > > > - return 0;
> > > > - }
> > > > -
> > > > - if (qkey) {
> > > > - id_priv->qkey = qkey;
> > > > - return 0;
> > > > - }
> > > > -
> > > > switch (id_priv->id.ps) {
> > > > case RDMA_PS_UDP:
> > > > case RDMA_PS_IB:
> > > > @@ -528,9 +517,22 @@ static int cma_set_qkey(struct rdma_id_private *id_priv, u32 qkey)
> > > > default:
> > > > break;
> > > > }
> > > > +
> > > > return ret;
> > > > }
> > > >
> > > > +static int cma_set_qkey(struct rdma_id_private *id_priv, u32 qkey)
> > > > +{
> > > > + if (!qkey)
> > > > + return cma_set_default_qkey(id_priv);
> > >
> > > This should be called in the couple of places that are actually
> > > allowed to set a default qkey. We have some confusion about when that
> > > is supposed to happen and when a 0 qkey can be presented.
> > >
> > > But isn't this not the same? The original behavior was to make the
> > > set_default a NOP if the id_priv already had a qkey:
> > >
> > > - if (id_priv->qkey) {
> > > - if (qkey && id_priv->qkey != qkey)
> > >
> > > But that is gone now?
> >
> > When I reviewed, I got an impression what once we create id_priv and set
> > qkey to default values, we won't hit this if (..).
>
> We don't set qkey during create, so I'm not so sure..
>
> The only places setting non-default qkeys are SIDR, maybe nobody uses
> SIDR with multicast.
>
>
> > > > static void cma_translate_ib(struct sockaddr_ib *sib, struct rdma_dev_addr *dev_addr)
> > > > {
> > > > dev_addr->dev_type = ARPHRD_INFINIBAND;
> > > > @@ -4762,8 +4764,7 @@ static int cma_iboe_join_multicast(struct rdma_id_private *id_priv,
> > > > cma_iboe_set_mgid(addr, &ib.rec.mgid, gid_type);
> > > >
> > > > ib.rec.pkey = cpu_to_be16(0xffff);
> > > > - if (id_priv->id.ps == RDMA_PS_UDP)
> > > > - ib.rec.qkey = cpu_to_be32(RDMA_UDP_QKEY);
> > > > + ib.rec.qkey = cpu_to_be32(RDMA_UDP_QKEY);
> > >
> > > Why isn't this symetrical with the IB side:
> > >
> > > ret = cma_set_default_qkey(id_priv);
> > > if (ret)
> > > return ret;
> > > rec.qkey = cpu_to_be32(id_priv->qkey);
> > >
> > >
> > > ??
> >
> > The original code didn't touch id_priv.
>
> I know, but I think that is a mistake, we should make it symmetric

ok, I added it to regression.

Thanks

>
> Jason