Add sanity checks to the ndis_packet and ndis_buffer pool handling

routines to guard against problems caused by (possibly) buggy drivers.

The RealTek 8180 wireless driver calls NdisFreeBuffer() to release
some of its buffers _after_ it's already called NdisFreeBufferPool()
to destroy the pool to which the buffers belong. In our implementation,
this error causes NdisFreeBuffer() to touch stale heap memory.

If you are running a release kernel, and hence have INVARIANTS et al
turned off, it turns out nothing happens. But if you're using a
development kernel config with INVARIANTS on, the malloc()/free()
sanity checks will scribble over the pool memory with 0xdeadc0de
once it's released so that any attempts to touch it will cause a
trap, and indeed this is what happens. It happens that I run 5.2-RELEASE
on my laptop, so when I tested the rtl8180.sys driver, it worked fine
for me, but people trying to run it with development systems checked
out or cvsupped from -current would get a page fault on driver load.

I can't find any reason why the NDISulator would cause the RealTek
driver to do the NdisFreeBufferPool() prematurely, and the same driver
obviously works with Windows -- or at least, it doesn't cause a crash:
the Microsoft documentation for NdisFreeBufferPool() says that failing
to return all buffers to the pool before calling  NdisFreeBufferPool()
causes a memory leak.

I've written to my contacts at RealTek asking them to check if this
is indeed a bug in their driver. In the meantime, these new sanity checks
will catch this problem and issue a warning rather than causing a trap.
The trick is to keep a count of outstanding buffers for each buffer pool,
and if the driver tries to call NdisFreeBufferPool() while there are still
buffers outstanding, we mark the pool for deletion and then defer
destroying it until after the last buffer has been reclaimed.

2 files changed, 88 insertions, 2 deletions

diff --git a/sys/compat/ndis/subr_ndis.c b/sys/compat/ndis/subr_ndis.c
index 5e93f894247c..df6a39accd3f 100644
--- a/sys/compat/ndis/subr_ndis.c
+++ b/sys/compat/ndis/subr_ndis.c
@@ -1559,6 +1559,7 @@ ndis_alloc_packetpool(status, pool, descnum, protrsvdlen)
 
 	cur = (ndis_packet *)*pool;
 	cur->np_private.npp_flags = 0x1; /* mark the head of the list */
+	cur->np_private.npp_totlen = 0; /* init deletetion flag */
 	for (i = 0; i < (descnum + NDIS_POOL_EXTRA); i++) {
 		cur->np_private.npp_head = (ndis_handle)(cur + 1);
 		cur++;
@@ -1595,7 +1596,21 @@ __stdcall static void
 ndis_free_packetpool(pool)
 	ndis_handle		pool;
 {
-	free(pool, M_DEVBUF);
+	ndis_packet		*head;
+
+	head = pool;
+
+	/* Mark this pool as 'going away.' */
+
+	head->np_private.npp_totlen = 1;
+
+	/* If there are no buffers loaned out, destroy the pool. */
+
+	if (head->np_private.npp_count == 0)
+		free(pool, M_DEVBUF);
+	else
+		printf("NDIS: buggy driver deleting active packet pool!\n");
+
 	return;
 }
 
@@ -1614,6 +1629,16 @@ ndis_alloc_packet(status, packet, pool)
 		return;
 	}
 
+	/*
+	 * If this pool is marked as 'going away' don't allocate any
+	 * more packets out of it.
+	 */
+
+	if (head->np_private.npp_totlen) {
+		*status = NDIS_STATUS_FAILURE;
+		return;
+	}
+
 	pkt = (ndis_packet *)head->np_private.npp_head;
 
 	if (pkt == NULL) {
@@ -1662,6 +1687,14 @@ ndis_release_packet(packet)
 	head->np_private.npp_head = (ndis_buffer *)packet;
 	head->np_private.npp_count--;
 
+	/*
+	 * If the pool has been marked for deletion and there are
+	 * no more packets outstanding, nuke the pool.
+	 */
+
+	if (head->np_private.npp_totlen && head->np_private.npp_count == 0)
+		free(head, M_DEVBUF);
+
 	return;
 }
 
@@ -1749,6 +1782,8 @@ ndis_alloc_bufpool(status, pool, descnum)
 
 	cur = (ndis_buffer *)*pool;
 	cur->nb_flags = 0x1; /* mark the head of the list */
+	cur->nb_bytecount = 0; /* init usage count */
+	cur->nb_byteoffset = 0; /* init deletetion flag */
 	for (i = 0; i < (descnum + NDIS_POOL_EXTRA); i++) {
 		cur->nb_next = cur + 1;
 		cur++;
@@ -1762,7 +1797,20 @@ __stdcall static void
 ndis_free_bufpool(pool)
 	ndis_handle		pool;
 {
-	free(pool, M_DEVBUF);
+	ndis_buffer		*head;
+
+	head = pool;
+
+	/* Mark this pool as 'going away.' */
+
+	head->nb_byteoffset = 1;
+
+	/* If there are no buffers loaned out, destroy the pool. */
+	if (head->nb_bytecount == 0)
+		free(pool, M_DEVBUF);
+	else
+		printf("NDIS: buggy driver deleting active buffer pool!\n");
+
 	return;
 }
 
@@ -1785,6 +1833,16 @@ ndis_alloc_buf(status, buffer, pool, vaddr, len)
 		return;
 	}
 
+	/*
+	 * If this pool is marked as 'going away' don't allocate any
+	 * more buffers out of it.
+	 */
+
+	if (head->nb_byteoffset) {
+		*status = NDIS_STATUS_FAILURE;
+		return;
+	}
+
 	buf = head->nb_next;
 
 	if (buf == NULL) {
@@ -1801,6 +1859,10 @@ ndis_alloc_buf(status, buffer, pool, vaddr, len)
 
 	*buffer = buf;
 
+	/* Increment count of busy buffers. */
+
+	head->nb_bytecount++;
+
 	*status = NDIS_STATUS_SUCCESS;
 	return;
 }
@@ -1822,6 +1884,18 @@ ndis_release_buf(buf)
 	buf->nb_next = head->nb_next;
 	head->nb_next = buf;
 
+	/* Decrement count of busy buffers. */
+
+	head->nb_bytecount--;
+
+	/*
+	 * If the pool has been marked for deletion and there are
+	 * no more buffers outstanding, nuke the pool.
+	 */
+
+	if (head->nb_byteoffset && head->nb_bytecount == 0)
+		free(head, M_DEVBUF);
+
 	return;
 }
 
diff --git a/sys/compat/ndis/subr_ntoskrnl.c b/sys/compat/ndis/subr_ntoskrnl.c
index c0a9535bbff6..f6d0df8991ba 100644
--- a/sys/compat/ndis/subr_ntoskrnl.c
+++ b/sys/compat/ndis/subr_ntoskrnl.c
@@ -1117,6 +1117,18 @@ ntoskrnl_freemdl(mdl)
         mdl->nb_next = head->nb_next;
         head->nb_next = mdl;
 
+	/* Decrement count of busy buffers. */
+
+	head->nb_bytecount--;
+
+	/*
+	 * If the pool has been marked for deletion and there are
+	 * no more buffers outstanding, nuke the pool.
+	 */
+
+	if (head->nb_byteoffset && head->nb_bytecount == 0)
+		free(head, M_DEVBUF);
+
         return;
 }