On Tue, Sep 23, 2008 at 10:19:13AM -0700, Maksim Yevmenkin wrote: > On 9/23/08, Kostik Belousov <kostikbel_at_gmail.com> wrote: > > [...] > > > > attached is a slightly better patch for tap(4). the idea is to use > > > extra ALLOCATED flag that prevents the race Kostik pointed out. could > > > you please give it a try? any review comments are greatly appreciated. > > > if this is acceptable, i will prepare something similar for tun(4) > > > > The tap should use make_dev_credf(MAKEDEV_REF) instead of > > make_dev/dev_ref sequence in the clone handler. For similar reasons, I > > think it is slightly better to do a dev_ref() immediately after setting > > the TAP_ALLOCATED flag without dropping tapmtx. > > could you please explain why it is better? > > > I cannot figure out how tap_clone_create/tap_clone_destroy are being > > called. Can it be garbage-collected ? > > ah, this is interface clone feature, i.e. one can do 'ifconfig tap0 > create/destroy' to create an interface and device node. take a look at > IFC_SIMPLE_DECLARE() macro. Thanks for the explanation. > > > The whole module unload sequence looks unsafe. > > yes, it is unsafe. it even has comment about it :) i guess, i could > fix it too while i'm at it :) One of the reason why the module unload is unsafe is the complete lack of synchronization between cloner and device destruction. Leaving tapmtx and tp->tap_mtx protected region in the clone handler, you allow for module unload routine to destroy device, and then dev_ref() would operate on the freed memory. Not that doing that without dropping the mutex(es) fix the bug, but at least it is a right move, it seems. At least this would trade a crash to a memory leak.
This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:39:35 UTC