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.