Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Tue, 12 Apr 2022 11:11:34 -0300
From:   Jason Gunthorpe <jgg@nvidia.com>
To:     Leon Romanovsky <leon@kernel.org>
Cc:     Mark Zhang <markzhang@nvidia.com>, linux-kernel@vger.kernel.org,
        linux-rdma@vger.kernel.org,
        syzbot+8fcbb77276d43cc8b693@syzkaller.appspotmail.com
Subject: Re: [PATCH rdma-rc] RDMA/cma: Limit join multicast to UD QP type only
Message-ID: <20220412141134.GI2120790@nvidia.com>
References: <4132fdbc9fbba5dca834c84ae383d7fe6a917760.1649083917.git.leonro@nvidia.com>
 <20220408182440.GA3647277@nvidia.com>
 <YlLHjFlR8BtCc5Hu@unreal>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YlLHjFlR8BtCc5Hu@unreal>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?pytf8xX/tEiwYhzfGWOPnS73iNiWLv8uMJC0k2Bj/jLOEuxgdZgpSZoNNhz9?=
 =?us-ascii?Q?eb7YqEMknU+PY/V+67J6IPnXtsNLc6oIGw5gmXe0EHZZR6Mwt0DUBP77Yy1U?=
 =?us-ascii?Q?uAtHeIKwIvgeKw+fHjfaYPllnSZozVyt51HNe8TFPrZuBKo+FLZ0pvi5w6ZT?=
 =?us-ascii?Q?MXiYIyzd7uDTt5ymu5/YZC/4WbgVW7esa3gpv136q1NGUlbjaKC5iNFXpR0w?=
 =?us-ascii?Q?8jfUIom1Z+o1UCyY0YAoaNugGdWRcP8iU3L+6eV2VigO36aIbNtnuizW81h7?=
 =?us-ascii?Q?eOxn9Bx/QnWD8JJrMxi31teGxFlJyxcPQ/RsPEsfYowYngzvTxWuXByfB8SN?=
 =?us-ascii?Q?lvol/rCGtX86V9zaz10ffVlKKE8zvs7YhCEC3HkrD/S/Bq31jFKD4d78QvTj?=
 =?us-ascii?Q?MD1Fkv8Kv5IbdIQQ9sfCTjlbM9ZDXlI0FHggTUM6Yx6mXpSYtyupaOpccdkO?=
 =?us-ascii?Q?GEL0TdM/OStQQ+M001tXiT8pXiwBRjevRk2plcwEw0p8S/S+pmZ+kMpzT/6n?=
 =?us-ascii?Q?8O8SSndp90msG7/LPfOAHUwFx+Dso0xFbPC3rWH30y0mef+8c3kSmmIyRZyR?=
 =?us-ascii?Q?Ih1ScOqjoVhwKBQOlMAK/ynmT8X0awCdwxDE3S96xj4CidCpAJq2W51nuIP8?=
 =?us-ascii?Q?B1wPuwrYkotjMErXBYimh2mkEi80cdR/6H1FdbALQPPmSCGW8Fljby3I0jpW?=
 =?us-ascii?Q?a+YD4CLUgforsWbXbfn06VerZqQpRkkWl2Q8jxb9pz/t+IiTUk/kUuDvULr/?=
 =?us-ascii?Q?NPZMa+lLWAaeosgPyI2YDP94qbfEjzU0voVKrgy1mWfOdJ6kTLl4EsFBGSXv?=
 =?us-ascii?Q?YpeexI4huEwW1QGUciz8KBWy+UsyyTPO1qAIaqqf4er//34eG0sIK/zel0Cs?=
 =?us-ascii?Q?3RsgiAm5o+ViOHoD1FLkHk2PCbPPdq/mAJkQ7QIPcVv1+Tm4EkeIWsTJvKGx?=
 =?us-ascii?Q?NW/yYytMVUETVuj+Mg3o5wB54lagf95i/qtsaQA5QKdaEhruycJHn9SEaBUh?=
 =?us-ascii?Q?UWI7ydUTSmOjOCaytvGaU+xQxIRnO96Ftmq5jotgx5Z6lS5GQAvmVVIvNlkK?=
 =?us-ascii?Q?7yAVGfgYaRDymbGc60JphKMfYAeoOB+2DXMo82VvcORTD9mbVnTi5vnK5Trv?=
 =?us-ascii?Q?5PnJ8CVJFvsNLJsGA6r4b8Wj/9QUGXW316PBYu/VWkCKQUPYuc8jqQxRxA2R?=
 =?us-ascii?Q?n/SyBaT41mLsBdhI9tpEEtBnW+lEvh6rBSI0j9CipOQk+lacwgGp7IpEblh4?=
 =?us-ascii?Q?tHdcKccHz3njgn7Nnkef29TR99YR/9ZZXROFZcUUlZECPgG9N/jl0VYltQXn?=
 =?us-ascii?Q?F/JDO6PXjwNoZzPn0rJlZ9yTaYf07oHlmXoQ97CPVY7xYYRSm/xmwASAG5lc?=
 =?us-ascii?Q?lhIA8ofPiju0KC+B5JOLgJLYIjHyS/nZv+Vk9lOpbcXTNj7SHgE1F074zgVV?=
 =?us-ascii?Q?ErpF5BLQKlv1fx+cYRXOYGGQZGs3UEEMJ5rZ/4mXN7l5pIX5aB4nvmWSrFYf?=
 =?us-ascii?Q?z43MTJqhTN2FgeMnLFRSx2r4qt4Mr116Va+hNo5NO4Gj/UZLpB5CuSVdzwZK?=
 =?us-ascii?Q?ojLkN/gN3EeaF0ccX1VIQ89YViq2s1NgCrDdm2jhivcceHnAl3Wlg6KxZEmk?=
 =?us-ascii?Q?D4gY4fhjtTEGjJOas1IitoaRtEqp+v4ng8crCZelsJhAOtbTOqyBMysJUVy8?=
 =?us-ascii?Q?2y4YBzjElt99nxPkQ/e/JZHu0zxlR+qSsDphDynN3qEsD2vq5EyjJeBLoHfk?=
 =?us-ascii?Q?ZQ2iU3NNpw=3D=3D?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 651122e4-839d-47ca-e0c5-08da1c8e5a5f
X-MS-Exchange-CrossTenant-AuthSource: CH2PR12MB4181.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Apr 2022 14:11:35.5700
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: CvGq9WMDftvbNidYWg5mFdXwUg7yDnqzPg79DOnlTTQmd4VX6g42qPdaqXaPS5/H
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR12MB1410
X-Spam-Status: No, score=-2.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
	DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,RDNS_NONE,SPF_HELO_NONE,T_SCC_BODY_TEXT_LINE
	autolearn=no autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
	lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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