netdev - Re: [PATCH net-next] openvswitch: fix vport-netdev unregister

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 11 Oct 2013 15:02:27 -0700
From:	Jesse Gross <jesse@...ira.com>
To:	Alexei Starovoitov <ast@...mgrid.com>
Cc:	Pravin Shelar <pshelar@...ira.com>,
	"David S. Miller" <davem@...emloft.net>,
	Jiri Pirko <jiri@...nulli.us>,
	"dev@...nvswitch.org" <dev@...nvswitch.org>,
	netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next] openvswitch: fix vport-netdev unregister

On Fri, Oct 11, 2013 at 1:03 PM, Alexei Starovoitov <ast@...mgrid.com> wrote:
> On Fri, Oct 11, 2013 at 11:11 AM, Jesse Gross <jesse@...ira.com> wrote:
>> On Thu, Oct 10, 2013 at 9:48 PM, Alexei Starovoitov <ast@...mgrid.com> wrote:
>>> On Thu, Oct 10, 2013 at 8:56 PM, Jesse Gross <jesse@...ira.com> wrote:
>>>> However, the check dev->reg_state in netdev_destroy() looks racy to
>>>> me, as it could already be in NETREG_UNREGISTERED even if we already
>>>> processed this device.
>>>
>>> you mean that netdev_destroy() will see reg_state == netreg_unregistered,
>>> while dp_device_event() didn't see reg_state == netreg_unregistering yet?
>>> or dp_device_event() saw it, proceeded to do unlink and
>>> netdev_destroy() ran in parallel?
>>> well, that's why reg_state == netreg_unregistering check in netdev_destroy()
>>> is done with rtnl_lock() held.
>>> reg_state cannot go into netreg_unregistered state skipping
>>> netreg_unregistering and notifier.
>>> therefore I don't think it's racy.
>>>
>>> In ovs_dp_notify_wq() you're checking for both unregistering and
>>> unregistered and that makes
>>> sense, since workq can run after unregistering notifier called and
>>> netdev_run_todo()
>>> already changed the state to unregistered.
>>> But here it's not the case.
>>
>> ovs_dp_notify_wq() calls ovs_dp_detach_port(), which indirectly calls
>> netdev_destroy() so it seems like it actually is the same case to me.
>
> yes. makes sense.
> how about:
> -       if (netdev_vport->dev->reg_state != NETREG_UNREGISTERING)
> +       if (netdev_vport->dev->priv_flags & IFF_OVS_DATAPATH)

Yes, this seems safer. Is the check for NETREG_UNREGISTERING in
dp_device_event() still needed given that we are checking the event?

> ovs_netdev_destroy_dev() name instead ovs_netdev_unlink_dev() name?

How about detach_dev?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html