From owner-freebsd-current@FreeBSD.ORG  Mon Mar 29 13:07:16 2004
Return-Path: <owner-freebsd-current@FreeBSD.ORG>
Delivered-To: freebsd-current@freebsd.org
Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125])
	by hub.freebsd.org (Postfix) with ESMTP id C8F2116A4CE
	for <current@freebsd.org>; Mon, 29 Mar 2004 13:07:16 -0800 (PST)
Received: from xorpc.icir.org (xorpc.icir.org [192.150.187.68])
	by mx1.FreeBSD.org (Postfix) with ESMTP id 7CCDF43D31
	for <current@freebsd.org>; Mon, 29 Mar 2004 13:07:16 -0800 (PST)
	(envelope-from rizzo@icir.org)
Received: from xorpc.icir.org (localhost [127.0.0.1])
	by xorpc.icir.org (8.12.9p1/8.12.8) with ESMTP id i2TL7Ggd027387;
	Mon, 29 Mar 2004 13:07:16 -0800 (PST)
	(envelope-from rizzo@xorpc.icir.org)
Received: (from rizzo@localhost)
	by xorpc.icir.org (8.12.9p1/8.12.3/Submit) id i2TL7Gi5027386;
	Mon, 29 Mar 2004 13:07:16 -0800 (PST)
	(envelope-from rizzo)
Date: Mon, 29 Mar 2004 13:07:16 -0800
From: Luigi Rizzo <rizzo@icir.org>
To: current@freebsd.org
Message-ID: <20040329130716.A26409@xorpc.icir.org>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
User-Agent: Mutt/1.2.5.1i
Subject: potential bug in tcp_hostcache.c
X-BeenThere: freebsd-current@freebsd.org
X-Mailman-Version: 2.1.1
Precedence: list
List-Id: Discussions about the use of FreeBSD-current
	<freebsd-current.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/freebsd-current>,
	<mailto:freebsd-current-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/freebsd-current>
List-Post: <mailto:freebsd-current@freebsd.org>
List-Help: <mailto:freebsd-current-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/freebsd-current>,
	<mailto:freebsd-current-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Mon, 29 Mar 2004 21:07:16 -0000

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