Re: panic on rmmod of nf_conntrack_irc

Re: panic on rmmod of nf_conntrack_irc

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 770 bytes --]

Mariusz Kozlowski wrote:
> 	Recent kernels (i.e. 2.6.30-rc1) will panic while doing rmmod of nf_conntrack_irc.
> 
> (gdb) l *(nf_conntrack_helper_unregister+0x158)
> 0x4f8 is in nf_conntrack_helper_unregister (/home/mako/linux/lkt/sources/linux-2.6/include/net/netfilter/nf_conntrack.h:133).
> 128	};
> 129	
> 130	static inline struct nf_conn *
> 131	nf_ct_tuplehash_to_ctrack(const struct nf_conntrack_tuple_hash *hash)
> 132	{
> 133		return container_of(hash, struct nf_conn,
> 134				    tuplehash[hash->tuple.dst.dir]);
> 135	}
> 136	
> 137	static inline u_int16_t nf_ct_l3num(const struct nf_conn *ct)
> 
> 
> I bisected it down to
> 
>     netfilter: nf_conntrack: use SLAB_DESTROY_BY_RCU and get rid of call_rcu()

Thanks for the report. Does this patch fix it?


[-- Attachment #2: x --]
[-- Type: text/plain, Size: 613 bytes --]

diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
index 30b8e90..0fa5a42 100644
--- a/net/netfilter/nf_conntrack_helper.c
+++ b/net/netfilter/nf_conntrack_helper.c
@@ -176,7 +176,7 @@ static void __nf_conntrack_helper_unregister(struct nf_conntrack_helper *me,
 	}
 
 	/* Get rid of expecteds, set helpers to NULL. */
-	hlist_for_each_entry(h, nn, &net->ct.unconfirmed, hnnode)
+	hlist_nulls_for_each_entry(h, nn, &net->ct.unconfirmed, hnnode)
 		unhelp(h, me);
 	for (i = 0; i < nf_conntrack_htable_size; i++) {
 		hlist_nulls_for_each_entry(h, nn, &net->ct.hash[i], hnnode)

Re: panic on rmmod of nf_conntrack_irc

[Eric Dumazet]

Patrick McHardy a écrit :
> Mariusz Kozlowski wrote:
>>     Recent kernels (i.e. 2.6.30-rc1) will panic while doing rmmod of
>> nf_conntrack_irc.
>>
>> (gdb) l *(nf_conntrack_helper_unregister+0x158)
>> 0x4f8 is in nf_conntrack_helper_unregister
>> (/home/mako/linux/lkt/sources/linux-2.6/include/net/netfilter/nf_conntrack.h:133).
>>
>> 128    };
>> 129   
>> 130    static inline struct nf_conn *
>> 131    nf_ct_tuplehash_to_ctrack(const struct nf_conntrack_tuple_hash
>> *hash)
>> 132    {
>> 133        return container_of(hash, struct nf_conn,
>> 134                    tuplehash[hash->tuple.dst.dir]);
>> 135    }
>> 136   
>> 137    static inline u_int16_t nf_ct_l3num(const struct nf_conn *ct)
>>
>>
>> I bisected it down to
>>
>>     netfilter: nf_conntrack: use SLAB_DESTROY_BY_RCU and get rid of
>> call_rcu()
> 
> Thanks for the report. Does this patch fix it?
> 

Hi Patrick, sorry for the delay, I was in holidays.


I should have used different fields names (from "next", "first", ...) to catch this
kind of errors at compile time :(

Something like :

diff --git a/include/linux/list_nulls.h b/include/linux/list_nulls.h
index 93150ec..b66c553 100644
--- a/include/linux/list_nulls.h
+++ b/include/linux/list_nulls.h
@@ -15,14 +15,14 @@
  */
 
 struct hlist_nulls_head {
-	struct hlist_nulls_node *first;
+	struct hlist_nulls_node *nfirst;
 };
 
 struct hlist_nulls_node {
-	struct hlist_nulls_node *next, **pprev;
+	struct hlist_nulls_node *nnext, **npprev;
 };
 #define INIT_HLIST_NULLS_HEAD(ptr, nulls) \
-	((ptr)->first = (struct hlist_nulls_node *) (1UL | (((long)nulls) << 1)))
+	((ptr)->nfirst = (struct hlist_nulls_node *) (1UL | (((long)nulls) << 1)))
 
 #define hlist_nulls_entry(ptr, type, member) container_of(ptr,type,member)
 /**
@@ -48,21 +48,21 @@ static inline unsigned long get_nulls_value(const struct hlist_nulls_node *ptr)
 
 static inline int hlist_nulls_unhashed(const struct hlist_nulls_node *h)
 {
-	return !h->pprev;
+	return !h->npprev;
 }
 
 static inline int hlist_nulls_empty(const struct hlist_nulls_head *h)
 {
-	return is_a_nulls(h->first);
+	return is_a_nulls(h->nfirst);
 }
 
 static inline void __hlist_nulls_del(struct hlist_nulls_node *n)
 {
-	struct hlist_nulls_node *next = n->next;
-	struct hlist_nulls_node **pprev = n->pprev;
+	struct hlist_nulls_node *next = n->nnext;
+	struct hlist_nulls_node **pprev = n->npprev;
 	*pprev = next;
 	if (!is_a_nulls(next))
-		next->pprev = pprev;
+		next->npprev = pprev;
 }
 
 /**
@@ -74,10 +74,10 @@ static inline void __hlist_nulls_del(struct hlist_nulls_node *n)
  *
  */
 #define hlist_nulls_for_each_entry(tpos, pos, head, member)		       \
-	for (pos = (head)->first;					       \
+	for (pos = (head)->nfirst;					       \
 	     (!is_a_nulls(pos)) &&					       \
 		({ tpos = hlist_nulls_entry(pos, typeof(*tpos), member); 1;}); \
-	     pos = pos->next)
+	     pos = pos->nnext)
 
 /**
  * hlist_nulls_for_each_entry_from - iterate over a hlist continuing from current point
@@ -89,6 +89,6 @@ static inline void __hlist_nulls_del(struct hlist_nulls_node *n)
 #define hlist_nulls_for_each_entry_from(tpos, pos, member)	\
 	for (; (!is_a_nulls(pos)) && 				\
 		({ tpos = hlist_nulls_entry(pos, typeof(*tpos), member); 1;}); \
-	     pos = pos->next)
+	     pos = pos->nnext)
 
 #endif
diff --git a/include/linux/rculist_nulls.h b/include/linux/rculist_nulls.h
index f9ddd03..2378c7c 100644
--- a/include/linux/rculist_nulls.h
+++ b/include/linux/rculist_nulls.h
@@ -33,7 +33,7 @@ static inline void hlist_nulls_del_init_rcu(struct hlist_nulls_node *n)
 {
 	if (!hlist_nulls_unhashed(n)) {
 		__hlist_nulls_del(n);
-		n->pprev = NULL;
+		n->npprev = NULL;
 	}
 }
 
@@ -59,7 +59,7 @@ static inline void hlist_nulls_del_init_rcu(struct hlist_nulls_node *n)
 static inline void hlist_nulls_del_rcu(struct hlist_nulls_node *n)
 {
 	__hlist_nulls_del(n);
-	n->pprev = LIST_POISON2;
+	n->npprev = LIST_POISON2;
 }
 
 /**
@@ -84,13 +84,13 @@ static inline void hlist_nulls_del_rcu(struct hlist_nulls_node *n)
 static inline void hlist_nulls_add_head_rcu(struct hlist_nulls_node *n,
 					struct hlist_nulls_head *h)
 {
-	struct hlist_nulls_node *first = h->first;
+	struct hlist_nulls_node *first = h->nfirst;
 
-	n->next = first;
-	n->pprev = &h->first;
-	rcu_assign_pointer(h->first, n);
+	n->nnext = first;
+	n->npprev = &h->nfirst;
+	rcu_assign_pointer(h->nfirst, n);
 	if (!is_a_nulls(first))
-		first->pprev = &n->next;
+		first->npprev = &n->nnext;
 }
 /**
  * hlist_nulls_for_each_entry_rcu - iterate over rcu list of given type
@@ -101,10 +101,10 @@ static inline void hlist_nulls_add_head_rcu(struct hlist_nulls_node *n,
  *
  */
 #define hlist_nulls_for_each_entry_rcu(tpos, pos, head, member) \
-	for (pos = rcu_dereference((head)->first);			 \
+	for (pos = rcu_dereference((head)->nfirst);			 \
 		(!is_a_nulls(pos)) && 			\
 		({ tpos = hlist_nulls_entry(pos, typeof(*tpos), member); 1; }); \
-		pos = rcu_dereference(pos->next))
+		pos = rcu_dereference(pos->nnext))
 
 #endif
 #endif
diff --git a/include/net/sock.h b/include/net/sock.h
index 4bb1ff9..5d94186 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -309,7 +309,7 @@ static inline struct sock *sk_head(const struct hlist_head *head)
 
 static inline struct sock *__sk_nulls_head(const struct hlist_nulls_head *head)
 {
-	return hlist_nulls_entry(head->first, struct sock, sk_nulls_node);
+	return hlist_nulls_entry(head->nfirst, struct sock, sk_nulls_node);
 }
 
 static inline struct sock *sk_nulls_head(const struct hlist_nulls_head *head)
@@ -325,8 +325,8 @@ static inline struct sock *sk_next(const struct sock *sk)
 
 static inline struct sock *sk_nulls_next(const struct sock *sk)
 {
-	return (!is_a_nulls(sk->sk_nulls_node.next)) ?
-		hlist_nulls_entry(sk->sk_nulls_node.next,
+	return (!is_a_nulls(sk->sk_nulls_node.nnext)) ?
+		hlist_nulls_entry(sk->sk_nulls_node.nnext,
 				  struct sock, sk_nulls_node) :
 		NULL;
 }
@@ -348,7 +348,7 @@ static __inline__ void sk_node_init(struct hlist_node *node)
 
 static __inline__ void sk_nulls_node_init(struct hlist_nulls_node *node)
 {
-	node->pprev = NULL;
+	node->npprev = NULL;
 }
 
 static __inline__ void __sk_del_node(struct sock *sk)
diff --git a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c
index 8668a3d..0bb6059 100644
--- a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c
+++ b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c
@@ -34,7 +34,7 @@ static struct hlist_nulls_node *ct_get_first(struct seq_file *seq)
 	for (st->bucket = 0;
 	     st->bucket < nf_conntrack_htable_size;
 	     st->bucket++) {
-		n = rcu_dereference(net->ct.hash[st->bucket].first);
+		n = rcu_dereference(net->ct.hash[st->bucket].nfirst);
 		if (!is_a_nulls(n))
 			return n;
 	}
@@ -47,13 +47,13 @@ static struct hlist_nulls_node *ct_get_next(struct seq_file *seq,
 	struct net *net = seq_file_net(seq);
 	struct ct_iter_state *st = seq->private;
 
-	head = rcu_dereference(head->next);
+	head = rcu_dereference(head->nnext);
 	while (is_a_nulls(head)) {
 		if (likely(get_nulls_value(head) == st->bucket)) {
 			if (++st->bucket >= nf_conntrack_htable_size)
 				return NULL;
 		}
-		head = rcu_dereference(net->ct.hash[st->bucket].first);
+		head = rcu_dereference(net->ct.hash[st->bucket].nfirst);
 	}
 	return head;
 }
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 5d427f8..1219f2d 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1851,13 +1851,13 @@ EXPORT_SYMBOL(tcp_v4_destroy_sock);
 static inline struct inet_timewait_sock *tw_head(struct hlist_nulls_head *head)
 {
 	return hlist_nulls_empty(head) ? NULL :
-		list_entry(head->first, struct inet_timewait_sock, tw_node);
+		list_entry(head->nfirst, struct inet_timewait_sock, tw_node);
 }
 
 static inline struct inet_timewait_sock *tw_next(struct inet_timewait_sock *tw)
 {
-	return !is_a_nulls(tw->tw_node.next) ?
-		hlist_nulls_entry(tw->tw_node.next, typeof(*tw), tw_node) : NULL;
+	return !is_a_nulls(tw->tw_node.nnext) ?
+		hlist_nulls_entry(tw->tw_node.nnext, typeof(*tw), tw_node) : NULL;
 }
 
 static void *listening_get_next(struct seq_file *seq, void *cur)
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 8020db6..56f9dc3 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -1140,7 +1140,7 @@ int nf_conntrack_set_hashsize(const char *val, struct kernel_param *kp)
 	spin_lock_bh(&nf_conntrack_lock);
 	for (i = 0; i < nf_conntrack_htable_size; i++) {
 		while (!hlist_nulls_empty(&init_net.ct.hash[i])) {
-			h = hlist_nulls_entry(init_net.ct.hash[i].first,
+			h = hlist_nulls_entry(init_net.ct.hash[i].nfirst,
 					struct nf_conntrack_tuple_hash, hnnode);
 			hlist_nulls_del_rcu(&h->hnnode);
 			bucket = __hash_conntrack(&h->tuple, hashsize, rnd);
diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
index 30b8e90..0fa5a42 100644
--- a/net/netfilter/nf_conntrack_helper.c
+++ b/net/netfilter/nf_conntrack_helper.c
@@ -176,7 +176,7 @@ static void __nf_conntrack_helper_unregister(struct nf_conntrack_helper *me,
 	}
 
 	/* Get rid of expecteds, set helpers to NULL. */
-	hlist_for_each_entry(h, nn, &net->ct.unconfirmed, hnnode)
+	hlist_nulls_for_each_entry(h, nn, &net->ct.unconfirmed, hnnode)
 		unhelp(h, me);
 	for (i = 0; i < nf_conntrack_htable_size; i++) {
 		hlist_nulls_for_each_entry(h, nn, &net->ct.hash[i], hnnode)
diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
index 1935153..e6881db 100644
--- a/net/netfilter/nf_conntrack_standalone.c
+++ b/net/netfilter/nf_conntrack_standalone.c
@@ -53,7 +53,7 @@ static struct hlist_nulls_node *ct_get_first(struct seq_file *seq)
 	for (st->bucket = 0;
 	     st->bucket < nf_conntrack_htable_size;
 	     st->bucket++) {
-		n = rcu_dereference(net->ct.hash[st->bucket].first);
+		n = rcu_dereference(net->ct.hash[st->bucket].nfirst);
 		if (!is_a_nulls(n))
 			return n;
 	}
@@ -66,13 +66,13 @@ static struct hlist_nulls_node *ct_get_next(struct seq_file *seq,
 	struct net *net = seq_file_net(seq);
 	struct ct_iter_state *st = seq->private;
 
-	head = rcu_dereference(head->next);
+	head = rcu_dereference(head->nnext);
 	while (is_a_nulls(head)) {
 		if (likely(get_nulls_value(head) == st->bucket)) {
 			if (++st->bucket >= nf_conntrack_htable_size)
 				return NULL;
 		}
-		head = rcu_dereference(net->ct.hash[st->bucket].first);
+		head = rcu_dereference(net->ct.hash[st->bucket].nfirst);
 	}
 	return head;
 }

Re: panic on rmmod of nf_conntrack_irc

[Patrick McHardy]

Eric Dumazet wrote:
> Patrick McHardy a écrit :
>> Mariusz Kozlowski wrote:
>>>     netfilter: nf_conntrack: use SLAB_DESTROY_BY_RCU and get rid of
>>> call_rcu()
>> Thanks for the report. Does this patch fix it?
>>
> 
> Hi Patrick, sorry for the delay, I was in holidays.

No problem, me too :)

> I should have used different fields names (from "next", "first", ...) to catch this
> kind of errors at compile time :(
 >
> Something like :

Thanks Eric. I guess at this point it doesn't really matter anymore
for the upstream kernel, but I'll apply your patch after getting
confirmation from Mariusz to make sure that people maintaining
external patches will notice the change (and won't send me broken
patches :)).

Re: panic on rmmod of nf_conntrack_irc

[Mariusz Kozlowski]

On Tue, 14 Apr 2009 14:16:37 +0200
Patrick McHardy <kaber@trash.net> wrote:

> Eric Dumazet wrote:
> > Patrick McHardy a écrit :
> >> Mariusz Kozlowski wrote:
> >>>     netfilter: nf_conntrack: use SLAB_DESTROY_BY_RCU and get rid of
> >>> call_rcu()
> >> Thanks for the report. Does this patch fix it?
> >>
> > 
> > Hi Patrick, sorry for the delay, I was in holidays.
> 
> No problem, me too :)
> 
> > I should have used different fields names (from "next", "first", ...) to catch this
> > kind of errors at compile time :(
>  >
> > Something like :
> 
> Thanks Eric. I guess at this point it doesn't really matter anymore
> for the upstream kernel, but I'll apply your patch after getting
> confirmation from Mariusz to make sure that people maintaining
> external patches will notice the change (and won't send me broken
> patches :)).

Ok good. It doesn't panic now on nf_conntrack_irc rmmod but I found another bug.
Now it's NULL pointer dereference I when doing rmmod of ebt_log. To reproduce simply:

# modprobe ebt_log && rmmod ebt_log

This is from the mainline kernel + your (Erics) patch:

BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
IP: [<ffffffff8048490a>] nf_log_unregister+0x2a/0x90
PGD 6f0e8067 PUD 6a2e0067 PMD 0 
Oops: 0002 [#1] PREEMPT SMP 
last sysfs file: /sys/devices/LNXSYSTM:00/device:00/PNP0A08:00/device:01/PNP0C09:00/PNP0C0A:00/power_supply/BAT0/energy_full
CPU 0 
Modules linked in: ebt_log(-) nfs lockd auth_rpcgss sunrpc netconsole af_packet i915 drm i2c_algo_bit i2c_core kvm_intel kvm ppdev acpi_cpufreq cpufreq_stats cpufreq_powersave cpufreq_userspace cpufreq_conservative cpufreq_ondemand freq_table fan sbs sbshc container pci_slot iptable_filter ip_tables x_tables sbp2 lp fuse snd_hda_codec_analog arc4 ecb snd_hda_intel snd_hda_codec snd_pcm_oss snd_mixer_oss iwl3945 iwlcore thinkpad_acpi sr_mod cdrom pcmcia mac80211 snd_pcm rfkill led_class snd_seq_dummy snd_seq_oss snd_seq_midi_event evdev thermal uhci_hcd ehci_hcd parport_pc ohci1394 nvram yenta_socket rsrc_nonstatic pcmcia_core snd_seq pcspkr psmouse serio_raw usbcore cfg80211 e1000e ieee1394 sg parport battery ac button processor snd_timer snd_seq_device intel_agp snd soundcore snd_page_alloc
Pid: 10249, comm: rmmod Not tainted 2.6.30-rc1-00204-gb0cbc86-dirty #33 7667Y24
RIP: 0010:[<ffffffff8048490a>]  [<ffffffff8048490a>] nf_log_unregister+0x2a/0x90
RSP: 0018:ffff88006a331ec8  EFLAGS: 00010207
RAX: 0000000000000000 RBX: ffffffffa02d1d00 RCX: 0000000000000000
RDX: 0000000000000000 RSI: 0000000000000002 RDI: ffffffff8065bb80
RBP: ffff88006a331ed8 R08: 0000000000000002 R09: ffffffff80676600
R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
R13: 0000000000000880 R14: 0000000000000000 R15: 0000000000000000
FS:  0000000000000000(0000) GS:ffff880001010000(0063) knlGS:00000000f7fc46b0
CS:  0010 DS: 002b ES: 002b CR0: 000000008005003b
CR2: 0000000000000008 CR3: 000000006f9f9000 CR4: 00000000000026a0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process rmmod (pid: 10249, threadinfo ffff88006a330000, task ffff88006f3c8000)
Stack:
 ffffffffa02d1e80 ffffffffa02d1e80 ffff88006a331ee8 ffffffffa02d1518
 ffff88006a331f78 ffffffff8026ddbb 00676f6c5f746265 ffffffff80229e77
 0000000000000282 ffff88006a331f58 ffff88006f3c8000 0000000000000000
Call Trace:
 [<ffffffffa02d1518>] ebt_log_fini+0x10/0x1e [ebt_log]
 [<ffffffff8026ddbb>] sys_delete_module+0x18b/0x240
 [<ffffffff80229e77>] ? do_page_fault+0x187/0x2a0
 [<ffffffff8022f08b>] sysenter_dispatch+0x7/0x32
Code: 90 55 48 89 e5 53 48 89 fb 48 83 ec 08 48 c7 c7 80 bb 65 80 e8 78 a9 06 00 31 c9 eb 31 0f 1f 40 00 48 8b 54 4b 18 48 8b 44 4b 20 <48> 89 42 08 48 89 10 48 c7 44 4b 20 00 02 20 00 48 c7 44 4b 18 
RIP  [<ffffffff8048490a>] nf_log_unregister+0x2a/0x90
 RSP <ffff88006a331ec8>
CR2: 0000000000000008

If this is not related or you don't have a clue I can bisect it.

	Mariusz

Re: panic on rmmod of nf_conntrack_irc

[Mariusz Kozlowski]

On Tue, 14 Apr 2009 21:19:46 +0200
Mariusz Kozlowski <m.kozlowski@tuxland.pl> wrote:

> On Tue, 14 Apr 2009 14:16:37 +0200
> Patrick McHardy <kaber@trash.net> wrote:
> 
> > Eric Dumazet wrote:
> > > Patrick McHardy a écrit :
> > >> Mariusz Kozlowski wrote:
> > >>>     netfilter: nf_conntrack: use SLAB_DESTROY_BY_RCU and get rid of
> > >>> call_rcu()
> > >> Thanks for the report. Does this patch fix it?
> > >>
> > > 
> > > Hi Patrick, sorry for the delay, I was in holidays.
> > 
> > No problem, me too :)
> > 
> > > I should have used different fields names (from "next", "first", ...) to catch this
> > > kind of errors at compile time :(
> >  >
> > > Something like :
> > 
> > Thanks Eric. I guess at this point it doesn't really matter anymore
> > for the upstream kernel, but I'll apply your patch after getting
> > confirmation from Mariusz to make sure that people maintaining
> > external patches will notice the change (and won't send me broken
> > patches :)).
> 
> Ok good. It doesn't panic now on nf_conntrack_irc rmmod

So feel free to add my rb and tb lines to it :)

> but I found another bug.
> Now it's NULL pointer dereference I when doing rmmod of ebt_log. To reproduce simply:
> 
> # modprobe ebt_log && rmmod ebt_log
> 
> This is from the mainline kernel + your (Erics) patch:
> 
> BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
> IP: [<ffffffff8048490a>] nf_log_unregister+0x2a/0x90
> PGD 6f0e8067 PUD 6a2e0067 PMD 0 
> Oops: 0002 [#1] PREEMPT SMP 
> last sysfs file: /sys/devices/LNXSYSTM:00/device:00/PNP0A08:00/device:01/PNP0C09:00/PNP0C0A:00/power_supply/BAT0/energy_full
> CPU 0 
> Modules linked in: ebt_log(-) nfs lockd auth_rpcgss sunrpc netconsole af_packet i915 drm i2c_algo_bit i2c_core kvm_intel kvm ppdev acpi_cpufreq cpufreq_stats cpufreq_powersave cpufreq_userspace cpufreq_conservative cpufreq_ondemand freq_table fan sbs sbshc container pci_slot iptable_filter ip_tables x_tables sbp2 lp fuse snd_hda_codec_analog arc4 ecb snd_hda_intel snd_hda_codec snd_pcm_oss snd_mixer_oss iwl3945 iwlcore thinkpad_acpi sr_mod cdrom pcmcia mac80211 snd_pcm rfkill led_class snd_seq_dummy snd_seq_oss snd_seq_midi_event evdev thermal uhci_hcd ehci_hcd parport_pc ohci1394 nvram yenta_socket rsrc_nonstatic pcmcia_core snd_seq pcspkr psmouse serio_raw usbcore cfg80211 e1000e ieee1394 sg parport battery ac button processor snd_timer snd_seq_device intel_agp snd soundcore snd_page_alloc
> Pid: 10249, comm: rmmod Not tainted 2.6.30-rc1-00204-gb0cbc86-dirty #33 7667Y24
> RIP: 0010:[<ffffffff8048490a>]  [<ffffffff8048490a>] nf_log_unregister+0x2a/0x90
> RSP: 0018:ffff88006a331ec8  EFLAGS: 00010207
> RAX: 0000000000000000 RBX: ffffffffa02d1d00 RCX: 0000000000000000
> RDX: 0000000000000000 RSI: 0000000000000002 RDI: ffffffff8065bb80
> RBP: ffff88006a331ed8 R08: 0000000000000002 R09: ffffffff80676600
> R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
> R13: 0000000000000880 R14: 0000000000000000 R15: 0000000000000000
> FS:  0000000000000000(0000) GS:ffff880001010000(0063) knlGS:00000000f7fc46b0
> CS:  0010 DS: 002b ES: 002b CR0: 000000008005003b
> CR2: 0000000000000008 CR3: 000000006f9f9000 CR4: 00000000000026a0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> Process rmmod (pid: 10249, threadinfo ffff88006a330000, task ffff88006f3c8000)
> Stack:
>  ffffffffa02d1e80 ffffffffa02d1e80 ffff88006a331ee8 ffffffffa02d1518
>  ffff88006a331f78 ffffffff8026ddbb 00676f6c5f746265 ffffffff80229e77
>  0000000000000282 ffff88006a331f58 ffff88006f3c8000 0000000000000000
> Call Trace:
>  [<ffffffffa02d1518>] ebt_log_fini+0x10/0x1e [ebt_log]
>  [<ffffffff8026ddbb>] sys_delete_module+0x18b/0x240
>  [<ffffffff80229e77>] ? do_page_fault+0x187/0x2a0
>  [<ffffffff8022f08b>] sysenter_dispatch+0x7/0x32
> Code: 90 55 48 89 e5 53 48 89 fb 48 83 ec 08 48 c7 c7 80 bb 65 80 e8 78 a9 06 00 31 c9 eb 31 0f 1f 40 00 48 8b 54 4b 18 48 8b 44 4b 20 <48> 89 42 08 48 89 10 48 c7 44 4b 20 00 02 20 00 48 c7 44 4b 18 
> RIP  [<ffffffff8048490a>] nf_log_unregister+0x2a/0x90
>  RSP <ffff88006a331ec8>
> CR2: 0000000000000008
> 
> If this is not related or you don't have a clue I can bisect it.

The same happens for ebt_ulog on clean latest mainline (as of 6f66cbc6)
so just for reference:

BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
IP: [<ffffffff80484b4a>] nf_log_unregister+0x2a/0x90
PGD 6f0ff067 PUD 6fb1a067 PMD 0 
Oops: 0002 [#1] PREEMPT SMP 
last sysfs file: /sys/devices/LNXSYSTM:00/device:00/PNP0A08:00/device:01/PNP0C09:00/PNP0C0A:00/power_supply/BAT0/energy_full
CPU 1 
Modules linked in: ebt_ulog(-) netconsole af_packet i915 drm i2c_algo_bit i2c_core kvm_intel kvm ppdev acpi_cpufreq cpufreq_stats cpufreq_powersave cpufreq_userspace cpufreq_conservative cpufreq_ondemand freq_table fan sbs sbshc container pci_slot iptable_filter ip_tables x_tables sbp2 lp fuse snd_hda_codec_analog snd_hda_intel snd_hda_codec snd_pcm_oss snd_mixer_oss snd_pcm snd_seq_dummy arc4 snd_seq_oss ecb snd_seq_midi_event thinkpad_acpi pcmcia snd_seq rfkill sr_mod cdrom iwl3945 iwlcore mac80211 snd_timer snd_seq_device ehci_hcd uhci_hcd ohci1394 pcspkr led_class yenta_socket rsrc_nonstatic evdev parport_pc serio_raw psmouse ieee1394 sg nvram pcmcia_core parport ac battery cfg80211 processor thermal button e1000e usbcore snd intel_agp soundcore snd_page_alloc
Pid: 4986, comm: rmmod Not tainted 2.6.30-rc1-00501-g6f66cbc #34 7667Y24
RIP: 0010:[<ffffffff80484b4a>]  [<ffffffff80484b4a>] nf_log_unregister+0x2a/0x90
RSP: 0018:ffff88006f91deb8  EFLAGS: 00010203
RAX: 0000000000000000 RBX: ffffffffa038d580 RCX: 0000000000000000
RDX: 0000000000000000 RSI: 0000000000000002 RDI: ffffffff8065cc40
RBP: ffff88006f91dec8 R08: 0000000000000002 R09: ffffffff80677600
R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
R13: 0000000000000880 R14: 0000000000000000 R15: 0000000000000000
FS:  0000000000000000(0000) GS:ffff880001029000(0063) knlGS:00000000f7f0b6b0
CS:  0010 DS: 002b ES: 002b CR0: 000000008005003b
CR2: 0000000000000008 CR3: 000000006f0ab000 CR4: 00000000000026a0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process rmmod (pid: 4986, threadinfo ffff88006f91c000, task ffff88006f1241a0)
Stack:
 ffff88006f91dee8 ffffffffa038d920 ffff88006f91dee8 ffffffffa038c6ca
 ffffffffa038d700 0000000000000000 ffff88006f91df78 ffffffff8026deab
 676f6c755f746265 ffffffff80229e00 0000000000000282 ffff88006f91df58
Call Trace:
 [<ffffffffa038c6ca>] ebt_ulog_fini+0x1a/0x7e [ebt_ulog]
 [<ffffffff8026deab>] sys_delete_module+0x18b/0x240
 [<ffffffff80229e00>] ? do_page_fault+0xd0/0x2a0
 [<ffffffff8022f12b>] sysenter_dispatch+0x7/0x32
Code: 90 55 48 89 e5 53 48 89 fb 48 83 ec 08 48 c7 c7 40 cc 65 80 e8 88 a9 06 00 31 c9 eb 31 0f 1f 40 00 48 8b 54 4b 18 48 8b 44 4b 20 <48> 89 42 08 48 89 10 48 c7 44 4b 20 00 02 20 00 48 c7 44 4b 18 
RIP  [<ffffffff80484b4a>] nf_log_unregister+0x2a/0x90
 RSP <ffff88006f91deb8>
CR2: 0000000000000008

(adding Bart and ebtables-devel to cc)

	Mariusz

Re: panic on rmmod of nf_conntrack_irc

[Eric Dumazet]

Mariusz Kozlowski a écrit :
> On Tue, 14 Apr 2009 14:16:37 +0200
> Patrick McHardy <kaber@trash.net> wrote:
> 
>> Eric Dumazet wrote:
>>> Patrick McHardy a écrit :
>>>> Mariusz Kozlowski wrote:
>>>>>     netfilter: nf_conntrack: use SLAB_DESTROY_BY_RCU and get rid of
>>>>> call_rcu()
>>>> Thanks for the report. Does this patch fix it?
>>>>
>>> Hi Patrick, sorry for the delay, I was in holidays.
>> No problem, me too :)
>>
>>> I should have used different fields names (from "next", "first", ...) to catch this
>>> kind of errors at compile time :(
>>  >
>>> Something like :
>> Thanks Eric. I guess at this point it doesn't really matter anymore
>> for the upstream kernel, but I'll apply your patch after getting
>> confirmation from Mariusz to make sure that people maintaining
>> external patches will notice the change (and won't send me broken
>> patches :)).
> 
> Ok good. It doesn't panic now on nf_conntrack_irc rmmod but I found another bug.
> Now it's NULL pointer dereference I when doing rmmod of ebt_log. To reproduce simply:
> 
> # modprobe ebt_log && rmmod ebt_log
> 
> This is from the mainline kernel + your (Erics) patch:
> 
> BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
> IP: [<ffffffff8048490a>] nf_log_unregister+0x2a/0x90
> PGD 6f0e8067 PUD 6a2e0067 PMD 0 
> Oops: 0002 [#1] PREEMPT SMP 
> last sysfs file: /sys/devices/LNXSYSTM:00/device:00/PNP0A08:00/device:01/PNP0C09:00/PNP0C0A:00/power_supply/BAT0/energy_full
> CPU 0 
> Modules linked in: ebt_log(-) nfs lockd auth_rpcgss sunrpc netconsole af_packet i915 drm i2c_algo_bit i2c_core kvm_intel kvm ppdev acpi_cpufreq cpufreq_stats cpufreq_powersave cpufreq_userspace cpufreq_conservative cpufreq_ondemand freq_table fan sbs sbshc container pci_slot iptable_filter ip_tables x_tables sbp2 lp fuse snd_hda_codec_analog arc4 ecb snd_hda_intel snd_hda_codec snd_pcm_oss snd_mixer_oss iwl3945 iwlcore thinkpad_acpi sr_mod cdrom pcmcia mac80211 snd_pcm rfkill led_class snd_seq_dummy snd_seq_oss snd_seq_midi_event evdev thermal uhci_hcd ehci_hcd parport_pc ohci1394 nvram yenta_socket rsrc_nonstatic pcmcia_core snd_seq pcspkr psmouse serio_raw usbcore cfg80211 e1000e ieee1394 sg parport battery ac button processor snd_timer snd_seq_device intel_agp snd soundcore snd_page_alloc
> Pid: 10249, comm: rmmod Not tainted 2.6.30-rc1-00204-gb0cbc86-dirty #33 7667Y24
> RIP: 0010:[<ffffffff8048490a>]  [<ffffffff8048490a>] nf_log_unregister+0x2a/0x90
> RSP: 0018:ffff88006a331ec8  EFLAGS: 00010207
> RAX: 0000000000000000 RBX: ffffffffa02d1d00 RCX: 0000000000000000
> RDX: 0000000000000000 RSI: 0000000000000002 RDI: ffffffff8065bb80
> RBP: ffff88006a331ed8 R08: 0000000000000002 R09: ffffffff80676600
> R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
> R13: 0000000000000880 R14: 0000000000000000 R15: 0000000000000000
> FS:  0000000000000000(0000) GS:ffff880001010000(0063) knlGS:00000000f7fc46b0
> CS:  0010 DS: 002b ES: 002b CR0: 000000008005003b
> CR2: 0000000000000008 CR3: 000000006f9f9000 CR4: 00000000000026a0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> Process rmmod (pid: 10249, threadinfo ffff88006a330000, task ffff88006f3c8000)
> Stack:
>  ffffffffa02d1e80 ffffffffa02d1e80 ffff88006a331ee8 ffffffffa02d1518
>  ffff88006a331f78 ffffffff8026ddbb 00676f6c5f746265 ffffffff80229e77
>  0000000000000282 ffff88006a331f58 ffff88006f3c8000 0000000000000000
> Call Trace:
>  [<ffffffffa02d1518>] ebt_log_fini+0x10/0x1e [ebt_log]
>  [<ffffffff8026ddbb>] sys_delete_module+0x18b/0x240
>  [<ffffffff80229e77>] ? do_page_fault+0x187/0x2a0
>  [<ffffffff8022f08b>] sysenter_dispatch+0x7/0x32
> Code: 90 55 48 89 e5 53 48 89 fb 48 83 ec 08 48 c7 c7 80 bb 65 80 e8 78 a9 06 00 31 c9 eb 31 0f 1f 40 00 48 8b 54 4b 18 48 8b 44 4b 20 <48> 89 42 08 48 89 10 48 c7 44 4b 20 00 02 20 00 48 c7 44 4b 18 
> RIP  [<ffffffff8048490a>] nf_log_unregister+0x2a/0x90
>  RSP <ffff88006a331ec8>
> CR2: 0000000000000008
> 
> If this is not related or you don't have a clue I can bisect it.
> 

Thanks a lot for this report Mariusz

Not related to my RCU patch IMHO (as it only touches conntrack)

Check commit ca735b3aaa945626ba65a3e51145bfe4ecd9e222

netfilter: use a linked list of loggers

This patch modifies nf_log to use a linked list of loggers for each
protocol. This list of loggers is read and write protected with a
mutex.

This patch separates registration and binding. To be used as
logging module, a module has to register calling nf_log_register()
and to bind to a protocol it has to call nf_log_bind_pf().
This patch also converts the logging modules to the new API. For nfnetlink_log,
it simply switchs call to register functions to call to bind function and
adds a call to nf_log_register() during init. For other modules, it just
remove a const flag from the logger structure and replace it with a
__read_mostly.

Signed-off-by: Eric Leblond <eric@inl.fr>
Signed-off-by: Patrick McHardy <kaber@trash.net>

It seems "struct list_head        list[NFPROTO_NUMPROTO];" is not initialized in "struct nf_logger" ?

Please try following patch ?

Thank you

diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index 8bb998f..d8b85ab 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -36,10 +36,14 @@ static struct nf_logger *__find_logger(int pf, const char *str_logger)
 int nf_log_register(u_int8_t pf, struct nf_logger *logger)
 {
 	const struct nf_logger *llog;
+	int i;
 
 	if (pf >= ARRAY_SIZE(nf_loggers))
 		return -EINVAL;
 
+	for (i = 0; i < NFPROTO_NUMPROTO; i++)
+		INIT_LIST_HEAD(&logger->list[i]);
+
 	mutex_lock(&nf_log_mutex);
 
 	if (pf == NFPROTO_UNSPEC) {

Re: panic on rmmod of nf_conntrack_irc

[Mariusz Kozlowski]

On Tue, 14 Apr 2009 22:14:20 +0200
Eric Dumazet <dada1@cosmosbay.com> wrote:

> Mariusz Kozlowski a écrit :
> > On Tue, 14 Apr 2009 14:16:37 +0200
> > Patrick McHardy <kaber@trash.net> wrote:
> > 
> >> Eric Dumazet wrote:
> >>> Patrick McHardy a écrit :
> >>>> Mariusz Kozlowski wrote:
> >>>>>     netfilter: nf_conntrack: use SLAB_DESTROY_BY_RCU and get rid of
> >>>>> call_rcu()
> >>>> Thanks for the report. Does this patch fix it?
> >>>>
> >>> Hi Patrick, sorry for the delay, I was in holidays.
> >> No problem, me too :)
> >>
> >>> I should have used different fields names (from "next", "first", ...) to catch this
> >>> kind of errors at compile time :(
> >>  >
> >>> Something like :
> >> Thanks Eric. I guess at this point it doesn't really matter anymore
> >> for the upstream kernel, but I'll apply your patch after getting
> >> confirmation from Mariusz to make sure that people maintaining
> >> external patches will notice the change (and won't send me broken
> >> patches :)).
> > 
> > Ok good. It doesn't panic now on nf_conntrack_irc rmmod but I found another bug.
> > Now it's NULL pointer dereference I when doing rmmod of ebt_log. To reproduce simply:
> > 
> > # modprobe ebt_log && rmmod ebt_log
> > 
> > This is from the mainline kernel + your (Erics) patch:
> > 
> > BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
> > IP: [<ffffffff8048490a>] nf_log_unregister+0x2a/0x90
> > PGD 6f0e8067 PUD 6a2e0067 PMD 0 
> > Oops: 0002 [#1] PREEMPT SMP 
> > last sysfs file: /sys/devices/LNXSYSTM:00/device:00/PNP0A08:00/device:01/PNP0C09:00/PNP0C0A:00/power_supply/BAT0/energy_full
> > CPU 0 
> > Modules linked in: ebt_log(-) nfs lockd auth_rpcgss sunrpc netconsole af_packet i915 drm i2c_algo_bit i2c_core kvm_intel kvm ppdev acpi_cpufreq cpufreq_stats cpufreq_powersave cpufreq_userspace cpufreq_conservative cpufreq_ondemand freq_table fan sbs sbshc container pci_slot iptable_filter ip_tables x_tables sbp2 lp fuse snd_hda_codec_analog arc4 ecb snd_hda_intel snd_hda_codec snd_pcm_oss snd_mixer_oss iwl3945 iwlcore thinkpad_acpi sr_mod cdrom pcmcia mac80211 snd_pcm rfkill led_class snd_seq_dummy snd_seq_oss snd_seq_midi_event evdev thermal uhci_hcd ehci_hcd parport_pc ohci1394 nvram yenta_socket rsrc_nonstatic pcmcia_core snd_seq pcspkr psmouse serio_raw usbcore cfg80211 e1000e ieee1394 sg parport battery ac button processor snd_timer snd_seq_device intel_agp snd soundcore snd_page_alloc
> > Pid: 10249, comm: rmmod Not tainted 2.6.30-rc1-00204-gb0cbc86-dirty #33 7667Y24
> > RIP: 0010:[<ffffffff8048490a>]  [<ffffffff8048490a>] nf_log_unregister+0x2a/0x90
> > RSP: 0018:ffff88006a331ec8  EFLAGS: 00010207
> > RAX: 0000000000000000 RBX: ffffffffa02d1d00 RCX: 0000000000000000
> > RDX: 0000000000000000 RSI: 0000000000000002 RDI: ffffffff8065bb80
> > RBP: ffff88006a331ed8 R08: 0000000000000002 R09: ffffffff80676600
> > R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
> > R13: 0000000000000880 R14: 0000000000000000 R15: 0000000000000000
> > FS:  0000000000000000(0000) GS:ffff880001010000(0063) knlGS:00000000f7fc46b0
> > CS:  0010 DS: 002b ES: 002b CR0: 000000008005003b
> > CR2: 0000000000000008 CR3: 000000006f9f9000 CR4: 00000000000026a0
> > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> > Process rmmod (pid: 10249, threadinfo ffff88006a330000, task ffff88006f3c8000)
> > Stack:
> >  ffffffffa02d1e80 ffffffffa02d1e80 ffff88006a331ee8 ffffffffa02d1518
> >  ffff88006a331f78 ffffffff8026ddbb 00676f6c5f746265 ffffffff80229e77
> >  0000000000000282 ffff88006a331f58 ffff88006f3c8000 0000000000000000
> > Call Trace:
> >  [<ffffffffa02d1518>] ebt_log_fini+0x10/0x1e [ebt_log]
> >  [<ffffffff8026ddbb>] sys_delete_module+0x18b/0x240
> >  [<ffffffff80229e77>] ? do_page_fault+0x187/0x2a0
> >  [<ffffffff8022f08b>] sysenter_dispatch+0x7/0x32
> > Code: 90 55 48 89 e5 53 48 89 fb 48 83 ec 08 48 c7 c7 80 bb 65 80 e8 78 a9 06 00 31 c9 eb 31 0f 1f 40 00 48 8b 54 4b 18 48 8b 44 4b 20 <48> 89 42 08 48 89 10 48 c7 44 4b 20 00 02 20 00 48 c7 44 4b 18 
> > RIP  [<ffffffff8048490a>] nf_log_unregister+0x2a/0x90
> >  RSP <ffff88006a331ec8>
> > CR2: 0000000000000008
> > 
> > If this is not related or you don't have a clue I can bisect it.
> > 
> 
> Thanks a lot for this report Mariusz
> 
> Not related to my RCU patch IMHO (as it only touches conntrack)
> 
> Check commit ca735b3aaa945626ba65a3e51145bfe4ecd9e222
> 
> netfilter: use a linked list of loggers
> 
> This patch modifies nf_log to use a linked list of loggers for each
> protocol. This list of loggers is read and write protected with a
> mutex.
> 
> This patch separates registration and binding. To be used as
> logging module, a module has to register calling nf_log_register()
> and to bind to a protocol it has to call nf_log_bind_pf().
> This patch also converts the logging modules to the new API. For nfnetlink_log,
> it simply switchs call to register functions to call to bind function and
> adds a call to nf_log_register() during init. For other modules, it just
> remove a const flag from the logger structure and replace it with a
> __read_mostly.
> 
> Signed-off-by: Eric Leblond <eric@inl.fr>
> Signed-off-by: Patrick McHardy <kaber@trash.net>
> 
> It seems "struct list_head        list[NFPROTO_NUMPROTO];" is not initialized in "struct nf_logger" ?
> 
> Please try following patch ?

Bingo! Your patch fixes it. BTW this was happening for a few more modules - ebt_log, ebt_ulog, ipt_LOG,
and probably others using nf_log_register/nf_log_unregister.

> Thank you

Thanks,

	Mariusz


> diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
> index 8bb998f..d8b85ab 100644
> --- a/net/netfilter/nf_log.c
> +++ b/net/netfilter/nf_log.c
> @@ -36,10 +36,14 @@ static struct nf_logger *__find_logger(int pf, const char *str_logger)
>  int nf_log_register(u_int8_t pf, struct nf_logger *logger)
>  {
>  	const struct nf_logger *llog;
> +	int i;
>  
>  	if (pf >= ARRAY_SIZE(nf_loggers))
>  		return -EINVAL;
>  
> +	for (i = 0; i < NFPROTO_NUMPROTO; i++)
> +		INIT_LIST_HEAD(&logger->list[i]);
> +
>  	mutex_lock(&nf_log_mutex);
>  
>  	if (pf == NFPROTO_UNSPEC) {
> 

Re: panic on rmmod of nf_conntrack_irc

[Eric Leblond]

[-- Attachment #1: Type: text/plain, Size: 1189 bytes --]

Hi,

Le mardi 14 avril 2009 à 22:14 +0200, Eric Dumazet a écrit :
> Mariusz Kozlowski a écrit :
> > On Tue, 14 Apr 2009 14:16:37 +0200
> > Patrick McHardy <kaber@trash.net> wrote:
> > 
> >> Eric Dumazet wrote:
> >>> Patrick McHardy a écrit :
> >>>> Mariusz Kozlowski wrote:
> >>>>>     netfilter: nf_conntrack: use SLAB_DESTROY_BY_RCU and get rid of
> >>>>> call_rcu()
> >>>> Thanks for the report. Does this patch fix it?
> >>>>
> >>> Hi Patrick, sorry for the delay, I was in holidays.
> >> No problem, me too :)
> >>
...

> 
> Check commit ca735b3aaa945626ba65a3e51145bfe4ecd9e222
> 
> netfilter: use a linked list of loggers

...

> Signed-off-by: Eric Leblond <eric@inl.fr>
> Signed-off-by: Patrick McHardy <kaber@trash.net>
> 
> It seems "struct list_head        list[NFPROTO_NUMPROTO];" is not initialized in "struct nf_logger" ?
> 
> Please try following patch ?

I've just tested your patch. Without it, I was able to trigger the bug
(modprobe ebt_ulog ; rmmod ebt_ulog). All run cleanly with it.

All my apologies for having forgot to do this test.

BR,
-- 
Eric Leblond <eric@inl.fr>
INL: http://www.inl.fr/
NuFW: http://www.nufw.org/

[-- Attachment #2: Ceci est une partie de message numériquement signée --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[PATCH] netfilter: nf_log fix

[Eric Dumazet]

Eric Leblond a écrit :
> Hi,
> 
> Le mardi 14 avril 2009 à 22:14 +0200, Eric Dumazet a écrit :
>> Mariusz Kozlowski a écrit :
>>> On Tue, 14 Apr 2009 14:16:37 +0200
>>> Patrick McHardy <kaber@trash.net> wrote:
>>>
>>>> Eric Dumazet wrote:
>>>>> Patrick McHardy a écrit :
>>>>>> Mariusz Kozlowski wrote:
>>>>>>>     netfilter: nf_conntrack: use SLAB_DESTROY_BY_RCU and get rid of
>>>>>>> call_rcu()
>>>>>> Thanks for the report. Does this patch fix it?
>>>>>>
>>>>> Hi Patrick, sorry for the delay, I was in holidays.
>>>> No problem, me too :)
>>>>
> ...
> 
>> Check commit ca735b3aaa945626ba65a3e51145bfe4ecd9e222
>>
>> netfilter: use a linked list of loggers
> 
> ...
> 
>> Signed-off-by: Eric Leblond <eric@inl.fr>
>> Signed-off-by: Patrick McHardy <kaber@trash.net>
>>
>> It seems "struct list_head        list[NFPROTO_NUMPROTO];" is not initialized in "struct nf_logger" ?
>>
>> Please try following patch ?
> 
> I've just tested your patch. Without it, I was able to trigger the bug
> (modprobe ebt_ulog ; rmmod ebt_ulog). All run cleanly with it.
> 

OK thanks everybody, I submit it more formally then, using ARRAY_SIZE() macro too :)

[PATCH] netfilter: nf_log fix

commit ca735b3aaa945626ba65a3e51145bfe4ecd9e222
'netfilter: use a linked list of loggers'
introduced an array of list_head in "struct nf_logger", but
forgot to initialize it in nf_log_register(). This resulted
in oops when calling nf_log_unregister() at module unload time.

Reported-and-tested-by: Mariusz Kozlowski <m.kozlowski@tuxland.pl>
Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>
Acked-by: Eric Leblond <eric@inl.fr>

diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index 8bb998f..d8b85ab 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -36,10 +36,14 @@ static struct nf_logger *__find_logger(int pf, const char *str_logger)
 int nf_log_register(u_int8_t pf, struct nf_logger *logger)
 {
 	const struct nf_logger *llog;
+	int i;
 
 	if (pf >= ARRAY_SIZE(nf_loggers))
 		return -EINVAL;
 
+	for (i = 0; i < ARRAY_SIZE(logger->list); i++)
+		INIT_LIST_HEAD(&logger->list[i]);
+
 	mutex_lock(&nf_log_mutex);
 
 	if (pf == NFPROTO_UNSPEC) {

Re: [PATCH] netfilter: nf_log fix

[Patrick McHardy]

Eric Dumazet wrote:
> OK thanks everybody, I submit it more formally then, using ARRAY_SIZE() macro too :)
> 
> [PATCH] netfilter: nf_log fix
> 
> commit ca735b3aaa945626ba65a3e51145bfe4ecd9e222
> 'netfilter: use a linked list of loggers'
> introduced an array of list_head in "struct nf_logger", but
> forgot to initialize it in nf_log_register(). This resulted
> in oops when calling nf_log_unregister() at module unload time.
> 
> Reported-and-tested-by: Mariusz Kozlowski <m.kozlowski@tuxland.pl>
> Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>
> Acked-by: Eric Leblond <eric@inl.fr>

Applied, thanks everyone.

Re: panic on rmmod of nf_conntrack_irc

[Patrick McHardy]

Eric Dumazet wrote:
> I should have used different fields names (from "next", "first", ...) to catch this
> kind of errors at compile time :(
> 
> Something like :
> 
> diff --git a/include/linux/list_nulls.h b/include/linux/list_nulls.h

Eric, on second thought, could you please resubmit this to Dave?
It touches too many files outside of netfilter to go through my
tree.

Re: panic on rmmod of nf_conntrack_irc

[Eric Dumazet]

Patrick McHardy a écrit :
> Eric Dumazet wrote:
>> I should have used different fields names (from "next", "first", ...)
>> to catch this
>> kind of errors at compile time :(
>>
>> Something like :
>>
>> diff --git a/include/linux/list_nulls.h b/include/linux/list_nulls.h
> 
> Eric, on second thought, could you please resubmit this to Dave?
> It touches too many files outside of netfilter to go through my
> tree.
> 
> 

Sure, but its a cleanup (and was an RFC to show my point)
so should wait a litle bit until David opens his -next tree (for 2.6.31)

Please just submit your patch for 2.6.30 ?

Re: panic on rmmod of nf_conntrack_irc

[Patrick McHardy]

Eric Dumazet wrote:
> Patrick McHardy a écrit :
>> Eric, on second thought, could you please resubmit this to Dave?
>> It touches too many files outside of netfilter to go through my
>> tree.
> 
> Sure, but its a cleanup (and was an RFC to show my point)
> so should wait a litle bit until David opens his -next tree (for 2.6.31)

Fine with me :)

> Please just submit your patch for 2.6.30 ?

OK, I'll do that.

Re: panic on rmmod of nf_conntrack_irc

[David Miller]

From: Eric Dumazet <dada1@cosmosbay.com>
Date: Wed, 15 Apr 2009 12:42:27 +0200

> Sure, but its a cleanup (and was an RFC to show my point)
> so should wait a litle bit until David opens his -next tree (for 2.6.31)

The net-next-2.6 tree is open for business since 2.6.30-rc1 :-)

> Please just submit your patch for 2.6.30 ?

I'll let Patrick voice his opinion on this, I don't care either
way.