[PATCH] priv_data 0/2

[PATCH] priv_data 0/2

[Massimiliano Hofer]

Hi,
this is a version of my priv_data patch that updates targets and renames 
functions as suggested by Patrick.

Since xt_init_match() and xt_init_target() (formerly xt_check_match() and 
xt_check_target()) no longer just check and they needed some argument changes 
anyway, I included some more common code previously replicated in 
ip_tables.c, ip6_tables.c and arp_tables.c.
Similarly I introduced xt_destroy_match() and xt_destroy_taget().

The resulting patches are larger than I anticipated, but most of the space is 
taken by function ranames and argument adjustments.

My previous example with xt_condition still applies (just rename checkentry to 
init in struct xt_match).

I tested several combination with iptables, ip6tables and arptables. It can't 
make it fail, but I didn't try it with a real world network load. Right now I 
don't have a 64 bit machine available for testing (I should be able to use 
one in a few days), so I didn't test compat at all.

Testing and comments, as always, are appreciated.

-- 
Saluti,
   Massimiliano Hofer

Re: [PATCH] priv_data 0/2

[Patrick McHardy]

Massimiliano Hofer wrote:
> Hi,
> this is a version of my priv_data patch that updates targets and renames 
> functions as suggested by Patrick.
> 
> Since xt_init_match() and xt_init_target() (formerly xt_check_match() and 
> xt_check_target()) no longer just check and they needed some argument changes 
> anyway, I included some more common code previously replicated in 
> ip_tables.c, ip6_tables.c and arp_tables.c.
> Similarly I introduced xt_destroy_match() and xt_destroy_taget().
> 
> The resulting patches are larger than I anticipated, but most of the space is 
> taken by function ranames and argument adjustments.
> 
> My previous example with xt_condition still applies (just rename checkentry to 
> init in struct xt_match).
> 
> I tested several combination with iptables, ip6tables and arptables. It can't 
> make it fail, but I didn't try it with a real world network load. Right now I 
> don't have a 64 bit machine available for testing (I should be able to use 
> one in a few days), so I didn't test compat at all.
> 
> Testing and comments, as always, are appreciated.

Nice work, thanks Massimiliano. I'll have a closer look later and will
probably apply it tommorrow. I hope we can still get this in 2.6.18.

Re: [PATCH] priv_data 0/2

[Massimiliano Hofer]

On Monday 3 July 2006 8:33 pm, Patrick McHardy wrote:

> Nice work, thanks Massimiliano. I'll have a closer look later and will
> probably apply it tommorrow. I hope we can still get this in 2.6.18.

Thank you, this would be great.
If priv_data is merged I plan on having condition in good shape before 2.6.18 
is released and hope it would be considered fit for merging in 2.6.19.

-- 
Bye,
   Massimiliano Hofer

Re: [PATCH] priv_data 0/2

[Sven Anders]

