On Wed, Jan 25, 2006 at 03:11:16PM -0800, Jason Thorpe wrote:
> Why would you want to turn them off if the hardware supports it?  How many 
> VLANs are you working with, anyway?  Looking them up is almost certainly 
> cheaper than copying data that is guaranteed not to be in cache.

I think I've just found another bug (another reason to turn this feature
off :-) ).

The VLAN_INPUT_TAG (see if_ether.h) has an _errcase argument which is a C
statement to be called if allocation of a tag fails:

#define	VLAN_INPUT_TAG(ifp, m, vlanid, _errcase)	\
	do {								\
                struct m_tag *mtag =					\
                    m_tag_get(PACKET_TAG_VLAN, sizeof(u_int), M_NOWAIT);\
                if (mtag == NULL) {					\
			ifp->if_ierrors++;				\
                        printf("%s: unable to allocate VLAN tag\n",	\
                            ifp->if_xname);				\
                        m_freem(m);					\
                        _errcase;					\
                }
...
	} while(0)

Before _errcase is executed, the mbuf is freed.

Drivers call this macro from a loop with _errcase set to the continue
statement:

(dev/ic/rtl8169.c)

	while (!RTK_OWN(&sc->rtk_ldata.rtk_rx_list[i])) {
...
		if (rxvlan & RTK_RDESC_VLANCTL_TAG) {
			VLAN_INPUT_TAG(ifp, m,
			     be16toh(rxvlan & RTK_RDESC_VLANCTL_DATA),
			     continue);
		}
#if NBPFILTER > 0
		if (ifp->if_bpf)
			bpf_mtap(ifp->if_bpf, m);
#endif
		(*ifp->if_input)(ifp, m);
	}

But as the macro is wrapped in a do {...} while(0) "loop", the continue
statement won't have the desired effect. It will exit this "loop" in the
macro, but not the main loop in the driver. The driver will continue to
use the freed mbuf. Oops...

Is this analysis correct? (I don't have access to the hardware to test it
on ATM.)

Pavel