[PATCH] netfilter: add locking for counters zeroing

[PATCH] netfilter: add locking for counters zeroing

[Krzysztof Piotr Oledzki]

>From 4407c0b11dde5235b1141ef63bc29f322a73c873 Mon Sep 17 00:00:00 2001
From: Krzysztof Piotr Oledzki <ole@ans.pl>
Date: Mon, 21 Jul 2008 17:20:45 +0200
Subject: netfilter: add locking for counters zeroing

The memset inside ctnetlink_dump_table() fuction needs locking.
The lock shoud be grabbed outside the loop to avoid repeatedly
taking and releasing it again.

Also add similar locking inside xt_connbytes match where
the counters get read.

Signed-off-by: Krzysztof Piotr Oledzki <ole@ans.pl>
---
 net/netfilter/nf_conntrack_netlink.c |    2 ++
 net/netfilter/xt_connbytes.c         |    2 ++
 2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index 9432da4..ff1bbb0 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -545,6 +545,7 @@ ctnetlink_dump_table(struct sk_buff *skb, struct netlink_callback *cb)
 	u_int8_t l3proto = nfmsg->nfgen_family;
 
 	rcu_read_lock();
+	spin_lock_bh(&nf_conntrack_lock);
 	last = (struct nf_conn *)cb->args[1];
 	for (; cb->args[0] < nf_conntrack_htable_size; cb->args[0]++) {
 restart:
@@ -588,6 +589,7 @@ restart:
 		}
 	}
 out:
+	spin_unlock_bh(&nf_conntrack_lock);
 	rcu_read_unlock();
 	if (last)
 		nf_ct_put(last);