[-- Attachment #1: Type: text/plain, Size: 864 bytes --]

Massimiliano Hofer schrieb:
> On Monday 3 July 2006 8:33 pm, Patrick McHardy wrote:
> 
>> Nice work, thanks Massimiliano. I'll have a closer look later and will
>> probably apply it tommorrow. I hope we can still get this in 2.6.18.

If this is merged, I would like to see a negated limit...
I need some advice to preserve compatibility here.

Regards
 Sven

-- 
 Sven Anders <anders@anduras.de>                 () Ascii Ribbon Campaign
                                                 /\ Support plain text e-mail
 ANDURAS service solutions AG
 Innstraße 71 - 94036 Passau - Germany
 Web: www.anduras.de - Tel: +49 (0)851-4 90 50-0 - Fax: +49 (0)851-4 90 50-55

Rechtsform: Aktiengesellschaft - Sitz: Passau - Amtsgericht Passau HRB 6032
Mitglieder des Vorstands: Sven Anders, Marcus Junker, Michael Schön
Vorsitzender des Aufsichtsrats: Dipl. Kfm. Thomas Träger

Re: [PATCH] priv_data 0/2

[Patrick McHardy]

Sven Anders wrote:
> If this is merged, I would like to see a negated limit...
> I need some advice to preserve compatibility here.

Once this is merged we can convert matches and targets to keep their
private state outside of the structure shared with userspace and can
start reusing the fields, since they are ignored by userspace anyway.
I'm not sure, but I think currently userspace initializes them to 0,
which should make this easy.

Re: [PATCH] priv_data 0/2

[Joakim Axelsson]

2006-07-04 02:20:40+0200, Patrick McHardy <kaber@trash.net> ->
> Sven Anders wrote:
> > If this is merged, I would like to see a negated limit...
> > I need some advice to preserve compatibility here.
> 
> Once this is merged we can convert matches and targets to keep their
> private state outside of the structure shared with userspace and can
> start reusing the fields, since they are ignored by userspace anyway.
> I'm not sure, but I think currently userspace initializes them to 0,
> which should make this easy.
> 

I wonder where all this priv_data changes ended? I have a few modules (that
I havn't submited, because i've been way to lazy :-/) that will benefit from
this. Among those a new limiter matcher simply called "lim". This match will
solve Sven Anders problem with negating match. It's also much more aqurate
and using a better limiter algoritm. Including being able to limit on packet
size etc etc. It has been working stable in large scale routers (several
100Mbit/s) for a year now I think.

So where did this end up. The reason I am asking is because I was working on
sending them in as patches. But they could really use this priv_data as they
are using alot of ugly things to work this problem around. Same goes for a
few target matches I have. They also need priv_data. Among them a "simple"
-j COUNT that presents counters in /proc in an easy and fancy way (for
scripts/rrdtool to read). I know hfpac wants this things they have droped
the counters built in for each rule.

Re: [PATCH] priv_data 0/2

[Patrick McHardy]

Joakim Axelsson wrote:
> 2006-07-04 02:20:40+0200, Patrick McHardy <kaber@trash.net> ->
> 
>>Sven Anders wrote:
>>
>>>If this is merged, I would like to see a negated limit...
>>>I need some advice to preserve compatibility here.
>>
>>Once this is merged we can convert matches and targets to keep their
>>private state outside of the structure shared with userspace and can
>>start reusing the fields, since they are ignored by userspace anyway.
>>I'm not sure, but I think currently userspace initializes them to 0,
>>which should make this easy.
>>
> 
> 
> I wonder where all this priv_data changes ended?

We missed the 2.6.18 merge window and I'm currently busy with a few
other things, so I delayed it a bit. It will go in 2.6.19, I'll post
the patches once I've merged them in my 2.6.19 tree.

> I have a few modules (that
> I havn't submited, because i've been way to lazy :-/) that will benefit from
> this. Among those a new limiter matcher simply called "lim". This match will
> solve Sven Anders problem with negating match. It's also much more aqurate
> and using a better limiter algoritm. Including being able to limit on packet
> size etc etc. It has been working stable in large scale routers (several
> 100Mbit/s) for a year now I think.

Please just post what you've got, I'd be interested in seeing why you
chose to introduce a new match instead of enhancing the existing limit
match.

Re: [PATCH] priv_data 0/2

[Joakim Axelsson]

2006-07-21 01:25:19+0200, Patrick McHardy <kaber@trash.net> ->
> Joakim Axelsson wrote:
> > 2006-07-04 02:20:40+0200, Patrick McHardy <kaber@trash.net> ->
> > 
> >>Sven Anders wrote:
> >>
> >>>If this is merged, I would like to see a negated limit...
> >>>I need some advice to preserve compatibility here.
> >>
> >>Once this is merged we can convert matches and targets to keep their
> >>private state outside of the structure shared with userspace and can
> >>start reusing the fields, since they are ignored by userspace anyway.
> >>I'm not sure, but I think currently userspace initializes them to 0,
> >>which should make this easy.
> >>
> > 
> > 
> > I wonder where all this priv_data changes ended?
> 
> We missed the 2.6.18 merge window and I'm currently busy with a few
> other things, so I delayed it a bit. It will go in 2.6.19, I'll post
> the patches once I've merged them in my 2.6.19 tree.
> 

Okie. Guess i'll wait with updating it.

> > I have a few modules (that
> > I havn't submited, because i've been way to lazy :-/) that will benefit from
> > this. Among those a new limiter matcher simply called "lim". This match will
> > solve Sven Anders problem with negating match. It's also much more aqurate
> > and using a better limiter algoritm. Including being able to limit on packet
> > size etc etc. It has been working stable in large scale routers (several
> > 100Mbit/s) for a year now I think.
> 
> Please just post what you've got, I'd be interested in seeing why you
> chose to introduce a new match instead of enhancing the existing limit
> match.

Because i wrote it before revisions existed and its takes totally different
userspace parameters. You can find it here:
http://www.gozem.se/~gozem/netfilter/kernel/lim/

Userspace here:
http://www.gozem.se/~gozem/netfilter/userspace/

I'll try to submit them in pom-ng -format as soon as possible. That is if
netfilter-dev wants them. I know there are supposed to be external
svn-repositories nowdays, but i havn't really figured out how that is
supposed to work yet. So please point me in the right direction.

Ihave a patch for kernel but it includes 3 more modules described
below:
http://www.gozem.se/~gozem/netfilter/kernel/lim-norm-COUNT-SPEED.patch

Please read the comments on the algoritm in the code. Its more accurate as
it never useds any division which will have rounding errors. However, its
uses a few 64 bits integers. But i trade memory for a more accurate
alogritm. Specially when dealing with a few 1000/s. The existing limit
doesn't do this correctly at all. It starts limiting at a lower level. Also
it has fetuares as:
- Negation
- bytes-limit instead of packets
- Blackout. Limit down to zero if limited.
- Much more flexible way ot taking the limit amount. The following is valid
for example: --lim-packets 10k/2sec. Meaning 10 000 for each two seconds.
- Also, the --lim-start makes more sense than --limit-burst. --limit-burst
is a multiplier of --limit. I belivie many thinks of it as an absolut figure
then to start the limiting rather than being related as a multiplier for the
base figure of --limit.
- lim is shorter to write :-) It a very commonly used match in my firewall
setup.

