| Message ID | 145865193086.6118.9200431804176858644.stgit@nfdev2.cica.es |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
On Tue, Mar 22, 2016 at 02:06:09PM +0100, Arturo Borrero Gonzalez wrote: > Some basic test regarding chains: jumps and validations. > > Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com> > --- > NOTE: the testcases/chains/0009masquerade_jump_1 file fails, seems like a bug > in the kernel validation. Needs more investigation. I can see this there: > +$NFT add chain t output {type nat hook output priority 0 \; } We only support masquerade from postrouting. static struct xt_target masquerade_tg_reg __read_mostly = { .name = "MASQUERADE", .family = NFPROTO_IPV4, .target = masquerade_tg, .targetsize = sizeof(struct nf_nat_ipv4_multi_range_compat), .table = "nat", .hooks = 1 << NF_INET_POST_ROUTING, BTW, it would be good to add more tests to exercise the chain loop detection code. Please, fix and resubmit, thanks. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Good day, On 22.03.2016 14:06, Arturo Borrero Gonzalez wrote: > Some basic test regarding chains: jumps and validations. > > Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com> > --- > NOTE: the testcases/chains/0009masquerade_jump_1 file fails, seems like a bug > in the kernel validation. Needs more investigation. > > tests/shell/testcases/chains/0001jumps_0 | 17 +++++++++++++++ > tests/shell/testcases/chains/0002jumps_1 | 22 ++++++++++++++++++++ > tests/shell/testcases/chains/0003jump_loop_1 | 21 +++++++++++++++++++ > tests/shell/testcases/chains/0004busy_1 | 11 ++++++++++ > tests/shell/testcases/chains/0005busy_map_1 | 11 ++++++++++ > tests/shell/testcases/chains/0006masquerade_0 | 7 ++++++ > tests/shell/testcases/chains/0007masquerade_1 | 9 ++++++++ > tests/shell/testcases/chains/0008masquerade_jump_1 | 11 ++++++++++ > tests/shell/testcases/chains/0009masquerade_jump_1 | 11 ++++++++++ > 9 files changed, 120 insertions(+) > create mode 100755 tests/shell/testcases/chains/0001jumps_0 > create mode 100755 tests/shell/testcases/chains/0002jumps_1 > create mode 100755 tests/shell/testcases/chains/0003jump_loop_1 > create mode 100755 tests/shell/testcases/chains/0004busy_1 > create mode 100755 tests/shell/testcases/chains/0005busy_map_1 > create mode 100755 tests/shell/testcases/chains/0006masquerade_0 > create mode 100755 tests/shell/testcases/chains/0007masquerade_1 > create mode 100755 tests/shell/testcases/chains/0008masquerade_jump_1 > create mode 100755 tests/shell/testcases/chains/0009masquerade_jump_1 > > diff --git a/tests/shell/testcases/chains/0001jumps_0 b/tests/shell/testcases/chains/0001jumps_0 > new file mode 100755 > index 0000000..b39df38 > --- /dev/null > +++ b/tests/shell/testcases/chains/0001jumps_0 > @@ -0,0 +1,17 @@ > +#!/bin/bash I've not looked up the code calling this, but: First: bash only? Second: It's not granted to be in /bin. Third: May not be the wanted version. So a shebang like: #!/usr/bin/env bash or #!/urs/bin/env sh should be more compatible and fail proof. > + > +set -e > + > +MAX_JUMPS=16 > + > +$NFT add table t Unquoted variable, may fail if, unlikely but possible, the name contains i.e. spaces. > + > +for i in $(seq 1 $MAX_JUMPS) > +do > + $NFT add chain t c${i} > +done Requires `seq' binary. I think for ((i=1; i<=$MAX_JUMPS; i++)) is more portable. > + > +for i in $(seq 1 $((MAX_JUMPS - 1))) > +do > + $NFT add rule t c${i} jump c$((i + 1)) > +done Why not add functions? i.e. runft() { "$NFT" "$@" } nfat() { runft add table "$@" } nfac() { runft add chain "$@" } .... [...] Best regards, Mart -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 22 March 2016 at 20:20, Pablo Neira Ayuso <pablo@netfilter.org> wrote: > On Tue, Mar 22, 2016 at 02:06:09PM +0100, Arturo Borrero Gonzalez wrote: >> Some basic test regarding chains: jumps and validations. >> >> Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com> >> --- >> NOTE: the testcases/chains/0009masquerade_jump_1 file fails, seems like a bug >> in the kernel validation. Needs more investigation. > > I can see this there: > >> +$NFT add chain t output {type nat hook output priority 0 \; } > > We only support masquerade from postrouting. > > static struct xt_target masquerade_tg_reg __read_mostly = { > .name = "MASQUERADE", > .family = NFPROTO_IPV4, > .target = masquerade_tg, > .targetsize = sizeof(struct nf_nat_ipv4_multi_range_compat), > .table = "nat", > .hooks = 1 << NF_INET_POST_ROUTING, > > BTW, it would be good to add more tests to exercise the chain loop > detection code. > > Please, fix and resubmit, thanks. Probably mi description of the problem was poor. The offending testcase is testing, in fact, that we can add a rule with a jump to a chain with a masquerade rule, thus connecting masquerade to a output hook: $NFT add table t $NFT add chain t output {type nat hook output priority 0 \; } $NFT add chain t c1 $NFT add rule t c1 masquerade $NFT add rule t output tcp dport vmap {1 :jump c1 } this don't fail, and that's the problem indeed.
On 23 March 2016 at 01:09, Mart Frauenlob <mart.frauenlob@chello.at> wrote: > > I've not looked up the code calling this, but: > First: bash only? > Second: It's not granted to be in /bin. > Third: May not be the wanted version. > > So a shebang like: > #!/usr/bin/env bash > or > #!/urs/bin/env sh > should be more compatible and fail proof. > >> + >> +set -e >> + >> +MAX_JUMPS=16 >> + >> +$NFT add table t > > Unquoted variable, may fail if, unlikely but possible, the name contains > i.e. spaces. > >> + >> +for i in $(seq 1 $MAX_JUMPS) >> +do >> + $NFT add chain t c${i} >> +done > > Requires `seq' binary. > I think for ((i=1; i<=$MAX_JUMPS; i++)) is more portable. > >> + >> +for i in $(seq 1 $((MAX_JUMPS - 1))) >> +do >> + $NFT add rule t c${i} jump c$((i + 1)) >> +done > > > Why not add functions? i.e. > Hi Mart, yes, bash only :-) I think it's a very good shell. I'm not using any bash operand which depends on the version... I don't expect anybody to run this testsuite in bash v1 (20 years old already). I didn't really care about portability... this is an internal script to perform internal tests, this is going to be run in very few systems. Also, no functions, because I want the nft commands to be really really clear, so debugging is easier. Is this testsuite failing for you? Have you run it? I would really like to review patches to incrementally improve the testsuite, which has been deliberately simplified to focus on what matter for us: testing nftables :-) thanks, best regards.
On Wed, Mar 23, 2016 at 08:44:31AM +0100, Arturo Borrero Gonzalez wrote: > On 22 March 2016 at 20:20, Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > On Tue, Mar 22, 2016 at 02:06:09PM +0100, Arturo Borrero Gonzalez wrote: > >> Some basic test regarding chains: jumps and validations. > >> > >> Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com> > >> --- > >> NOTE: the testcases/chains/0009masquerade_jump_1 file fails, seems like a bug > >> in the kernel validation. Needs more investigation. > > > > I can see this there: > > > >> +$NFT add chain t output {type nat hook output priority 0 \; } > > > > We only support masquerade from postrouting. > > > > static struct xt_target masquerade_tg_reg __read_mostly = { > > .name = "MASQUERADE", > > .family = NFPROTO_IPV4, > > .target = masquerade_tg, > > .targetsize = sizeof(struct nf_nat_ipv4_multi_range_compat), > > .table = "nat", > > .hooks = 1 << NF_INET_POST_ROUTING, > > > > BTW, it would be good to add more tests to exercise the chain loop > > detection code. > > > > Please, fix and resubmit, thanks. > > Probably mi description of the problem was poor. > > The offending testcase is testing, in fact, that we can add a rule > with a jump to a chain with a masquerade rule, thus connecting > masquerade to a output hook: > > $NFT add table t > $NFT add chain t output {type nat hook output priority 0 \; } > $NFT add chain t c1 > $NFT add rule t c1 masquerade > $NFT add rule t output tcp dport vmap {1 :jump c1 } > > this don't fail, and that's the problem indeed. Right, I'm applying this so we keep this in the radar, thanks. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/tests/shell/testcases/chains/0001jumps_0 b/tests/shell/testcases/chains/0001jumps_0 new file mode 100755 index 0000000..b39df38 --- /dev/null +++ b/tests/shell/testcases/chains/0001jumps_0 @@ -0,0 +1,17 @@ +#!/bin/bash + +set -e + +MAX_JUMPS=16 + +$NFT add table t + +for i in $(seq 1 $MAX_JUMPS) +do + $NFT add chain t c${i} +done + +for i in $(seq 1 $((MAX_JUMPS - 1))) +do + $NFT add rule t c${i} jump c$((i + 1)) +done diff --git a/tests/shell/testcases/chains/0002jumps_1 b/tests/shell/testcases/chains/0002jumps_1 new file mode 100755 index 0000000..0cc8928 --- /dev/null +++ b/tests/shell/testcases/chains/0002jumps_1 @@ -0,0 +1,22 @@ +#!/bin/bash + +set -e + +MAX_JUMPS=16 + +$NFT add table t + +for i in $(seq 1 $MAX_JUMPS) +do + $NFT add chain t c${i} +done + +for i in $(seq 1 $((MAX_JUMPS - 1))) +do + $NFT add rule t c${i} jump c$((i + 1)) +done + +# this last jump should fail: too many links +$NFT add chain t c$((MAX_JUMPS + 1)) +$NFT add rule t c${MAX_JUMPS} jump c$((MAX_JUMPS + 1)) 2>/dev/null +echo "E: max jumps ignored?" >&2 diff --git a/tests/shell/testcases/chains/0003jump_loop_1 b/tests/shell/testcases/chains/0003jump_loop_1 new file mode 100755 index 0000000..f74361f --- /dev/null +++ b/tests/shell/testcases/chains/0003jump_loop_1 @@ -0,0 +1,21 @@ +#!/bin/bash + +set -e + +MAX_JUMPS=16 + +$NFT add table t + +for i in $(seq 1 $MAX_JUMPS) +do + $NFT add chain t c${i} +done + +for i in $(seq 1 $((MAX_JUMPS - 1))) +do + $NFT add rule t c${i} jump c$((i + 1)) +done + +# this last jump should fail: loop +$NFT add rule t c${MAX_JUMPS} jump c1 2>/dev/null +echo "E: loop of jumps ignored?" >&2 diff --git a/tests/shell/testcases/chains/0004busy_1 b/tests/shell/testcases/chains/0004busy_1 new file mode 100755 index 0000000..cc9a0da --- /dev/null +++ b/tests/shell/testcases/chains/0004busy_1 @@ -0,0 +1,11 @@ +#!/bin/bash + +set -e + +$NFT add table t +$NFT add chain t c1 +$NFT add chain t c2 +$NFT add rule t c1 jump c2 +# kernel should return EBUSY +$NFT delete chain t c2 2>/dev/null +echo "E: deleted a busy chain?" >&2 diff --git a/tests/shell/testcases/chains/0005busy_map_1 b/tests/shell/testcases/chains/0005busy_map_1 new file mode 100755 index 0000000..93eca82 --- /dev/null +++ b/tests/shell/testcases/chains/0005busy_map_1 @@ -0,0 +1,11 @@ +#!/bin/bash + +set -e + +$NFT add table t +$NFT add chain t c1 +$NFT add chain t c2 +$NFT add rule t c1 tcp dport vmap { 1 : jump c2 } +# kernel should return EBUSY +$NFT delete chain t c2 2>/dev/null +echo "E: deleted a busy chain?" >&2 diff --git a/tests/shell/testcases/chains/0006masquerade_0 b/tests/shell/testcases/chains/0006masquerade_0 new file mode 100755 index 0000000..7934998 --- /dev/null +++ b/tests/shell/testcases/chains/0006masquerade_0 @@ -0,0 +1,7 @@ +#!/bin/bash + +set -e + +$NFT add table t +$NFT add chain t c1 {type nat hook postrouting priority 0 \; } +$NFT add rule t c1 masquerade diff --git a/tests/shell/testcases/chains/0007masquerade_1 b/tests/shell/testcases/chains/0007masquerade_1 new file mode 100755 index 0000000..4e98d10 --- /dev/null +++ b/tests/shell/testcases/chains/0007masquerade_1 @@ -0,0 +1,9 @@ +#!/bin/bash + +set -e + +$NFT add table t +$NFT add chain t c1 {type filter hook output priority 0 \; } +# wrong hook output, only postrouting is valid +$NFT add rule t c1 masquerade 2>/dev/null +echo "E: accepted masquerade in output hook" >&2 diff --git a/tests/shell/testcases/chains/0008masquerade_jump_1 b/tests/shell/testcases/chains/0008masquerade_jump_1 new file mode 100755 index 0000000..7754ed0 --- /dev/null +++ b/tests/shell/testcases/chains/0008masquerade_jump_1 @@ -0,0 +1,11 @@ +#!/bin/bash + +set -e + +$NFT add table t +$NFT add chain t output {type nat hook output priority 0 \; } +$NFT add chain t c1 +$NFT add rule t c1 masquerade +# kernel should return EOPNOTSUPP +$NFT add rule t output jump c1 2>/dev/null +echo "E: accepted masquerade in output hook" >&2 diff --git a/tests/shell/testcases/chains/0009masquerade_jump_1 b/tests/shell/testcases/chains/0009masquerade_jump_1 new file mode 100755 index 0000000..684d441 --- /dev/null +++ b/tests/shell/testcases/chains/0009masquerade_jump_1 @@ -0,0 +1,11 @@ +#!/bin/bash + +set -e + +$NFT add table t +$NFT add chain t output {type nat hook output priority 0 \; } +$NFT add chain t c1 +$NFT add rule t c1 masquerade +# kernel should return EOPNOTSUPP +$NFT add rule t output tcp dport vmap {1 :jump c1 } 2>/dev/null +echo "E: accepted masquerade in output hook in a vmap" >&2
Some basic test regarding chains: jumps and validations. Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com> --- NOTE: the testcases/chains/0009masquerade_jump_1 file fails, seems like a bug in the kernel validation. Needs more investigation. tests/shell/testcases/chains/0001jumps_0 | 17 +++++++++++++++ tests/shell/testcases/chains/0002jumps_1 | 22 ++++++++++++++++++++ tests/shell/testcases/chains/0003jump_loop_1 | 21 +++++++++++++++++++ tests/shell/testcases/chains/0004busy_1 | 11 ++++++++++ tests/shell/testcases/chains/0005busy_map_1 | 11 ++++++++++ tests/shell/testcases/chains/0006masquerade_0 | 7 ++++++ tests/shell/testcases/chains/0007masquerade_1 | 9 ++++++++ tests/shell/testcases/chains/0008masquerade_jump_1 | 11 ++++++++++ tests/shell/testcases/chains/0009masquerade_jump_1 | 11 ++++++++++ 9 files changed, 120 insertions(+) create mode 100755 tests/shell/testcases/chains/0001jumps_0 create mode 100755 tests/shell/testcases/chains/0002jumps_1 create mode 100755 tests/shell/testcases/chains/0003jump_loop_1 create mode 100755 tests/shell/testcases/chains/0004busy_1 create mode 100755 tests/shell/testcases/chains/0005busy_map_1 create mode 100755 tests/shell/testcases/chains/0006masquerade_0 create mode 100755 tests/shell/testcases/chains/0007masquerade_1 create mode 100755 tests/shell/testcases/chains/0008masquerade_jump_1 create mode 100755 tests/shell/testcases/chains/0009masquerade_jump_1 -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html