NetBSD Problem Report #52515

From www@NetBSD.org  Thu Aug 31 10:46:49 2017
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (verified OK))
	by mollari.NetBSD.org (Postfix) with ESMTPS id E2A9F7A212
	for <gnats-bugs@gnats.NetBSD.org>; Thu, 31 Aug 2017 10:46:49 +0000 (UTC)
Message-Id: <20170831104648.C8BA57A26A@mollari.NetBSD.org>
Date: Thu, 31 Aug 2017 10:46:48 +0000 (UTC)
From: knakahara@netbsd.org
Reply-To: knakahara@netbsd.org
To: gnats-bugs@NetBSD.org
Subject: percpu'ed data (e.g. psref_cpu) can cause panic when percpu_cpu_enlarge() run
X-Send-Pr-Version: www-1.0

>Number:         52515
>Category:       kern
>Synopsis:       percpu'ed data (e.g. psref_cpu) can cause panic when percpu_cpu_enlarge() run
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Thu Aug 31 10:50:00 +0000 2017
>Closed-Date:    Mon Feb 12 13:21:34 +0000 2018
>Last-Modified:  Mon Feb 12 13:21:34 +0000 2018
>Originator:     Kengo NAKAHARA
>Release:        -current and -8(potentially)
>Organization:
Internet Initiative Japan
>Environment:
>Description:
I tested IPsec heavy loading test on -current kernel(later than Augest 22).
That is like following test.
    + setup
      - setup gif(4) interface between DUT1 and DUT2
      - proctect the gif(4) with IPsec transport mode
    + network load
      - sending packets(about 10000pps) over the gif/IPsec
    + ioctl load
      - repeat "ifconfig gifX tunnel src dst" and "ifconfig gifX deletetunnel"
      - add many security associates(SAs) and security policys(SPs)

I met a panic, that is below back trace.
====================
uvm_fault(0xffffffff81554c80, 0xffff800011fb1000, 2) -> e
fatal page fault in supervisor mode
trap type 6 code 0x2 rip 0xffffffff809c1681 cs 0x8 rflags 0x10246 cr2 0xffff800011fb1410 ilevel 0x4 rsp 0xffffe401109ddbb0
curlwp 0xffffe4027d918180 pid 4440.1 lowest kstack 0xffffe401109da2c0
kernel: page fault trap, code=0
Stopped in pid 4440.1 (ifconfig) at     netbsd:psref_release+0x8a:      movq    %rdx,0(%rcx)
db{4}> bt
psref_release() at netbsd:psref_release+0x8a
doifioctl() at netbsd:doifioctl+0x815
soo_ioctl() at netbsd:soo_ioctl+0x2b5
sys_ioctl() at netbsd:sys_ioctl+0x101
syscall() at netbsd:syscall+0x1ed
--- syscall (number 54) ---
7806c3f175aa:
====================

As a result of my analysis, the reason is following.
    [1] psref_acquire() is called (by doifioctl() in this case):
        [1-1] get "pcpu" pointer by percpu_getref().
        [1-2] insert a element to "pcpu->pcpu_head" list.
              # that is, the inserted element has a pointer to "pcpu->pcpu_head"
              # as "le_prev".
    [2] percpu_cpu_enlarge() can be called:
        # percpu_cpu_enlarge() is called when percpu data region become starved
        # of memory.
        # After MP-ify IPsec, each SA and SP has localcount(9), that is,
        # localcount_init() => percpu_alloc() is called for each SA and SP.
        # That can cause a lack of percpu data region easily...
        [2-1] percpu_cpu_enlarge() allocate new larger memory region.
        [2-2] copy old percpu data to new memory region.
        [2-3] free old memory region.
    [3] psref_release() is called  (by doifioctl() in this case):
        [3-1] try to remove the element added by psref_acquire() of [1-2].
        [3-2] reference "le_prev" pointer of the element.
        [3-3] the "le_prev" pointer still points *old* percpu data region.
              that is already freed by [2-3] of percpu_cpu_enlarge()!

Yes, this problem is caused by not only psref but also the component who uses
percpu as struct which can be pointed by non-percpu data.
e.g. ipforward_rt_percpu in ip_input.c
# Actually, I met other panic caused by ipforward_rt_percpu when debugging.

>How-To-Repeat:
do heavy load IPsec test like the test in full description.
>Fix:
I am investigating in detail and checking all component which calls
percpu_alloc().
I think it may fix to let the back pointer to percpu data change from
raw pointer to percpu_t pointer. Yes, check all of the percpu struct and
modify problematic ones...

Does anyone have better idea?

>Release-Note:

