Interface auto-cloning bug or feature?
Kostik Belousov
kostikbel at gmail.com
Wed Sep 24 09:12:09 UTC 2008
On Tue, Sep 23, 2008 at 11:00:26AM -0700, Maksim Yevmenkin wrote:
> On 9/23/08, Kostik Belousov <kostikbel at gmail.com> wrote:
> > 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.
>
> well, unload race is easy to fix, no? just add a global flag protected
> by taphead (tapmtx) mutex. in unload path (after checking all the
> devices for OPEN and ALLOCATED) we will set this flag counter. each
> clone and open routines will check for the flag and refuse to
> open/clone if its set.
Then you would get a transient failures when attempt to unload module
fails because some devices are busy.
-------------- 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/20080924/49849687/attachment.pgp
More information about the freebsd-current
mailing list