Re: Interface auto-cloning bug or feature?

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.