>Audit-Trail:
From: "Ryota Ozaki" <ozaki-r@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/52515 CVS commit: src/sys
Date: Thu, 21 Sep 2017 07:15:35 +0000

 Module Name:	src
 Committed By:	ozaki-r
 Date:		Thu Sep 21 07:15:35 UTC 2017

 Modified Files:
 	src/sys/net: route.c route.h
 	src/sys/netatalk: at_proto.c
 	src/sys/netinet: in_proto.c
 	src/sys/netinet6: in6_proto.c
 	src/sys/netmpls: mpls_proto.c
 	src/sys/netnatm: natm_proto.c
 	src/sys/rump/net/lib/libsockin: sockin.c
 	src/sys/sys: domain.h

 Log Message:
 Invalidate rtcache based on a global generation counter

 The change introduces a global generation counter that is incremented when any
 routes have been added or deleted. When a rtcache caches a rtentry into itself,
 it also stores a snapshot of the generation counter. If the snapshot equals to
 the global counter, the cache is still valid, otherwise invalidated.

 One drawback of the change is that all rtcaches of all protocol families are
 invalidated when any routes of any protocol families are added or deleted.
 If that matters, we should have separate generation counters based on
 protocol families.

 This change removes LIST_ENTRY from struct route, which fixes a part of
 PR kern/52515.


 To generate a diff of this commit:
 cvs rdiff -u -r1.198 -r1.199 src/sys/net/route.c
 cvs rdiff -u -r1.113 -r1.114 src/sys/net/route.h
 cvs rdiff -u -r1.21 -r1.22 src/sys/netatalk/at_proto.c
 cvs rdiff -u -r1.123 -r1.124 src/sys/netinet/in_proto.c
 cvs rdiff -u -r1.117 -r1.118 src/sys/netinet6/in6_proto.c
 cvs rdiff -u -r1.30 -r1.31 src/sys/netmpls/mpls_proto.c
 cvs rdiff -u -r1.17 -r1.18 src/sys/netnatm/natm_proto.c
 cvs rdiff -u -r1.64 -r1.65 src/sys/rump/net/lib/libsockin/sockin.c
 cvs rdiff -u -r1.32 -r1.33 src/sys/sys/domain.h

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

From: "Kengo NAKAHARA" <knakahara@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/52515 CVS commit: src/sys/opencrypto
Date: Fri, 22 Sep 2017 03:04:06 +0000

 Module Name:	src
 Committed By:	knakahara
 Date:		Fri Sep 22 03:04:06 UTC 2017

 Modified Files:
 	src/sys/opencrypto: crypto.c

 Log Message:
 fix opencrypto(9) part of PR kern/52515

 percpu data use pointers to TAILQ instead of TAILQ itself.


 To generate a diff of this commit:
 cvs rdiff -u -r1.100 -r1.101 src/sys/opencrypto/crypto.c

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

