Re: [patch 11/11] netfilter warning fix

[David Miller <davem@davemloft.net>]

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.