The 3 other modules:
COUNT - As i've said before. A counting target that represents its figures
in /proc

SPEED - A speed-caclulating target, other the same as COUNT.

norm - A normalizer of traffic/packet flow. Uses the algoritm of my 'lim'
and my 'speed' combined. It has not been tested that well had the code needs
cleaning up.

A few more coming is:
-j LOG new revision. It will take a longer --log-prefix. 29 chars are way to
short for me. I have a large scale firewall setup and need to log the chain
name and a reason. This simply doesn't fit in 29 chars for most of the time.
Also a --log-limit and --drop. Its very common that you want to log the
packets you DROP but do not want to flood yourself with messages in case of
attacks. This solution will otherwise need a new chain and 2 or 3 rules in
that new chain. Unnessasary waste of memory, CPU and cachelines.

-m log, Same as -J LOG but (wrongly) using a match so not an extra rule or
chain is needed to actually do something with the packets later.

-m unclean new revision. With _alot_ of options so you can specifty exactly
which tests you want. Also with a --log that logs the packet with a reason
and an option prefix, rather than using the printk it does now. I know the
general option is that unclean is dangerous and possibly wrong. However i
use it very succesfully for several years now. A bit patched though to
correct a few things.

If anything above sounds intressting (or wrong). Please let me know.

Re: [PATCH] priv_data 0/2

[Amin Azez]

* Patrick McHardy wrote, On 04/07/06 01:20:
> Sven Anders wrote:
>> If this is merged, I would like to see a negated limit...
>> I need some advice to preserve compatibility here.
> 
> Once this is merged we can convert matches and targets to keep their
> private state outside of the structure shared with userspace and can
> start reusing the fields, since they are ignored by userspace anyway.
> I'm not sure, but I think currently userspace initializes them to 0,
> which should make this easy.

