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--
(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.