[patch 11/11] netfilter warning fix

[patch 11/11] netfilter warning fix

[akpm]

From: Andrew Morton <akpm@osdl.org>

"using smp_processor_id() in preemptible code"

Cc: Patrick McHardy <kaber@trash.net>
Cc: "David S. Miller" <davem@davemloft.net>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 include/net/netfilter/nf_conntrack.h |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff -puN include/net/netfilter/nf_conntrack.h~netfilter-warning-fix include/net/netfilter/nf_conntrack.h
--- a/include/net/netfilter/nf_conntrack.h~netfilter-warning-fix
+++ a/include/net/netfilter/nf_conntrack.h
@@ -254,7 +254,12 @@ extern atomic_t nf_conntrack_count;
 extern int nf_conntrack_max;
 
 DECLARE_PER_CPU(struct ip_conntrack_stat, nf_conntrack_stat);
-#define NF_CT_STAT_INC(count) (__get_cpu_var(nf_conntrack_stat).count++)
+#define NF_CT_STAT_INC(count)						\
+	do {								\
+		preempt_disable();					\
+		__get_cpu_var(nf_conntrack_stat).count++;		\
+		preempt_enable();					\
+	} while (0);
 
 /* no helper, no nat */
 #define	NF_CT_F_BASIC	0
_

Re: [patch 11/11] netfilter warning fix

[David Miller]

From: akpm@linux-foundation.org
Date: Mon, 05 Feb 2007 16:31:11 -0800

> From: Andrew Morton <akpm@osdl.org>
> 
> "using smp_processor_id() in preemptible code"
> 
> Cc: Patrick McHardy <kaber@trash.net>
> Cc: "David S. Miller" <davem@davemloft.net>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

We NAK'd this because every patch in the lockdep trace should
have preemption disabled because software interrupts ought
to be disabled in those code paths.

Someone needs to track down why this is really happening.

Re: [patch 11/11] netfilter warning fix

[Andrew Morton]

On Mon, 05 Feb 2007 18:10:26 -0800 (PST) David Miller <davem@davemloft.net> wrote:

> From: akpm@linux-foundation.org
> Date: Mon, 05 Feb 2007 16:31:11 -0800
> 
> > From: Andrew Morton <akpm@osdl.org>
> > 
> > "using smp_processor_id() in preemptible code"
> > 
> > Cc: Patrick McHardy <kaber@trash.net>
> > Cc: "David S. Miller" <davem@davemloft.net>
> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> 
> We NAK'd this because every patch in the lockdep trace should
> have preemption disabled because software interrupts ought
> to be disabled in those code paths.
> 
> Someone needs to track down why this is really happening.

I think the finger was pointed at preemptible rcu, in -mm.  iirc, the net
stats code is assuming that rcu_read_lock() disables preemption as a side-effect,
which rcu-preempt makes no-longer-true.

Not sure what to do there.  Perhaps add a new
rcu_read_lock_with_preempt_disable() thing which will dtrt with either
config.

Re: [patch 11/11] netfilter warning fix

[David Miller]

From: Andrew Morton <akpm@linux-foundation.org>
Date: Mon, 5 Feb 2007 18:18:10 -0800

> I think the finger was pointed at preemptible rcu, in -mm.  iirc,
> the net stats code is assuming that rcu_read_lock() disables
> preemption as a side-effect, which rcu-preempt makes no-longer-true.
>
> Not sure what to do there.  Perhaps add a new
> rcu_read_lock_with_preempt_disable() thing which will dtrt with
> either config.

Hmmm, really?

Let's audit NF_CT_STAT_INC() usage to make sure :-)

net/netfilter/nf_conntrack_core.c:

	destroy_conntrack: Inside write_{lock,unlock}_bh().
	death_by_timeout: Ditto.
	__nf_conntrack_find: Inside read_{lock,unlock}_bh() via callers.
	__nf_conntrack_confirm: Inside write_{lock,unlock}_bh().
	early_drop: This one looks like it could be unprotected.
	init_conntrack: Inside of write_{lock,unlock}_bh().
	nf_conntrack_in: Packet receive path, softints disabled.

net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c:

	ipv6_prepare: Packet input path, BH's disabled.

net/netfilter/nf_conntrack_expect.c:

	nf_ct_unlink_expect: Inside if write_{lock,unlock}_bh() via callers.
	nf_conntrack_expect_insert: Ditto.

So that leaves early_drop() as the only suspicious case that might
not run inside of disabled BH's.

And in fact that case is a bug regardless of the preemptible rcu
changes because this allows the counter bump to be corrupted by
software interrupt context.

And OK, I see in the lockdep trace that it's the packet transmit
path...   In fact, this assumption of preemption being disabled
by the netfilter top-level dispatch is very deep.

