netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Fw: [Bug 84991] New: Adding a gre tunnel causes a kernel panic
@ 2014-09-22 16:02 Stephen Hemminger
  2014-09-22 17:09 ` Eric Dumazet
  0 siblings, 1 reply; 10+ messages in thread
From: Stephen Hemminger @ 2014-09-22 16:02 UTC (permalink / raw)
  To: netdev

Not good, and user did bisection.

using git bisect, the below commit introduced the above bug.

[7d442fab0a6777fd7612cfcada32ea859553d370] ipv4: Cache dst in tunnels


commit 7d442fab0a6777fd7612cfcada32ea859553d370
Author: Tom Herbert <therbert@google.com>
Date:   Thu Jan 2 11:48:26 2014 -0800

    ipv4: Cache dst in tunnels

    Avoid doing a route lookup on every packet being tunneled.

    In ip_tunnel.c cache the route returned from ip_route_output if
    the tunnel is "connected" so that all the rouitng parameters are
    taken from tunnel parms for a packet. Specifically, not NBMA tunnel
    and tos is from tunnel parms (not inner packet).

    Signed-off-by: Tom Herbert <therbert@google.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>


Begin forwarded message:

Date: Sun, 21 Sep 2014 16:56:08 -0700
From: "bugzilla-daemon@bugzilla.kernel.org" <bugzilla-daemon@bugzilla.kernel.org>
To: "stephen@networkplumber.org" <stephen@networkplumber.org>
Subject: [Bug 84991] New: Adding a gre tunnel causes a kernel panic


https://bugzilla.kernel.org/show_bug.cgi?id=84991

            Bug ID: 84991
           Summary: Adding a gre tunnel causes a kernel panic
           Product: Networking
           Version: 2.5
    Kernel Version: 3.17.0-rc5 #82 SMP PREEMPT
          Hardware: x86-64
                OS: Linux
              Tree: Mainline
            Status: NEW
          Severity: high
          Priority: P1
         Component: IPV4
          Assignee: shemminger@linux-foundation.org
          Reporter: joe9mail@gmail.com
        Regression: No

command:

sudo ip tunnel add mastergrevpn mode gre local 192.168.1.232 remote
192.168.0.11 ttl 255

192.168.0.11 is connected over an ipsec connection.

extract from kern.log

Sep 22 05:04:45 br kernel: [   23.550046] r8169 0000:03:00.0 enp3s0: link up
Sep 22 05:04:46 br kernel: [   24.811978] NET: Registered protocol family 10
Sep 22 05:05:55 br kernel: [   94.471313] NET: Registered protocol family 15
Sep 22 05:05:55 br kernel: [   94.502502] Initializing XFRM netlink socket
Sep 22 05:05:55 br kernel: [   94.550294] gre: GRE over IPv4 demultiplexor
driver
Sep 22 05:05:55 br kernel: [   94.550682] ip_gre: GRE over IPv4 tunneling
driver
Sep 22 05:05:55 br kernel: [   94.608310] BUG: using smp_processor_id() in
preemptible [00000000] code: ip/2261
Sep 22 05:05:55 br kernel: [   94.608316] caller is
tunnel_dst_set.isra.28+0x20/0x60 [ip_tunnel]
Sep 22 05:05:55 br kernel: [   94.608319] CPU: 3 PID: 2261 Comm: ip Not tainted
3.17.0-rc5 #82
Sep 22 05:05:55 br kernel: [   94.608321] Hardware name: System manufacturer
System Product Name/F2A85-M, BIOS 5202 01/22/2013
Sep 22 05:05:55 br kernel: [   94.608323]  0000000000000000 ffffffff816b4926
ffffffff8155f634 0000000000000003
Sep 22 05:05:55 br kernel: [   94.608326]  ffffffff8128e1e1 ffffffff817a3b80
ffffffff816ac8e9 00000000000199b8
Sep 22 05:05:55 br kernel: [   94.608329]  ffff88013922e6c0 00000000e801a8c0
ffffffffa05a1f40 ffff880093237000
Sep 22 05:05:55 br kernel: [   94.608331] Call Trace:
Sep 22 05:05:55 br kernel: [   94.608339]  [<ffffffff8155f634>] ?
dump_stack+0x4a/0x75
Sep 22 05:05:55 br kernel: [   94.608343]  [<ffffffff8128e1e1>] ?
check_preemption_disabled+0xf1/0x100
Sep 22 05:05:55 br kernel: [   94.608346]  [<ffffffffa05a1f40>] ?
tunnel_dst_set.isra.28+0x20/0x60 [ip_tunnel]
Sep 22 05:05:55 br kernel: [   94.608349]  [<ffffffffa05a209e>] ?
ip_tunnel_bind_dev+0x11e/0x190 [ip_tunnel]
Sep 22 05:05:55 br kernel: [   94.608352]  [<ffffffffa05a332b>] ?
ip_tunnel_ioctl+0x25b/0x360 [ip_tunnel]
Sep 22 05:05:55 br kernel: [   94.608357]  [<ffffffffa05ac5fb>] ?
ipgre_tunnel_ioctl+0x16b/0x270 [ip_gre]
Sep 22 05:05:55 br kernel: [   94.608361]  [<ffffffff814dd562>] ?
dev_ifsioc+0x352/0x390
Sep 22 05:05:55 br kernel: [   94.608363]  [<ffffffff814dd726>] ?
dev_ioctl+0xc6/0x530
Sep 22 05:05:55 br kernel: [   94.608366]  [<ffffffff814af7a2>] ?
sock_ioctl+0xd2/0x260
Sep 22 05:05:55 br kernel: [   94.608370]  [<ffffffff811174ae>] ?
do_vfs_ioctl+0x7e/0x510
Sep 22 05:05:55 br kernel: [   94.608373]  [<ffffffff8110839f>] ?
alloc_file+0x1f/0xc0
Sep 22 05:05:55 br kernel: [   94.608376]  [<ffffffff81061339>] ?
get_parent_ip+0x9/0x20
Sep 22 05:05:55 br kernel: [   94.608378]  [<ffffffff81061397>] ?
preempt_count_add+0x47/0x90
Sep 22 05:05:55 br kernel: [   94.608382]  [<ffffffff815645ae>] ?
_raw_spin_lock+0xe/0x30
Sep 22 05:05:55 br kernel: [   94.608385]  [<ffffffff811220ad>] ?
__fd_install+0x2d/0x70
Sep 22 05:05:55 br kernel: [   94.608387]  [<ffffffff81117987>] ?
SyS_ioctl+0x47/0xa0
Sep 22 05:05:55 br kernel: [   94.608390]  [<ffffffff81566462>] ?
page_fault+0x22/0x30
Sep 22 05:05:55 br kernel: [   94.608392]  [<ffffffff81564e56>] ?
system_call_fastpath+0x1a/0x1f


uname -a
Linux br 3.17.0-rc5 #82 SMP PREEMPT Mon Sep 22 05:01:36 IST 2014 x86_64 AMD
A10-5800K APU with Radeon(tm) HD Graphics AuthenticAMD GNU/Linux

cat /proc/cpuinfo
processor    : 0
vendor_id    : AuthenticAMD
cpu family    : 21
model        : 16
model name    : AMD A10-5800K APU with Radeon(tm) HD Graphics
stepping    : 1
microcode    : 0x6001116
cpu MHz        : 3800.000
cache size    : 2048 KB
physical id    : 0
siblings    : 4
core id        : 0
cpu cores    : 2
apicid        : 16
initial apicid    : 0
fpu        : yes
fpu_exception    : yes
cpuid level    : 13
wp        : yes
flags        : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov
pat pse36 clflush mmx fxsr sse sse2 ht syscall nx mmxext fxsr_opt pdpe1gb
rdtscp lm constant_tsc rep_good nopl nonstop_tsc extd_apicid aperfmperf pni
pclmulqdq monitor ssse3 fma cx16 sse4_1 sse4_2 popcnt aes xsave avx f16c
lahf_lm cmp_legacy svm extapic cr8_legacy abm sse4a misalignsse 3dnowprefetch
osvw ibs xop skinit wdt lwp fma4 tce nodeid_msr tbm topoext perfctr_core
perfctr_nb arat cpb hw_pstate npt lbrv svm_lock nrip_save tsc_scale vmcb_clean
flushbyasid decodeassists pausefilter pfthreshold bmi1
bugs        : fxsave_leak
bogomips    : 7641.38
TLB size    : 1536 4K pages
clflush size    : 64
cache_alignment    : 64
address sizes    : 48 bits physical, 48 bits virtual
power management: ts ttp tm 100mhzsteps hwpstate cpb eff_freq_ro

processor    : 1
vendor_id    : AuthenticAMD
cpu family    : 21
model        : 16
model name    : AMD A10-5800K APU with Radeon(tm) HD Graphics
stepping    : 1
microcode    : 0x6001116
cpu MHz        : 3800.000
cache size    : 2048 KB
physical id    : 0
siblings    : 4
core id        : 1
cpu cores    : 2
apicid        : 17
initial apicid    : 1
fpu        : yes
fpu_exception    : yes
cpuid level    : 13
wp        : yes
flags        : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov
pat pse36 clflush mmx fxsr sse sse2 ht syscall nx mmxext fxsr_opt pdpe1gb
rdtscp lm constant_tsc rep_good nopl nonstop_tsc extd_apicid aperfmperf pni
pclmulqdq monitor ssse3 fma cx16 sse4_1 sse4_2 popcnt aes xsave avx f16c
lahf_lm cmp_legacy svm extapic cr8_legacy abm sse4a misalignsse 3dnowprefetch
osvw ibs xop skinit wdt lwp fma4 tce nodeid_msr tbm topoext perfctr_core
perfctr_nb arat cpb hw_pstate npt lbrv svm_lock nrip_save tsc_scale vmcb_clean
flushbyasid decodeassists pausefilter pfthreshold bmi1
bugs        : fxsave_leak
bogomips    : 7641.38
TLB size    : 1536 4K pages
clflush size    : 64
cache_alignment    : 64
address sizes    : 48 bits physical, 48 bits virtual
power management: ts ttp tm 100mhzsteps hwpstate cpb eff_freq_ro

processor    : 2
vendor_id    : AuthenticAMD
cpu family    : 21
model        : 16
model name    : AMD A10-5800K APU with Radeon(tm) HD Graphics
stepping    : 1
microcode    : 0x6001116
cpu MHz        : 3800.000
cache size    : 2048 KB
physical id    : 0
siblings    : 4
core id        : 2
cpu cores    : 2
apicid        : 18
initial apicid    : 2
fpu        : yes
fpu_exception    : yes
cpuid level    : 13
wp        : yes
flags        : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov
pat pse36 clflush mmx fxsr sse sse2 ht syscall nx mmxext fxsr_opt pdpe1gb
rdtscp lm constant_tsc rep_good nopl nonstop_tsc extd_apicid aperfmperf pni
pclmulqdq monitor ssse3 fma cx16 sse4_1 sse4_2 popcnt aes xsave avx f16c
lahf_lm cmp_legacy svm extapic cr8_legacy abm sse4a misalignsse 3dnowprefetch
osvw ibs xop skinit wdt lwp fma4 tce nodeid_msr tbm topoext perfctr_core
perfctr_nb arat cpb hw_pstate npt lbrv svm_lock nrip_save tsc_scale vmcb_clean
flushbyasid decodeassists pausefilter pfthreshold bmi1
bugs        : fxsave_leak
bogomips    : 7641.38
TLB size    : 1536 4K pages
clflush size    : 64
cache_alignment    : 64
address sizes    : 48 bits physical, 48 bits virtual
power management: ts ttp tm 100mhzsteps hwpstate cpb eff_freq_ro

processor    : 3
vendor_id    : AuthenticAMD
cpu family    : 21
model        : 16
model name    : AMD A10-5800K APU with Radeon(tm) HD Graphics
stepping    : 1
microcode    : 0x6001116
cpu MHz        : 3800.000
cache size    : 2048 KB
physical id    : 0
siblings    : 4
core id        : 3
cpu cores    : 2
apicid        : 19
initial apicid    : 3
fpu        : yes
fpu_exception    : yes
cpuid level    : 13
wp        : yes
flags        : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov
pat pse36 clflush mmx fxsr sse sse2 ht syscall nx mmxext fxsr_opt pdpe1gb
rdtscp lm constant_tsc rep_good nopl nonstop_tsc extd_apicid aperfmperf pni
pclmulqdq monitor ssse3 fma cx16 sse4_1 sse4_2 popcnt aes xsave avx f16c
lahf_lm cmp_legacy svm extapic cr8_legacy abm sse4a misalignsse 3dnowprefetch
osvw ibs xop skinit wdt lwp fma4 tce nodeid_msr tbm topoext perfctr_core
perfctr_nb arat cpb hw_pstate npt lbrv svm_lock nrip_save tsc_scale vmcb_clean
flushbyasid decodeassists pausefilter pfthreshold bmi1
bugs        : fxsave_leak
bogomips    : 7641.38
TLB size    : 1536 4K pages
clflush size    : 64
cache_alignment    : 64
address sizes    : 48 bits physical, 48 bits virtual
power management: ts ttp tm 100mhzsteps hwpstate cpb eff_freq_ro

Please let me know if you need more details.

Thanks
Joe

--
You are receiving this mail because:
You are the assignee for the bug.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: Fw: [Bug 84991] New: Adding a gre tunnel causes a kernel panic
  2014-09-22 16:02 Fw: [Bug 84991] New: Adding a gre tunnel causes a kernel panic Stephen Hemminger
@ 2014-09-22 17:09 ` Eric Dumazet
  2014-09-22 17:21   ` [PATCH net] ipv4: do not use this_cpu_ptr() in preemptible context Eric Dumazet
  0 siblings, 1 reply; 10+ messages in thread
From: Eric Dumazet @ 2014-09-22 17:09 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: netdev

On Mon, 2014-09-22 at 09:02 -0700, Stephen Hemminger wrote:
> Not good, and user did bisection.
> 
> using git bisect, the below commit introduced the above bug.
> 
> [7d442fab0a6777fd7612cfcada32ea859553d370] ipv4: Cache dst in tunnels
> 
> 
> commit 7d442fab0a6777fd7612cfcada32ea859553d370
> Author: Tom Herbert <therbert@google.com>
> Date:   Thu Jan 2 11:48:26 2014 -0800
> 
>     ipv4: Cache dst in tunnels
> 
>     Avoid doing a route lookup on every packet being tunneled.
> 
>     In ip_tunnel.c cache the route returned from ip_route_output if
>     the tunnel is "connected" so that all the rouitng parameters are
>     taken from tunnel parms for a packet. Specifically, not NBMA tunnel
>     and tos is from tunnel parms (not inner packet).
> 
>     Signed-off-by: Tom Herbert <therbert@google.com>
>     Signed-off-by: David S. Miller <davem@davemloft.net>
> 
> 
> Begin forwarded message:
> 
> Date: Sun, 21 Sep 2014 16:56:08 -0700
> From: "bugzilla-daemon@bugzilla.kernel.org" <bugzilla-daemon@bugzilla.kernel.org>
> To: "stephen@networkplumber.org" <stephen@networkplumber.org>
> Subject: [Bug 84991] New: Adding a gre tunnel causes a kernel panic
> 
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=84991
> 
>             Bug ID: 84991
>            Summary: Adding a gre tunnel causes a kernel panic
>            Product: Networking
>            Version: 2.5
>     Kernel Version: 3.17.0-rc5 #82 SMP PREEMPT
>           Hardware: x86-64
>                 OS: Linux
>               Tree: Mainline
>             Status: NEW
>           Severity: high
>           Priority: P1
>          Component: IPV4
>           Assignee: shemminger@linux-foundation.org
>           Reporter: joe9mail@gmail.com
>         Regression: No

Thanks, I'll send a fix.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH net] ipv4: do not use this_cpu_ptr() in preemptible context
  2014-09-22 17:09 ` Eric Dumazet
