>Number:         32646
>Category:       kern
>Synopsis:       wrong error handling in VLAN_INPUT_TAG (if_ether.h)
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Thu Jan 26 23:35:00 +0000 2006
>Originator:     Pavel Cahyna
>Release:        NetBSD 3.0_RC5
>Organization:
>Environment:
System: NetBSD beta 3.0_RC5 NetBSD 3.0_RC5 (EV56) #3: Mon Dec 12 20:28:20 CET 2005 pavel@beta:/usr/src/sys/arch/alpha/compile/EV56 alpha
Architecture: alpha
Machine: alpha
>Description:
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:

(example: 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 restart the main loop in the driver. The
driver will continue to use the freed mbuf. Oops...
>How-To-Repeat:
code analysis.
>Fix:
FreeBSD fixed it recently (the diff does not include necessary
changes to drivers):

- Fix VLAN_INPUT_TAG() macro, so that it doesn't touch mtag in
  case if memory allocation failed.
- Remove fourth argument from VLAN_INPUT_TAG(), that was used
  incorrectly in almost all drivers. Indicate failure with
  mbuf value of NULL.

In collaboration with:	yongari, ru, sam
diff -u -p -r1.22 -r1.23
--- src/sys/net/if_vlan_var.h	2005/08/31 11:36:50	1.22
+++ src/sys/net/if_vlan_var.h	2005/12/18 18:24:27	1.23
@@ -26,7 +26,7 @@
  * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
  * SUCH DAMAGE.
  *
- * $FreeBSD: /usr/local/www/cvsroot/FreeBSD/src/sys/net/if_vlan_var.h,v 1.22 2005/08/31 11:36:50 yar Exp $
+ * $FreeBSD: /usr/local/www/cvsroot/FreeBSD/src/sys/net/if_vlan_var.h,v 1.23 2005/12/18 18:24:27 glebius Exp $
  */
 
 #ifndef _NET_IF_VLAN_VAR_H_
@@ -102,18 +102,19 @@ struct	vlanreq {
  */
 #define	VLAN_TAG_VALUE(_mt)	(*(u_int *)((_mt) + 1))
 
-#define	VLAN_INPUT_TAG(_ifp, _m, _t, _errcase) do {		\
+#define	VLAN_INPUT_TAG(_ifp, _m, _t) do {			\
 	struct m_tag *mtag;					\
 	mtag = m_tag_alloc(MTAG_VLAN, MTAG_VLAN_TAG,		\
 			   sizeof (u_int), M_NOWAIT);		\
-	if (mtag == NULL) {					\
+	if (mtag != NULL) {					\
+		VLAN_TAG_VALUE(mtag) = (_t);			\
+		m_tag_prepend((_m), mtag);			\
+		(_m)->m_flags |= M_VLANTAG;			\
+	} else {						\
 		(_ifp)->if_ierrors++;				\
 		m_freem(_m);					\
-		_errcase;					\
+		_m = NULL;					\
 	}							\
-	VLAN_TAG_VALUE(mtag) = (_t);				\
-	m_tag_prepend((_m), mtag);				\
-	(_m)->m_flags |= M_VLANTAG;				\
 } while (0)
 
 #define	VLAN_OUTPUT_TAG(_ifp, _m)				\