NetBSD Problem Report #45618

From www@NetBSD.org  Wed Nov 16 13:07:19 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 C285163D862
	for <gnats-bugs@gnats.NetBSD.org>; Wed, 16 Nov 2011 13:07:19 +0000 (UTC)
Message-Id: <20111116130718.D583063D729@www.NetBSD.org>
Date: Wed, 16 Nov 2011 13:07:18 +0000 (UTC)
From: ohmori@chikushi-u.ac.jp
Reply-To: ohmori@chikushi-u.ac.jp
To: gnats-bugs@NetBSD.org
Subject: kqueue EVFILT_TIMER with smaller timeout value makes kernel busy or panic
X-Send-Pr-Version: www-1.0

>Number:         45618
>Category:       kern
>Synopsis:       kqueue EVFILT_TIMER with smaller timeout value makes kernel busy or panic
>Confidential:   no
>Severity:       critical
>Priority:       high
>Responsible:    kern-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Wed Nov 16 13:10:01 +0000 2011
>Closed-Date:    Sun Nov 20 17:15:34 +0000 2011
>Last-Modified:  Mon Nov 21 01:40:01 +0000 2011
>Originator:     Motoyuki OHMORI
>Release:        NetBSD current 5.99.22 (2010/04/26) and 5.1-RELEASE
>Organization:
Chikushi Jogakuen University
>Environment:
NetBSD xxx 5.1 NetBSD 5.1 (GENERIC) #0: Sat Nov  6 13:19:33 UTC 2010  builds@b6.netbsd.org:/home/builds/ab/netbsd-5-1-RELEASE/amd64/201011061943Z-obj/home/builds/ab/netbsd-5-1-RELEASE/src/sys/arch/amd64/compile/GENERIC amd64

>Description:
When EVFILT_TIMER with smaller interval value kernel gets busy or sometimes panic.

The kernel seems to be alive because it answers for ping from remote
host.  However, kernel seems to be too busy after the EVFILT_TIMER is 
added and expires once.  While a similar problem is reported by #42685, 
I here do not use pthread.

Even though my environment is a little bit old, I suspect that a function,
filt_timerexpire, in kern_event.c even in current seems to have same 
issue.  The problem seems that filt_timerexpire() does not consider the 
case where tticks is equal to zero after a timer expires at least once.
>How-To-Repeat:
Execute kevent like below:

    EV_SET(&ev, ident, EVFILT_TIMER, EV_ADD, 0, 1, 0);
    kevent(kqueue, &ev, 1, NULL, 0, NULL);

We then get kernel busy or panic.
>Fix:
At this moment, I cannot examine a patch.  However, I hope that a below
patch would be effective. 

Index: sys/kern/kern_event.c
===================================================================
--- sys/kern/kern_event.c       (revision 147)
+++ sys/kern/kern_event.c       (working copy)
@@ -598,6 +598,8 @@
        knote_activate(kn);
        if ((kn->kn_flags & EV_ONESHOT) == 0) {
                tticks = mstohz(kn->kn_sdata);
+               if (tticks == 0)
+                       tticks = 1;
                callout_schedule((callout_t *)kn->kn_hook, tticks);
        }
        mutex_exit(&kqueue_misc_lock);

>Release-Note:

>Audit-Trail:
From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/45618 CVS commit: src/tests/lib/libc/sys
Date: Wed, 16 Nov 2011 20:14:13 -0500

 Module Name:	src
 Committed By:	christos
 Date:		Thu Nov 17 01:14:13 UTC 2011

 Modified Files:
 	src/tests/lib/libc/sys: Makefile
 Added Files:
 	src/tests/lib/libc/sys: t_kevent.c

 Log Message:
 Add a test for PR/45618: Motoyuki OHMORI: kqueue EVFILT_TIMER with smaller
 timeout value makes kernel busy or panic


 To generate a diff of this commit:
 cvs rdiff -u -r1.19 -r1.20 src/tests/lib/libc/sys/Makefile
 cvs rdiff -u -r0 -r1.1 src/tests/lib/libc/sys/t_kevent.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->feedback
State-Changed-By: christos@NetBSD.org
State-Changed-When: Wed, 16 Nov 2011 20:20:10 -0500
State-Changed-Why:
fixed