@ 2014-09-22 17:21   ` Eric Dumazet
  2014-09-22 17:28     ` Eric Dumazet
  2014-09-22 17:38     ` [PATCH v2 " Eric Dumazet
  0 siblings, 2 replies; 10+ messages in thread
From: Eric Dumazet @ 2014-09-22 17:21 UTC (permalink / raw)
  To: Stephen Hemminger, David Miller; +Cc: netdev, Tom Herbert, joe9mail

From: Eric Dumazet <edumazet@google.com>

this_cpu_ptr() in preemptible context is generally bad

Sep 22 05:05:55 br kernel: [   94.608310] BUG: using smp_processor_id() in
preemptible [00000000] code: ip/2261
Sep 22 05:05:55 br kernel: [   94.608316] caller is
tunnel_dst_set.isra.28+0x20/0x60 [ip_tunnel]
Sep 22 05:05:55 br kernel: [   94.608319] CPU: 3 PID: 2261 Comm: ip Not tainted
3.17.0-rc5 #82

We can simply use __this_cpu_ptr(), as preemption is safe in these contexts.

Should fix https://bugzilla.kernel.org/show_bug.cgi?id=84991

Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Joe <joe9mail@gmail.com>
Fixes: 9a4aa9af447f ("ipv4: Use percpu Cache route in IP tunnels")
---
 net/ipv4/ip_tunnel.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index afed1aac2638..421b4f01ee29 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -82,7 +82,7 @@ static void __tunnel_dst_set(struct ip_tunnel_dst *idst,
 static void tunnel_dst_set(struct ip_tunnel *t,
 			   struct dst_entry *dst, __be32 saddr)
 {
-	__tunnel_dst_set(this_cpu_ptr(t->dst_cache), dst, saddr);
+	__tunnel_dst_set(__this_cpu_ptr(t->dst_cache), dst, saddr);
 }
 
 static void tunnel_dst_reset(struct ip_tunnel *t)
@@ -106,7 +106,7 @@ static struct rtable *tunnel_rtable_get(struct ip_tunnel *t,
 	struct dst_entry *dst;
 
 	rcu_read_lock();
-	idst = this_cpu_ptr(t->dst_cache);
+	idst = __this_cpu_ptr(t->dst_cache);
 	dst = rcu_dereference(idst->dst);
 	if (dst && !atomic_inc_not_zero(&dst->__refcnt))
 		dst = NULL;

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH net] ipv4: do not use this_cpu_ptr() in preemptible context
  2014-09-22 17:21   ` [PATCH net] ipv4: do not use this_cpu_ptr() in preemptible context Eric Dumazet
