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

[Zhu Yanjun <yanjun.zhu@linux.dev>]

在 2022/11/13 12:58, Parav Pandit 写道:
> 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.

Sorry. It is late to reply.
Got it. I will add the above example in the cover letter.

> 
> 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.

No. The netdev device and rxe device are one-to-one correspondence. When 
the netdev device is removed from
the net namespace, the rxe device will be removed. So this will not 
happen that rxe_setup_udp_tunnel is executed
while net namespace of the netdev can change.
In the latest commits, I will add register_pernet_subsys() because the 
init and exit functions can help initialization
and cleanup.

> 
> 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.
> 

No. In the rxe, the sock listeing to the udp port 4791 does not bind 
with the netdev device. A sock can listen to the packets from several
netdev devices. That is, this port 4791 sock is shared by many rxe rdma 
links.

> 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.
> 

Got it. I found the use-after-free problem with the sock. And now it is 
fixed in the latest commits.
I will send it out very soon.

> 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.
> 

The port 4791 udp tunnel is shared by many rxe rdma links. If one rdma 
link exists in net namespace, this udp tunnel should not be
shutdown. Only if no rxe rdma link exist, this udp tunnel will be destroyed.

> 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.
> 

Got it. the l_sk6 is replaced with the sk6 in net namespace notifier.

> 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.

No. As I mentioned in the above, because the netdev device and rxe 
device are one-to-one correspondence, if one net device is moved out of 
the net namespace,
the related rxe device is also removed. So dev_net seems safe.

I will send out the latest commits very soon.

Zhu Yanjun