NetBSD Problem Report #45634

From www@NetBSD.org  Sat Nov 19 20:55:58 2011
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [204.152.190.11])
	by www.NetBSD.org (Postfix) with ESMTP id 55E2E63CD2A
	for <gnats-bugs@gnats.NetBSD.org>; Sat, 19 Nov 2011 20:55:58 +0000 (UTC)
Message-Id: <20111119205557.75F5C63C3D5@www.NetBSD.org>
Date: Sat, 19 Nov 2011 20:55:57 +0000 (UTC)
From: christianbiere@gmx.de
Reply-To: christianbiere@gmx.de
To: gnats-bugs@NetBSD.org
Subject: hardclock_ticks corner cases in vflushnext() et al
X-Send-Pr-Version: www-1.0

>Number:         45634
>Category:       kern
>Synopsis:       hardclock_ticks corner cases in vflushnext() et al
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sat Nov 19 21:00:00 +0000 2011
>Last-Modified:  Thu Nov 24 21:30:03 +0000 2011
>Originator:     Christian Biere
>Release:        
>Organization:
>Environment:
>Description:
File: sys/kern/vfs_mount.c
Function: vflushnext()

1. The comparision "hardclock_ticks > *when" apparently assumes perfect integers whilst "hardclock_ticks" will actually wrap around (at least that's the hope) after some time. Therefore this comparision is insufficient to verify has advanced one or more ticks. If *when is initialized just before a wrap around (e.g. to INT_MAX) and hardclock_ticks wraps around to INT_MIN, the comparision will unintentionally fail. [*]

2. The expression "hardclock_ticks + hz / 10" may result in an integer overflow and hence cause undefined behavior.

File: sys/kern/kern_clock.c
Function: hardclock()

3. As "hardclock_ticks" is of type signed int an integer overflow occures after some defined run-time depending on the value of "hz". [*]
While changing its type to unsigned int would fix this issue allowing a well-defined wrap around, this would require further changes in code using this variable. Therefore, side effects can be avoid by the following:

 hardclock_ticks = (unsigned int) hardclock_ticks + 1;

However, it might actually be worthwhile to check the uses of "hardclock_ticks" elsewhere anyways. For example, it seems the wrap around is not taken into account in lacp_sm_tx() in net/agr/ieee8023ad_lacp_sm_tx.c as well as other files.

[*] Assuming hz is set to 100, the apparent compile-time default, these circumstances may arise about every 249 days. Thus, a higher value of hz will increase the likeliness in proportion (e.g. 25 days at 1000 hz). 

>How-To-Repeat:

>Fix:

>Audit-Trail:
From: Christian Biere <christianbiere@gmx.de>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/45634: hardclock_ticks corner cases in vflushnext() et al
Date: Wed, 23 Nov 2011 20:53:02 +0100

 This is a multi-part message in MIME format.

 --Multipart=_Wed__23_Nov_2011_20_53_02_+0100_odB7VCvGv/OwDNAR
 Content-Type: text/plain; charset=US-ASCII
 Content-Transfer-Encoding: 7bit

 The attached patch should avoid the mentioned integer overflows in vflushnext()
 by using the proper arithmetic methods and ensure that the conditional code is actually invoked when the declared amount of ticks has passed.

 --Multipart=_Wed__23_Nov_2011_20_53_02_+0100_odB7VCvGv/OwDNAR
 Content-Type: text/x-diff;
  name="vfs_mount.c.diff"
 Content-Disposition: attachment;
  filename="vfs_mount.c.diff"
 Content-Transfer-Encoding: 7bit

 --- vfs_mount.c.orig	2011-11-23 19:28:41.334186185 +0100
 +++ vfs_mount.c	2011-11-23 20:36:55.004184864 +0100
 @@ -420,14 +420,15 @@
  #endif

  static vnode_t *
 -vflushnext(vnode_t *mvp, int *when)
 +vflushnext(vnode_t *mvp, unsigned *when, unsigned dly)
  {
 +	unsigned dt = hardclock_ticks + dly - *when;

 -	if (hardclock_ticks > *when) {
 +	if (dt > dly) {
  		mutex_exit(&mntvnode_lock);
  		yield();
  		mutex_enter(&mntvnode_lock);
 -		*when = hardclock_ticks + hz / 10;
 +		*when = hardclock_ticks + dly;
  	}
  	return vunmark(mvp);
  }
 @@ -436,7 +437,7 @@
  vflush(struct mount *mp, vnode_t *skipvp, int flags)
  {
  	vnode_t *vp, *mvp;
 -	int busy = 0, when = 0;
 +	unsigned busy = 0, dly = 10 / hz, when = hardclock_ticks - dly - 1;

  	/* First, flush out any vnode references from vrele_list. */
  	vrele_flush();
 @@ -450,7 +451,7 @@
  	 */
  	mutex_enter(&mntvnode_lock);
  	for (vp = TAILQ_FIRST(&mp->mnt_vnodelist); vp != NULL;
 -	    vp = vflushnext(mvp, &when)) {
 +	    vp = vflushnext(mvp, &when, dly)) {
  		vmark(mvp, vp);
  		if (vp->v_mount != mp || vismarker(vp))
  			continue;

 --Multipart=_Wed__23_Nov_2011_20_53_02_+0100_odB7VCvGv/OwDNAR--

From: christos@zoulas.com (Christos Zoulas)
To: gnats-bugs@NetBSD.org, kern-bug-people@netbsd.org, 
	gnats-admin@netbsd.org, netbsd-bugs@netbsd.org, christianbiere@gmx.de
Cc: 
Subject: Re: kern/45634: hardclock_ticks corner cases in vflushnext() et al
Date: Wed, 23 Nov 2011 15:50:28 -0500

 On Nov 23,  7:55pm, christianbiere@gmx.de (Christian Biere) wrote:
 -- Subject: Re: kern/45634: hardclock_ticks corner cases in vflushnext() et a

 |  @@ -436,7 +437,7 @@
 |   vflush(struct mount *mp, vnode_t *skipvp, int flags)
 |   {
 |   	vnode_t *vp, *mvp;
 |  -	int busy = 0, when = 0;
 |  +	unsigned busy = 0, dly = 10 / hz, when = hardclock_ticks - dly - 1;
 |   

 Did you mean to delete the declaration of when?

 christos

From: Christian Biere <christianbiere@gmx.de>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/45634: hardclock_ticks corner cases in vflushnext() et al
Date: Wed, 23 Nov 2011 21:28:10 +0100

 This is a multi-part message in MIME format.

 --Multipart=_Wed__23_Nov_2011_21_28_10_+0100_m.Y6y0xomZ4R3QzA
 Content-Type: text/plain; charset=US-ASCII
 Content-Transfer-Encoding: 7bit

 The following should fix a related issue in sys/kern/kern_ras.c in ras_sync().

 Assume "target" is (close to) INT_MAX, then "hardclock_ticks" might advance
 sufficiently during kpause() to wrap around and become negative. It will
 take a very long time before the loop terminates, if ever.

 --Multipart=_Wed__23_Nov_2011_21_28_10_+0100_m.Y6y0xomZ4R3QzA
 Content-Type: text/x-diff;
  name="kern_ras.c.diff"
 Content-Disposition: attachment;
  filename="kern_ras.c.diff"
 Content-Transfer-Encoding: 7bit

 --- kern_ras.c.orig	2011-11-23 21:04:04.174184339 +0100
 +++ kern_ras.c	2011-11-23 21:17:37.914184076 +0100
 @@ -82,11 +82,12 @@
  		 * o ras_lookup() plus loads reordered in advance
  		 *   will take no longer than 1/8s to complete.
  		 */
 -		const int delta = hz >> 3;
 -		int target = hardclock_ticks + delta;
 +		const unsigned delta = hz >> 3;
 +		unsigned dt, t0 = hardclock_ticks;
  		do {
  			kpause("ras", false, delta, NULL);
 -		} while (hardclock_ticks < target);
 +			dt = hardclock_ticks - t0;
 +		} while (dt < delta);
  #endif
  	}
  }

 --Multipart=_Wed__23_Nov_2011_21_28_10_+0100_m.Y6y0xomZ4R3QzA--

From: Christian Biere <christianbiere@gmx.de>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/45634: hardclock_ticks corner cases in vflushnext() et al
Date: Wed, 23 Nov 2011 22:54:19 +0100

 On Wed, 23 Nov 2011 20:55:02 +0000 (UTC)
 christos@zoulas.com (Christos Zoulas) wrote:

 >  On Nov 23,  7:55pm, christianbiere@gmx.de (Christian Biere) wrote:
 >  -- Subject: Re: kern/45634: hardclock_ticks corner cases in vflushnext() et a
 >  
 >  |  @@ -436,7 +437,7 @@
 >  |   vflush(struct mount *mp, vnode_t *skipvp, int flags)
 >  |   {
 >  |   	vnode_t *vp, *mvp;
 >  |  -	int busy = 0, when = 0;
 >  |  +	unsigned busy = 0, dly = 10 / hz, when = hardclock_ticks - dly - 1;
 >  |   
 >  
 >  Did you mean to delete the declaration of when?

 Not exactly. For clarity it could also splitted:

 unsigned busy, dly, when;

 busy = 0;
 dly = 10 / hz;
 when = hardclock_ticks - dly - 1;

 Note that "when" must be properly initialized and not just be set to zero.
 Consider "hardclock_ticks" being negative. If "when" was zero, a lot of
 ticks would pass before "dt > dly" evaluates true. The same applies to
 the current code with respect to "hardclock_ticks > *when", which I think
 is a bug, too.

 -- 
 Christian Biere

From: Christian Biere <christianbiere@gmx.de>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/45634: hardclock_ticks corner cases in vflushnext() et al
Date: Thu, 24 Nov 2011 21:33:07 +0100

 This is a multi-part message in MIME format.

 --Multipart=_Thu__24_Nov_2011_21_33_07_+0100_r7SGJGTvBRSv2ifH
 Content-Type: text/plain; charset=US-ASCII
 Content-Transfer-Encoding: 7bit

 The attached patch should fix a related issue in sys/dev/i2c/ibmhawk.c.


 --Multipart=_Thu__24_Nov_2011_21_33_07_+0100_r7SGJGTvBRSv2ifH
 Content-Type: text/x-diff;
  name="ibmhawk.c.diff"
 Content-Disposition: attachment;
  filename="ibmhawk.c.diff"
 Content-Transfer-Encoding: 7bit

 --- ibmhawk.c.orig	2011-11-24 21:08:37.230962151 +0100
 +++ ibmhawk.c	2011-11-24 21:26:09.240962777 +0100
 @@ -48,6 +48,9 @@
  #endif
  #endif

 +/* No more than two refreshes per second. */
 +#define IBMHAWK_REFRESH_DELAY (hz / 2U)
 +
  /*
   * Known sensors.
   */
 @@ -117,6 +120,7 @@ ibmhawk_attach(device_t parent, device_t
  	sc->sc_dev = self;
  	sc->sc_tag = ia->ia_tag;
  	sc->sc_addr = ia->ia_addr;
 +	sc->sc_refresh = hardclock_ticks - IBMHAWK_REFRESH_DELAY;

  	if (!pmf_device_register(self, NULL, NULL))
  		aprint_error_dev(self, "couldn't establish power handler\n");
 @@ -357,12 +361,11 @@ ibmhawk_refresh(struct sysmon_envsys *sm
  {
  	struct ibmhawk_softc *sc = sme->sme_cookie;

 -	/* No more than two refreshes per second. */
 -	if (hardclock_ticks-sc->sc_refresh < hz/2)
 +	if (hardclock_ticks - sc->sc_refresh + 0U < IBMHAWK_REFRESH_DELAY)
  		return;
  #if IBMHAWK_DEBUG > 1
 -	aprint_normal_dev(sc->sc_dev, "refresh \"%s\" delta %d\n",
 -	    edata->desc, hardclock_ticks-sc->sc_refresh);
 +	aprint_normal_dev(sc->sc_dev, "refresh \"%s\" delta %u\n",
 +	    edata->desc, hardclock_ticks - sc->sc_refresh + 0U);
  #endif
  	sc->sc_refresh = hardclock_ticks;
  	ibmhawk_refreshall(sc, false);

 --Multipart=_Thu__24_Nov_2011_21_33_07_+0100_r7SGJGTvBRSv2ifH--

From: Christian Biere <christianbiere@gmx.de>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/45634: hardclock_ticks corner cases in vflushnext() et al
Date: Thu, 24 Nov 2011 22:29:25 +0100

 This is a multi-part message in MIME format.

 --Multipart=_Thu__24_Nov_2011_22_29_25_+0100_PqbLoL6DawYb392d
 Content-Type: text/plain; charset=US-ASCII
 Content-Transfer-Encoding: 7bit

 The following patch should fix a related issue in sys/kern/vfs_cache.c.

 --Multipart=_Thu__24_Nov_2011_22_29_25_+0100_PqbLoL6DawYb392d
 Content-Type: text/x-diff;
  name="vfs_cache.c.diff"
 Content-Disposition: attachment;
  filename="vfs_cache.c.diff"
 Content-Transfer-Encoding: 7bit

 --- vfs_cache.c.orig	2011-11-24 21:59:03.670963953 +0100
 +++ vfs_cache.c	2011-11-24 22:17:24.560964607 +0100
 @@ -133,7 +133,7 @@ static pool_cache_t namecache_cache;

  int cache_lowat = 95;
  int cache_hiwat = 98;
 -int cache_hottime = 5;			/* number of seconds */
 +unsigned cache_hottime = 5;		/* number of seconds */
  int doingcache = 1;			/* 1 => enable the cache */

  static struct evcnt cache_ev_scan;
 @@ -619,7 +619,7 @@ cache_enter(struct vnode *dvp, struct vn
  	mutex_enter(&ncp->nc_lock);
  	ncp->nc_vp = vp;
  	ncp->nc_flags = 0;
 -	ncp->nc_hittime = 0;
 +	ncp->nc_hittime = hardclock_ticks;
  	ncp->nc_gcqueue = NULL;
  	if (vp == NULL) {
  		/*
 @@ -874,13 +874,15 @@ static void
  cache_prune(int incache, int target)
  {
  	struct namecache *ncp, *nxtcp, *sentinel;
 -	int items, recent, tryharder;
 +	int items, tryharder;
 +	unsigned now, hottime;

  	KASSERT(mutex_owned(namecache_lock));

  	items = 0;
  	tryharder = 0;
 -	recent = hardclock_ticks - hz * cache_hottime;
 +	now = hardclock_ticks;
 +	hottime = cache_hottime * hz;
  	sentinel = NULL;
  	for (ncp = TAILQ_FIRST(&nclruhead); ncp != NULL; ncp = nxtcp) {
  		if (incache <= target)
 @@ -896,7 +898,7 @@ cache_prune(int incache, int target)
  			 */
  			tryharder = 1;
  		}
 -		if (!tryharder && (ncp->nc_hittime - recent) > 0) {
 +		if (!tryharder && (now - ncp->nc_hittime) <= hottime) {
  			if (sentinel == NULL)
  				sentinel = ncp;
  			TAILQ_REMOVE(&nclruhead, ncp, nc_lru);

 --Multipart=_Thu__24_Nov_2011_22_29_25_+0100_PqbLoL6DawYb392d--

NetBSD Home
NetBSD PR Database Search

(Contact us) $NetBSD: query-full-pr,v 1.39 2013/11/01 18:47:49 spz Exp $
$NetBSD: gnats_config.sh,v 1.8 2006/05/07 09:23:38 tsutsui Exp $
Copyright © 1994-2007 The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.