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

[Jason Gunthorpe]

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

Jason