Re: Interface auto-cloning bug or feature?

[...]

>> --- if_tap.c.orig     2008-09-08 17:20:57.000000000 -0700
>> +++ if_tap.c  2008-09-19 15:35:02.000000000 -0700
>> _at__at_ -94,6 +94,7 _at__at_
>>  static int           tapifioctl(struct ifnet *, u_long, caddr_t);
>>  static void          tapifinit(void *);
>>
>> +static int           tap_clone_lookup(struct cdev **, u_short);
>>  static int           tap_clone_create(struct if_clone *, int, caddr_t);
>>  static void          tap_clone_destroy(struct ifnet *);
>>  static int           vmnet_clone_create(struct if_clone *, int, caddr_t);
>> _at__at_ -176,6 +177,28 _at__at_
>>  DEV_MODULE(if_tap, tapmodevent, NULL);
>>
>>  static int
>> +tap_clone_lookup(struct cdev **dev, u_short extra)
>> +{
>> +     struct tap_softc *tp;
>> +
>> +     mtx_lock(&tapmtx);
>> +     SLIST_FOREACH(tp, &taphead, tap_next) {
>> +             mtx_lock(&tp->tap_mtx);
>> +             if ((tp->tap_flags & (TAP_OPEN|extra)) == extra) {
>> +                     *dev = tp->tap_dev;
>> +                     mtx_unlock(&tp->tap_mtx);
>> +                     mtx_unlock(&tapmtx);
>> +
>> +                     return (1);
>> +             }
>> +             mtx_unlock(&tp->tap_mtx);
>> +     }
>> +     mtx_unlock(&tapmtx);
>> +
>> +     return (0);
>> +}
>> +
>> +static int
>>  tap_clone_create(struct if_clone *ifc, int unit, caddr_t params)
>>  {
>>       struct cdev *dev;
>> _at__at_ -353,8 +376,18 _at__at_
>>
>>       /* We're interested in only tap/vmnet devices. */
>>       if (strcmp(name, TAP) == 0) {
>> +             if (tap_clone_lookup(dev, 0)) {
>> +                     dev_ref(*dev);
>> +                     return;
> What would prevent two concurrent threads from selecting the same device
> there ? First thread could look up the device, unloc tapmtx and be
> preempted. Then second thread is put on CPU, do the same selection.
> Now you have a problem.

i told it was *very* quick :) and, yes, it seems like the race you
describing would be possible. clone handler gets invoked with sx lock
held, however its shared lock. also devfs mount point exclusive sx
lock is dropped before invoking clone handler. anyway, the fix
probably would be to move dev_rel() into the clone_lookup() routine
and protect it by tap softc mutex. also clone_lookup() should check
the number of references to the device to make sure 2 threads do not
grab the same device. another approach would be to add another flag
like TAP_OPEN to indicate that device is about to be open ans set it
in the clone_lookup() routine. i'll put something together on monday
(may be soon if i will have a free minute). please feel free to beat
me to it :)

thanks,
max