Relying on userspace to initialize kernel structures to 0 smells like
danger, and a possible exploit point.

Sam

Re: [PATCH] priv_data 0/2

[Patrick McHardy]

Amin Azez wrote:
> * Patrick McHardy wrote, On 04/07/06 01:20:
> 
>>Sven Anders wrote:
>>
>>>If this is merged, I would like to see a negated limit...
>>>I need some advice to preserve compatibility here.
>>
>>Once this is merged we can convert matches and targets to keep their
>>private state outside of the structure shared with userspace and can
>>start reusing the fields, since they are ignored by userspace anyway.
>>I'm not sure, but I think currently userspace initializes them to 0,
>>which should make this easy.
> 
> 
> Relying on userspace to initialize kernel structures to 0 smells like
> danger, and a possible exploit point.

There is no difference here to a million other spots in the kernel.
If something can create trouble, it needs to be verified. In the
case of negation it just means "0 - don't negate", "1 - negate",
and the fact that it used to be 0 can be used to provide backwards
compatibility without introducing a new revision.


BTW, can you please fix your mailer so every mail doesn't go to the
list twice?

Re: [PATCH] priv_data 0/2

[Joakim Axelsson]

2006-06-26 16:41:46+0200, Massimiliano Hofer <max@nucleus.it> ->
> Hi,
> this is a version of my priv_data patch that updates targets and renames 
> functions as suggested by Patrick.
> 
> Since xt_init_match() and xt_init_target() (formerly xt_check_match() and 
> xt_check_target()) no longer just check and they needed some argument changes 
> anyway, I included some more common code previously replicated in 
> ip_tables.c, ip6_tables.c and arp_tables.c.
> Similarly I introduced xt_destroy_match() and xt_destroy_taget().
> 
> The resulting patches are larger than I anticipated, but most of the space is 
> taken by function ranames and argument adjustments.
> 
> My previous example with xt_condition still applies (just rename checkentry to 
> init in struct xt_match).
> 
> I tested several combination with iptables, ip6tables and arptables. It can't 
> make it fail, but I didn't try it with a real world network load. Right now I 
> don't have a 64 bit machine available for testing (I should be able to use 
> one in a few days), so I didn't test compat at all.
> 
> Testing and comments, as always, are appreciated.
> 
> -- 
> Saluti,
>    Massimiliano Hofer

I'm currently porting some of my modules to use this API with priv_data.
However, I ran into some troubles. For example i'm writing a revision 1 for
the quota module which allows the count to go negative, and in the same time
I'm porting it to use this API (as it's using the same stupid thing as -m
limit with q->master = q;). However, having the quota counter in the
priv_data struct makes it impossible to report the counter to userspace when
the user issues iptables -L.

"Hidden", kernel data only is good with priv_data, but sometimes it will be
too hidden.

Do we have any good solution for this?

--
Joakim Axelsson

Re: [PATCH] priv_data 0/2

[Patrick McHardy]

Joakim Axelsson wrote:
> 
> I'm currently porting some of my modules to use this API with priv_data.
> However, I ran into some troubles. For example i'm writing a revision 1 for
> the quota module which allows the count to go negative, and in the same time
> I'm porting it to use this API (as it's using the same stupid thing as -m
> limit with q->master = q;). However, having the quota counter in the
> priv_data struct makes it impossible to report the counter to userspace when
> the user issues iptables -L.

That is actually a bug in the current module IMO (I looked into fixing
this yesterday as well). iptables-save/restore won't work properly since
they will save the _remaining_ quota, not the one the rule was created
with. This might be useful as well, but it diverges from what other
modules do.

> "Hidden", kernel data only is good with priv_data, but sometimes it will be
> too hidden.
> 
> Do we have any good solution for this?

Not really, see the mail I just wrote.

Re: [PATCH] priv_data 0/2

