* Rule counter update bug in ebtables-v2.0.10-2
@ 2011-11-29 21:08 James Sinclair
2011-12-04 9:36 ` Bart De Schuymer
0 siblings, 1 reply; 5+ messages in thread
From: James Sinclair @ 2011-11-29 21:08 UTC (permalink / raw)
To: netfilter-devel
I was doing some testing with the latest ebtables and I think I've found a bug in ebt_deliver_counters that was introduced in the following commit:
http://ebtables.cvs.sourceforge.net/viewvc/ebtables/ebtables2/userspace/ebtables2/communication.c?r1=1.40&r2=1.41
It seems that the chainnr++ on line 308 is only reached when entries is NULL, causing the code to repeatedly loop over the rules for the first non-empty chain. This manifests as every chain having its counters copied from the first non-empty chain instead of getting the counters assigned with -c:
ebtables -t nat -N CHAIN1
ebtables -t nat -A CHAIN1 -s 0:0:0:0:1:1 -j ACCEPT -c 101 101
ebtables -t nat -A CHAIN1 -s 0:0:0:0:1:2 -j ACCEPT -c 102 102
ebtables -t nat -N CHAIN2
ebtables -t nat -A CHAIN2 -s 0:0:0:0:2:1 -j ACCEPT -c 201 201
ebtables -t nat -A CHAIN2 -s 0:0:0:0:2:2 -j ACCEPT -c 202 202
ebtables -t nat -N CHAIN3
ebtables -t nat -A CHAIN3 -s 0:0:0:0:3:1 -j ACCEPT -c 302 302
ebtables -t nat -A CHAIN3 -s 0:0:0:0:3:2 -j ACCEPT -c 303 303
ebtables -t nat -L --Lc
Bridge table: nat
Bridge chain: PREROUTING, entries: 0, policy: ACCEPT
Bridge chain: OUTPUT, entries: 0, policy: ACCEPT
Bridge chain: POSTROUTING, entries: 0, policy: ACCEPT
Bridge chain: CHAIN1, entries: 2, policy: ACCEPT
-s 0:0:0:0:1:1 -j ACCEPT , pcnt = 101 -- bcnt = 101
-s 0:0:0:0:1:2 -j ACCEPT , pcnt = 102 -- bcnt = 102
Bridge chain: CHAIN2, entries: 2, policy: ACCEPT
-s 0:0:0:0:2:1 -j ACCEPT , pcnt = 101 -- bcnt = 101
-s 0:0:0:0:2:2 -j ACCEPT , pcnt = 102 -- bcnt = 102
Bridge chain: CHAIN3, entries: 2, policy: ACCEPT
-s 0:0:0:0:3:1 -j ACCEPT , pcnt = 101 -- bcnt = 101
-s 0:0:0:0:3:2 -j ACCEPT , pcnt = 102 -- bcnt = 102
I've attempted to fix the bug, and my patch is included below. I tried to account for all of the edge cases, but I don't have a solid enough understanding of the data structures used to claim that I've been successful. My code is on GitHub as well if that's easier:
https://github.com/irgeek/ebtables/commit/29221fea0021795a7005d17288b656bf21519e84
diff --git a/communication.c b/communication.c
index 0917f6e..f1a6f08 100644
--- a/communication.c
+++ b/communication.c
@@ -308,13 +308,14 @@ void ebt_deliver_counters(struct ebt_u_replace *u_repl)
old = u_repl->counters;
new = newcounters;
while (cc != u_repl->cc) {
- if (!next || next == entries->entries) {
- while (chainnr < u_repl->num_chains && (!(entries = u_repl->chains[chainnr]) ||
- (next = entries->entries->next) == entries->entries))
- chainnr++;
- if (chainnr == u_repl->num_chains)
- break;
+ while (!next || (next == entries->entries && chainnr < u_repl->num_chains)) {
+ next = NULL;
+ if ((entries = u_repl->chains[chainnr++])) {
+ next = entries->entries->next;
+ }
}
+ if (chainnr >= u_repl->num_chains && (!entries || next == entries->entries))
+ break;
if (next == NULL)
ebt_print_bug("next == NULL");
if (cc->type == CNT_NORM) {
James Sinclair
Linode, LLC
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: Rule counter update bug in ebtables-v2.0.10-2
2011-11-29 21:08 Rule counter update bug in ebtables-v2.0.10-2 James Sinclair
@ 2011-12-04 9:36 ` Bart De Schuymer
2011-12-05 15:38 ` James Sinclair
0 siblings, 1 reply; 5+ messages in thread
From: Bart De Schuymer @ 2011-12-04 9:36 UTC (permalink / raw)
To: James Sinclair; +Cc: netfilter-devel
On 29-11-11 21:08, James Sinclair wrote:
> I was doing some testing with the latest ebtables and I think I've found a bug in ebt_deliver_counters that was introduced in the following commit:
>
> http://ebtables.cvs.sourceforge.net/viewvc/ebtables/ebtables2/userspace/ebtables2/communication.c?r1=1.40&r2=1.41
>
> It seems that the chainnr++ on line 308 is only reached when entries is NULL, causing the code to repeatedly loop over the rules for the first non-empty chain. This manifests as every chain having its counters copied from the first non-empty chain instead of getting the counters assigned with -c:
Thanks for the bug report. I've applied the following fix instead.
--- ebtables-v2.0.10-2/communication.c 2011-08-11 19:56:16.000000000 +0100
+++ ebtables-v2.0.10-3/communication.c 2011-12-04 09:29:23.000000000 +0000
@@ -309,6 +309,7 @@ void ebt_deliver_counters(struct ebt_u_r
new = newcounters;
while (cc != u_repl->cc) {
if (!next || next == entries->entries) {
+ chainnr++;
while (chainnr < u_repl->num_chains && (!(entries = u_repl->chains[chainnr]) ||
(next = entries->entries->next) == entries->entries))
chainnr++;
cheers,
Bart
--
Bart De Schuymer
www.artinalgorithms.be
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Rule counter update bug in ebtables-v2.0.10-2
2011-12-04 9:36 ` Bart De Schuymer
@ 2011-12-05 15:38 ` James Sinclair
2011-12-05 20:34 ` Bart De Schuymer
0 siblings, 1 reply; 5+ messages in thread
From: James Sinclair @ 2011-12-05 15:38 UTC (permalink / raw)
To: Bart De Schuymer; +Cc: netfilter-devel
On Dec 4, 2011, at 4:36 AM, Bart De Schuymer wrote:
> On 29-11-11 21:08, James Sinclair wrote:
>> I was doing some testing with the latest ebtables and I think I've found a bug in ebt_deliver_counters that was introduced in the following commit:
>>
>> http://ebtables.cvs.sourceforge.net/viewvc/ebtables/ebtables2/userspace/ebtables2/communication.c?r1=1.40&r2=1.41
>>
>> It seems that the chainnr++ on line 308 is only reached when entries is NULL, causing the code to repeatedly loop over the rules for the first non-empty chain. This manifests as every chain having its counters copied from the first non-empty chain instead of getting the counters assigned with -c:
>
> Thanks for the bug report. I've applied the following fix instead.
>
> --- ebtables-v2.0.10-2/communication.c 2011-08-11 19:56:16.000000000 +0100
> +++ ebtables-v2.0.10-3/communication.c 2011-12-04 09:29:23.000000000 +0000
> @@ -309,6 +309,7 @@ void ebt_deliver_counters(struct ebt_u_r
> new = newcounters;
> while (cc != u_repl->cc) {
> if (!next || next == entries->entries) {
> + chainnr++;
> while (chainnr < u_repl->num_chains && (!(entries = u_repl->chains[chainnr]) ||
> (next = entries->entries->next) == entries->entries))
> chainnr++;
>
> cheers,
> Bart
>
>
>
> --
> Bart De Schuymer
> www.artinalgorithms.be
Thanks for taking the time to look at my patch, Bart.
It looks like the fix you applied introduces a new bug. It works in most cases, but when a rules is set in the first built-in chain (such as PREROUTING in the nat table) all counters get reset to zero.
ebtables -t nat -A PREROUTING -s 0:0:0:0:0:1 -j ACCEPT -c 10 10
ebtables -t nat -N CHAIN1
ebtables -t nat -A CHAIN1 -s 0:0:0:0:1:1 -j ACCEPT -c 101 101
ebtables -t nat -A CHAIN1 -s 0:0:0:0:1:2 -j ACCEPT -c 102 102
ebtables -t nat -N CHAIN2
ebtables -t nat -A CHAIN2 -s 0:0:0:0:2:1 -j ACCEPT -c 201 201
ebtables -t nat -A CHAIN2 -s 0:0:0:0:2:2 -j ACCEPT -c 202 202
ebtables -t nat -N CHAIN3
ebtables -t nat -A CHAIN3 -s 0:0:0:0:3:1 -j ACCEPT -c 302 302
ebtables -t nat -A CHAIN3 -s 0:0:0:0:3:2 -j ACCEPT -c 303 303
ebtables -t nat -L --Lc
Bridge table: nat
Bridge chain: PREROUTING, entries: 1, policy: ACCEPT
-s 0:0:0:0:0:1 -j ACCEPT , pcnt = 0 -- bcnt = 0
Bridge chain: OUTPUT, entries: 0, policy: ACCEPT
Bridge chain: POSTROUTING, entries: 0, policy: ACCEPT
Bridge chain: CHAIN1, entries: 2, policy: ACCEPT
-s 0:0:0:0:1:1 -j ACCEPT , pcnt = 0 -- bcnt = 0
-s 0:0:0:0:1:2 -j ACCEPT , pcnt = 0 -- bcnt = 0
Bridge chain: CHAIN2, entries: 2, policy: ACCEPT
-s 0:0:0:0:2:1 -j ACCEPT , pcnt = 0 -- bcnt = 0
-s 0:0:0:0:2:2 -j ACCEPT , pcnt = 0 -- bcnt = 0
Bridge chain: CHAIN3, entries: 2, policy: ACCEPT
-s 0:0:0:0:3:1 -j ACCEPT , pcnt = 0 -- bcnt = 0
-s 0:0:0:0:3:2 -j ACCEPT , pcnt = 0 -- bcnt = 0
James Sinclair
Linode, LLC
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Rule counter update bug in ebtables-v2.0.10-2
2011-12-05 15:38 ` James Sinclair
@ 2011-12-05 20:34 ` Bart De Schuymer
2011-12-07 17:18 ` James Sinclair
0 siblings, 1 reply; 5+ messages in thread
From: Bart De Schuymer @ 2011-12-05 20:34 UTC (permalink / raw)
To: James Sinclair; +Cc: netfilter-devel
On 05-12-11 15:38, James Sinclair wrote:
> On Dec 4, 2011, at 4:36 AM, Bart De Schuymer wrote:
>
>> On 29-11-11 21:08, James Sinclair wrote:
>>> I was doing some testing with the latest ebtables and I think I've found a bug in ebt_deliver_counters that was introduced in the following commit:
>>>
>>> http://ebtables.cvs.sourceforge.net/viewvc/ebtables/ebtables2/userspace/ebtables2/communication.c?r1=1.40&r2=1.41
>>>
>>> It seems that the chainnr++ on line 308 is only reached when entries is NULL, causing the code to repeatedly loop over the rules for the first non-empty chain. This manifests as every chain having its counters copied from the first non-empty chain instead of getting the counters assigned with -c:
>>
>> Thanks for the bug report. I've applied the following fix instead.
>>
>> --- ebtables-v2.0.10-2/communication.c 2011-08-11 19:56:16.000000000 +0100
>> +++ ebtables-v2.0.10-3/communication.c 2011-12-04 09:29:23.000000000 +0000
>> @@ -309,6 +309,7 @@ void ebt_deliver_counters(struct ebt_u_r
>> new = newcounters;
>> while (cc != u_repl->cc) {
>> if (!next || next == entries->entries) {
>> + chainnr++;
>> while (chainnr< u_repl->num_chains&& (!(entries = u_repl->chains[chainnr]) ||
>> (next = entries->entries->next) == entries->entries))
>> chainnr++;
>>
>> cheers,
>> Bart
>>
>>
>>
>> --
>> Bart De Schuymer
>> www.artinalgorithms.be
>
>
> Thanks for taking the time to look at my patch, Bart.
>
> It looks like the fix you applied introduces a new bug. It works in most cases, but when a rules is set in the first built-in chain (such as PREROUTING in the nat table) all counters get reset to zero.
Thanks for verifying this.
Please try the incremental patch below (patch -p1 < file). I'll wait for your verification this time before making another release :)
--- ebtables-v2.0.10-3/communication.c 2011-12-04 09:46:26.000000000 +0000
+++ ebtables-v2.0.10-4/communication.c 2011-12-05 20:29:17.864018957 +0000
@@ -295,7 +295,7 @@ void ebt_deliver_counters(struct ebt_u_r
struct ebt_cntchanges *cc = u_repl->cc->next, *cc2;
struct ebt_u_entries *entries = NULL;
struct ebt_u_entry *next = NULL;
- int i, chainnr = 0;
+ int i, chainnr = -1;
if (u_repl->nentries == 0)
return;
Best regards,
Bart
--
Bart De Schuymer
www.artinalgorithms.be
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Rule counter update bug in ebtables-v2.0.10-2
2011-12-05 20:34 ` Bart De Schuymer
@ 2011-12-07 17:18 ` James Sinclair
0 siblings, 0 replies; 5+ messages in thread
From: James Sinclair @ 2011-12-07 17:18 UTC (permalink / raw)
To: Bart De Schuymer; +Cc: netfilter-devel
On Dec 5, 2011, at 3:34 PM, Bart De Schuymer wrote:
> On 05-12-11 15:38, James Sinclair wrote:
>> On Dec 4, 2011, at 4:36 AM, Bart De Schuymer wrote:
>>
>>> On 29-11-11 21:08, James Sinclair wrote:
>>>> I was doing some testing with the latest ebtables and I think I've found a bug in ebt_deliver_counters that was introduced in the following commit:
>>>>
>>>> http://ebtables.cvs.sourceforge.net/viewvc/ebtables/ebtables2/userspace/ebtables2/communication.c?r1=1.40&r2=1.41
>>>>
>>>> It seems that the chainnr++ on line 308 is only reached when entries is NULL, causing the code to repeatedly loop over the rules for the first non-empty chain. This manifests as every chain having its counters copied from the first non-empty chain instead of getting the counters assigned with -c:
>>>
>>> Thanks for the bug report. I've applied the following fix instead.
>>>
>>> --- ebtables-v2.0.10-2/communication.c 2011-08-11 19:56:16.000000000 +0100
>>> +++ ebtables-v2.0.10-3/communication.c 2011-12-04 09:29:23.000000000 +0000
>>> @@ -309,6 +309,7 @@ void ebt_deliver_counters(struct ebt_u_r
>>> new = newcounters;
>>> while (cc != u_repl->cc) {
>>> if (!next || next == entries->entries) {
>>> + chainnr++;
>>> while (chainnr< u_repl->num_chains&& (!(entries = u_repl->chains[chainnr]) ||
>>> (next = entries->entries->next) == entries->entries))
>>> chainnr++;
>>>
>>> cheers,
>>> Bart
>>>
>>>
>>>
>>> --
>>> Bart De Schuymer
>>> www.artinalgorithms.be
>>
>>
>> Thanks for taking the time to look at my patch, Bart.
>>
>> It looks like the fix you applied introduces a new bug. It works in most cases, but when a rules is set in the first built-in chain (such as PREROUTING in the nat table) all counters get reset to zero.
>
> Thanks for verifying this.
>
> Please try the incremental patch below (patch -p1 < file). I'll wait for your verification this time before making another release :)
>
> --- ebtables-v2.0.10-3/communication.c 2011-12-04 09:46:26.000000000 +0000
> +++ ebtables-v2.0.10-4/communication.c 2011-12-05 20:29:17.864018957 +0000
> @@ -295,7 +295,7 @@ void ebt_deliver_counters(struct ebt_u_r
> struct ebt_cntchanges *cc = u_repl->cc->next, *cc2;
> struct ebt_u_entries *entries = NULL;
> struct ebt_u_entry *next = NULL;
> - int i, chainnr = 0;
> + int i, chainnr = -1;
>
> if (u_repl->nentries == 0)
> return;
>
>
> Best regards,
> Bart
>
>
> --
> Bart De Schuymer
> www.artinalgorithms.be
>
It looks like that patch works with all of the tests I've been using. Thanks for taking care of this.
James Sinclair
Linode, LLC
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-12-07 17:18 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-11-29 21:08 Rule counter update bug in ebtables-v2.0.10-2 James Sinclair
2011-12-04 9:36 ` Bart De Schuymer
2011-12-05 15:38 ` James Sinclair
2011-12-05 20:34 ` Bart De Schuymer
2011-12-07 17:18 ` James Sinclair
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.