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(>lock);
> > if (inst->skb) {
> > + /* timer "holds" one reference (we have one more) */
> > + if (timer_pending(>timer)) {
> > + del_timer(>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(>lock);
if (inst->skb) {
+ /* timer "holds" one reference (we have one more) */
+ if (del_timer(>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(>timer))
- del_timer(>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(>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
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(>lock);
> if (inst->skb) {
> + /* timer "holds" one reference (we have one more) */
> + if (timer_pending(>timer)) {
> + del_timer(>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/
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(>lock);
if (inst->skb) {
+ /* timer "holds" one reference (we have one more) */
+ if (timer_pending(>timer)) {
+ del_timer(>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(>timer))
- del_timer(>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(>lock);
+ if (timer_pending(>timer))/* is it always true or false
here? */
+ del_timer(>timer);
__nfulnl_send(inst);
spin_unlock_bh(>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(>timer)) {
+ del_timer(>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/
[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/
