Re: [PATCH 2.6.20 13/14] nfnetlink_log: fix reference counting
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 <[EMAIL PROTECTED]> > > > > --- linux-2.6.20/net/netfilter/nfnetlink_log.c.11 2007-02-12 > > 17:35:50.0 +0100 > > +++ linux-2.6.20/net/netfilter/nfnetlink_log.c 2007-02-12 > > 17:58:01.0 +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 <[EMAIL PROTECTED]> --- linux-2.6.20/net/netfilter/nfnetlink_log.c.11 2007-02-12 17:35:50.0 +0100 +++ linux-2.6.20/net/netfilter/nfnetlink_log.c.12 2007-02-14 12:27:09.0 +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); } - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2.6.20 13/14] nfnetlink_log: fix reference counting
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 <[EMAIL PROTECTED]> > > --- linux-2.6.20/net/netfilter/nfnetlink_log.c.11 2007-02-12 > 17:35:50.0 +0100 > +++ linux-2.6.20/net/netfilter/nfnetlink_log.c2007-02-12 > 17:58:01.0 +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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2.6.20 13/14] nfnetlink_log: fix reference counting
Fix reference counting (memory leak) problem in __nfulnl_send() and callers related to packet queueing. Signed-off-by: Michał Mirosław <[EMAIL PROTECTED]> --- linux-2.6.20/net/netfilter/nfnetlink_log.c.11 2007-02-12 17:35:50.0 +0100 +++ linux-2.6.20/net/netfilter/nfnetlink_log.c 2007-02-12 17:58:01.0 +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); } - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/