Re: [PATCH 2.6.20 07/10] nfnetlink_log: fix module reference counting

[Patrick McHardy <kaber@trash.net>]

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

Micha³ Miros³aw wrote:
> On Tue, Feb 13, 2007 at 01:48:03PM +0100, Patrick McHardy wrote:
> 
>>I think we should just cancel the timer on destruction.
> 
> 
> It won't solve a race between destroying the instance and logging a
> packet. It could happen that:
> 
> CPU1				CPU2
> 				nfulnl_log_packet()
> 				 -> instance_lookup_get()
> 				 -> lock inst
> 				 ** got lock on inst
> _instance_destroy(inst)
>  -> remove inst from list
>  -> remove timer and unref inst
>  -> lock inst
>  				 -> start a timer
> 				 -> unlock inst
>  ** got lock on inst
>  -> clear the skb
>  -> unlock inst
>  -> unref inst
>  -> unref module... but we still have a timer pending!


This is easily fixable by adding a synchronize_rcu() call
after removing the instance from the global list.
nfulnl_log_packet() is called within a RCU read-side
critical section, so once synchronize_rcu() returns we're
guaranteed no CPU is within nfulnl_log_packet anymore with
this instance. And I think it is really preferable to having
the timer armed after destroying the instance.

This patch should take care of both the module reference
problem and the refcount leak.


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

diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
index 8a460b1..1d45ff6 100644
--- a/net/netfilter/nfnetlink_log.c
+++ b/net/netfilter/nfnetlink_log.c
@@ -213,6 +213,9 @@ _instance_destroy2(struct nfulnl_instanc
 	if (lock)
 		write_unlock_bh(&instances_lock);
 
+	sychronize_rcu();
+	if (del_timer_sync(&inst->timer))
+		instance_put(inst);
 	/* then flush all pending packets from skb */
 
 	spin_lock_bh(&inst->lock);
@@ -363,9 +366,6 @@ __nfulnl_send(struct nfulnl_instance *in
 {
 	int status;
 
-	if (timer_pending(&inst->timer))
-		del_timer(&inst->timer);
-
 	if (!inst->skb)
 		return 0;
 
@@ -676,6 +676,9 @@ #endif
 		 * enough room in the skb left. flush to userspace. */
 		UDEBUG("flushing old skb\n");
 
+		if (del_timer(&inst->timer))
+			instance_put(inst);
+
 		__nfulnl_send(inst);
 	}