On 22.05.2014 00:48, Luigi Rizzo wrote:
On Wed, May 21, 2014 at 10:10:26PM +0400, Alexander V. Chernikov wrote:
On 21.05.2014 15:10, Luigi Rizzo wrote:
On Mon, May 19, 2014 at 04:51:08PM +0400, Alexander V. Chernikov wrote:
Hello list!
This patch adds ability to name tables / reference them by name.
Additionally, it simplifies adding new table types.
Hi Alex,
at a high level, i think your changes here are in the right direction.
Hello!
However i think part of the design, and also of the patch below,
is not sound/correct so i would like to wait to commit at least
until some of the problems below are resolved.
1. The patch as is has several issues (variable declarations in the
middle of a block, assignment in conditionals, incorrect
comments in cut&pasted code, missing checks on strings)
that should be corrected.
..missing documentation and so on :)
Of course, I'll fix all these (or most of these :))
good thanks
3. there is no explanation, here or in the code, on how the
names are managed. I could try to figure out from the code
but it would be the wrong way to understand things so let me
ask, and please explain what you have in mind.
Currently it is very simple non-resizable hash table with fnv hash based
on table name.
that is not an issue. The question is whether one needs to lookup
the hash table every time you have a 'table' argument (of course i
think one should not, and the implementation i propose below gives
direct access to the table without name lookups, as the internal
identifier is still poiniter or small integer, just one that is not exposed
to userspace).
Let me address first the name <-> table-id thing.
Introducing a symbolic name for tables is a great and useful feature.
However the implementation has some tricky issues:
- do you want to retain the old numeric identifiers or not ?
I think it is a bad idea to have two namespaces for the same thing,
so if we are switching to alphanumeric strings for tables we should
as well get rid of the numbers (i.e. consider them as strings as well).
I am strongly in favour of not using names as aliases for numbers.
It would require no changes for clients issuing ipfw commands
from a script, and would not require users to to manually handle
the name-id translations.
Well. I'd prefer not to. However, code we're discussing assumes that
numeric ids
are primary ones (e.g. you can't have named, but unnumbered table,
you have to choose number by yourself). Switching to named-only tables
can be tricky since we don't want to match them by inside rules and we
don't want
to loose atomicity when allocating table ids via separate cmds before
adding rule.
i think this is solved by the implementation i proposed below.
It looks like we can do the following:
1) Add another IP_FW3_ADD opcode with the following layout:
{
rule len;
unmodified rule itself;
tlv1 {type=table name;len=..;id=1;"_TABLENAME11_"}
tlv2 {type=table name;len=..;id=2;"_TABLENAME4_"}
..
}
Values inside appropriate opcodes { O_IP_XXX_LOOKUP and so on } won't be
real values
but values described in attached TLVs.
After validating/parsing/checking under UH lock we replace fake values
with real ones (probably, newly allocated)
and return or rollback atomically.
yes this should work, probably not even requiring a new IP_FW3_ADD opcode
because the extra tlv entries can appear in a block by themselves.
The same thing can be done for displaying ruleset, however I'd prefer
client to ask for table names and caching them while displaying.
Old clients will retain the ability to operate on tables but will see
table ids, so nothing will change for them
(except the case when new binary adds table "5" and new binary sees id
which is not 5, but that shouldn't be a problem).
we can solve this by using 'low' numbers for the numeric tables
(these were limited anyways) and allocate the fake entries in
another range.
Currently we have u16 space available in base opcode.
Introducing another range will require additional opcode, additional
array for new tables and so on.
I'd prefer not to do this. Since table ids are allocated by ourselves we
can (and should) pack them
efficiently and 65k _real tables_ currently available is a quite good value.
We preserve compability for old binaries so people with old userland and
scripts should
not not notice anything. We have no public userland API so the only base
binary is /sbin/ipfw which is either old or new.
- The rename command worries me a lot:
> ipfw table <num> name XXX
> Names (or renames) table. Not the name has to be unique.
because it is neither clear nor intuitive whether you want to
1. just rename a table (possibly breaking or creating references
for rules in the firewall), or
2. modify the name-id translation preserving existing pointers.
Consider the sequence
ipfw table 13 name bar
ipfw add 100 count dst-ip table bar
ipfw table 13 name foo
ipfw table 14 name bar
ipfw add 200 count src-port 22 dst-ip table bar
Approach #1 would detach rule 100 from table 13 and then connect to 14
(in a non-atomic way), whereas approach #2 would make rule 100 and 200
point to different tables (which is confusing).
Now part of this problem goes away if we get rid of number altogether.
You may legitimately want to swap tables in an atomic way (we have
something
similar for rulesets):
ipfw set swap number number
There is some problem here:
Atomic ruleset changes is a great thing, but tables have no atomic support.
We, for example, are solving this by using different table range on
every new configuration.
It won't be possible with named-only tables (since names usually care
some semantic in contrast to numbers).
The only thing I can think of is separate namespace per each set.
What do you think?
maybe i am missing some detail but it seems reasonably easy to implement
the atomic swap -- and the use case is when you want to move from
one configuration to a new one:
ipfw table foo-new flush // clear initial content
ipfw table foo-new add ... <repeat as needed>
ipfw table swap foo-current foo-new // swap the content of the table
objects
so you preserve the semantic of the name very easily.
Yes. We can easily add atomic table swap that way. However, I'm talking
about different use scenario:
Atomically swap entire ruleset which has some tables depency:
e.g. we have:
"
100 allow ip from table(TABLE1) to me
200 allow ip from table(TABLE2) to (TABLE3) 80
table TABLE1 1.1.1.1/32
table TABLE1 1.0.0.0/16
table TABLE2 2.2.2.2/32
table TABLE3 3.3.3.3/32
"
and we want to _atomically_ change this to
"
100 allow ip from table(TABLE1) to me
+200 allow ip from table(TABLE4) to any
300 allow ip from table(TABLE2) to (TABLE3) 80
table TABLE1 1.1.1.1/32
-table TABLE1 1.0.0.0/16
-table TABLE2 2.2.2.2/32
+table TABLE2 77.77.77.0/24
table TABLE3 3.3.3.3/32
+table TABLE4 4.4.4.4/32
"
So here is what i would propose:
- [gradually] introduce new commands that accept strings for every place where
a number was previously accepted. Internally in the firewall, the old
16-bit number is interpreted as a string.
Yup.
- within the kernel create a small set of functions (invoked on insert, list,
delete)
that do proper checks on the string and translate it to a pointer (or short
integer,
i.e. an index in an array) to the proper object. Done properly, we can reuse
this code also for pipes, sets, nat groups and whatnot.
Yup.
When a rule references an object that does not exist, you create an empty
object that a subsequent table/pipe/XXX 'create' would initialize.
On 'destroy', no need to touch the ruleset, just clear the object.
Yes. This looks better than requiring user to create table of given type
_before_
referencing it.
- for renames, try to follow the approach used for sets i.e.
ipfw table rename old-name new-name
changes the name, preserves the object.
Does not touch the ruleset, only the translation table
Well, I'm not sure about renaming. I'd prefer permitting renaming iff
table is not referenced.
(and why do we need to rename tables?)
you are right, let's forget it. And 'ipfw set move' actually does a different
thing
which has no use here.
ipfw table swap first-table second-table
atomically swap the content of the two table objects
(once again not touching the rulesets)
cheers
luigi
_______________________________________________
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