From: "Soren Jacobsen" <snj@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/52515 CVS commit: [netbsd-8] src
Date: Tue, 24 Oct 2017 08:55:56 +0000

 Module Name:	src
 Committed By:	snj
 Date:		Tue Oct 24 08:55:56 UTC 2017

 Modified Files:
 	src/distrib/sets/lists/tests [netbsd-8]: mi
 	src/sys/net [netbsd-8]: route.c route.h
 	src/sys/netatalk [netbsd-8]: at_proto.c
 	src/sys/netinet [netbsd-8]: in_proto.c
 	src/sys/netinet6 [netbsd-8]: in6_proto.c
 	src/sys/netmpls [netbsd-8]: mpls_proto.c
 	src/sys/netnatm [netbsd-8]: natm_proto.c
 	src/sys/rump/net/lib/libsockin [netbsd-8]: sockin.c
 	src/sys/sys [netbsd-8]: domain.h
 	src/tests/net/route [netbsd-8]: Makefile
 Added Files:
 	src/tests/net/route [netbsd-8]: t_rtcache.sh

 Log Message:
 Pull up following revision(s) (requested by ozaki-r in ticket #305):
 	distrib/sets/lists/tests/mi: revision 1.762
 	sys/net/route.c: revision 1.198-1.201
 	sys/net/route.h: revision 1.114
 	sys/netatalk/at_proto.c: revision 1.22
 	sys/netinet/in_proto.c: revision 1.124
 	sys/netinet6/in6_proto.c: revision 1.118
 	sys/netmpls/mpls_proto.c: revision 1.31
 	sys/netnatm/natm_proto.c: revision 1.18
 	sys/rump/net/lib/libsockin/sockin.c: revision 1.65
 	sys/sys/domain.h: revision 1.33
 	tests/net/route/Makefile: revision 1.6
 	tests/net/route/t_rtcache.sh: revision 1.1
 Add tests of rtcache invalidation
 Remove unnecessary NULL check of rt_ifp
 It's always non-NULL.
 Invalidate rtcache based on a global generation counter
 The change introduces a global generation counter that is incremented when any
 routes have been added or deleted. When a rtcache caches a rtentry into itself,
 it also stores a snapshot of the generation counter. If the snapshot equals to
 the global counter, the cache is still valid, otherwise invalidated.
 One drawback of the change is that all rtcaches of all protocol families are
 invalidated when any routes of any protocol families are added or deleted.
 If that matters, we should have separate generation counters based on
 protocol families.
 This change removes LIST_ENTRY from struct route, which fixes a part of
 PR kern/52515.
 Remove the global lock for rtcache
 Thanks to removal of LIST_ENTRY of struct route, rtcaches are accessed only by
 their users. And in existing usages a rtcache is guranteed to be not accessed
 simultaneously. So the rtcache framework doesn't need any exclusion controls
 in itself.
 Synchronize on rtcache_generation with rtlock
 It's racy if NET_MPSAFE is enabled.
 Pointed out by joerg@


 To generate a diff of this commit:
 cvs rdiff -u -r1.752.2.4 -r1.752.2.5 src/distrib/sets/lists/tests/mi
 cvs rdiff -u -r1.194.6.2 -r1.194.6.3 src/sys/net/route.c
 cvs rdiff -u -r1.112.4.1 -r1.112.4.2 src/sys/net/route.h
 cvs rdiff -u -r1.21 -r1.21.10.1 src/sys/netatalk/at_proto.c
 cvs rdiff -u -r1.123.4.1 -r1.123.4.2 src/sys/netinet/in_proto.c
 cvs rdiff -u -r1.117.4.1 -r1.117.4.2 src/sys/netinet6/in6_proto.c
 cvs rdiff -u -r1.30 -r1.30.8.1 src/sys/netmpls/mpls_proto.c
 cvs rdiff -u -r1.17 -r1.17.8.1 src/sys/netnatm/natm_proto.c
 cvs rdiff -u -r1.64 -r1.64.8.1 src/sys/rump/net/lib/libsockin/sockin.c
 cvs rdiff -u -r1.32 -r1.32.10.1 src/sys/sys/domain.h
 cvs rdiff -u -r1.5 -r1.5.6.1 src/tests/net/route/Makefile
 cvs rdiff -u -r0 -r1.1.2.2 src/tests/net/route/t_rtcache.sh

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

From: "Kengo NAKAHARA" <knakahara@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/52515 CVS commit: src/sys
Date: Mon, 11 Dec 2017 02:33:17 +0000

 Module Name:	src
 Committed By:	knakahara
 Date:		Mon Dec 11 02:33:17 UTC 2017

 Modified Files:
 	src/sys/kern: subr_psref.c
 	src/sys/sys: psref.h

 Log Message:
 Fix psref(9) part of PR kern/52515. It is complete to fix the PR now.

 implementated by ozaki-r@n.o, reviewed by riastradh@n.o, thanks.

 XXX need pullup-8


 To generate a diff of this commit:
 cvs rdiff -u -r1.7 -r1.8 src/sys/kern/subr_psref.c
 cvs rdiff -u -r1.2 -r1.3 src/sys/sys/psref.h

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

State-Changed-From-To: open->pending-pullups
State-Changed-By: knakahara@NetBSD.org
State-Changed-When: Thu, 21 Dec 2017 10:14:37 +0000
State-Changed-Why:
305, 458, and 460


From: "Soren Jacobsen" <snj@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/52515 CVS commit: [netbsd-8] src/sys
Date: Tue, 2 Jan 2018 10:36:13 +0000

 Module Name:	src
 Committed By:	snj
 Date:		Tue Jan  2 10:36:13 UTC 2018

 Modified Files:
 	src/sys/kern [netbsd-8]: subr_psref.c
 	src/sys/sys [netbsd-8]: psref.h

 Log Message:
 Pull up following revision(s) (requested by ozaki-r in ticket #458):
 	sys/sys/psref.h: 1.3
 	sys/kern/subr_psref.c: 1.8-1.9
 Fix psref(9) part of PR kern/52515. It is complete to fix the PR now.
 implementated by ozaki-r@n.o, reviewed by riastradh@n.o, thanks.
 --
 Improve debugging functions
 - Make psref_check_duplication check just if a given psref is on the list
   - It checked both psref and target
   - Suggested by riastradh@ some time ago
 - Add psref_check_existence that checks a releasing psref is surely on the list


 To generate a diff of this commit:
 cvs rdiff -u -r1.7 -r1.7.2.1 src/sys/kern/subr_psref.c
 cvs rdiff -u -r1.2 -r1.2.8.1 src/sys/sys/psref.h

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

From: "Soren Jacobsen" <snj@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/52515 CVS commit: [netbsd-8] src/sys/opencrypto
Date: Tue, 2 Jan 2018 10:38:36 +0000

 Module Name:	src
 Committed By:	snj
 Date:		Tue Jan  2 10:38:36 UTC 2018

 Modified Files:
 	src/sys/opencrypto [netbsd-8]: crypto.c

 Log Message:
 Pull up following revision(s) (requested by knakahara in ticket #460):
 	sys/opencrypto/crypto.c: revision 1.101
 fix opencrypto(9) part of PR kern/52515
 percpu data use pointers to TAILQ instead of TAILQ itself.


 To generate a diff of this commit:
 cvs rdiff -u -r1.78.2.3 -r1.78.2.4 src/sys/opencrypto/crypto.c

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

State-Changed-From-To: pending-pullups->closed
State-Changed-By: maxv@NetBSD.org
State-Changed-When: Mon, 12 Feb 2018 13:21:34 +0000
State-Changed-Why:
The fixes were pulled up.


>Unformatted:

Home	PR Database Search