From owner-freebsd-current@FreeBSD.ORG Mon Mar 29 13:21:07 2004 Return-Path: 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 5C2FA16A4CF for ; Mon, 29 Mar 2004 13:21:07 -0800 (PST) Received: from mailtoaster1.pipeline.ch (mailtoaster1.pipeline.ch [62.48.0.70]) by mx1.FreeBSD.org (Postfix) with ESMTP id 9538D43D1F for ; Mon, 29 Mar 2004 13:21:06 -0800 (PST) (envelope-from andre@freebsd.org) Received: (qmail 31252 invoked from network); 29 Mar 2004 21:21:05 -0000 Received: from unknown (HELO freebsd.org) ([62.48.0.47]) (envelope-sender ) by mailtoaster1.pipeline.ch (qmail-ldap-1.03) with SMTP for ; 29 Mar 2004 21:21:05 -0000 Message-ID: <4068933A.5090302@freebsd.org> Date: Mon, 29 Mar 2004 23:20:58 +0200 From: Andre Oppermann User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7b) Gecko/20040316 X-Accept-Language: en-us, en MIME-Version: 1.0 To: Luigi Rizzo References: <20040329130716.A26409@xorpc.icir.org> In-Reply-To: <20040329130716.A26409@xorpc.icir.org> Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit cc: current@freebsd.org Subject: Re: 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 29 Mar 2004 21:21:07 -0000 Luigi Rizzo wrote: > 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 ? Hi Luigi, I have not disappeared since you told me about the bug, I just had a very busy week. The fix is on my commit list and will go in shortly. -- Andre