[PATCH 2.6.20 00/10] nfnetlink_log: patch series introduction

[PATCH 2.6.20 00/10] nfnetlink_log: patch series introduction

[Michał Mirosław]

Dear list,

After meeting a faint-hearted Linux kernel lately I decided to try myself
at persuading it to not be afraid of NFLOG. This chat gave a series of
ten commandments I present today. Take a look and agree or comment.

Here's the list:

  1. nfulnl_log_packet() - don't count the same thing twice
* 2. nfulnl_log_packet() - don't leak references
  3. nfulnl_log_packet() - don't copy-and-paste code
* 4. nfulnl_timer() - don't touch freed memory
  5. nfulnl_recv_config() - don't free what isn't there
  6. nfulnl_recv_config() - don't touch dead animals
* 7. instance_destroy() - remember of your appointments
  8. instance_create() - don't lock the bookshelf for too long
! 9. __build_packet_message() - don't forget your tail
 10. instance_create() - wear your watch when going out

Ninth is THE one. Those marked with stars are important I believe.
Others came out on the way. I'll send them one-in-a-mail right after
this message. I'm CC:ing this to LKML, so someone could put the important
ones in -stable maybe?

Have a nice reading,
Michal Miroslaw

PS. Please CC: me if you need my answer as I read the list's archive
only and I am not subscribed.

Re: [PATCH 2.6.20 +0/14] nfnetlink_log: patch series season 2

[Michał Mirosław]

Dear list,

As it turned out, not all worms eating nfnetlink_log have been exterminated
by my last patch series. I'll append next four patches to the end of the
series and I hope that it doesn't make your patching scripts unhappy.

Those patches fix two bugs and make two other code beautifications:

  11. procfs file handling - don't pass seq_file when you don't have to
* 12. nfulnl_recv_config() - don't modify what isn't there
* 13. __nfulnl_send() and friends - return your books timely
  14. __nfulnl_send() - don't prove the obvious

There are some other bugs I found that I'm looking for a fix. One of them
is wrong /proc/net/netfilter/nfnetlink_log contents:

natownica:~# cat /proc/net/netfilter/nfnetlink_log
    0  -4100     0 2 65535    100  1
    2  -4099     2 2 65535    100  2
    4  15355     0 2 65535    100  1

Those three entries are created by a single ulogd2 listening in three
packet logging groups. I believe that's some problem with generating
the file contents because after shutting down ulogd all disappear.

The two groups: 2, 4 are stuffed with packets by those iptables rules:

natownica:~# iptables-save |grep NFLOG
-A LOG_and_DROP_fakenet -m hashlimit --hashlimit 1/sec --hashlimit-mode \
	srcip --hashlimit-name fw_fakenet_src -j NFLOG --nflog-prefix \
	"fakenet" --nflog-group 2 --nflog-threshold 30
-A LOG_and_DROP_p2p -m hashlimit --hashlimit 1/sec --hashlimit-mode srcip \
	--hashlimit-name fw_p2p_src -j NFLOG --nflog-prefix "p2p" \
	--nflog-group 2 --nflog-threshold 30
-A invalid -m mark --mark 0x3000/0x3000 -j NFLOG --nflog-prefix \
	"nonregistered" --nflog-group 3
-A invalid -j NFLOG --nflog-prefix "invalid" --nflog-group 2

As you can see, there's no group 4 among the rules - 3 is. This seems to
be xt_NFLOG's fault, but I haven't looked there yet.

Greets,
Michal Miroslaw

[PATCH 2.6.20 11/14] nfnetlink_log: iterator functions need iter_state * only

[Michał Mirosław]

get_*() don't need access to seq_file - iter_state is enough for them.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>

--- linux-2.6.20/net/netfilter/nfnetlink_log.c.9	2007-02-12 00:19:16.000000000 +0100
+++ linux-2.6.20/net/netfilter/nfnetlink_log.c	2007-02-12 17:05:14.000000000 +0100
@@ -929,10 +929,8 @@ struct iter_state {
 	unsigned int bucket;
 };
 