@ 2014-09-22 17:28     ` Eric Dumazet
  2014-09-22 17:38     ` [PATCH v2 " Eric Dumazet
  1 sibling, 0 replies; 10+ messages in thread
From: Eric Dumazet @ 2014-09-22 17:28 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: David Miller, netdev, Tom Herbert, joe9mail

On Mon, 2014-09-22 at 10:21 -0700, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> this_cpu_ptr() in preemptible context is generally bad
> 
> Sep 22 05:05:55 br kernel: [   94.608310] BUG: using smp_processor_id() in
> preemptible [00000000] code: ip/2261
> Sep 22 05:05:55 br kernel: [   94.608316] caller is
> tunnel_dst_set.isra.28+0x20/0x60 [ip_tunnel]
> Sep 22 05:05:55 br kernel: [   94.608319] CPU: 3 PID: 2261 Comm: ip Not tainted
> 3.17.0-rc5 #82
> 
> We can simply use __this_cpu_ptr(), as preemption is safe in these contexts.
> 
> Should fix https://bugzilla.kernel.org/show_bug.cgi?id=84991
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: Joe <joe9mail@gmail.com>
> Fixes: 9a4aa9af447f ("ipv4: Use percpu Cache route in IP tunnels")
> ---

Hmm, I just saw raw_cpu_ptr() should be used.

I'll send a v2

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 net] ipv4: do not use this_cpu_ptr() in preemptible context
  2014-09-22 17:21   ` [PATCH net] ipv4: do not use this_cpu_ptr() in preemptible context Eric Dumazet
  2014-09-22 17:28     ` Eric Dumazet