For example, several bits besides the NF_CT_STATIC_INC of
nf_conntrack_in() (where the lockdep trigger backtrace hits) assume
that preemption is enabled by that rcu_read_lock() in the top-level
netfilter dispatch.

The __nf_ct_l{3,4}proto_find() calls there are just two examples.

I imagine this assumption is quite pervasive throughout the
netfilter code, so just patching up this NF_CT_STAT_INC() case
will merely shut up lockdep and paper over the issue.

I bet this rcu_read_lock()-implies-preempt_disable() assumption has
spread into other areas of the tree as well.

Re: [patch 11/11] netfilter warning fix

[Andrew Morton]

On Mon, 05 Feb 2007 18:44:08 -0800 (PST) David Miller <davem@davemloft.net> wrote:

> I bet this rcu_read_lock()-implies-preempt_disable() assumption has
> spread into other areas of the tree as well.

Me too.  Although one expects that other holes will cause might_sleep or
lockdep warnings pretty easily.

Still, life is hard enough as it is.  I think I'll shelve rcu-preempt
pending resolution of this.

Re: [patch 11/11] netfilter warning fix

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> On Mon, 05 Feb 2007 18:44:08 -0800 (PST) David Miller 
> <davem@davemloft.net> wrote:
> 
> > I bet this rcu_read_lock()-implies-preempt_disable() assumption has 
> > spread into other areas of the tree as well.
> 
> Me too.  Although one expects that other holes will cause might_sleep 
> or lockdep warnings pretty easily.
> 
> Still, life is hard enough as it is.  I think I'll shelve rcu-preempt 
> pending resolution of this.

that was pretty much the only place in the whole kernel that got hit by 
some rcu-preempt side-effect - and even this appears to show that it's a 
real bug that was in hiding. So please dont shelve RCU-preempt for this 
reason :-)

	Ingo

Re: [patch 11/11] netfilter warning fix

[David Miller]

From: Ingo Molnar <mingo@elte.hu>
Date: Tue, 6 Feb 2007 13:34:01 +0100

> 
> * Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > On Mon, 05 Feb 2007 18:44:08 -0800 (PST) David Miller 
> > <davem@davemloft.net> wrote:
> > 
> > > I bet this rcu_read_lock()-implies-preempt_disable() assumption has 
> > > spread into other areas of the tree as well.
> > 
> > Me too.  Although one expects that other holes will cause might_sleep 
> > or lockdep warnings pretty easily.
> > 
> > Still, life is hard enough as it is.  I think I'll shelve rcu-preempt 
> > pending resolution of this.
> 
> that was pretty much the only place in the whole kernel that got hit by 
> some rcu-preempt side-effect - and even this appears to show that it's a 
> real bug that was in hiding.

No, rather, it's the only location that triggered an automated
debugging check.  The very first set of code paths we checked, in
response to the bug trigger, showed beyond a shadow of a doubt that
this assumption is pervasive and in many locations that none of the
automated debugging checks live.

The tree should be fully audited before such a huge semantic change
gets added into the tree.

Re: [patch 11/11] netfilter warning fix

[Ingo Molnar]

* David Miller <davem@davemloft.net> wrote:

> > that was pretty much the only place in the whole kernel that got hit 
> > by some rcu-preempt side-effect - and even this appears to show that 
> > it's a real bug that was in hiding.
> 
> No, rather, it's the only location that triggered an automated 
> debugging check.  The very first set of code paths we checked, in 
> response to the bug trigger, showed beyond a shadow of a doubt that 
> this assumption is pervasive and in many locations that none of the 
> automated debugging checks live.
> 
> The tree should be fully audited before such a huge semantic change 
> gets added into the tree.

i'd like to add more automated checks to the tree. 'naked' 
smp_processor_id() use is one telltale sign of such a problem - what 
other instances could you suggest me to check? The thing is, in 99% of 
the cases the smp_processor_id() check caught such assumptions in other 
code because 'preempt off' is inextricably connected to /some/ use of 
smp_processor_id() - be it get_cpu_var() or any other derivative 
interface. So i'm wondering what other assumptions there are (or can be) 
about rcu_read_lock() also being a preempt-off point. Thanks!

(btw., i always argued that neither preempt_disable() nor 
rcu_read_lock() is an ideal interface for locking because both hide 
critical assumptions and dependencies - and they both are able to create 
little versions of the 'big BKL mess' that we had to fight a few years 
ago. (and that we still have to fight today, in certain areas of code.))

	Ingo

Re: [patch 11/11] netfilter warning fix

[David Miller]

From: Ingo Molnar <mingo@elte.hu>
Date: Tue, 6 Feb 2007 22:02:51 +0100

> So i'm wondering what other assumptions there are (or can be) 
> about rcu_read_lock() also being a preempt-off point. Thanks!

