Subject: Adding KASSERT() calls to "sys/sys/queue.h"
To: None <tech-kern@NetBSD.org>
From: Matthias Scheler <tron@zhadum.de>
List: tech-kern
Date: 06/16/2004 20:48:11
	Hello,

while I was examining PR kern/25868 I discoverd that a mbuf was freed twice
and therefore inserted into a pool twice. I finally found the code causing
the bug by modifying MFREE() like this:

Index: mbuf.h
===================================================================
RCS file: /cvsroot/src/sys/sys/mbuf.h,v
retrieving revision 1.90.2.1
diff -u -r1.90.2.1 mbuf.h
--- mbuf.h      9 Apr 2004 16:10:59 -0000       1.90.2.1
+++ mbuf.h      16 Jun 2004 13:20:41 -0000
@@ -607,6 +607,11 @@
  */
 #define        MFREE(m, n)                                             \
        MBUFLOCK(                                                       \
+               struct mbuf *h;                                         \
+               h = pool_cache_get(&mbpool_cache, 0);                   \
+               KASSERT(m != h);                                        \
+               if (h != NULL)                                          \
+                       pool_cache_put(&mbpool_cache, h);               \
                mbstat.m_mtypes[(m)->m_type]--;                         \
                if ((m)->m_flags & M_PKTHDR)                            \
                        m_tag_delete_chain((m), NULL);                  \

The above code is of course quite inefficient. So I wondered if it is
possible to modify the pool system to detect such mistakes. If I understand
its implementation correctly it uses queues as defined in "queue.h". So
maybe we should add KASSERT() calls like this one:

#define	TAILQ_INSERT_HEAD(head, elm, field) do {			\
	QUEUEDEBUG_TAILQ_INSERT_HEAD((head), (elm), field)		\
	KASSERT((elm) != (head)->tqh_first);				\
	if (((elm)->field.tqe_next = (head)->tqh_first) != NULL)	\
		(head)->tqh_first->field.tqe_prev =			\
		    &(elm)->field.tqe_next;				\
	else								\
		(head)->tqh_last = &(elm)->field.tqe_next;		\
	(head)->tqh_first = (elm);					\
	(elm)->field.tqe_prev = &(head)->tqh_first;			\
} while (/*CONSTCOND*/0)

They will of course not catch all possible errors but they would have
noticed the problem described above.

Opinions?

	Kind regards

-- 
Matthias Scheler                                  http://scheler.de/~matthias/