@ 2014-09-22 17:38     ` Eric Dumazet
  2014-09-22 18:44       ` Tom Herbert
  1 sibling, 1 reply; 10+ messages in thread
From: Eric Dumazet @ 2014-09-22 17:38 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: David Miller, netdev, Tom Herbert, joe9mail

From: Eric Dumazet <edumazet@google.com>

this_cpu_ptr() in preemptible context is generally bad

Sep 22 05:05:55 br kernel: [   94.608310] BUG: using smp_processor_id()
in
preemptible [00000000] code: ip/2261
Sep 22 05:05:55 br kernel: [   94.608316] caller is
tunnel_dst_set.isra.28+0x20/0x60 [ip_tunnel]
Sep 22 05:05:55 br kernel: [   94.608319] CPU: 3 PID: 2261 Comm: ip Not
tainted
3.17.0-rc5 #82

We can simply use raw_cpu_ptr(), as preemption is safe in these
contexts.

Should fix https://bugzilla.kernel.org/show_bug.cgi?id=84991

Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Joe <joe9mail@gmail.com>
Fixes: 9a4aa9af447f ("ipv4: Use percpu Cache route in IP tunnels")
---
v2: use latest and shiny raw_cpu_ptr(), as it seems the latest
incantation of ever changing percpu interface.

 net/ipv4/ip_tunnel.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index afed1aac2638..bd41dd1948b6 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -79,10 +79,10 @@ static void __tunnel_dst_set(struct ip_tunnel_dst *idst,
 	idst->saddr = saddr;
 }
 
