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

[Leon Romanovsky]

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