I showed the examples in my detailed analysis yesterday.

Beause I love hearing myself say the same thing over and over so much
I'll restate it for you.

net/netfilter/nf_conntrack_core.c, calls:

	l4proto = __nf_ct_l4proto_find((u_int16_t)pf, protonum);

whichs assumes that preemption is disabled.

Re: [patch 11/11] netfilter warning fix

[Ingo Molnar]

* David Miller <davem@davemloft.net> wrote:

> From: Ingo Molnar <mingo@elte.hu>
> Date: Tue, 6 Feb 2007 22:02:51 +0100
> 
> > So i'm wondering what other assumptions there are (or can be) about 
> > rcu_read_lock() also being a preempt-off point. Thanks!
> 
> I showed the examples in my detailed analysis yesterday.
> 
> Beause I love hearing myself say the same thing over and over so much 
> I'll restate it for you.
> 
> net/netfilter/nf_conntrack_core.c, calls:
> 
> 	l4proto = __nf_ct_l4proto_find((u_int16_t)pf, protonum);
> 
> whichs assumes that preemption is disabled.

you are right - i mistakenly read that mail only up to the point where 
you point out the (slightly) buggy NF_CT_STATIC_INC use and missed your 
final point about other coding having implicit preempt_disable() 
assumptions.

I've looked at __nf_ct_l4proto_find() and it's not obvious to me what 
the hidden preempt_disable() assumption is. Its main use seems to be of 
nf_ct_protos[] array, which is protected by nf_conntrack_lock. I'm 
wondering whether what you say suggests that it's safe to call 
__nf_ct_l4proto_find() without the nf_conntrack_lock locked (as read or 
as write), and if it's safe, how it protects against simultaneous 
modifications to the nf_ct_protos[] array. 

Ahh ... unregister does a synchronize_net(), right? That means that 
removal of the pointer only happens if all CPUs have gone through a 
quiescent state.

this means that this particular use could be fixed by converting the 
preempt_disable()/enable() pair in nf_ct_l4proto_find_get() to 
rcu_read_lock()/unlock(), correct? Furthermore, every user of 
synchronize_net() [and synchronize_rcu() in general] needs to be 
reviewed.

	Ingo

Re: [patch 11/11] netfilter warning fix

[Patrick McHardy]

Ingo Molnar wrote:
> * David Miller <davem@davemloft.net> wrote:
> 
>>net/netfilter/nf_conntrack_core.c, calls:
>>
>>	l4proto = __nf_ct_l4proto_find((u_int16_t)pf, protonum);
>>
>>whichs assumes that preemption is disabled.

Yes, that is certainly broken.

> you are right - i mistakenly read that mail only up to the point where 
> you point out the (slightly) buggy NF_CT_STATIC_INC use and missed your 
> final point about other coding having implicit preempt_disable() 
> assumptions.
> 
> I've looked at __nf_ct_l4proto_find() and it's not obvious to me what 
> the hidden preempt_disable() assumption is. Its main use seems to be of 
> nf_ct_protos[] array, which is protected by nf_conntrack_lock. I'm 
> wondering whether what you say suggests that it's safe to call 
> __nf_ct_l4proto_find() without the nf_conntrack_lock locked (as read or 
> as write), and if it's safe, how it protects against simultaneous 
> modifications to the nf_ct_protos[] array. 
> 
> Ahh ... unregister does a synchronize_net(), right? That means that 
> removal of the pointer only happens if all CPUs have gone through a 
> quiescent state.
> 
> this means that this particular use could be fixed by converting the 
> preempt_disable()/enable() pair in nf_ct_l4proto_find_get() to 
> rcu_read_lock()/unlock(), correct? 

That is another bug (all uses of preempt_disable in netfilter
actually), but calling __nf_ct_l[34]proto_find without
rcu_read_lock is broken as well.

> Furthermore, every user of 
> synchronize_net() [and synchronize_rcu() in general] needs to be 
> reviewed.

I'll take care of netfilter.

Re: [patch 11/11] netfilter warning fix

[Ingo Molnar]

* Patrick McHardy <kaber@trash.net> wrote:

> > this means that this particular use could be fixed by converting the 
> > preempt_disable()/enable() pair in nf_ct_l4proto_find_get() to 
> > rcu_read_lock()/unlock(), correct?
> 
> That is another bug (all uses of preempt_disable in netfilter 
> actually), but calling __nf_ct_l[34]proto_find without rcu_read_lock 
> is broken as well.
> 
> > Furthermore, every user of synchronize_net() [and synchronize_rcu() 
> > in general] needs to be reviewed.
> 
> I'll take care of netfilter.

