Re: [PATCH net] openvswitch: vport: fix race between linking and the device notifier

[Aaron Conole <aconole@redhat.com>]

Hi Ilya,

Ilya Maximets <i.maximets@ovn.org> 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 <i.maximets@ovn.org>
> ---
>  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);