-static struct hlist_node *get_first(struct seq_file *seq)
+static struct hlist_node *get_first(struct iter_state *st)
 {
-	struct iter_state *st = seq->private;
-
 	if (!st)
 		return NULL;
 
@@ -943,10 +941,8 @@ static struct hlist_node *get_first(stru
 	return NULL;
 }
 
-static struct hlist_node *get_next(struct seq_file *seq, struct hlist_node *h)
+static struct hlist_node *get_next(struct iter_state *st, struct hlist_node *h)
 {
-	struct iter_state *st = seq->private;
-
 	h = h->next;
 	while (!h) {
 		if (++st->bucket >= INSTANCE_BUCKETS)
@@ -957,13 +953,13 @@ static struct hlist_node *get_next(struc
 	return h;
 }
 
-static struct hlist_node *get_idx(struct seq_file *seq, loff_t pos)
+static struct hlist_node *get_idx(struct iter_state *st, loff_t pos)
 {
 	struct hlist_node *head;
-	head = get_first(seq);
+	head = get_first(st);
 
 	if (head)
-		while (pos && (head = get_next(seq, head)))
+		while (pos && (head = get_next(st, head)))
 			pos--;
 	return pos ? NULL : head;
 }
@@ -971,13 +967,13 @@ static struct hlist_node *get_idx(struct
 static void *seq_start(struct seq_file *seq, loff_t *pos)
 {
 	read_lock_bh(&instances_lock);
-	return get_idx(seq, *pos);
+	return get_idx(seq->private, *pos);
 }
 
 static void *seq_next(struct seq_file *s, void *v, loff_t *pos)
 {
 	(*pos)++;
-	return get_next(s, v);
+	return get_next(s->private, v);
 }
 
 static void seq_stop(struct seq_file *s, void *v)

[PATCH 2.6.20 12/14] nfnetlink_log: possible NULL pointer dereference in nfulnl_recv_config()

[Michał Mirosław]

Eliminate possible NULL pointer dereference in nfulnl_recv_config().

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>

--- linux-2.6.20/net/netfilter/nfnetlink_log.c.10	2007-02-12 17:05:14.000000000 +0100
+++ linux-2.6.20/net/netfilter/nfnetlink_log.c	2007-02-12 17:35:50.000000000 +0100
@@ -853,6 +853,9 @@ nfulnl_recv_config(struct sock *ctnl, st
 			ret = -EINVAL;
 			break;
 		}
+
+		if (!inst)
+			goto out_null;
 	} else {
 		if (!inst) {
 			UDEBUG("no config command, and no instance for "

[PATCH 2.6.20 13/14] nfnetlink_log: fix reference counting

[Michał Mirosław]

Fix reference counting (memory leak) problem in __nfulnl_send() and callers
related to packet queueing.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>

--- linux-2.6.20/net/netfilter/nfnetlink_log.c.11	2007-02-12 17:35:50.000000000 +0100
+++ linux-2.6.20/net/netfilter/nfnetlink_log.c	2007-02-12 17:58:01.000000000 +0100
@@ -223,6 +223,11 @@ _instance_destroy2(struct nfulnl_instanc
 
 	spin_lock_bh(&inst->lock);
 	if (inst->skb) {
+		/* timer "holds" one reference (we have one more) */
+		if (timer_pending(&inst->timer)) {
+			del_timer(&inst->timer);
+			instance_put(inst);
+		}
 		if (inst->qlen)
 			__nfulnl_send(inst);
 		if (inst->skb) {
@@ -370,9 +375,6 @@ __nfulnl_send(struct nfulnl_instance *in
 {
 	int status;
 
-	if (timer_pending(&inst->timer))
-		del_timer(&inst->timer);
-
 	if (!inst->skb)
 		return 0;
 
@@ -399,6 +401,8 @@ static void nfulnl_timer(unsigned long d
 	UDEBUG("timer function called, flushing buffer\n");
 
 	spin_lock_bh(&inst->lock);
+	if (timer_pending(&inst->timer))	/* is it always true or false here? */
+		del_timer(&inst->timer);
 	__nfulnl_send(inst);
 	spin_unlock_bh(&inst->lock);
 	instance_put(inst);
@@ -683,6 +687,11 @@ nfulnl_log_packet(unsigned int pf,
 		 * enough room in the skb left. flush to userspace. */
 		UDEBUG("flushing old skb\n");
 
+		/* timer "holds" one reference (we have another one) */
+		if (timer_pending(&inst->timer)) {
+			del_timer(&inst->timer);
+			instance_put(inst);
+		}
 		__nfulnl_send(inst);
 	}
 

[PATCH 2.6.20 14/14] nfnetlink_log: micro-optimization: inst->skb != NULL in __nfulnl_send()

[Michał Mirosław]

No other function calls __nfulnl_send() with inst->skb == NULL than nfulnl_timer().

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>

--- linux-2.6.20/net/netfilter/nfnetlink_log.c.12	2007-02-12 17:58:01.000000000 +0100
+++ linux-2.6.20/net/netfilter/nfnetlink_log.c	2007-02-12 17:58:56.000000000 +0100
@@ -375,9 +375,6 @@ __nfulnl_send(struct nfulnl_instance *in
 {
 	int status;
 
-	if (!inst->skb)
-		return 0;
-
 	if (inst->qlen > 1)
 		inst->lastnlh->nlmsg_type = NLMSG_DONE;
 
@@ -403,7 +400,8 @@ static void nfulnl_timer(unsigned long d
 	spin_lock_bh(&inst->lock);
 	if (timer_pending(&inst->timer))	/* is it always true or false here? */
 		del_timer(&inst->timer);
-	__nfulnl_send(inst);
+	if (inst->skb)
+		__nfulnl_send(inst);
 	spin_unlock_bh(&inst->lock);
 	instance_put(inst);
 }

Re: [PATCH 2.6.20 11/14] nfnetlink_log: iterator functions need iter_state * only

[Patrick McHardy]

Micha³ Miros³aw wrote:
> get_*() don't need access to seq_file - iter_state is enough for them.

Applied, thanks.

Re: [PATCH 2.6.20 12/14] nfnetlink_log: possible NULL pointer dereference in nfulnl_recv_config()

[Patrick McHardy]

Micha³ Miros³aw wrote:
> Eliminate possible NULL pointer dereference in nfulnl_recv_config().
> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> 
> --- linux-2.6.20/net/netfilter/nfnetlink_log.c.10	2007-02-12 17:05:14.000000000 +0100
> +++ linux-2.6.20/net/netfilter/nfnetlink_log.c	2007-02-12 17:35:50.000000000 +0100
> @@ -853,6 +853,9 @@ nfulnl_recv_config(struct sock *ctnl, st
>  			ret = -EINVAL;
>  			break;
>  		}
> +
> +		if (!inst)
> +			goto out_null;

I think we should check that an instance is present before doing
any changes any return an error if the user tries to change the
configuration for a non-existant instance.

Re: [PATCH 2.6.20 13/14] nfnetlink_log: fix reference counting

[Patrick McHardy]

Micha³ Miros³aw wrote:
> Fix reference counting (memory leak) problem in __nfulnl_send() and callers
> related to packet queueing.
> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> 
> --- linux-2.6.20/net/netfilter/nfnetlink_log.c.11	2007-02-12 17:35:50.000000000 +0100
> +++ linux-2.6.20/net/netfilter/nfnetlink_log.c	2007-02-12 17:58:01.000000000 +0100
> @@ -223,6 +223,11 @@ _instance_destroy2(struct nfulnl_instanc
>  
>  	spin_lock_bh(&inst->lock);
>  	if (inst->skb) {
> +		/* timer "holds" one reference (we have one more) */
> +		if (timer_pending(&inst->timer)) {
> +			del_timer(&inst->timer);
> +			instance_put(inst);

This should be done outside of the locked section and using
del_timer_sync to make sure the timer is not already active
and waiting for the lock.

Please combine this with 07/10 if possible.

Re: [PATCH 2.6.20 13/14] nfnetlink_log: fix reference counting

[Michał Mirosław]

On Tue, Feb 13, 2007 at 01:58:34PM +0100, Patrick McHardy wrote:
> Micha³ Miros³aw wrote:
> > Fix reference counting (memory leak) problem in __nfulnl_send() and callers
> > related to packet queueing.
> > 
> > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > 
> > --- linux-2.6.20/net/netfilter/nfnetlink_log.c.11	2007-02-12 17:35:50.000000000 +0100
> > +++ linux-2.6.20/net/netfilter/nfnetlink_log.c	2007-02-12 17:58:01.000000000 +0100
> > @@ -223,6 +223,11 @@ _instance_destroy2(struct nfulnl_instanc
> >  
> >  	spin_lock_bh(&inst->lock);
> >  	if (inst->skb) {
> > +		/* timer "holds" one reference (we have one more) */
> > +		if (timer_pending(&inst->timer)) {
> > +			del_timer(&inst->timer);
> > +			instance_put(inst);
> This should be done outside of the locked section and using
> del_timer_sync to make sure the timer is not already active
> and waiting for the lock.

I found another solution, as this won't work (I explained the case in my
reply to your comment on 07/10). I noticed that we should just check
what del_timer() returns instead of timer_pending().

Heres the patch:

Fix reference counting (memory leak) problem in __nfulnl_send() and callers
related to packet queueing.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>

--- linux-2.6.20/net/netfilter/nfnetlink_log.c.11	2007-02-12 17:35:50.000000000 +0100
+++ linux-2.6.20/net/netfilter/nfnetlink_log.c.12	2007-02-14 12:27:09.000000000 +0100
@@ -223,6 +223,9 @@ _instance_destroy2(struct nfulnl_instanc
 
 	spin_lock_bh(&inst->lock);
 	if (inst->skb) {
+		/* timer "holds" one reference (we have one more) */
+		if (del_timer(&inst->timer))
+			instance_put(inst);
 		if (inst->qlen)
 			__nfulnl_send(inst);
 		if (inst->skb) {
@@ -370,9 +373,6 @@ __nfulnl_send(struct nfulnl_instance *in
 {
 	int status;
 
-	if (timer_pending(&inst->timer))
-		del_timer(&inst->timer);
-
 	if (!inst->skb)
 		return 0;
 
@@ -683,6 +683,9 @@ nfulnl_log_packet(unsigned int pf,
 		 * enough room in the skb left. flush to userspace. */
 		UDEBUG("flushing old skb\n");
 
+		/* timer "holds" one reference (we have another one) */
+		if (del_timer(&inst->timer))
+			instance_put(inst);
 		__nfulnl_send(inst);
 	}
 

Re: [PATCH 2.6.20 14/14] nfnetlink_log: micro-optimization: inst->skb != NULL in __nfulnl_send()

[Michał Mirosław]

Patch updated to apply after a new version of 13/14:

No other function calls __nfulnl_send() with inst->skb == NULL than nfulnl_timer().

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>

--- linux-2.6.20/net/netfilter/nfnetlink_log.c.12	2007-02-14 12:27:09.000000000 +0100
+++ linux-2.6.20/net/netfilter/nfnetlink_log.c	2007-02-14 12:53:04.000000000 +0100
@@ -373,9 +373,6 @@ __nfulnl_send(struct nfulnl_instance *in
 {
 	int status;
 
-	if (!inst->skb)
-		return 0;
-
 	if (inst->qlen > 1)
 		inst->lastnlh->nlmsg_type = NLMSG_DONE;
 
@@ -399,7 +396,8 @@ static void nfulnl_timer(unsigned long d
 	UDEBUG("timer function called, flushing buffer\n");
 
 	spin_lock_bh(&inst->lock);
-	__nfulnl_send(inst);
+	if (inst->skb)
+		__nfulnl_send(inst);
 	spin_unlock_bh(&inst->lock);
 	instance_put(inst);
 }