potential bug in tcp_hostcache.c

From: Luigi Rizzo <rizzo_at_icir.org>
Date: Mon, 29 Mar 2004 13:07:16 -0800
Hi,
while doing a backport to RELENG_4 of the tcp_hostcache stuff,
a student of mine found a potential bug in tcp_hc_purge():

in sys/netinet/tcp_hostcache.c:tcp_hc_purge() near line 714
		
    for (i = 0; i < tcp_hostcache.hashsize; i++) {
	THC_LOCK(&tcp_hostcache.hashbase[i].hch_mtx);
	TAILQ_FOREACH(hc_entry, &tcp_hostcache.hashbase[i].hch_bucket,
		      rmx_q) {
		if (all || hc_entry->rmx_expire <= 0) {
			TAILQ_REMOVE(&tcp_hostcache.hashbase[i].hch_bucket,
				      hc_entry, rmx_q);
			uma_zfree(tcp_hostcache.zone, hc_entry);
			tcp_hostcache.hashbase[i].hch_length--;
			tcp_hostcache.cache_count--;
		} else
			hc_entry->rmx_expire -= TCP_HOSTCACHE_PRUNE;
	}
	THC_UNLOCK(&tcp_hostcache.hashbase[i].hch_mtx);

the TAILQ_FOREACH will dereference hc_entry after the struct has
been freed by uma_zfree() in the previous loop. While the code
seems to work because uma does not clobber the record,
the tcp_hostcache.zone is not locked around the loop so it
might well happen that some other thread will grab and overwrite
the record we are trying to use.


The usual range of possible fixes apply e.g.

+	struct hc_metrics *tmp = NULL;
	...
	TAILQ_FOREACH(hc_entry, &tcp_hostcache.hashbase[i].hch_bucket,
		      rmx_q) {
+		if (tmp)
+			uma_zfree(tcp_hostcache.zone, tmp);
+		tmp = NULL;
		if (all || hc_entry->rmx_expire <= 0) {
			TAILQ_REMOVE(&tcp_hostcache.hashbase[i].hch_bucket,
				      hc_entry, rmx_q);
			uma_zfree(tcp_hostcache.zone, hc_entry);
			tcp_hostcache.hashbase[i].hch_length--;
			tcp_hostcache.cache_count--;
		} else
			hc_entry->rmx_expire -= TCP_HOSTCACHE_PRUNE;
	}
+	if (tmp)
+		uma_zfree(tcp_hostcache.zone, tmp);

Anyone going to commit a fix (this or something equivalent)
or i should do it ?

	thanks
	luigi
Received on Mon Mar 29 2004 - 11:07:16 UTC

This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:37:49 UTC