Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1473636 > unrolled thread
| Started by | Sebastian Andrzej Siewior <bigeasy@linutronix.de> |
|---|---|
| First post | 2016-08-31 18:10 +0200 |
| Last post | 2016-09-01 15:20 +0200 |
| Articles | 5 — 3 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
[RT PATCH 2/2] net: add a lock around icmp_sk() Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-08-31 18:10 +0200
Re: [RT PATCH 2/2] net: add a lock around icmp_sk() Eric Dumazet <eric.dumazet@gmail.com> - 2016-08-31 18:40 +0200
Re: [RT PATCH 2/2] net: add a lock around icmp_sk() Steven Rostedt <rostedt@goodmis.org> - 2016-08-31 19:10 +0200
Re: [RT PATCH 2/2] net: add a lock around icmp_sk() Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-09-01 10:20 +0200
Re: [RT PATCH 2/2] net: add a lock around icmp_sk() Eric Dumazet <eric.dumazet@gmail.com> - 2016-09-01 15:20 +0200
| From | Sebastian Andrzej Siewior <bigeasy@linutronix.de> |
|---|---|
| Date | 2016-08-31 18:10 +0200 |
| Subject | [RT PATCH 2/2] net: add a lock around icmp_sk() |
| Message-ID | <scgfn-4zJ-13@gated-at.bofh.it> |
It looks like the this_cpu_ptr() access in icmp_sk() is protected with
local_bh_disable(). To avoid missing serialization in -RT I am adding
here a local lock. No crash has been observed, this is just precaution.
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
net/ipv4/icmp.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index c1f1d5030d37..63731fd6af3e 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -78,6 +78,7 @@
#include <linux/string.h>
#include <linux/netfilter_ipv4.h>
#include <linux/slab.h>
+#include <linux/locallock.h>
#include <net/snmp.h>
#include <net/ip.h>
#include <net/route.h>
@@ -205,6 +206,8 @@ static const struct icmp_control icmp_pointers[NR_ICMP_TYPES+1];
*
* On SMP we have one ICMP socket per-cpu.
*/
+static DEFINE_LOCAL_IRQ_LOCK(icmp_sk_lock);
+
static struct sock *icmp_sk(struct net *net)
{
return *this_cpu_ptr(net->ipv4.icmp_sk);
@@ -216,12 +219,14 @@ static inline struct sock *icmp_xmit_lock(struct net *net)
local_bh_disable();
+ local_lock(icmp_sk_lock);
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_unlock(icmp_sk_lock);
local_bh_enable();
return NULL;
}
@@ -231,6 +236,7 @@ static inline struct sock *icmp_xmit_lock(struct net *net)
static inline void icmp_xmit_unlock(struct sock *sk)
{
spin_unlock_bh(&sk->sk_lock.slock);
+ local_unlock(icmp_sk_lock);
}
int sysctl_icmp_msgs_per_sec __read_mostly = 1000;
@@ -359,6 +365,7 @@ static void icmp_push_reply(struct icmp_bxm *icmp_param,
struct sock *sk;
struct sk_buff *skb;
+ local_lock(icmp_sk_lock);
sk = icmp_sk(dev_net((*rt)->dst.dev));
if (ip_append_data(sk, fl4, icmp_glue_bits, icmp_param,
icmp_param->data_len+icmp_param->head_len,
@@ -381,6 +388,7 @@ static void icmp_push_reply(struct icmp_bxm *icmp_param,
skb->ip_summed = CHECKSUM_NONE;
ip_push_pending_frames(sk, fl4);
}
+ local_unlock(icmp_sk_lock);
}
/*
--
2.9.3
[toc] | [next] | [standalone]
| From | Eric Dumazet <eric.dumazet@gmail.com> |
|---|---|
| Date | 2016-08-31 18:40 +0200 |
| Message-ID | <scgIp-4Kp-5@gated-at.bofh.it> |
| In reply to | #1473636 |
On Wed, 2016-08-31 at 18:00 +0200, Sebastian Andrzej Siewior wrote:
> It looks like the this_cpu_ptr() access in icmp_sk() is protected with
> local_bh_disable(). To avoid missing serialization in -RT I am adding
> here a local lock. No crash has been observed, this is just precaution.
>
Hmm...
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> net/ipv4/icmp.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
> index c1f1d5030d37..63731fd6af3e 100644
> --- a/net/ipv4/icmp.c
> +++ b/net/ipv4/icmp.c
> @@ -78,6 +78,7 @@
> #include <linux/string.h>
> #include <linux/netfilter_ipv4.h>
> #include <linux/slab.h>
> +#include <linux/locallock.h>
> #include <net/snmp.h>
> #include <net/ip.h>
> #include <net/route.h>
> @@ -205,6 +206,8 @@ static const struct icmp_control icmp_pointers[NR_ICMP_TYPES+1];
> *
> * On SMP we have one ICMP socket per-cpu.
> */
> +static DEFINE_LOCAL_IRQ_LOCK(icmp_sk_lock);
> +
> static struct sock *icmp_sk(struct net *net)
> {
> return *this_cpu_ptr(net->ipv4.icmp_sk);
> @@ -216,12 +219,14 @@ static inline struct sock *icmp_xmit_lock(struct net *net)
>
> local_bh_disable();
>
> + local_lock(icmp_sk_lock);
Deadlock alert ?
Please read the comment few lines after, explaining why we have to use
spin_trylock().
Or maybe I should double check what is local_lock() in RT
> 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_unlock(icmp_sk_lock);
> local_bh_enable();
> return NULL;
[toc] | [prev] | [next] | [standalone]
| From | Steven Rostedt <rostedt@goodmis.org> |
|---|---|
| Date | 2016-08-31 19:10 +0200 |
| Message-ID | <schbs-59T-33@gated-at.bofh.it> |
| In reply to | #1473668 |
On Wed, 31 Aug 2016 09:37:43 -0700
Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Wed, 2016-08-31 at 18:00 +0200, Sebastian Andrzej Siewior wrote:
> > It looks like the this_cpu_ptr() access in icmp_sk() is protected with
> > local_bh_disable(). To avoid missing serialization in -RT I am adding
> > here a local lock. No crash has been observed, this is just precaution.
> >
>
>
> Hmm...
>
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > ---
> > net/ipv4/icmp.c | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
> > index c1f1d5030d37..63731fd6af3e 100644
> > --- a/net/ipv4/icmp.c
> > +++ b/net/ipv4/icmp.c
> > @@ -78,6 +78,7 @@
> > #include <linux/string.h>
> > #include <linux/netfilter_ipv4.h>
> > #include <linux/slab.h>
> > +#include <linux/locallock.h>
> > #include <net/snmp.h>
> > #include <net/ip.h>
> > #include <net/route.h>
> > @@ -205,6 +206,8 @@ static const struct icmp_control icmp_pointers[NR_ICMP_TYPES+1];
> > *
> > * On SMP we have one ICMP socket per-cpu.
> > */
> > +static DEFINE_LOCAL_IRQ_LOCK(icmp_sk_lock);
> > +
> > static struct sock *icmp_sk(struct net *net)
> > {
> > return *this_cpu_ptr(net->ipv4.icmp_sk);
> > @@ -216,12 +219,14 @@ static inline struct sock *icmp_xmit_lock(struct net *net)
> >
> > local_bh_disable();
> >
> > + local_lock(icmp_sk_lock);
>
> Deadlock alert ?
> Please read the comment few lines after, explaining why we have to use
> spin_trylock().
> Or maybe I should double check what is local_lock() in RT
And I don't know exactly what the deadlock scenario of the comment
below is. Is it racing with a softirq somehow?
Note, in RT softirqs can schedule out, and are preemptable. But I don't
know enough about this code to know if that is enough to not have a
deadlock here.
-- Steve
>
> > 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_unlock(icmp_sk_lock);
> > local_bh_enable();
> > return NULL;
>
[toc] | [prev] | [next] | [standalone]
| From | Sebastian Andrzej Siewior <bigeasy@linutronix.de> |
|---|---|
| Date | 2016-09-01 10:20 +0200 |
| Message-ID | <scvo5-6SP-9@gated-at.bofh.it> |
| In reply to | #1473668 |
On 2016-08-31 09:37:43 [-0700], Eric Dumazet wrote:
> > --- a/net/ipv4/icmp.c
> > +++ b/net/ipv4/icmp.c
> > @@ -216,12 +219,14 @@ static inline struct sock *icmp_xmit_lock(struct net *net)
> >
> > local_bh_disable();
> >
> > + local_lock(icmp_sk_lock);
>
> Deadlock alert ?
> Please read the comment few lines after, explaining why we have to use
> spin_trylock().
> Or maybe I should double check what is local_lock() in RT
On !RT local_lock() is preempt_disable().
On RT local_lock() is a per-CPU sleeping spinlock which can be taken
recursively by the owner.
On a "normal" ping reply we have
- icmp_reply()
- icmp_xmit_lock()
- local_lock()
- icmp_push_reply()
icmp_send()
- local_lock()
so that works. I didn't manage to resolve a possible dst_link_failure()
path but I would assume it is like the above (where the owner takes the
same local_lock() multiple times).
Sebastian
[toc] | [prev] | [next] | [standalone]
| From | Eric Dumazet <eric.dumazet@gmail.com> |
|---|---|
| Date | 2016-09-01 15:20 +0200 |
| Message-ID | <scA4q-2mS-11@gated-at.bofh.it> |
| In reply to | #1474158 |
On Thu, 2016-09-01 at 10:11 +0200, Sebastian Andrzej Siewior wrote: > On !RT local_lock() is preempt_disable(). > On RT local_lock() is a per-CPU sleeping spinlock which can be taken > recursively by the owner. Ok, thanks for the explanation, the 'recursively' part I was not aware of ;)
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web