Hi Ilya,
Ilya Maximets <[email protected]> writes:
> Sashiko reports that it is technically possible that we got the device
> reference, but by the time we're linking it to the OVS datapath, it
> may be already in the process of being deleted. In this case if the
> notifier wins the race for RTNL, it will see that the device is not
> yet in the OVS datapath (ovs_netdev_get_vport() will fail in the
> dp_device_event()) and will do nothing. Then the ovs_netdev_link()
> will take the RTNL and link the unregistering device to OVS datapath.
>
> Eventually, netdev_wait_allrefs_any() will re-broadcast the event and
> the device will be properly detached, but it will take at least a
> second before that happens, so it's not something we should rely on.
>
> Let's avoid linking the non-registered device in the first place.
>
> Note: As per documentation, RTNL doesn't protect the reg_state, but
> it actually does for all the state transitions we care about here,
> so it should not be necessary to use READ_ONCE or taking the instance
> lock. We can still do that, but we have a few more places even in
> this file where the reg_state is accessed without those while under
> RTNL, and many more places like this across the kernel code, so it
> might make more sense to change all of them in a more centralized
> fashion in the future, if necessary.
>
> Fixes: ccb1352e76cf ("net: Add Open vSwitch kernel components.")
> Signed-off-by: Ilya Maximets <[email protected]>
> ---
> net/openvswitch/vport-netdev.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/net/openvswitch/vport-netdev.c b/net/openvswitch/vport-netdev.c
> index c42642075685d..de90d0541e172 100644
> --- a/net/openvswitch/vport-netdev.c
> +++ b/net/openvswitch/vport-netdev.c
> @@ -83,6 +83,11 @@ struct vport *ovs_netdev_link(struct vport *vport, bool
> tunnel)
> }
>
> rtnl_lock();
As noted in your commit, this shouldn't cause any kind of issues, since
the next netdev_wait_allrefs_any() will make sure things look correct to
the users again.
That said, I agree this is good to do to prevent some confusion going to
the users. I wonder if it makes sense to add a comment here noting
that. Otherwise, if I were just freshly reading through the code it
wouldn't follow (all the places where ovs_netdev_link get called are in
the 'create' path).
WDYT?
> + if (vport->dev->reg_state != NETREG_REGISTERED) {
> + err = -ENODEV;
> + goto error_put_unlock;
> + }
> +
> err = netdev_master_upper_dev_link(vport->dev,
> get_dpdev(vport->dp),
> NULL, NULL, NULL);
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
