Discussion:
[PATCH net-next] bridge: ebtables: Avoid resetting limit rule state
(too old to reply)
Linus Lüssing
2017-11-25 07:44:18 UTC
Permalink
So far any changes with ebtables will reset the state of limit rules,
leading to spikes in traffic. This is especially noticeable if changes
are done frequently, for instance via a daemon.

This patch fixes this by bailing out from (re)setting if the limit
rule was initialized before.

When sending packets every 250ms for 600s, with a
"--limit 1/sec --limit-burst 50" rule and a command like this
in the background:

$ ebtables -N VOIDCHAIN
$ while true; do ebtables -F VOIDCHAIN; sleep 30; done

The results are:

Before: ~1600 packets
After: 650 packets

Signed-off-by: Linus Lüssing <***@c0d3.blue>
---
net/bridge/netfilter/ebt_limit.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/net/bridge/netfilter/ebt_limit.c b/net/bridge/netfilter/ebt_limit.c
index 61a9f1be1263..f74b48633feb 100644
--- a/net/bridge/netfilter/ebt_limit.c
+++ b/net/bridge/netfilter/ebt_limit.c
@@ -69,6 +69,10 @@ static int ebt_limit_mt_check(const struct xt_mtchk_param *par)
{
struct ebt_limit_info *info = par->matchinfo;

+ /* Do not reset state on unrelated table changes */
+ if (info->prev)
+ return 0;
+
/* Check for overflow. */
if (info->burst == 0 ||
user2credits(info->avg * info->burst) < user2credits(info->avg)) {
--
2.11.0
Pablo Neira Ayuso
2017-11-27 23:30:08 UTC
Permalink
Hi Linus,
Post by Linus Lüssing
So far any changes with ebtables will reset the state of limit rules,
leading to spikes in traffic. This is especially noticeable if changes
are done frequently, for instance via a daemon.
This patch fixes this by bailing out from (re)setting if the limit
rule was initialized before.
When sending packets every 250ms for 600s, with a
"--limit 1/sec --limit-burst 50" rule and a command like this
$ ebtables -N VOIDCHAIN
$ while true; do ebtables -F VOIDCHAIN; sleep 30; done
Before: ~1600 packets
After: 650 packets
---
net/bridge/netfilter/ebt_limit.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/net/bridge/netfilter/ebt_limit.c b/net/bridge/netfilter/ebt_limit.c
index 61a9f1be1263..f74b48633feb 100644
--- a/net/bridge/netfilter/ebt_limit.c
+++ b/net/bridge/netfilter/ebt_limit.c
@@ -69,6 +69,10 @@ static int ebt_limit_mt_check(const struct xt_mtchk_param *par)
{
struct ebt_limit_info *info = par->matchinfo;
+ /* Do not reset state on unrelated table changes */
+ if (info->prev)
+ return 0;
What kernel version are you using? I suspect you don't have this
applied?

commit ec23189049651b16dc2ffab35a4371dc1f491aca
Author: Willem de Bruijn <***@google.com>
Date: Mon Jan 2 17:19:46 2017 -0500

xtables: extend matches and targets with .usersize
Linus Lüssing
2017-12-04 04:53:35 UTC
Permalink
Hi Pablo,

Thanks for your reply!
Post by Pablo Neira Ayuso
[...]
Post by Linus Lüssing
diff --git a/net/bridge/netfilter/ebt_limit.c b/net/bridge/netfilter/ebt_limit.c
index 61a9f1be1263..f74b48633feb 100644
--- a/net/bridge/netfilter/ebt_limit.c
+++ b/net/bridge/netfilter/ebt_limit.c
@@ -69,6 +69,10 @@ static int ebt_limit_mt_check(const struct xt_mtchk_param *par)
{
struct ebt_limit_info *info = par->matchinfo;
+ /* Do not reset state on unrelated table changes */
+ if (info->prev)
+ return 0;
What kernel version are you using? I suspect you don't have this
applied?
I'm indeed using a 4.4.102 kernel, as LEDE is still in the process
of updating to 4.14. So 4.4 with LEDE is where I got the measurement
results from.
Post by Pablo Neira Ayuso
commit ec23189049651b16dc2ffab35a4371dc1f491aca
Date: Mon Jan 2 17:19:46 2017 -0500
xtables: extend matches and targets with .usersize
And so, no I do not have this patch. I looked at it now, but it
does not seem to have any relation with .matchinfo, does it?

I also had a quick look at a 4.15-rc1 kernel in a VM now. I still
end up in ebt_limit_mt_check() with the variables being reset
when editing the table somewhere.

Regards, Linus
Linus Lüssing
2017-12-04 05:20:06 UTC
Permalink
Post by Linus Lüssing
And so, no I do not have this patch. I looked at it now, but it
does not seem to have any relation with .matchinfo, does it?
Relation between .usersize and .checkentry I ment, not
.usersize and .matchinfo.
Pablo Neira Ayuso
2017-12-04 10:13:40 UTC
Permalink
Post by Linus Lüssing
Post by Linus Lüssing
And so, no I do not have this patch. I looked at it now, but it
does not seem to have any relation with .matchinfo, does it?
Relation between .usersize and .checkentry I ment, not
.usersize and .matchinfo.
In your patch, info->prev comes set to a value from userspace, right?

commit 324318f0248c31be8a08984146e7e4dd7cdd091d
Author: Willem de Bruijn <***@google.com>
Date: Tue May 9 16:17:37 2017 -0400

netfilter: xtables: zero padding in data_to_user

Since that patch above, the data area is zero'ed before dumped to
userspace, so we would get a null info->prev, hence defeating the
trick your patch relies on.

Am I missing anything?
Pablo Neira Ayuso
2017-12-07 00:26:19 UTC
Permalink
Hi Linus,
Post by Linus Lüssing
Hi Pablo,
Thanks for your reply!
Post by Pablo Neira Ayuso
[...]
Post by Linus Lüssing
diff --git a/net/bridge/netfilter/ebt_limit.c b/net/bridge/netfilter/ebt_limit.c
index 61a9f1be1263..f74b48633feb 100644
--- a/net/bridge/netfilter/ebt_limit.c
+++ b/net/bridge/netfilter/ebt_limit.c
@@ -69,6 +69,10 @@ static int ebt_limit_mt_check(const struct xt_mtchk_param *par)
{
struct ebt_limit_info *info = par->matchinfo;
+ /* Do not reset state on unrelated table changes */
+ if (info->prev)
+ return 0;
What kernel version are you using? I suspect you don't have this
applied?
I'm indeed using a 4.4.102 kernel, as LEDE is still in the process
of updating to 4.14. So 4.4 with LEDE is where I got the measurement
results from.
Post by Pablo Neira Ayuso
commit ec23189049651b16dc2ffab35a4371dc1f491aca
Date: Mon Jan 2 17:19:46 2017 -0500
xtables: extend matches and targets with .usersize
And so, no I do not have this patch. I looked at it now, but it
does not seem to have any relation with .matchinfo, does it?
I also had a quick look at a 4.15-rc1 kernel in a VM now. I still
end up in ebt_limit_mt_check() with the variables being reset
when editing the table somewhere.
My question is if your fix would work with 4.15-rc1.
Linus Lüssing
2017-12-08 05:46:06 UTC
Permalink
Post by Pablo Neira Ayuso
Post by Linus Lüssing
I also had a quick look at a 4.15-rc1 kernel in a VM now. I still
end up in ebt_limit_mt_check() with the variables being reset
when editing the table somewhere.
My question is if your fix would work with 4.15-rc1.
You are absoluetly right, it's not working anymore since the
commit you mentioned initially :-(
("xtables: extend matches and targets with .usersize").
info->prev is always 0 since exactly this commit.

That means, trying tricks in ebt_limit_mt_check() is too
late now, the old values are already overwritten? (or is there
some commit scheme which installs the ebt_limit_info provided
by ebt_limit_check() some time after its call?)

Extending the usersize to include info->prev would probably be too
hackish/ugly, right?
Linus Lüssing
2017-12-08 05:49:29 UTC
Permalink
Post by Linus Lüssing
Extending the usersize to include info->prev would probably be too
hackish/ugly, right?
And wouldn't be enough anyway, since
info->{credit,credit_cap,cost} would still be zeroed... Hm.

Continue reading on narkive:
Loading...