[Joakim Axelsson]

2006-08-14 16:22:31+0200, Patrick McHardy <kaber@trash.net> ->
> Joakim Axelsson wrote:
> > 
> > I'm currently porting some of my modules to use this API with priv_data.
> > However, I ran into some troubles. For example i'm writing a revision 1 for
> > the quota module which allows the count to go negative, and in the same time
> > I'm porting it to use this API (as it's using the same stupid thing as -m
> > limit with q->master = q;). However, having the quota counter in the
> > priv_data struct makes it impossible to report the counter to userspace when
> > the user issues iptables -L.
> 
> That is actually a bug in the current module IMO (I looked into fixing
> this yesterday as well). iptables-save/restore won't work properly since
> they will save the _remaining_ quota, not the one the rule was created
> with. This might be useful as well, but it diverges from what other
> modules do.
> 

Well, the priv_data patch will then solve this problem. As the userspace
struct will always have the initial quota unaltered. The priv_data quota will
decrease. However i guess you want to be able to see both. And in my case,
saving the remaining quota with iptables-save is what i want. (I case of the
router for some reason craches, the last state must be saved and as accurate
as possible.)

Solution for this might be using two parameters for the userspace struct.
One for initial quota and one for remaining, saving both. Possible even an
option saying with figure should be set when iptables-restore is used.

Still, we can't nicly access the remaining quota with priv_data unless the
modules each time it "matches" writes in both priv_data and userspace info,
which is kinda ugly.

There is also the way of using a /proc-entry to list the current remaining
quota. Also restoring it by echoing a new figure to the entry. Keep the
iptables-save/restore only with the initial quota figure.

I can write this if this is what we want?

--
Joakim Axelsson

Re: [PATCH] priv_data 0/2

[Patrick McHardy]

Joakim Axelsson wrote:
> 2006-08-14 16:22:31+0200, Patrick McHardy <kaber@trash.net> ->
> 
>>Joakim Axelsson wrote:
>>
>>>I'm currently porting some of my modules to use this API with priv_data.
>>>However, I ran into some troubles. For example i'm writing a revision 1 for
>>>the quota module which allows the count to go negative, and in the same time
>>>I'm porting it to use this API (as it's using the same stupid thing as -m
>>>limit with q->master = q;). However, having the quota counter in the
>>>priv_data struct makes it impossible to report the counter to userspace when
>>>the user issues iptables -L.
>>
>>That is actually a bug in the current module IMO (I looked into fixing
>>this yesterday as well). iptables-save/restore won't work properly since
>>they will save the _remaining_ quota, not the one the rule was created
>>with. This might be useful as well, but it diverges from what other
>>modules do.
> 
> Well, the priv_data patch will then solve this problem. As the userspace
> struct will always have the initial quota unaltered. The priv_data quota will
> decrease.

That was the initial idea, forgetting about the atomic table exchange.

> However i guess you want to be able to see both.

Not really "want", its necessary, otherwise the quota will be refilled
on each ruleset update.

> And in my case,
> saving the remaining quota with iptables-save is what i want. (I case of the
> router for some reason craches, the last state must be saved and as accurate
> as possible.)

I can see that this would also be useful, however none of the other
stateful matches does this, so the more important point IMO is to
get quota to do the expected thing. If we can do both, also fine,
but it would have to be optional and I can't see how to cleanly do
this.

> Solution for this might be using two parameters for the userspace struct.
> One for initial quota and one for remaining, saving both. Possible even an
> option saying with figure should be set when iptables-restore is used.

Yes. Without breaking compatibility, we could make quota fixed, use
the master pointer to store the current quota and put the new master
pointer somewhere outside of the struct. But thats really not very
pretty.

> Still, we can't nicly access the remaining quota with priv_data unless the
> modules each time it "matches" writes in both priv_data and userspace info,
> which is kinda ugly.

As I said, I don't see saving current state as necessary (we don't do
it anywhere and it solves an entirely different problem). The
unfortunate fact though is that we need to pass it to userspace anyway,
because of the limitations of the userspace interface.