great! Netfilter is one of the main users of synchronize_net() (about 
90% of the synchronize_net() callsites are in netfilter), and one of the 
main (well, in fact pretty much only) users of preempt_disable().

Dave, Patrick, can we lift the nack on PREEMPT_RCU, as far as networking 
goes? We really need some progress regarding it, and i think Paul is way 
too modest about pushing it. His patchset been in beta testing for like 
2 years meanwhile. PREEMPT_RCU has already found a nice set of 
bugs/uncleanlinesses elsewhere too, so even if you never actually enable 
PREEMPT_RCU (it's off by default), it's a nice generalization of the RCU 
framework that also tightens up its semantics. I think we need that, 
before existing code learns to depend on non-generic details of 
'classic' RCU too much. [my other interest in it, as you might have 
guessed, is that PREEMPT_RT depends on PREEMPT_RCU architecturally, but 
that shouldnt worry you - i think the upstream arguments alone are 
strong enough.]

	Ingo

Re: [patch 11/11] netfilter warning fix

[David Miller]

From: Ingo Molnar <mingo@elte.hu>
Date: Wed, 7 Feb 2007 09:07:22 +0100

> Dave, Patrick, can we lift the nack on PREEMPT_RCU, as far as networking 
> goes?

After Patrick's fixups, sure.

Re: [patch 11/11] netfilter warning fix

[Patrick McHardy]

David Miller wrote:
> From: Ingo Molnar <mingo@elte.hu>
> Date: Wed, 7 Feb 2007 09:07:22 +0100
> 
> 
>>Dave, Patrick, can we lift the nack on PREEMPT_RCU, as far as networking 
>>goes?
> 
> 
> After Patrick's fixups, sure.


I'm about to send my queued netfilter patches, after which I will look
into this. Shouldn't take more than a one or two days.

Re: [patch 11/11] netfilter warning fix

[Ingo Molnar]

* Patrick McHardy <kaber@trash.net> wrote:

> David Miller wrote:
> > From: Ingo Molnar <mingo@elte.hu>
> > Date: Wed, 7 Feb 2007 09:07:22 +0100
> > 
> > 
> >>Dave, Patrick, can we lift the nack on PREEMPT_RCU, as far as networking 
> >>goes?
> > 
> > 
> > After Patrick's fixups, sure.
> 
> I'm about to send my queued netfilter patches, after which I will look 
> into this. Shouldn't take more than a one or two days.

thanks, that's great!

	Ingo

Re: [patch 11/11] netfilter warning fix

[Martin Josefsson]

On Mon, 5 Feb 2007, David Miller wrote:

> Let's audit NF_CT_STAT_INC() usage to make sure :-)
>
> net/netfilter/nf_conntrack_core.c:
>
> 	destroy_conntrack: Inside write_{lock,unlock}_bh().
> 	death_by_timeout: Ditto.
> 	__nf_conntrack_find: Inside read_{lock,unlock}_bh() via callers.
> 	__nf_conntrack_confirm: Inside write_{lock,unlock}_bh().
> 	early_drop: This one looks like it could be unprotected.
> 	init_conntrack: Inside of write_{lock,unlock}_bh().
> 	nf_conntrack_in: Packet receive path, softints disabled.

early_drop() is called from __nf_conntrack_alloc() which is called from
init_conntrack() and these days from ctnetlink_create_conntrack() as well.

init_conntrack() is called from nf_conntrack_in() which is packet receive
path. But ctnetlink_create_conntrack() is unprotected.

> So that leaves early_drop() as the only suspicious case that might
> not run inside of disabled BH's.

Looks that way.

> And in fact that case is a bug regardless of the preemptible rcu
> changes because this allows the counter bump to be corrupted by
> software interrupt context.

Yes, it's a bug, although not a very critical one.

> And OK, I see in the lockdep trace that it's the packet transmit
> path...   In fact, this assumption of preemption being disabled
> by the netfilter top-level dispatch is very deep.

Yes it is, and I have some future changes that wants to take this
assumption even further in order to possibly avoid the per packet
conntrack refcounts as far as possible, but this might not be possible and
there's still a long way to go before we are there. Currently it's just a
wet dream of mine.

> For example, several bits besides the NF_CT_STATIC_INC of
> nf_conntrack_in() (where the lockdep trigger backtrace hits) assume
> that preemption is enabled by that rcu_read_lock() in the top-level
> netfilter dispatch.

s/enabled/disabled/

> The __nf_ct_l{3,4}proto_find() calls there are just two examples.
>
> I imagine this assumption is quite pervasive throughout the
> netfilter code, so just patching up this NF_CT_STAT_INC() case
> will merely shut up lockdep and paper over the issue.
>
> I bet this rcu_read_lock()-implies-preempt_disable() assumption has
> spread into other areas of the tree as well.

I think so too.

/Martin