Hmm, but why only fragments need skb_orphan()? It seems like
any kfree_skb() inside a nf hook needs to have a preceding
skb_orphan().
Also, I am not convinced it is similar to commit 8282f27449bf15548
which is on RX path.
Hmm, but why only fragments need skb_orphan()? It seems like
any kfree_skb() inside a nf hook needs to have a preceding
skb_orphan().
Also, I am not convinced it is similar to commit 8282f27449bf15548
which is on RX path.
Well, we clearly see IPv6 reassembly being part of the equation in both
cases.
I was replying to first part of the splat [1], which was already
diagnosed and had a non official patch.
use after free is also a bug, regardless of jump label being used or
not.
I still do not really understand this nf_hook issue, I thought we were
disabling BH in netfilter.
So the in_interrupt() check in net_disable_timestamp() should trigger,
this was the intent of netstamp_needed_deferred existence.
Not sure if we can test for rcu_read_lock() as well.
[1]
sk_destruct+0x47/0x80 net/core/sock.c:1460
__sk_free+0x57/0x230 net/core/sock.c:1468
sock_wfree+0xae/0x120 net/core/sock.c:1645
skb_release_head_state+0xfc/0x200 net/core/skbuff.c:655
skb_release_all+0x15/0x60 net/core/skbuff.c:668
__kfree_skb+0x15/0x20 net/core/skbuff.c:684
kfree_skb+0x16e/0x4c0 net/core/skbuff.c:705
inet_frag_destroy+0x121/0x290 net/ipv4/inet_fragment.c:304
inet_frag_put include/net/inet_frag.h:133 [inline]
nf_ct_frag6_gather+0x1106/0x3840
net/ipv6/netfilter/nf_conntrack_reasm.c:617
ipv6_defrag+0x1be/0x2b0 net/ipv6/netfilter/nf_defrag_ipv6_hooks.c:68
nf_hook_entry_hookfn include/linux/netfilter.h:102 [inline]
nf_hook_slow+0xc3/0x290 net/netfilter/core.c:310
nf_hook include/linux/netfilter.h:212 [inline]
__ip6_local_out+0x489/0x840 net/ipv6/output_core.c:160
ip6_local_out+0x2d/0x170 net/ipv6/output_core.c:170
Hmm, but why only fragments need skb_orphan()? It seems like
any kfree_skb() inside a nf hook needs to have a preceding
skb_orphan().
quoted
Also, I am not convinced it is similar to commit 8282f27449bf15548
which is on RX path.
Well, we clearly see IPv6 reassembly being part of the equation in both
cases.
Yeah, of course. My worry is that this problem is more than just
IPv6 reassembly.
I was replying to first part of the splat [1], which was already
diagnosed and had a non official patch.
use after free is also a bug, regardless of jump label being used or
not.
I still do not really understand this nf_hook issue, I thought we were
disabling BH in netfilter.
It is a different warning from use-after-free, this one is about sleep
in atomic context, mutex lock is acquired with RCU read lock held.
So the in_interrupt() check in net_disable_timestamp() should trigger,
this was the intent of netstamp_needed_deferred existence.
Not sure if we can test for rcu_read_lock() as well.
The context is process context (TX path before hitting qdisc), and
BH is not disabled, so in_interrupt() doesn't catch it. Hmm, this
makes me thinking maybe we really need to disable BH in this
case for nf_hook()? But it is called in RX path too, and BH is
already disabled there.
From: Eric Dumazet <hidden> Date: 2017-01-31 15:46:06
On Mon, 2017-01-30 at 22:19 -0800, Cong Wang wrote:
The context is process context (TX path before hitting qdisc), and
BH is not disabled, so in_interrupt() doesn't catch it. Hmm, this
makes me thinking maybe we really need to disable BH in this
case for nf_hook()? But it is called in RX path too, and BH is
already disabled there.
ipt_do_table() and similar netfilter entry points disable BH.
Maybe it is done too late.
From: Cong Wang <hidden> Date: 2017-02-01 20:52:10
On Tue, Jan 31, 2017 at 7:44 AM, Eric Dumazet [off-list ref] wrote:
On Mon, 2017-01-30 at 22:19 -0800, Cong Wang wrote:
quoted
The context is process context (TX path before hitting qdisc), and
BH is not disabled, so in_interrupt() doesn't catch it. Hmm, this
makes me thinking maybe we really need to disable BH in this
case for nf_hook()? But it is called in RX path too, and BH is
already disabled there.
ipt_do_table() and similar netfilter entry points disable BH.
Maybe it is done too late.
I think we need a fix like the following one for minimum impact.
From: Eric Dumazet <hidden> Date: 2017-02-01 21:16:16
On Wed, 2017-02-01 at 12:51 -0800, Cong Wang wrote:
quoted hunk
On Tue, Jan 31, 2017 at 7:44 AM, Eric Dumazet [off-list ref] wrote:
quoted
On Mon, 2017-01-30 at 22:19 -0800, Cong Wang wrote:
quoted
The context is process context (TX path before hitting qdisc), and
BH is not disabled, so in_interrupt() doesn't catch it. Hmm, this
makes me thinking maybe we really need to disable BH in this
case for nf_hook()? But it is called in RX path too, and BH is
already disabled there.
ipt_do_table() and similar netfilter entry points disable BH.
Maybe it is done too late.
I think we need a fix like the following one for minimum impact.
This would permanently leave the kernel in the netstamp_needed state.
I would prefer the patch using a process context to perform the
cleanup ? Note there is a race window, but probably not a big deal.
net/core/dev.c | 30 ++++++++++--------------------
1 file changed, 10 insertions(+), 20 deletions(-)
@@ -1695,37 +1695,27 @@ EXPORT_SYMBOL_GPL(net_dec_egress_queue);staticstructstatic_keynetstamp_needed__read_mostly;#ifdef HAVE_JUMP_LABEL-/* We are not allowed to call static_key_slow_dec() from irq context-*Ifnet_disable_timestamp()iscalledfromirqcontext,deferthe-*static_key_slow_dec()calls.-*/staticatomic_tnetstamp_needed_deferred;-#endif--voidnet_enable_timestamp(void)+staticvoidnetstamp_clear(structwork_struct*work){-#ifdef HAVE_JUMP_LABELintdeferred=atomic_xchg(&netstamp_needed_deferred,0);-if(deferred){-while(--deferred)-static_key_slow_dec(&netstamp_needed);-return;-}+while(deferred--)+static_key_slow_dec(&netstamp_needed);+}+staticDECLARE_WORK(netstamp_work,netstamp_clear);#endif++voidnet_enable_timestamp(void)+{static_key_slow_inc(&netstamp_needed);}EXPORT_SYMBOL(net_enable_timestamp);voidnet_disable_timestamp(void){-#ifdef HAVE_JUMP_LABEL-if(in_interrupt()){-atomic_inc(&netstamp_needed_deferred);-return;-}-#endif-static_key_slow_dec(&netstamp_needed);+atomic_inc(&netstamp_needed_deferred);+schedule_work(&netstamp_work);}EXPORT_SYMBOL(net_disable_timestamp);
From: Eric Dumazet <hidden> Date: 2017-02-01 21:22:47
On Wed, 2017-02-01 at 13:16 -0800, Eric Dumazet wrote:
This would permanently leave the kernel in the netstamp_needed state.
I would prefer the patch using a process context to perform the
cleanup ? Note there is a race window, but probably not a big deal.
net/core/dev.c | 30 ++++++++++--------------------
1 file changed, 10 insertions(+), 20 deletions(-)
Patch is not complete (for the HAVE_JUMP_LABEL=n case)
Would you like to author/submit it ?
Thanks.
From: Cong Wang <hidden> Date: 2017-02-01 23:30:25
On Wed, Feb 1, 2017 at 1:16 PM, Eric Dumazet [off-list ref] wrote:
On Wed, 2017-02-01 at 12:51 -0800, Cong Wang wrote:
quoted
On Tue, Jan 31, 2017 at 7:44 AM, Eric Dumazet [off-list ref] wrote:
quoted
On Mon, 2017-01-30 at 22:19 -0800, Cong Wang wrote:
quoted
The context is process context (TX path before hitting qdisc), and
BH is not disabled, so in_interrupt() doesn't catch it. Hmm, this
makes me thinking maybe we really need to disable BH in this
case for nf_hook()? But it is called in RX path too, and BH is
already disabled there.
ipt_do_table() and similar netfilter entry points disable BH.
Maybe it is done too late.
I think we need a fix like the following one for minimum impact.
This would permanently leave the kernel in the netstamp_needed state.
I would prefer the patch using a process context to perform the
cleanup ? Note there is a race window, but probably not a big deal.
Not sure if it is better. The difference is caught up in net_enable_timestamp(),
which is called setsockopt() path and sk_clone() path, so we could be
in netstamp_needed state for a long time too until user-space exercises
these paths.
I am feeling we probably need to get rid of netstamp_needed_deferred,
and simply defer the whole static_key_slow_dec(), like the attached patch
(compile only).
What do you think?
From: Eric Dumazet <edumazet@google.com> Date: 2017-02-01 23:48:37
On Wed, Feb 1, 2017 at 3:29 PM, Cong Wang [off-list ref] wrote:
Not sure if it is better. The difference is caught up in net_enable_timestamp(),
which is called setsockopt() path and sk_clone() path, so we could be
in netstamp_needed state for a long time too until user-space exercises
these paths.
I am feeling we probably need to get rid of netstamp_needed_deferred,
and simply defer the whole static_key_slow_dec(), like the attached patch
(compile only).
What do you think?
I think we need to keep the atomic.
If two cpus call net_disable_timestamp() roughly at the same time, the
work will be scheduled once.
From: Eric Dumazet <hidden> Date: 2017-02-01 23:59:48
On Wed, 2017-02-01 at 15:48 -0800, Eric Dumazet wrote:
On Wed, Feb 1, 2017 at 3:29 PM, Cong Wang [off-list ref] wrote:
quoted
Not sure if it is better. The difference is caught up in net_enable_timestamp(),
which is called setsockopt() path and sk_clone() path, so we could be
in netstamp_needed state for a long time too until user-space exercises
these paths.
I am feeling we probably need to get rid of netstamp_needed_deferred,
and simply defer the whole static_key_slow_dec(), like the attached patch
(compile only).
What do you think?
I think we need to keep the atomic.
If two cpus call net_disable_timestamp() roughly at the same time, the
work will be scheduled once.
@@ -1695,24 +1695,19 @@ EXPORT_SYMBOL_GPL(net_dec_egress_queue);staticstructstatic_keynetstamp_needed__read_mostly;#ifdef HAVE_JUMP_LABEL-/* We are not allowed to call static_key_slow_dec() from irq context-*Ifnet_disable_timestamp()iscalledfromirqcontext,deferthe-*static_key_slow_dec()calls.-*/staticatomic_tnetstamp_needed_deferred;-#endif--voidnet_enable_timestamp(void)+staticvoidnetstamp_clear(structwork_struct*work){-#ifdef HAVE_JUMP_LABELintdeferred=atomic_xchg(&netstamp_needed_deferred,0);-if(deferred){-while(--deferred)-static_key_slow_dec(&netstamp_needed);-return;-}+while(deferred--)+static_key_slow_dec(&netstamp_needed);+}+staticDECLARE_WORK(netstamp_work,netstamp_clear);#endif++voidnet_enable_timestamp(void)+{static_key_slow_inc(&netstamp_needed);}EXPORT_SYMBOL(net_enable_timestamp);
@@ -1720,12 +1715,12 @@ EXPORT_SYMBOL(net_enable_timestamp);voidnet_disable_timestamp(void){#ifdef HAVE_JUMP_LABEL-if(in_interrupt()){-atomic_inc(&netstamp_needed_deferred);-return;-}-#endif+/* net_disable_timestamp() can be called from non process context */+atomic_inc(&netstamp_needed_deferred);+schedule_work(&netstamp_work);+#elsestatic_key_slow_dec(&netstamp_needed);+#endif}EXPORT_SYMBOL(net_disable_timestamp);
From: Cong Wang <hidden> Date: 2017-02-02 18:01:26
On Wed, Feb 1, 2017 at 3:59 PM, Eric Dumazet [off-list ref] wrote:
On Wed, 2017-02-01 at 15:48 -0800, Eric Dumazet wrote:
quoted
On Wed, Feb 1, 2017 at 3:29 PM, Cong Wang [off-list ref] wrote:
quoted
Not sure if it is better. The difference is caught up in net_enable_timestamp(),
which is called setsockopt() path and sk_clone() path, so we could be
in netstamp_needed state for a long time too until user-space exercises
these paths.
I am feeling we probably need to get rid of netstamp_needed_deferred,
and simply defer the whole static_key_slow_dec(), like the attached patch
(compile only).
What do you think?
I think we need to keep the atomic.
If two cpus call net_disable_timestamp() roughly at the same time, the
work will be scheduled once.
Good point! Yeah, the same work will not be schedule twice.
Updated patch (but not tested yet)
I can't think out a better way to fix this. I expect jump_label to provide
an API for this, but it doesn't, static_key_slow_dec_deferred()
is just for batching. Probably we should introduce one to avoid these
ugly #ifdef HAVE_JUMP_LABEL here, but that is a -next material.
So, please feel free to send it formally.
Thanks.
@@ -1695,24 +1695,19 @@ EXPORT_SYMBOL_GPL(net_dec_egress_queue);staticstructstatic_keynetstamp_needed__read_mostly;#ifdef HAVE_JUMP_LABEL-/* We are not allowed to call static_key_slow_dec() from irq context-*Ifnet_disable_timestamp()iscalledfromirqcontext,deferthe-*static_key_slow_dec()calls.-*/staticatomic_tnetstamp_needed_deferred;-#endif--voidnet_enable_timestamp(void)+staticvoidnetstamp_clear(structwork_struct*work){-#ifdef HAVE_JUMP_LABELintdeferred=atomic_xchg(&netstamp_needed_deferred,0);-if(deferred){-while(--deferred)-static_key_slow_dec(&netstamp_needed);-return;-}+while(deferred--)+static_key_slow_dec(&netstamp_needed);+}+staticDECLARE_WORK(netstamp_work,netstamp_clear);#endif++voidnet_enable_timestamp(void)+{static_key_slow_inc(&netstamp_needed);}EXPORT_SYMBOL(net_enable_timestamp);
@@ -1720,12 +1715,12 @@ EXPORT_SYMBOL(net_enable_timestamp);voidnet_disable_timestamp(void){#ifdef HAVE_JUMP_LABEL-if(in_interrupt()){-atomic_inc(&netstamp_needed_deferred);-return;-}-#endif+/* net_disable_timestamp() can be called from non process context */+atomic_inc(&netstamp_needed_deferred);+schedule_work(&netstamp_work);+#elsestatic_key_slow_dec(&netstamp_needed);+#endif}EXPORT_SYMBOL(net_disable_timestamp);
From: David Miller <davem@davemloft.net> Date: 2017-02-03 21:18:18
From: Eric Dumazet <redacted>
Date: Thu, 02 Feb 2017 10:31:35 -0800
From: Eric Dumazet <edumazet@google.com>
Dmitry reported a warning [1] showing that we were calling
net_disable_timestamp() -> static_key_slow_dec() from a non
process context.
Grabbing a mutex while holding a spinlock or rcu_read_lock()
is not allowed.
As Cong suggested, we now use a work queue.
It is possible netstamp_clear() exits while netstamp_needed_deferred
is not zero, but it is probably not worth trying to do better than that.
netstamp_needed_deferred atomic tracks the exact number of deferred
decrements.
...
Fixes: b90e5794c5bd ("net: dont call jump_label_dec from irq context")
Suggested-by: Cong Wang <redacted>
Reported-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>