-static void tunnel_dst_set(struct ip_tunnel *t,
+static noinline void tunnel_dst_set(struct ip_tunnel *t,
 			   struct dst_entry *dst, __be32 saddr)
 {
-	__tunnel_dst_set(this_cpu_ptr(t->dst_cache), dst, saddr);
+	__tunnel_dst_set(raw_cpu_ptr(t->dst_cache), dst, saddr);
 }
 
 static void tunnel_dst_reset(struct ip_tunnel *t)
@@ -106,7 +106,7 @@ static struct rtable *tunnel_rtable_get(struct ip_tunnel *t,
 	struct dst_entry *dst;
 
 	rcu_read_lock();
-	idst = this_cpu_ptr(t->dst_cache);
+	idst = raw_cpu_ptr(t->dst_cache);
 	dst = rcu_dereference(idst->dst);
 	if (dst && !atomic_inc_not_zero(&dst->__refcnt))
 		dst = NULL;

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 net] ipv4: do not use this_cpu_ptr() in preemptible context
  2014-09-22 17:38     ` [PATCH v2 " Eric Dumazet
@ 2014-09-22 18:44       ` Tom Herbert
  2014-09-22 19:09         ` Joe M
  2014-09-22 19:17         ` Joe M
  0 siblings, 2 replies; 10+ messages in thread
From: Tom Herbert @ 2014-09-22 18:44 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: Stephen Hemminger, David Miller, Linux Netdev List, joe9mail

On Mon, Sep 22, 2014 at 10:38 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> this_cpu_ptr() in preemptible context is generally bad
>
> Sep 22 05:05:55 br kernel: [   94.608310] BUG: using smp_processor_id()
> in
> preemptible [00000000] code: ip/2261
> Sep 22 05:05:55 br kernel: [   94.608316] caller is
> tunnel_dst_set.isra.28+0x20/0x60 [ip_tunnel]
> Sep 22 05:05:55 br kernel: [   94.608319] CPU: 3 PID: 2261 Comm: ip Not
> tainted
> 3.17.0-rc5 #82
>
> We can simply use raw_cpu_ptr(), as preemption is safe in these
> contexts.
>
> Should fix https://bugzilla.kernel.org/show_bug.cgi?id=84991
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: Joe <joe9mail@gmail.com>
> Fixes: 9a4aa9af447f ("ipv4: Use percpu Cache route in IP tunnels")

Acked-by: Tom Herbert <therbert@google.com>

> ---
> v2: use latest and shiny raw_cpu_ptr(), as it seems the latest
> incantation of ever changing percpu interface.
>
>  net/ipv4/ip_tunnel.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
> index afed1aac2638..bd41dd1948b6 100644
> --- a/net/ipv4/ip_tunnel.c
> +++ b/net/ipv4/ip_tunnel.c
> @@ -79,10 +79,10 @@ static void __tunnel_dst_set(struct ip_tunnel_dst *idst,
>         idst->saddr = saddr;
>  }
>
> -static void tunnel_dst_set(struct ip_tunnel *t,
> +static noinline void tunnel_dst_set(struct ip_tunnel *t,
>                            struct dst_entry *dst, __be32 saddr)
>  {
> -       __tunnel_dst_set(this_cpu_ptr(t->dst_cache), dst, saddr);
> +       __tunnel_dst_set(raw_cpu_ptr(t->dst_cache), dst, saddr);
>  }
>
>  static void tunnel_dst_reset(struct ip_tunnel *t)
> @@ -106,7 +106,7 @@ static struct rtable *tunnel_rtable_get(struct ip_tunnel *t,
>         struct dst_entry *dst;
>
>         rcu_read_lock();
> -       idst = this_cpu_ptr(t->dst_cache);
> +       idst = raw_cpu_ptr(t->dst_cache);
>         dst = rcu_dereference(idst->dst);
>         if (dst && !atomic_inc_not_zero(&dst->__refcnt))
>                 dst = NULL;
>
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 net] ipv4: do not use this_cpu_ptr() in preemptible context
  2014-09-22 18:44       ` Tom Herbert
@ 2014-09-22 19:09         ` Joe M
  2014-09-22 19:34           ` Joe M
  2014-09-22 19:17         ` Joe M
  1 sibling, 1 reply; 10+ messages in thread
From: Joe M @ 2014-09-22 19:09 UTC (permalink / raw)
  To: Tom Herbert
  Cc: Eric Dumazet, Stephen Hemminger, David Miller, Linux Netdev List

[-- Attachment #1: Type: text/plain, Size: 632 bytes --]

Hello Eric,

Thanks for the quick fix.

> > v2: use latest and shiny raw_cpu_ptr(), as it seems the latest
> > incantation of ever changing percpu interface.
> >
> >  net/ipv4/ip_tunnel.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
The below grep shows 3 uses of this_cpu_ptr, whereas the patch only
replaces 2 instances. Just want to check if that is ok.

grep --ignore-case --exclude-dir=".git" --recursive this_cpu_ptr ip_tunnel.c
        __tunnel_dst_set(this_cpu_ptr(t->dst_cache), dst, saddr);
        idst = this_cpu_ptr(t->dst_cache);
        tstats = this_cpu_ptr(tunnel->dev->tstats);

Thanks
Joe

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 net] ipv4: do not use this_cpu_ptr() in preemptible context
  2014-09-22 18:44       ` Tom Herbert
  2014-09-22 19:09         ` Joe M
@ 2014-09-22 19:17         ` Joe M
  1 sibling, 0 replies; 10+ messages in thread
From: Joe M @ 2014-09-22 19:17 UTC (permalink / raw)
  To: Tom Herbert
  Cc: Eric Dumazet, Stephen Hemminger, David Miller, Linux Netdev List

[-- Attachment #1: Type: text/plain, Size: 643 bytes --]

Hello Eric, Tom,

> > this_cpu_ptr() in preemptible context is generally bad
> >
> > Sep 22 05:05:55 br kernel: [   94.608310] BUG: using smp_processor_id()
> > in
> > preemptible [00000000] code: ip/2261
> > Sep 22 05:05:55 br kernel: [   94.608316] caller is
> > tunnel_dst_set.isra.28+0x20/0x60 [ip_tunnel]
> > Sep 22 05:05:55 br kernel: [   94.608319] CPU: 3 PID: 2261 Comm: ip Not
> > tainted
> > 3.17.0-rc5 #82
> >
> > We can simply use raw_cpu_ptr(), as preemption is safe in these
> > contexts.
> >
> > Should fix https://bugzilla.kernel.org/show_bug.cgi?id=84991
> >
Thanks, the fix worked. I do not see the BUG message anymore.

Joe

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 net] ipv4: do not use this_cpu_ptr() in preemptible context
  2014-09-22 19:09         ` Joe M
@ 2014-09-22 19:34           ` Joe M
  2014-09-22 20:11             ` Eric Dumazet
  0 siblings, 1 reply; 10+ messages in thread
From: Joe M @ 2014-09-22 19:34 UTC (permalink / raw)
  To: Tom Herbert
  Cc: Eric Dumazet, Stephen Hemminger, David Miller, Linux Netdev List

[-- Attachment #1: Type: text/plain, Size: 1120 bytes --]

Hello Eric,

> > > v2: use latest and shiny raw_cpu_ptr(), as it seems the latest
> > > incantation of ever changing percpu interface.
> > >
> > >  net/ipv4/ip_tunnel.c |    4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> The below grep shows 3 uses of this_cpu_ptr, whereas the patch only
> replaces 2 instances. Just want to check if that is ok.
> 
> grep --ignore-case --exclude-dir=".git" --recursive this_cpu_ptr ip_tunnel.c
>         __tunnel_dst_set(this_cpu_ptr(t->dst_cache), dst, saddr);
>         idst = this_cpu_ptr(t->dst_cache);
>         tstats = this_cpu_ptr(tunnel->dev->tstats);

grep --ignore-case --exclude-dir=".git" --recursive this_cpu_ptr *.c
ip_input.c:             struct ip_rt_acct *st = this_cpu_ptr(ip_rt_acct);
ip_vti.c:       tstats = this_cpu_ptr(dev->tstats);
route.c:                p = (struct rtable **)__this_cpu_ptr(nh->nh_pcpu_rth_output);
route.c:                        prth = __this_cpu_ptr(nh->nh_pcpu_rth_output);
tcp.c:          return __this_cpu_ptr(p);

Is it ok for ip_vti.c and ip_input.c to use this_cpu_ptr?

Thanks
Joe

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 net] ipv4: do not use this_cpu_ptr() in preemptible context
  2014-09-22 19:34           ` Joe M
@ 2014-09-22 20:11             ` Eric Dumazet
  0 siblings, 0 replies; 10+ messages in thread
From: Eric Dumazet @ 2014-09-22 20:11 UTC (permalink / raw)
  To: Joe M; +Cc: Tom Herbert, Stephen Hemminger, David Miller, Linux Netdev List

On Mon, 2014-09-22 at 14:34 -0500, Joe M wrote:
> p --ignore-case --exclude-dir=".git" --recursive this_cpu_ptr *.c
> ip_input.c:             struct ip_rt_acct *st = this_cpu_ptr(ip_rt_acct);
> ip_vti.c:       tstats = this_cpu_ptr(dev->tstats);
> route.c:                p = (struct rtable **)__this_cpu_ptr(nh->nh_pcpu_rth_output);
> route.c:                        prth = __this_cpu_ptr(nh->nh_pcpu_rth_output);
> tcp.c:          return __this_cpu_ptr(p);
> 
> Is it ok for ip_vti.c and ip_input.c to use this_cpu_ptr?

It is ok.

We run with BH disabled in these contexts.

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2014-09-22 20:11 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-22 16:02 Fw: [Bug 84991] New: Adding a gre tunnel causes a kernel panic Stephen Hemminger
2014-09-22 17:09 ` Eric Dumazet
2014-09-22 17:21   ` [PATCH net] ipv4: do not use this_cpu_ptr() in preemptible context Eric Dumazet
2014-09-22 17:28     ` Eric Dumazet
2014-09-22 17:38     ` [PATCH v2 " Eric Dumazet
2014-09-22 18:44       ` Tom Herbert
2014-09-22 19:09         ` Joe M
2014-09-22 19:34           ` Joe M
2014-09-22 20:11             ` Eric Dumazet
2014-09-22 19:17         ` Joe M

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).