From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/45618 CVS commit: src/sys/kern
Date: Wed, 16 Nov 2011 20:19:37 -0500

 Module Name:	src
 Committed By:	christos
 Date:		Thu Nov 17 01:19:37 UTC 2011

 Modified Files:
 	src/sys/kern: kern_event.c

 Log Message:
 PR/45618: Motoyuki OHMORI: kqueue EVFILT_TIMER with smaller timeout value
 makes DIAGNOSTIC kernel panic:
 	KASSERT((c->c_flags & CALLOUT_PENDING) != 0);
 If the computed ticks are <= 0 set it to 1


 To generate a diff of this commit:
 cvs rdiff -u -r1.72 -r1.73 src/sys/kern/kern_event.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Stephen Borrill" <sborrill@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/45618 CVS commit: [netbsd-5] src/sys/kern
Date: Fri, 18 Nov 2011 23:17:54 +0000

 Module Name:	src
 Committed By:	sborrill
 Date:		Fri Nov 18 23:17:53 UTC 2011

 Modified Files:
 	src/sys/kern [netbsd-5]: kern_event.c

 Log Message:
 Pull up the following revisions(s) (requested by christos in ticket #1693):
 	sys/kern/kern_event.c:	revision 1.73

 PR/45618: Motoyuki OHMORI: kqueue EVFILT_TIMER with smaller timeout value
 makes DIAGNOSTIC kernel panic. If the computed ticks are <= 0 set it to 1.


 To generate a diff of this commit:
 cvs rdiff -u -r1.60.6.2 -r1.60.6.3 src/sys/kern/kern_event.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Manuel Bouyer" <bouyer@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/45618 CVS commit: [netbsd-4] src/sys/kern
Date: Sat, 19 Nov 2011 14:37:02 +0000

 Module Name:	src
 Committed By:	bouyer
 Date:		Sat Nov 19 14:37:02 UTC 2011

 Modified Files:
 	src/sys/kern [netbsd-4]: kern_event.c

 Log Message:
 Pull up following revision(s) (requested by christos in ticket #1438):
 	sys/kern/kern_event.c: revision 1.73
 PR/45618: Motoyuki OHMORI: kqueue EVFILT_TIMER with smaller timeout value
 makes DIAGNOSTIC kernel panic:
         KASSERT((c->c_flags & CALLOUT_PENDING) !=3D 0);
 If the computed ticks are <= 0 set it to 1


 To generate a diff of this commit:
 cvs rdiff -u -r1.33 -r1.33.2.1 src/sys/kern/kern_event.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Manuel Bouyer" <bouyer@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/45618 CVS commit: [netbsd-4-0] src/sys/kern
Date: Sat, 19 Nov 2011 14:37:09 +0000

 Module Name:	src
 Committed By:	bouyer
 Date:		Sat Nov 19 14:37:09 UTC 2011

 Modified Files:
 	src/sys/kern [netbsd-4-0]: kern_event.c

 Log Message:
 Pull up following revision(s) (requested by christos in ticket #1438):
 	sys/kern/kern_event.c: revision 1.73
 PR/45618: Motoyuki OHMORI: kqueue EVFILT_TIMER with smaller timeout value
 makes DIAGNOSTIC kernel panic:
         KASSERT((c->c_flags & CALLOUT_PENDING) !=3D 0);
 If the computed ticks are <= 0 set it to 1


 To generate a diff of this commit:
 cvs rdiff -u -r1.33 -r1.33.8.1 src/sys/kern/kern_event.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: Motoyuki OHMORI <ohmori@chikushi-u.ac.jp>
To: gnats-bugs@NetBSD.org
Cc: kern-bug-people@netbsd.org, gnats-admin@netbsd.org,
	netbsd-bugs@netbsd.org, ohmori@chikushi-u.ac.jp
Subject: Re: PR/45618 CVS commit: src/sys/kern
Date: Sun, 20 Nov 2011 19:49:13 +0900

 Dear Christos,

 # This is just reply because GNAT requests me to reply to close the ticket.

 Thank you very much for your quick response.  Now, the problem has been solved.
 Please close the ticket.

 BTW, I am curious about one trivial thing of your patch if there is a case
 where computed ticks can be negative.  I thought that `kn_sdata' is unsigned
 and computed ticks would be unsigned.  I then thought that `tticks == 0' was
 enough.

 Anyway, thank you very much for your fix.  I think that now our kernel gets
 stronger and more stable against an unresonable timeout interval from user land.

 Best regards,
 --
 Motoyuki OHMORI <ohmori@chikushi-u.ac.jp>

 On Thu, Nov 17, 2011 at 01:35:02 +0000, Christos Zoulas wrote:
 > The following reply was made to PR kern/45618; it has been noted by GNATS.
 > 
 > From: "Christos Zoulas" <christos@netbsd.org>
 > To: gnats-bugs@gnats.NetBSD.org
 > Cc: 
 > Subject: PR/45618 CVS commit: src/sys/kern
 > Date: Wed, 16 Nov 2011 20:19:37 -0500
 > 
 >  Module Name:	src
 >  Committed By:	christos
 >  Date:		Thu Nov 17 01:19:37 UTC 2011
 >  
 >  Modified Files:
 >  	src/sys/kern: kern_event.c
 >  
 >  Log Message:
 >  PR/45618: Motoyuki OHMORI: kqueue EVFILT_TIMER with smaller timeout value
 >  makes DIAGNOSTIC kernel panic:
 >  	KASSERT((c->c_flags & CALLOUT_PENDING) != 0);
 >  If the computed ticks are <= 0 set it to 1
 >  
 >  
 >  To generate a diff of this commit:
 >  cvs rdiff -u -r1.72 -r1.73 src/sys/kern/kern_event.c
 >  
 >  Please note that diffs are not public domain; they are subject to the
 >  copyright notices on the relevant files.
 >  

From: christos@zoulas.com (Christos Zoulas)
To: Motoyuki OHMORI <ohmori@chikushi-u.ac.jp>, gnats-bugs@NetBSD.org
Cc: kern-bug-people@netbsd.org, gnats-admin@netbsd.org, 
	netbsd-bugs@netbsd.org
Subject: Re: PR/45618 CVS commit: src/sys/kern
Date: Sun, 20 Nov 2011 12:12:59 -0500

 On Nov 20,  7:49pm, ohmori@chikushi-u.ac.jp (Motoyuki OHMORI) wrote:
 -- Subject: Re: PR/45618 CVS commit: src/sys/kern

 | Dear Christos,
 | 
 | # This is just reply because GNAT requests me to reply to close the ticket.
 | 
 | Thank you very much for your quick response.  Now, the problem has been solved.
 | Please close the ticket.
 | 
 | BTW, I am curious about one trivial thing of your patch if there is a case
 | where computed ticks can be negative.  I thought that `kn_sdata' is unsigned
 | and computed ticks would be unsigned.  I then thought that `tticks == 0' was
 | enough.

 But ticks is signed so I checked against negative too.

 christos

State-Changed-From-To: feedback->closed
State-Changed-By: christos@NetBSD.org
State-Changed-When: Sun, 20 Nov 2011 12:15:34 -0500
State-Changed-Why:
submitter confirms fixed


From: Motoyuki OHMORI <ohmori@chikushi-u.ac.jp>
To: Christos Zoulas <christos@zoulas.com>
Cc: Motoyuki OHMORI <ohmori@chikushi-u.ac.jp>, gnats-bugs@NetBSD.org,
	kern-bug-people@netbsd.org, gnats-admin@netbsd.org,
	netbsd-bugs@netbsd.org
Subject: Re: PR/45618 CVS commit: src/sys/kern
Date: Mon, 21 Nov 2011 10:39:00 +0900

 Dear Christos,

 > | BTW, I am curious about one trivial thing of your patch if there is a case
 > | where computed ticks can be negative.  I thought that `kn_sdata' is unsigned
 > | and computed ticks would be unsigned.  I then thought that `tticks == 0' was
 > | enough.
 > 
 > But ticks is signed so I checked against negative too.

 Okay, I agree with you.

 Best regards,
 --
 Motoyuki OHMORI <ohmori@chikushi-u.ac.jp>

 On Sun, Nov 20, 2011 at 12:12:59 -0500, Christos Zoulas wrote:
 > On Nov 20,  7:49pm, ohmori@chikushi-u.ac.jp (Motoyuki OHMORI) wrote:
 > -- Subject: Re: PR/45618 CVS commit: src/sys/kern
 > 
 > | Dear Christos,
 > | 
 > | # This is just reply because GNAT requests me to reply to close the ticket.
 > | 
 > | Thank you very much for your quick response.  Now, the problem has been solved.
 > | Please close the ticket.
 > | 
 > | BTW, I am curious about one trivial thing of your patch if there is a case
 > | where computed ticks can be negative.  I thought that `kn_sdata' is unsigned
 > | and computed ticks would be unsigned.  I then thought that `tticks == 0' was
 > | enough.
 > 
 > But ticks is signed so I checked against negative too.
 > 
 > christos

>Unformatted:

Home	PR Database Search