* 2.6.27-rc4: icmp_sk() uses smp_processor_id() in preemptible code @ 2008-08-21 12:20 adobriyan 2008-08-21 12:50 ` Herbert Xu 2008-08-22 13:54 ` [PATCH 1/1] icmp: icmp_sk() should not use " Denis V. Lunev 0 siblings, 2 replies; 10+ messages in thread From: adobriyan @ 2008-08-21 12:20 UTC (permalink / raw) To: den, dlezcano; +Cc: netdev Steps to reproduce: CONFIG_PREEMPT=y CONFIG_DEBUG_PREEMPT=y tracepath <something> icmp6_sk() is similar (I haven't checked at runtime, though). BUG: using smp_processor_id() in preemptible [00000000] code: tracepath/3205 caller is icmp_sk+0x15/0x30 Pid: 3205, comm: tracepath Not tainted 2.6.27-rc4 #1 Call Trace: [<ffffffff8031af14>] debug_smp_processor_id+0xe4/0xf0 [<ffffffff80409405>] icmp_sk+0x15/0x30 [<ffffffff8040a17b>] icmp_send+0x4b/0x3f0 [<ffffffff8025a415>] ? trace_hardirqs_on_caller+0xd5/0x160 [<ffffffff8025a4ad>] ? trace_hardirqs_on+0xd/0x10 [<ffffffff8023a475>] ? local_bh_enable_ip+0x95/0x110 [<ffffffff804285b9>] ? _spin_unlock_bh+0x39/0x40 [<ffffffff8025a26c>] ? mark_held_locks+0x4c/0x90 [<ffffffff8025a4ad>] ? trace_hardirqs_on+0xd/0x10 [<ffffffff8025a415>] ? trace_hardirqs_on_caller+0xd5/0x160 [<ffffffff803e91b4>] ip_fragment+0x8d4/0x900 [<ffffffff803e7030>] ? ip_finish_output2+0x0/0x290 [<ffffffff803e91e0>] ? ip_finish_output+0x0/0x60 [<ffffffff803e6650>] ? dst_output+0x0/0x10 [<ffffffff803e922c>] ip_finish_output+0x4c/0x60 [<ffffffff803e92e3>] ip_output+0xa3/0xf0 [<ffffffff803e68d0>] ip_local_out+0x20/0x30 [<ffffffff803e753f>] ip_push_pending_frames+0x27f/0x400 [<ffffffff80406313>] udp_push_pending_frames+0x233/0x3d0 [<ffffffff804067d1>] udp_sendmsg+0x321/0x6f0 [<ffffffff8040d155>] inet_sendmsg+0x45/0x80 [<ffffffff803b967f>] sock_sendmsg+0xdf/0x110 [<ffffffff8024a100>] ? autoremove_wake_function+0x0/0x40 [<ffffffff80257ce5>] ? validate_chain+0x415/0x1010 [<ffffffff8027dc10>] ? __do_fault+0x140/0x450 [<ffffffff802597d0>] ? __lock_acquire+0x260/0x590 [<ffffffff803b9e55>] ? sockfd_lookup_light+0x45/0x80 [<ffffffff803ba50a>] sys_sendto+0xea/0x120 [<ffffffff80428e42>] ? _spin_unlock_irqrestore+0x42/0x80 [<ffffffff803134bc>] ? __up_read+0x4c/0xb0 [<ffffffff8024e0c6>] ? up_read+0x26/0x30 [<ffffffff8020b8bb>] system_call_fastpath+0x16/0x1b ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: 2.6.27-rc4: icmp_sk() uses smp_processor_id() in preemptible code 2008-08-21 12:20 2.6.27-rc4: icmp_sk() uses smp_processor_id() in preemptible code adobriyan @ 2008-08-21 12:50 ` Herbert Xu 2008-08-22 11:54 ` David Miller 2008-08-22 13:54 ` [PATCH 1/1] icmp: icmp_sk() should not use " Denis V. Lunev 1 sibling, 1 reply; 10+ messages in thread From: Herbert Xu @ 2008-08-21 12:50 UTC (permalink / raw) To: adobriyan; +Cc: den, dlezcano, netdev adobriyan@gmail.com wrote: > Steps to reproduce: > > CONFIG_PREEMPT=y > CONFIG_DEBUG_PREEMPT=y > tracepath <something> > > icmp6_sk() is similar (I haven't checked at runtime, though). > > BUG: using smp_processor_id() in preemptible [00000000] code: tracepath/3205 > caller is icmp_sk+0x15/0x30 > Pid: 3205, comm: tracepath Not tainted 2.6.27-rc4 #1 > > Call Trace: > [<ffffffff8031af14>] debug_smp_processor_id+0xe4/0xf0 > [<ffffffff80409405>] icmp_sk+0x15/0x30 > [<ffffffff8040a17b>] icmp_send+0x4b/0x3f0 Caused by: commit 405666db84b984b68fc75794069f424c02e5796c Author: Denis V. Lunev <den@openvz.org> Date: Fri Feb 29 11:16:46 2008 -0800 [ICMP]: Pass proper ICMP socket into icmp(v6)_xmit_(un)lock. We need to disable preemption (was provided by disabling BH) before getting the per-cpu ICMP socket. Cheers, -- Visit Openswan at http://www.openswan.org/ Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: 2.6.27-rc4: icmp_sk() uses smp_processor_id() in preemptible code 2008-08-21 12:50 ` Herbert Xu @ 2008-08-22 11:54 ` David Miller 2008-08-22 13:12 ` Denis V. Lunev 0 siblings, 1 reply; 10+ messages in thread From: David Miller @ 2008-08-22 11:54 UTC (permalink / raw) To: herbert; +Cc: adobriyan, den, dlezcano, netdev From: Herbert Xu <herbert@gondor.apana.org.au> Date: Thu, 21 Aug 2008 22:50:00 +1000 > adobriyan@gmail.com wrote: > > Steps to reproduce: > > > > CONFIG_PREEMPT=y > > CONFIG_DEBUG_PREEMPT=y > > tracepath <something> > > > > icmp6_sk() is similar (I haven't checked at runtime, though). > > > > BUG: using smp_processor_id() in preemptible [00000000] code: tracepath/3205 > > caller is icmp_sk+0x15/0x30 > > Pid: 3205, comm: tracepath Not tainted 2.6.27-rc4 #1 > > > > Call Trace: > > [<ffffffff8031af14>] debug_smp_processor_id+0xe4/0xf0 > > [<ffffffff80409405>] icmp_sk+0x15/0x30 > > [<ffffffff8040a17b>] icmp_send+0x4b/0x3f0 > > Caused by: > > commit 405666db84b984b68fc75794069f424c02e5796c > Author: Denis V. Lunev <den@openvz.org> > Date: Fri Feb 29 11:16:46 2008 -0800 > > [ICMP]: Pass proper ICMP socket into icmp(v6)_xmit_(un)lock. > > We need to disable preemption (was provided by disabling BH) before > getting the per-cpu ICMP socket. Denis, please fix this. The icmp_sk was always evaluated inside of icmp*_xmit_lock() exactly because we needed the BH disabling in order for the smp_processor_id() evaluation to be safe. It's probably better to pass the namespace into the lock and unlock routines. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: 2.6.27-rc4: icmp_sk() uses smp_processor_id() in preemptible code 2008-08-22 11:54 ` David Miller @ 2008-08-22 13:12 ` Denis V. Lunev 0 siblings, 0 replies; 10+ messages in thread From: Denis V. Lunev @ 2008-08-22 13:12 UTC (permalink / raw) To: David Miller; +Cc: herbert, adobriyan, dlezcano, netdev On Fri, 2008-08-22 at 04:54 -0700, David Miller wrote: > From: Herbert Xu <herbert@gondor.apana.org.au> > Date: Thu, 21 Aug 2008 22:50:00 +1000 > > > adobriyan@gmail.com wrote: > > > Steps to reproduce: > > > > > > CONFIG_PREEMPT=y > > > CONFIG_DEBUG_PREEMPT=y > > > tracepath <something> > > > > > > icmp6_sk() is similar (I haven't checked at runtime, though). > > > > > > BUG: using smp_processor_id() in preemptible [00000000] code: tracepath/3205 > > > caller is icmp_sk+0x15/0x30 > > > Pid: 3205, comm: tracepath Not tainted 2.6.27-rc4 #1 > > > > > > Call Trace: > > > [<ffffffff8031af14>] debug_smp_processor_id+0xe4/0xf0 > > > [<ffffffff80409405>] icmp_sk+0x15/0x30 > > > [<ffffffff8040a17b>] icmp_send+0x4b/0x3f0 > > > > Caused by: > > > > commit 405666db84b984b68fc75794069f424c02e5796c > > Author: Denis V. Lunev <den@openvz.org> > > Date: Fri Feb 29 11:16:46 2008 -0800 > > > > [ICMP]: Pass proper ICMP socket into icmp(v6)_xmit_(un)lock. > > > > We need to disable preemption (was provided by disabling BH) before > > getting the per-cpu ICMP socket. > > Denis, please fix this. > > The icmp_sk was always evaluated inside of icmp*_xmit_lock() > exactly because we needed the BH disabling in order for the > smp_processor_id() evaluation to be safe. > > It's probably better to pass the namespace into the lock and > unlock routines. for sure. I am sorry for the delay, I am ill and at home. I'll try to do my best and cook the patch today. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/1] icmp: icmp_sk() should not use smp_processor_id() in preemptible code 2008-08-21 12:20 2.6.27-rc4: icmp_sk() uses smp_processor_id() in preemptible code adobriyan 2008-08-21 12:50 ` Herbert Xu @ 2008-08-22 13:54 ` Denis V. Lunev 2008-08-22 23:15 ` Daniel Lezcano ` (2 more replies) 1 sibling, 3 replies; 10+ messages in thread From: Denis V. Lunev @ 2008-08-22 13:54 UTC (permalink / raw) To: davem; +Cc: dlezcano, adobriyan, herbert, netdev, Denis V. Lunev Pass namespace into icmp_xmit_lock, obtain socket inside and return it as a result for caller. Thanks Alexey Dobryan for this report: Steps to reproduce: CONFIG_PREEMPT=y CONFIG_DEBUG_PREEMPT=y tracepath <something> BUG: using smp_processor_id() in preemptible [00000000] code: tracepath/3205 caller is icmp_sk+0x15/0x30 Pid: 3205, comm: tracepath Not tainted 2.6.27-rc4 #1 Call Trace: [<ffffffff8031af14>] debug_smp_processor_id+0xe4/0xf0 [<ffffffff80409405>] icmp_sk+0x15/0x30 [<ffffffff8040a17b>] icmp_send+0x4b/0x3f0 [<ffffffff8025a415>] ? trace_hardirqs_on_caller+0xd5/0x160 [<ffffffff8025a4ad>] ? trace_hardirqs_on+0xd/0x10 [<ffffffff8023a475>] ? local_bh_enable_ip+0x95/0x110 [<ffffffff804285b9>] ? _spin_unlock_bh+0x39/0x40 [<ffffffff8025a26c>] ? mark_held_locks+0x4c/0x90 [<ffffffff8025a4ad>] ? trace_hardirqs_on+0xd/0x10 [<ffffffff8025a415>] ? trace_hardirqs_on_caller+0xd5/0x160 [<ffffffff803e91b4>] ip_fragment+0x8d4/0x900 [<ffffffff803e7030>] ? ip_finish_output2+0x0/0x290 [<ffffffff803e91e0>] ? ip_finish_output+0x0/0x60 [<ffffffff803e6650>] ? dst_output+0x0/0x10 [<ffffffff803e922c>] ip_finish_output+0x4c/0x60 [<ffffffff803e92e3>] ip_output+0xa3/0xf0 [<ffffffff803e68d0>] ip_local_out+0x20/0x30 [<ffffffff803e753f>] ip_push_pending_frames+0x27f/0x400 [<ffffffff80406313>] udp_push_pending_frames+0x233/0x3d0 [<ffffffff804067d1>] udp_sendmsg+0x321/0x6f0 [<ffffffff8040d155>] inet_sendmsg+0x45/0x80 [<ffffffff803b967f>] sock_sendmsg+0xdf/0x110 [<ffffffff8024a100>] ? autoremove_wake_function+0x0/0x40 [<ffffffff80257ce5>] ? validate_chain+0x415/0x1010 [<ffffffff8027dc10>] ? __do_fault+0x140/0x450 [<ffffffff802597d0>] ? __lock_acquire+0x260/0x590 [<ffffffff803b9e55>] ? sockfd_lookup_light+0x45/0x80 [<ffffffff803ba50a>] sys_sendto+0xea/0x120 [<ffffffff80428e42>] ? _spin_unlock_irqrestore+0x42/0x80 [<ffffffff803134bc>] ? __up_read+0x4c/0xb0 [<ffffffff8024e0c6>] ? up_read+0x26/0x30 [<ffffffff8020b8bb>] system_call_fastpath+0x16/0x1b icmp6_sk() is similar. Signed-off-by: Denis V. Lunev <den@openvz.org> --- net/ipv4/icmp.c | 22 ++++++++++++++-------- net/ipv6/icmp.c | 23 ++++++++++++----------- 2 files changed, 26 insertions(+), 19 deletions(-) diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c index 8605586..55c355e 100644 --- a/net/ipv4/icmp.c +++ b/net/ipv4/icmp.c @@ -204,18 +204,22 @@ static struct sock *icmp_sk(struct net *net) return net->ipv4.icmp_sk[smp_processor_id()]; } -static inline int icmp_xmit_lock(struct sock *sk) +static inline struct sock *icmp_xmit_lock(struct net *net) { + struct sock *sk; + local_bh_disable(); + sk = icmp_sk(net); + if (unlikely(!spin_trylock(&sk->sk_lock.slock))) { /* This can happen if the output path signals a * dst_link_failure() for an outgoing ICMP packet. */ local_bh_enable(); - return 1; + return NULL; } - return 0; + return sk; } static inline void icmp_xmit_unlock(struct sock *sk) @@ -354,15 +358,17 @@ static void icmp_reply(struct icmp_bxm *icmp_param, struct sk_buff *skb) struct ipcm_cookie ipc; struct rtable *rt = skb->rtable; struct net *net = dev_net(rt->u.dst.dev); - struct sock *sk = icmp_sk(net); - struct inet_sock *inet = inet_sk(sk); + struct sock *sk; + struct inet_sock *inet; __be32 daddr; if (ip_options_echo(&icmp_param->replyopts, skb)) return; - if (icmp_xmit_lock(sk)) + sk = icmp_xmit_lock(net); + if (sk == NULL) return; + inet = inet_sk(sk); icmp_param->data.icmph.checksum = 0; @@ -419,7 +425,6 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info) if (!rt) goto out; net = dev_net(rt->u.dst.dev); - sk = icmp_sk(net); /* * Find the original header. It is expected to be valid, of course. @@ -483,7 +488,8 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info) } } - if (icmp_xmit_lock(sk)) + sk = icmp_xmit_lock(net); + if (sk == NULL) return; /* diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c index abedf95..b3157a0 100644 --- a/net/ipv6/icmp.c +++ b/net/ipv6/icmp.c @@ -91,19 +91,22 @@ static struct inet6_protocol icmpv6_protocol = { .flags = INET6_PROTO_NOPOLICY|INET6_PROTO_FINAL, }; -static __inline__ int icmpv6_xmit_lock(struct sock *sk) +static __inline__ struct sock *icmpv6_xmit_lock(struct net *net) { + struct sock *sk; + local_bh_disable(); + sk = icmpv6_sk(net); if (unlikely(!spin_trylock(&sk->sk_lock.slock))) { /* This can happen if the output path (f.e. SIT or * ip6ip6 tunnel) signals dst_link_failure() for an * outgoing ICMP6 packet. */ local_bh_enable(); - return 1; + return NULL; } - return 0; + return sk; } static __inline__ void icmpv6_xmit_unlock(struct sock *sk) @@ -392,11 +395,10 @@ void icmpv6_send(struct sk_buff *skb, int type, int code, __u32 info, fl.fl_icmp_code = code; security_skb_classify_flow(skb, &fl); - sk = icmpv6_sk(net); - np = inet6_sk(sk); - - if (icmpv6_xmit_lock(sk)) + sk = icmpv6_xmit_lock(net); + if (sk == NULL) return; + np = inet6_sk(sk); if (!icmpv6_xrlim_allow(sk, type, &fl)) goto out; @@ -539,11 +541,10 @@ static void icmpv6_echo_reply(struct sk_buff *skb) fl.fl_icmp_type = ICMPV6_ECHO_REPLY; security_skb_classify_flow(skb, &fl); - sk = icmpv6_sk(net); - np = inet6_sk(sk); - - if (icmpv6_xmit_lock(sk)) + sk = icmpv6_xmit_lock(net); + if (sk == NULL) return; + np = inet6_sk(sk); if (!fl.oif && ipv6_addr_is_multicast(&fl.fl6_dst)) fl.oif = np->mcast_oif; -- 1.5.6.4 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] icmp: icmp_sk() should not use smp_processor_id() in preemptible code 2008-08-22 13:54 ` [PATCH 1/1] icmp: icmp_sk() should not use " Denis V. Lunev @ 2008-08-22 23:15 ` Daniel Lezcano 2008-08-23 0:48 ` Herbert Xu 2008-08-22 23:21 ` adobriyan 2008-08-23 11:45 ` David Miller 2 siblings, 1 reply; 10+ messages in thread From: Daniel Lezcano @ 2008-08-22 23:15 UTC (permalink / raw) To: Denis V. Lunev; +Cc: davem, adobriyan, herbert, netdev Denis V. Lunev wrote: > Pass namespace into icmp_xmit_lock, obtain socket inside and return > it as a result for caller. > > Thanks Alexey Dobryan for this report: > > Steps to reproduce: > > CONFIG_PREEMPT=y > CONFIG_DEBUG_PREEMPT=y > tracepath <something> > > BUG: using smp_processor_id() in preemptible [00000000] code: tracepath/3205 > caller is icmp_sk+0x15/0x30 > Pid: 3205, comm: tracepath Not tainted 2.6.27-rc4 #1 > > Call Trace: > [<ffffffff8031af14>] debug_smp_processor_id+0xe4/0xf0 > [<ffffffff80409405>] icmp_sk+0x15/0x30 > [<ffffffff8040a17b>] icmp_send+0x4b/0x3f0 > [<ffffffff8025a415>] ? trace_hardirqs_on_caller+0xd5/0x160 > [<ffffffff8025a4ad>] ? trace_hardirqs_on+0xd/0x10 > [<ffffffff8023a475>] ? local_bh_enable_ip+0x95/0x110 > [<ffffffff804285b9>] ? _spin_unlock_bh+0x39/0x40 > [<ffffffff8025a26c>] ? mark_held_locks+0x4c/0x90 > [<ffffffff8025a4ad>] ? trace_hardirqs_on+0xd/0x10 > [<ffffffff8025a415>] ? trace_hardirqs_on_caller+0xd5/0x160 > [<ffffffff803e91b4>] ip_fragment+0x8d4/0x900 > [<ffffffff803e7030>] ? ip_finish_output2+0x0/0x290 > [<ffffffff803e91e0>] ? ip_finish_output+0x0/0x60 > [<ffffffff803e6650>] ? dst_output+0x0/0x10 > [<ffffffff803e922c>] ip_finish_output+0x4c/0x60 > [<ffffffff803e92e3>] ip_output+0xa3/0xf0 > [<ffffffff803e68d0>] ip_local_out+0x20/0x30 > [<ffffffff803e753f>] ip_push_pending_frames+0x27f/0x400 > [<ffffffff80406313>] udp_push_pending_frames+0x233/0x3d0 > [<ffffffff804067d1>] udp_sendmsg+0x321/0x6f0 > [<ffffffff8040d155>] inet_sendmsg+0x45/0x80 > [<ffffffff803b967f>] sock_sendmsg+0xdf/0x110 > [<ffffffff8024a100>] ? autoremove_wake_function+0x0/0x40 > [<ffffffff80257ce5>] ? validate_chain+0x415/0x1010 > [<ffffffff8027dc10>] ? __do_fault+0x140/0x450 > [<ffffffff802597d0>] ? __lock_acquire+0x260/0x590 > [<ffffffff803b9e55>] ? sockfd_lookup_light+0x45/0x80 > [<ffffffff803ba50a>] sys_sendto+0xea/0x120 > [<ffffffff80428e42>] ? _spin_unlock_irqrestore+0x42/0x80 > [<ffffffff803134bc>] ? __up_read+0x4c/0xb0 > [<ffffffff8024e0c6>] ? up_read+0x26/0x30 > [<ffffffff8020b8bb>] system_call_fastpath+0x16/0x1b > > icmp6_sk() is similar. > > Signed-off-by: Denis V. Lunev <den@openvz.org> > --- > net/ipv4/icmp.c | 22 ++++++++++++++-------- > net/ipv6/icmp.c | 23 ++++++++++++----------- > 2 files changed, 26 insertions(+), 19 deletions(-) > > diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c > index 8605586..55c355e 100644 > --- a/net/ipv4/icmp.c > +++ b/net/ipv4/icmp.c > @@ -204,18 +204,22 @@ static struct sock *icmp_sk(struct net *net) > return net->ipv4.icmp_sk[smp_processor_id()]; > } Hi Denis, thanks for the fix. For my personal knowledge, why can we just use in the icmp_sk function: { struct sock *isk = net->ipv4.icmp_sk[get_cpu()]; put_cpu(); return isk; } ? > -static inline int icmp_xmit_lock(struct sock *sk) > +static inline struct sock *icmp_xmit_lock(struct net *net) > { > + struct sock *sk; > + > local_bh_disable(); > > + sk = icmp_sk(net); > + > if (unlikely(!spin_trylock(&sk->sk_lock.slock))) { > /* This can happen if the output path signals a > * dst_link_failure() for an outgoing ICMP packet. > */ > local_bh_enable(); > - return 1; > + return NULL; > } > - return 0; > + return sk; > } > > static inline void icmp_xmit_unlock(struct sock *sk) > @@ -354,15 +358,17 @@ static void icmp_reply(struct icmp_bxm *icmp_param, struct sk_buff *skb) > struct ipcm_cookie ipc; > struct rtable *rt = skb->rtable; > struct net *net = dev_net(rt->u.dst.dev); > - struct sock *sk = icmp_sk(net); > - struct inet_sock *inet = inet_sk(sk); > + struct sock *sk; > + struct inet_sock *inet; > __be32 daddr; > > if (ip_options_echo(&icmp_param->replyopts, skb)) > return; > > - if (icmp_xmit_lock(sk)) > + sk = icmp_xmit_lock(net); > + if (sk == NULL) > return; > + inet = inet_sk(sk); > > icmp_param->data.icmph.checksum = 0; > > @@ -419,7 +425,6 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info) > if (!rt) > goto out; > net = dev_net(rt->u.dst.dev); > - sk = icmp_sk(net); > > /* > * Find the original header. It is expected to be valid, of course. > @@ -483,7 +488,8 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info) > } > } > > - if (icmp_xmit_lock(sk)) > + sk = icmp_xmit_lock(net); > + if (sk == NULL) > return; > > /* > diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c > index abedf95..b3157a0 100644 > --- a/net/ipv6/icmp.c > +++ b/net/ipv6/icmp.c > @@ -91,19 +91,22 @@ static struct inet6_protocol icmpv6_protocol = { > .flags = INET6_PROTO_NOPOLICY|INET6_PROTO_FINAL, > }; > > -static __inline__ int icmpv6_xmit_lock(struct sock *sk) > +static __inline__ struct sock *icmpv6_xmit_lock(struct net *net) > { > + struct sock *sk; > + > local_bh_disable(); > > + sk = icmpv6_sk(net); > if (unlikely(!spin_trylock(&sk->sk_lock.slock))) { > /* This can happen if the output path (f.e. SIT or > * ip6ip6 tunnel) signals dst_link_failure() for an > * outgoing ICMP6 packet. > */ > local_bh_enable(); > - return 1; > + return NULL; > } > - return 0; > + return sk; > } > > static __inline__ void icmpv6_xmit_unlock(struct sock *sk) > @@ -392,11 +395,10 @@ void icmpv6_send(struct sk_buff *skb, int type, int code, __u32 info, > fl.fl_icmp_code = code; > security_skb_classify_flow(skb, &fl); > > - sk = icmpv6_sk(net); > - np = inet6_sk(sk); > - > - if (icmpv6_xmit_lock(sk)) > + sk = icmpv6_xmit_lock(net); > + if (sk == NULL) > return; > + np = inet6_sk(sk); > > if (!icmpv6_xrlim_allow(sk, type, &fl)) > goto out; > @@ -539,11 +541,10 @@ static void icmpv6_echo_reply(struct sk_buff *skb) > fl.fl_icmp_type = ICMPV6_ECHO_REPLY; > security_skb_classify_flow(skb, &fl); > > - sk = icmpv6_sk(net); > - np = inet6_sk(sk); > - > - if (icmpv6_xmit_lock(sk)) > + sk = icmpv6_xmit_lock(net); > + if (sk == NULL) > return; > + np = inet6_sk(sk); > > if (!fl.oif && ipv6_addr_is_multicast(&fl.fl6_dst)) > fl.oif = np->mcast_oif; ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] icmp: icmp_sk() should not use smp_processor_id() in preemptible code 2008-08-22 23:15 ` Daniel Lezcano @ 2008-08-23 0:48 ` Herbert Xu 2008-08-23 9:12 ` Daniel Lezcano 0 siblings, 1 reply; 10+ messages in thread From: Herbert Xu @ 2008-08-23 0:48 UTC (permalink / raw) To: Daniel Lezcano; +Cc: Denis V. Lunev, davem, adobriyan, netdev On Sat, Aug 23, 2008 at 01:15:05AM +0200, Daniel Lezcano wrote: > > thanks for the fix. For my personal knowledge, why can we just use in > the icmp_sk function: > { > struct sock *isk = net->ipv4.icmp_sk[get_cpu()]; > put_cpu(); > return isk; > } > > ? Because then you can get rescheduled to another CPU while still using someone else's ICMP socket. Cheers, -- Visit Openswan at http://www.openswan.org/ Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] icmp: icmp_sk() should not use smp_processor_id() in preemptible code 2008-08-23 0:48 ` Herbert Xu @ 2008-08-23 9:12 ` Daniel Lezcano 0 siblings, 0 replies; 10+ messages in thread From: Daniel Lezcano @ 2008-08-23 9:12 UTC (permalink / raw) To: Herbert Xu; +Cc: Denis V. Lunev, davem, adobriyan, netdev Herbert Xu wrote: > On Sat, Aug 23, 2008 at 01:15:05AM +0200, Daniel Lezcano wrote: >> thanks for the fix. For my personal knowledge, why can we just use in >> the icmp_sk function: >> { >> struct sock *isk = net->ipv4.icmp_sk[get_cpu()]; >> put_cpu(); >> return isk; >> } >> >> ? > > Because then you can get rescheduled to another CPU while still > using someone else's ICMP socket. Obviously :) Thanks Herbert. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] icmp: icmp_sk() should not use smp_processor_id() in preemptible code 2008-08-22 13:54 ` [PATCH 1/1] icmp: icmp_sk() should not use " Denis V. Lunev 2008-08-22 23:15 ` Daniel Lezcano @ 2008-08-22 23:21 ` adobriyan 2008-08-23 11:45 ` David Miller 2 siblings, 0 replies; 10+ messages in thread From: adobriyan @ 2008-08-22 23:21 UTC (permalink / raw) To: Denis V. Lunev; +Cc: davem, dlezcano, herbert, netdev On Fri, Aug 22, 2008 at 05:54:33PM +0400, Denis V. Lunev wrote: > Pass namespace into icmp_xmit_lock, obtain socket inside and return > it as a result for caller. > > Thanks Alexey Dobryan for this report: > > Steps to reproduce: > > CONFIG_PREEMPT=y > CONFIG_DEBUG_PREEMPT=y > tracepath <something> > > BUG: using smp_processor_id() in preemptible [00000000] code: tracepath/3205 > caller is icmp_sk+0x15/0x30 > Pid: 3205, comm: tracepath Not tainted 2.6.27-rc4 #1 > > Call Trace: > [<ffffffff8031af14>] debug_smp_processor_id+0xe4/0xf0 > [<ffffffff80409405>] icmp_sk+0x15/0x30 > [<ffffffff8040a17b>] icmp_send+0x4b/0x3f0 > [<ffffffff8025a415>] ? trace_hardirqs_on_caller+0xd5/0x160 > [<ffffffff8025a4ad>] ? trace_hardirqs_on+0xd/0x10 > [<ffffffff8023a475>] ? local_bh_enable_ip+0x95/0x110 > [<ffffffff804285b9>] ? _spin_unlock_bh+0x39/0x40 > [<ffffffff8025a26c>] ? mark_held_locks+0x4c/0x90 > [<ffffffff8025a4ad>] ? trace_hardirqs_on+0xd/0x10 > [<ffffffff8025a415>] ? trace_hardirqs_on_caller+0xd5/0x160 > [<ffffffff803e91b4>] ip_fragment+0x8d4/0x900 > [<ffffffff803e7030>] ? ip_finish_output2+0x0/0x290 > [<ffffffff803e91e0>] ? ip_finish_output+0x0/0x60 > [<ffffffff803e6650>] ? dst_output+0x0/0x10 > [<ffffffff803e922c>] ip_finish_output+0x4c/0x60 > [<ffffffff803e92e3>] ip_output+0xa3/0xf0 > [<ffffffff803e68d0>] ip_local_out+0x20/0x30 > [<ffffffff803e753f>] ip_push_pending_frames+0x27f/0x400 > [<ffffffff80406313>] udp_push_pending_frames+0x233/0x3d0 > [<ffffffff804067d1>] udp_sendmsg+0x321/0x6f0 > [<ffffffff8040d155>] inet_sendmsg+0x45/0x80 > [<ffffffff803b967f>] sock_sendmsg+0xdf/0x110 > [<ffffffff8024a100>] ? autoremove_wake_function+0x0/0x40 > [<ffffffff80257ce5>] ? validate_chain+0x415/0x1010 > [<ffffffff8027dc10>] ? __do_fault+0x140/0x450 > [<ffffffff802597d0>] ? __lock_acquire+0x260/0x590 > [<ffffffff803b9e55>] ? sockfd_lookup_light+0x45/0x80 > [<ffffffff803ba50a>] sys_sendto+0xea/0x120 > [<ffffffff80428e42>] ? _spin_unlock_irqrestore+0x42/0x80 > [<ffffffff803134bc>] ? __up_read+0x4c/0xb0 > [<ffffffff8024e0c6>] ? up_read+0x26/0x30 > [<ffffffff8020b8bb>] system_call_fastpath+0x16/0x1b This helps. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] icmp: icmp_sk() should not use smp_processor_id() in preemptible code 2008-08-22 13:54 ` [PATCH 1/1] icmp: icmp_sk() should not use " Denis V. Lunev 2008-08-22 23:15 ` Daniel Lezcano 2008-08-22 23:21 ` adobriyan @ 2008-08-23 11:45 ` David Miller 2 siblings, 0 replies; 10+ messages in thread From: David Miller @ 2008-08-23 11:45 UTC (permalink / raw) To: den; +Cc: dlezcano, adobriyan, herbert, netdev From: "Denis V. Lunev" <den@openvz.org> Date: Fri, 22 Aug 2008 17:54:33 +0400 > Pass namespace into icmp_xmit_lock, obtain socket inside and return > it as a result for caller. ... > Signed-off-by: Denis V. Lunev <den@openvz.org> Applied, thanks Denis! ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2008-08-23 11:45 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-08-21 12:20 2.6.27-rc4: icmp_sk() uses smp_processor_id() in preemptible code adobriyan 2008-08-21 12:50 ` Herbert Xu 2008-08-22 11:54 ` David Miller 2008-08-22 13:12 ` Denis V. Lunev 2008-08-22 13:54 ` [PATCH 1/1] icmp: icmp_sk() should not use " Denis V. Lunev 2008-08-22 23:15 ` Daniel Lezcano 2008-08-23 0:48 ` Herbert Xu 2008-08-23 9:12 ` Daniel Lezcano 2008-08-22 23:21 ` adobriyan 2008-08-23 11:45 ` David Miller
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).