RE: [PATCHv2 0/6] Fix the problem that rxe can not work in net

From: Parav Pandit <parav@nvidia.com>
To: Yanjun Zhu <yanjun.zhu@linux.dev>, "jgg@ziepe.ca" <jgg@ziepe.ca>,
	"leon@kernel.org" <leon@kernel.org>,
	"zyjzyj2000@gmail.com" <zyjzyj2000@gmail.com>,
	"linux-rdma@vger.kernel.org" <linux-rdma@vger.kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"davem@davemloft.net" <davem@davemloft.net>
Cc: Zhu Yanjun <yanjun.zhu@intel.com>
Subject: RE: [PATCHv2 0/6] Fix the problem that rxe can not work in net
Date: Sun, 13 Nov 2022 04:58:11 +0000	[thread overview]
Message-ID: <PH0PR12MB548101B6A19568A3E1FBD50ADC029@PH0PR12MB5481.namprd12.prod.outlook.com> (raw)
In-Reply-To: <ef09ae0a-ad22-8791-a972-ea33e16011ba@linux.dev>

Hi Yanjun,

> From: Yanjun Zhu <yanjun.zhu@linux.dev>
> Sent: Thursday, November 10, 2022 10:38 PM
> 
> 
> 在 2022/11/11 11:35, Parav Pandit 写道:
> >> From: Yanjun Zhu <yanjun.zhu@linux.dev>
> >> Sent: Thursday, November 10, 2022 9:37 PM
> >
> >> Can you help to review these patches?
> > I will try to review it before 13th.

I did a brief review of patch set.
I didn’t go line by line for each patch; hence I give lumped comments here for overall series.

1. Add example and test results in below test flow in exclusive mode in cover letter.
   # ip netns exec net1 rdma link add rxe1 type rxe netdev eno3
   # ip netns del net0
   Verify that rdma device rxe1 is deleted.

2. Usage of dev_net() in rxe_setup_udp_tunnel() is unsafe.
   This is because when rxe_setup_udp_tunnel() is executed, net ns of netdev can change. 
   This needs to be synchronized with per net notifier register_pernet_subsys() of exit or exit_batch.
   This notifiers callback should be added to rxe module.

3. You need to set bind_ifindex of udp config to the netdev given in newlink in rxe_setup_udp_tunnel.
   Should be a separate pre-patch to ensure that close and right relation to udp socket with netdev in a given netns.

4. Rearrange series to implement delete link as separate series from net ns securing series.
They are unrelated. Current delink series may have use after free accesses. Those needs to be guarded in likely larger series.

5. udp tunnel must shutdown synchronously when rdma link del is done.
   This means any new packet arriving after this point, will be dropped.
   Any existing packet handling present is flushed.
   From your cover letter description, it appears that sock deletion is refcount based and above semantics is not ensured.

6. In patch 5, rxe_get_dev_from_net() can return NULL, hence l_sk6 check can be unsafe. Please add check for rdev null before rdev->l_sk6 check.

7. In patch 5, I didn't fully inspect, but seems like call to rxe_find_route4() is not rcu safe. 
Hence, extension of dev_net() in rxe_find_route4() doesn't look secure.
Accessing sock_net() is more accurate, because at this layer, it is processing packets at socket layer.