* [PATCH 2.6.20 11/14] nfnetlink_log: iterator functions need iter_state * only
2007-02-12 20:20 ` [PATCH 2.6.20 +0/14] nfnetlink_log: patch series season 2 Michał Mirosław
@ 2007-02-12 20:22 ` Michał Mirosław
2007-02-13 12:51 ` Patrick McHardy
2007-02-12 20:22 ` [PATCH 2.6.20 12/14] nfnetlink_log: possible NULL pointer dereference in nfulnl_recv_config() Michał Mirosław
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Michał Mirosław @ 2007-02-12 20:22 UTC (permalink / raw)
To: netfilter-devel; +Cc: linux-kernel
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)
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2.6.20 12/14] nfnetlink_log: possible NULL pointer dereference in nfulnl_recv_config()
2007-02-12 20:20 ` [PATCH 2.6.20 +0/14] nfnetlink_log: patch series season 2 Michał Mirosław
2007-02-12 20:22 ` [PATCH 2.6.20 11/14] nfnetlink_log: iterator functions need iter_state * only Michał Mirosław
@ 2007-02-12 20:22 ` Michał Mirosław
2007-02-13 12:55 ` Patrick McHardy
2007-02-12 20:22 ` [PATCH 2.6.20 13/14] nfnetlink_log: fix reference counting Michał Mirosław
2007-02-12 20:23 ` [PATCH 2.6.20 14/14] nfnetlink_log: micro-optimization: inst->skb != NULL in __nfulnl_send() Michał Mirosław
3 siblings, 1 reply; 11+ messages in thread
From: Michał Mirosław @ 2007-02-12 20:22 UTC (permalink / raw)
To: netfilter-devel; +Cc: linux-kernel
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 "
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2.6.20 12/14] nfnetlink_log: possible NULL pointer dereference in nfulnl_recv_config()
2007-02-12 20:22 ` [PATCH 2.6.20 12/14] nfnetlink_log: possible NULL pointer dereference in nfulnl_recv_config() Michał Mirosław
@ 2007-02-13 12:55 ` Patrick McHardy
0 siblings, 0 replies; 11+ messages in thread
From: Patrick McHardy @ 2007-02-13 12:55 UTC (permalink / raw)
To: Michał Mirosław; +Cc: netfilter-devel, linux-kernel
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2.6.20 13/14] nfnetlink_log: fix reference counting
2007-02-12 20:20 ` [PATCH 2.6.20 +0/14] nfnetlink_log: patch series season 2 Michał Mirosław
2007-02-12 20:22 ` [PATCH 2.6.20 11/14] nfnetlink_log: iterator functions need iter_state * only Michał Mirosław
2007-02-12 20:22 ` [PATCH 2.6.20 12/14] nfnetlink_log: possible NULL pointer dereference in nfulnl_recv_config() Michał Mirosław
@ 2007-02-12 20:22 ` Michał Mirosław
2007-02-13 12:58 ` Patrick McHardy
2007-02-12 20:23 ` [PATCH 2.6.20 14/14] nfnetlink_log: micro-optimization: inst->skb != NULL in __nfulnl_send() Michał Mirosław
3 siblings, 1 reply; 11+ messages in thread
From: Michał Mirosław @ 2007-02-12 20:22 UTC (permalink / raw)
To: netfilter-devel; +Cc: linux-kernel
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);
}
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2.6.20 13/14] nfnetlink_log: fix reference counting
2007-02-12 20:22 ` [PATCH 2.6.20 13/14] nfnetlink_log: fix reference counting Michał Mirosław
@ 2007-02-13 12:58 ` Patrick McHardy
2007-02-14 11:38 ` Michał Mirosław
0 siblings, 1 reply; 11+ messages in thread
From: Patrick McHardy @ 2007-02-13 12:58 UTC (permalink / raw)
To: Michał Mirosław; +Cc: netfilter-devel, linux-kernel
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2.6.20 13/14] nfnetlink_log: fix reference counting
2007-02-13 12:58 ` Patrick McHardy
@ 2007-02-14 11:38 ` Michał Mirosław
0 siblings, 0 replies; 11+ messages in thread
From: Michał Mirosław @ 2007-02-14 11:38 UTC (permalink / raw)
To: netfilter-devel; +Cc: linux-kernel
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);
}
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2.6.20 14/14] nfnetlink_log: micro-optimization: inst->skb != NULL in __nfulnl_send()
2007-02-12 20:20 ` [PATCH 2.6.20 +0/14] nfnetlink_log: patch series season 2 Michał Mirosław
` (2 preceding siblings ...)
2007-02-12 20:22 ` [PATCH 2.6.20 13/14] nfnetlink_log: fix reference counting Michał Mirosław
@ 2007-02-12 20:23 ` Michał Mirosław
2007-02-14 11:57 ` Michał Mirosław
3 siblings, 1 reply; 11+ messages in thread
From: Michał Mirosław @ 2007-02-12 20:23 UTC (permalink / raw)
To: netfilter-devel; +Cc: linux-kernel
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);
}
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2.6.20 14/14] nfnetlink_log: micro-optimization: inst->skb != NULL in __nfulnl_send()
2007-02-12 20:23 ` [PATCH 2.6.20 14/14] nfnetlink_log: micro-optimization: inst->skb != NULL in __nfulnl_send() Michał Mirosław
@ 2007-02-14 11:57 ` Michał Mirosław
0 siblings, 0 replies; 11+ messages in thread
From: Michał Mirosław @ 2007-02-14 11:57 UTC (permalink / raw)
To: netfilter-devel; +Cc: linux-kernel
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);
}
^ permalink raw reply [flat|nested] 11+ messages in thread