diff --git a/net/netfilter/xt_connbytes.c b/net/netfilter/xt_connbytes.c
index 3e39c4f..9d0253a 100644
--- a/net/netfilter/xt_connbytes.c
+++ b/net/netfilter/xt_connbytes.c
@@ -38,6 +38,7 @@ connbytes_mt(const struct sk_buff *skb, const struct net_device *in,
 	if (!counters)
 		return false;
 
+	spin_lock_bh(&nf_conntrack_lock);
 	switch (sinfo->what) {
 	case XT_CONNBYTES_PKTS:
 		switch (sinfo->direction) {
@@ -88,6 +89,7 @@ connbytes_mt(const struct sk_buff *skb, const struct net_device *in,
 			what = div64_u64(bytes, pkts);
 		break;
 	}
+	spin_unlock_bh(&nf_conntrack_lock);
 
 	if (sinfo->count.to)
 		return what <= sinfo->count.to && what >= sinfo->count.from;
-- 
1.5.6.1

Re: [PATCH] netfilter: add locking for counters zeroing

[Patrick McHardy]

Krzysztof Piotr Oledzki wrote:
>>From 4407c0b11dde5235b1141ef63bc29f322a73c873 Mon Sep 17 00:00:00 2001
> From: Krzysztof Piotr Oledzki <ole@ans.pl>
> Date: Mon, 21 Jul 2008 17:20:45 +0200
> Subject: netfilter: add locking for counters zeroing
> 
> The memset inside ctnetlink_dump_table() fuction needs locking.
> The lock shoud be grabbed outside the loop to avoid repeatedly
> taking and releasing it again.
> 
> Also add similar locking inside xt_connbytes match where
> the counters get read.

Sorry for the delay. About the patch:

> 
> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
> index 9432da4..ff1bbb0 100644
> --- a/net/netfilter/nf_conntrack_netlink.c
> +++ b/net/netfilter/nf_conntrack_netlink.c
> @@ -545,6 +545,7 @@ ctnetlink_dump_table(struct sk_buff *skb, struct netlink_callback *cb)
>  	u_int8_t l3proto = nfmsg->nfgen_family;
>  
>  	rcu_read_lock();
> +	spin_lock_bh(&nf_conntrack_lock);

We only need the spinlock. I'm not so happy about taking it
unconditionally even though we might not be zeroing the
counters. Moving it in the inner loop will greatly increase
the amount of locks/unlocks on the other hand.

How about moving the inner loop to a new function and adding
back the ctnetlink_dump_counterzero (or whatever it was called)
function? It would take the spinlock, while normal dumping
would only use rcu_read_lock().

Re: [PATCH] netfilter: add locking for counters zeroing

[Jan Engelhardt]

On Wednesday 2008-09-24 12:01, Patrick McHardy wrote:
>> diff --git a/net/netfilter/nf_conntrack_netlink.c
>> b/net/netfilter/nf_conntrack_netlink.c
>> index 9432da4..ff1bbb0 100644
>> --- a/net/netfilter/nf_conntrack_netlink.c
>> +++ b/net/netfilter/nf_conntrack_netlink.c
>> @@ -545,6 +545,7 @@ ctnetlink_dump_table(struct sk_buff *skb, struct
>> netlink_callback *cb)
>>   u_int8_t l3proto = nfmsg->nfgen_family;
>>  
>> 	rcu_read_lock();
>> +	spin_lock_bh(&nf_conntrack_lock);
>
> We only need the spinlock. I'm not so happy about taking it
> unconditionally even though we might not be zeroing the
> counters. Moving it in the inner loop will greatly increase
> the amount of locks/unlocks on the other hand.
>
> How about moving the inner loop to a new function and adding
> back the ctnetlink_dump_counterzero (or whatever it was called)
> function? It would take the spinlock, while normal dumping
> would only use rcu_read_lock().

Perhaps this might work?

+	if (cb->args[0] >= nf_conntrack_htable_size) {
+		nf_ct_put(cb->args[1]);
+		return skb->len;
+	}

	rcu_read_lock();
	last = (struct nf_conn *)cb->args[1];
	for (...) {
		...
	}
out:
	if (last)
		nf_ct_put(last);
	return skb->len;

Re: [PATCH] netfilter: add locking for counters zeroing

[Patrick McHardy]

Jan Engelhardt wrote:
> On Wednesday 2008-09-24 12:01, Patrick McHardy wrote:
>>> @@ -545,6 +545,7 @@ ctnetlink_dump_table(struct sk_buff *skb, struct
>>> netlink_callback *cb)
>>>   u_int8_t l3proto = nfmsg->nfgen_family;
>>>  
>>> 	rcu_read_lock();
>>> +	spin_lock_bh(&nf_conntrack_lock);
>> We only need the spinlock. I'm not so happy about taking it
>> unconditionally even though we might not be zeroing the
>> counters. Moving it in the inner loop will greatly increase
>> the amount of locks/unlocks on the other hand.
>>
>> How about moving the inner loop to a new function and adding
>> back the ctnetlink_dump_counterzero (or whatever it was called)
>> function? It would take the spinlock, while normal dumping
>> would only use rcu_read_lock().
> 
> Perhaps this might work?
> 
> +	if (cb->args[0] >= nf_conntrack_htable_size) {
> +		nf_ct_put(cb->args[1]);
> +		return skb->len;
> +	}

I'm not sure what you're trying to fix here. Its a race
between counter zeroing and changing them during packet
processing. Any patch that doesn't include a spin_lock
can't really fix the problem :)

Re: [PATCH] netfilter: add locking for counters zeroing

[Jan Engelhardt]

On Wednesday 2008-09-24 12:11, Patrick McHardy wrote:
>> On Wednesday 2008-09-24 12:01, Patrick McHardy wrote:
>> > > @@ -545,6 +545,7 @@ ctnetlink_dump_table(struct sk_buff *skb, struct
>> > > netlink_callback *cb)
>> > >   u_int8_t l3proto = nfmsg->nfgen_family;
>> > >  
>> > > 	rcu_read_lock();
>> > > +	spin_lock_bh(&nf_conntrack_lock);
>> > We only need the spinlock. I'm not so happy about taking it
>> > unconditionally even though we might not be zeroing the
>> > counters. Moving it in the inner loop will greatly increase
>> > the amount of locks/unlocks on the other hand.
>> >
>> > How about moving the inner loop to a new function and adding
>> > back the ctnetlink_dump_counterzero (or whatever it was called)
>> > function? It would take the spinlock, while normal dumping
>> > would only use rcu_read_lock().
>> 
>> Perhaps this might work?
>> 
>> +	if (cb->args[0] >= nf_conntrack_htable_size) {
>> +		nf_ct_put(cb->args[1]);
>> +		return skb->len;
>> +	}
>
> I'm not sure what you're trying to fix here.

If the for() loop never runs because cb->args<nf_conntrack_htable_size is not
fulfilled, no counter changes, and no locking is needed, hence the early
return.

> Any patch that doesn't include a spin_lock
> can't really fix the problem :)

Of course. You still have to add the spin_lock, preferably outside of the loop
so it does not get necessarily dropped/re-picked-up. But in the case that the
loop never runs, the lock does not need to be taken at all as I see it, 
does it?

Re: [PATCH] netfilter: add locking for counters zeroing

[Patrick McHardy]

Jan Engelhardt wrote:
> On Wednesday 2008-09-24 12:11, Patrick McHardy wrote:
>   
>>> On Wednesday 2008-09-24 12:01, Patrick McHardy wrote:
>>>       
>>>>> @@ -545,6 +545,7 @@ ctnetlink_dump_table(struct sk_buff *skb, struct
>>>>> netlink_callback *cb)
>>>>>   u_int8_t l3proto = nfmsg->nfgen_family;
>>>>>  
>>>>> 	rcu_read_lock();
>>>>> +	spin_lock_bh(&nf_conntrack_lock);
>>>>>           
>>>> We only need the spinlock. I'm not so happy about taking it
>>>> unconditionally even though we might not be zeroing the
>>>> counters. Moving it in the inner loop will greatly increase
>>>> the amount of locks/unlocks on the other hand.
>>>>
>>>> How about moving the inner loop to a new function and adding
>>>> back the ctnetlink_dump_counterzero (or whatever it was called)
>>>> function? It would take the spinlock, while normal dumping
>>>> would only use rcu_read_lock().
>>>>         
>>> Perhaps this might work?
>>>
>>> +	if (cb->args[0] >= nf_conntrack_htable_size) {
>>> +		nf_ct_put(cb->args[1]);
>>> +		return skb->len;
>>> +	}
>>>       
>> I'm not sure what you're trying to fix here.
>>     
>
> If the for() loop never runs because cb->args<nf_conntrack_htable_size is not
> fulfilled, no counter changes, and no locking is needed, hence the early
> return.
>   
Thats a very rare condition and not something worth optimizing for.


>> Any patch that doesn't include a spin_lock
>> can't really fix the problem :)
>>     
>
> Of course. You still have to add the spin_lock, preferably outside of the loop
> so it does not get necessarily dropped/re-picked-up.

Yes, but even more preferrably is don't huring normal dumps for counter 
zeroing.
So I think we should split the operations.

Re: [PATCH] netfilter: add locking for counters zeroing

[Patrick McHardy]

Patrick McHardy wrote:
> Jan Engelhardt wrote:
>>
>> Of course. You still have to add the spin_lock, preferably outside of 
>> the loop
>> so it does not get necessarily dropped/re-picked-up.
>
> Yes, but even more preferrably is don't huring normal dumps for 
> counter zeroing.
-EUNPARSABLE :)

It meant to say: "even more preferrably it shouldn't hurt normal dumps".


> So I think we should split the operations.