netdev - Re: [PATCH] net: deadlock during net device unregistration

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]

Message-ID: <20080930063231.GA4792@ff.dom.local>
Date:	Tue, 30 Sep 2008 06:32:31 +0000
From:	Jarek Poplawski <jarkao2@...il.com>
To:	Benjamin Thery <benjamin.thery@...l.net>
Cc:	"David S. Miller" <davem@...emloft.net>,
	netdev <netdev@...r.kernel.org>,
	Daniel Lezcano <dlezcano@...ibm.com>
Subject: Re: [PATCH] net: deadlock during net device unregistration

On 29-09-2008 19:54, Benjamin Thery wrote:
> This patch proposes to replace the rtnl_unlock() call in 
> linkwatch_event() by __rtnl_unlock(). The difference between the two
> routines being that __rtnl_unlock() will not call netdev_run_todo()
> after it unlocks rtnl_mutex.
> 
> This is to fix a "deadlock" we observed when unregistering a net device.
> 
> In some circumstances, linkwatch_event() blocks the whole "events" 
> workqueue while blocking in rtnl_unlock().
> 
> Here is what happens:
> 
> 1. Unregister a device, the following routines are called:
> 
> -> unregister_netdev
>   -> rtnl_lock
>   -> unregister_netdevice
>   -> rtnl_unlock
>     -> netdev_run_todo
>       -> netdev_wait_allrefs
> 
> 2. In netdev_wait_allrefs(), the device's refcount is greater than 0
>   because there are still some routes to be garbage collected later.
> 
> 3. Also, some link watch events are pending. netdev_wait_allrefs()
>   will run the linkwatch event queue, calls linkwatch_run_queue().
> 
> 
> Both the route garbage collector dst_gc_task() and the linkwatch task
> linkwatch_event() are queued in the same generic workqueue: "events".
> 
> 
> 4. linkwatch_event() is enqueued earlier in the queue. It will grab
>   rtnl_lock(), deliver the link watch events pending, and then call
>   rtnl_unlock(). 
>   rtnl_unlock() will then call netdev_run_todo() and block on
>   mutex_lock(&net_todo_run_mutex).
> 
>   At this point, the workqueue "events" is _blocked_ until the
>   netdev_wait_allrefs() call above returns when the device refcount 
>   reaches 0.
> 
>   Problem: it will never happens if dst_gc_task() was enqueued behind
>   linkwatch_event() in the "events" workqueue as the queue is now 
>   blocked.
...

If it's really like this, I wonder if this can happen without linkwatch
too in a non-preemptive config? So maybe this should be fixed somewhere
else? According to a comment above netdev_wait_allrefs() it seems
references should be rather put down on an UNREGISTER event, so this
dst_gc_task() scheduling shouldn't bother us, I guess.

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