> There is also the way of using a /proc-entry to list the current remaining
> quota. Also restoring it by echoing a new figure to the entry. Keep the
> iptables-save/restore only with the initial quota figure.
> 
> I can write this if this is what we want?


Definitely not :) We want a better userspace interface that doesn't
require us to put in hacks like this.

Re: xt_quota (Was: [PATCH] priv_data 0/2)

[Joakim Axelsson]

2006-08-14 17:43:29+0200, Patrick McHardy <kaber@trash.net> ->
>
> As I said, I don't see saving current state as necessary (we don't do
> it anywhere and it solves an entirely different problem). The
> unfortunate fact though is that we need to pass it to userspace anyway,
> because of the limitations of the userspace interface.
> 

That might be correct. However the other modules doesn't normally suffer
that much as they will farily quickly work thier way to the same state
again. I guess it's very uncommon that you are using so low limits like a
few packets each hour.

> > There is also the way of using a /proc-entry to list the current remaining
> > quota. Also restoring it by echoing a new figure to the entry. Keep the
> > iptables-save/restore only with the initial quota figure.
> > 
> > I can write this if this is what we want?
> 
> 
> Definitely not :) We want a better userspace interface that doesn't
> require us to put in hacks like this.

I however really needs some way of figuring out how much of the quota that
remains. This is to be able to report this to our users (that receives a
certain number of gigabytes each day). So they can see how much they have
left (using som scripted interface to iptables). Also saving this holy
figure (as it has become :-)) for the user if the router for some reason
craches. This is also the reason i need negative quota figures. The users
are allowed to "borrow" from their future quota. Doing so only under a byte
limiting match (-m lim --limit-bytes 20k/s).

In my opinion its more important to save the remaining quota, rather than
the original. And most important to in some way be able to see how much
is left of the quota.

Perhaps this wil satisfy both of us:

Somehting put out by iptables-save
iptables -m quota --init-quota 1000 --remain-quota 123 --use-quota remain

Somthing you write with iptables to create a new rule:
iptables -m quota --init-quota 1000   (using --use-quota init explicity)
iptables -m quota --init-quota 1000 --use-quota remain 

But this sure is ugly.

--
Joakim Axelsson

Re: xt_quota (Was: [PATCH] priv_data 0/2)

[Patrick McHardy]

Joakim Axelsson wrote:
> I however really needs some way of figuring out how much of the quota that
> remains. This is to be able to report this to our users (that receives a
> certain number of gigabytes each day). So they can see how much they have
> left (using som scripted interface to iptables). Also saving this holy
> figure (as it has become :-)) for the user if the router for some reason
> craches. This is also the reason i need negative quota figures. The users
> are allowed to "borrow" from their future quota. Doing so only under a byte
> limiting match (-m lim --limit-bytes 20k/s).
> 
> In my opinion its more important to save the remaining quota, rather than
> the original. And most important to in some way be able to see how much
> is left of the quota.
> 
> Perhaps this wil satisfy both of us:
> 
> Somehting put out by iptables-save
> iptables -m quota --init-quota 1000 --remain-quota 123 --use-quota remain
> 
> Somthing you write with iptables to create a new rule:
> iptables -m quota --init-quota 1000   (using --use-quota init explicity)
> iptables -m quota --init-quota 1000 --use-quota remain 
> 
> But this sure is ugly.


It should be an explicit flag to iptable-save/restore to save the
current state, because we don't do it anywhere else and therefore
it is unexpected. The limit match for example does neither show nor
save the current amount of tokens, last refill time, ...
And I'm not too much of a fan of adding such a flag because it can
only be done for a subset of all modules, hashlimit, recent etc.
all can't do it. The most extreme case would be the state match :)

With a netlink API we could actually dump all internal state (including
things like recently seen IP addresses) and accept changes from
userspace. This would allow us to get rid of the ugly proc interfaces
and covers your need as well.