Re: Interface auto-cloning bug or feature?

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.

thanks,
max