Interface auto-cloning bug or feature?
kostikbel at gmail.com
Tue Sep 23 17:34:41 UTC 2008
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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Size: 195 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-current/attachments/20080923/81cbfcea/attachment.pgp
More information about the freebsd-current