Interface auto-cloning bug or feature?

[Kostik Belousov]

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
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-current/attachments/20080923/81cbfcea/attachment.pgp