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/