Re: Interface auto-cloning bug or feature?

On Mon, Sep 22, 2008 at 06:49:14PM -0700, Maksim Yevmenkin wrote:
> [...]
> 
> >>> ok, how about attached patch. i put it together *very* quickly and
> >>> only gave it a light testing. its for tap(4), because i could compile
> >>> it as a module and tun(4) is compiled into kernel by default, but the
> >>> idea should identical for tun(4). should be even simpler for tun(4)
> >>> because it does not have to deal with 2 kind of devices (i.e. tap and
> >>> vmnet). give it a try, and see if it works. please try both cloning
> >>> paths, i.e.
> >>>
> >>> 1) cat /dev/tap (/dev/vmnet) with and/or without unit number
> >>>
> >>> and
> >>>
> >>> 2) ifconfig tapX (vmnetX) create/destroy
> >>>
> >>> in the mean time i will prepare something similar for tun(4).
> >>
> >> attached is similar patch for tun(4). i only made sure it compiles :)
> >> rebuilding kernel now...
> 
> 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.

I cannot figure out how tap_clone_create/tap_clone_destroy are being
called. Can it be garbage-collected ?

The whole module unload sequence looks unsafe.

> 
> thanks,
> max

> --- if_tap.c.orig	2008-09-08 17:20:57.000000000 -0700
> +++ if_tap.c	2008-09-22 18:36:16.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,30 _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|TAP_ALLOCATED|extra)) == extra) {
> +			tp->tap_flags |= TAP_ALLOCATED;
> +			*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_ -288,7 +313,7 _at__at_
>  		mtx_lock(&tapmtx);
>  		SLIST_FOREACH(tp, &taphead, tap_next) {
>  			mtx_lock(&tp->tap_mtx);
> -			if (tp->tap_flags & TAP_OPEN) {
> +			if (tp->tap_flags & (TAP_OPEN|TAP_ALLOCATED)) {
>  				mtx_unlock(&tp->tap_mtx);
>  				mtx_unlock(&tapmtx);
>  				return (EBUSY);
> _at__at_ -353,8 +378,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;
> +		}
> +
>  		unit = -1;
>  	} else if (strcmp(name, VMNET) == 0) {
> +		if (tap_clone_lookup(dev, TAP_VMNET)) {
> +			dev_ref(*dev);
> +			return;
> +		}
> +
>  		unit = -1;
>  		extra = VMNET_DEV_MASK;
>  	} else if (dev_stdclone(name, NULL, TAP, &unit) != 1) {
> _at__at_ -559,12 +594,11 _at__at_
>  	KNOTE_UNLOCKED(&tp->tap_rsel.si_note, 0);
>  
>  	mtx_lock(&tp->tap_mtx);
> -	tp->tap_flags &= ~TAP_OPEN;
> +	tp->tap_flags &= ~(TAP_OPEN|TAP_ALLOCATED);
>  	tp->tap_pid = 0;
>  	mtx_unlock(&tp->tap_mtx);
>  
> -	TAPDEBUG("%s is closed. minor = %#x\n", 
> -		ifp->if_xname, minor(dev));
> +	TAPDEBUG("%s is closed. minor = %#x\n", ifp->if_xname, minor(dev));
>  
>  	return (0);
>  } /* tapclose */
> --- if_tapvar.h.orig	2005-06-10 12:04:52.000000000 -0700
> +++ if_tapvar.h	2008-09-22 17:34:00.000000000 -0700
> _at__at_ -54,6 +54,7 _at__at_
>  #define	TAP_ASYNC	(1 << 3)
>  #define TAP_READY       (TAP_OPEN|TAP_INITED)
>  #define	TAP_VMNET	(1 << 4)
> +#define TAP_ALLOCATED	(1 << 5)
>  
>  	u_int8_t 	ether_addr[ETHER_ADDR_LEN]; /* ether addr of the remote side */
>