All of lore.kernel.org
 help / color / mirror / Atom feed
* [nft PATCH] tests/shell: add chain validations tests
@ 2016-03-22 13:06 Arturo Borrero Gonzalez
  2016-03-22 19:20 ` Pablo Neira Ayuso
  2016-03-23  0:09 ` Mart Frauenlob
  0 siblings, 2 replies; 6+ messages in thread
From: Arturo Borrero Gonzalez @ 2016-03-22 13:06 UTC (permalink / raw)
  To: netfilter-devel; +Cc: pablo

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
+
+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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [nft PATCH] tests/shell: add chain validations tests
  2016-03-22 13:06 [nft PATCH] tests/shell: add chain validations tests Arturo Borrero Gonzalez
@ 2016-03-22 19:20 ` Pablo Neira Ayuso
  2016-03-23  7:44   ` Arturo Borrero Gonzalez
  2016-03-23  0:09 ` Mart Frauenlob
  1 sibling, 1 reply; 6+ messages in thread
From: Pablo Neira Ayuso @ 2016-03-22 19:20 UTC (permalink / raw)
  To: Arturo Borrero Gonzalez; +Cc: netfilter-devel

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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [nft PATCH] tests/shell: add chain validations tests
  2016-03-22 13:06 [nft PATCH] tests/shell: add chain validations tests Arturo Borrero Gonzalez
  2016-03-22 19:20 ` Pablo Neira Ayuso
@ 2016-03-23  0:09 ` Mart Frauenlob
  2016-03-23  8:02   ` Arturo Borrero Gonzalez
  1 sibling, 1 reply; 6+ messages in thread
From: Mart Frauenlob @ 2016-03-23  0:09 UTC (permalink / raw)
  To: Arturo Borrero Gonzalez; +Cc: netfilter-devel, pablo

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [nft PATCH] tests/shell: add chain validations tests
  2016-03-22 19:20 ` Pablo Neira Ayuso
@ 2016-03-23  7:44   ` Arturo Borrero Gonzalez
  2016-03-29 11:16     ` Pablo Neira Ayuso
  0 siblings, 1 reply; 6+ messages in thread
From: Arturo Borrero Gonzalez @ 2016-03-23  7:44 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: Netfilter Development Mailing list

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.

-- 
Arturo Borrero González
--
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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [nft PATCH] tests/shell: add chain validations tests
  2016-03-23  0:09 ` Mart Frauenlob
@ 2016-03-23  8:02   ` Arturo Borrero Gonzalez
  0 siblings, 0 replies; 6+ messages in thread
From: Arturo Borrero Gonzalez @ 2016-03-23  8:02 UTC (permalink / raw)
  To: mart.frauenlob; +Cc: Netfilter Development Mailing list, Pablo Neira Ayuso

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.
-- 
Arturo Borrero González
--
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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [nft PATCH] tests/shell: add chain validations tests
  2016-03-23  7:44   ` Arturo Borrero Gonzalez
@ 2016-03-29 11:16     ` Pablo Neira Ayuso
  0 siblings, 0 replies; 6+ messages in thread
From: Pablo Neira Ayuso @ 2016-03-29 11:16 UTC (permalink / raw)
  To: Arturo Borrero Gonzalez; +Cc: Netfilter Development Mailing list

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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2016-03-29 11:17 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-22 13:06 [nft PATCH] tests/shell: add chain validations tests Arturo Borrero Gonzalez
2016-03-22 19:20 ` Pablo Neira Ayuso
2016-03-23  7:44   ` Arturo Borrero Gonzalez
2016-03-29 11:16     ` Pablo Neira Ayuso
2016-03-23  0:09 ` Mart Frauenlob
2016-03-23  8:02   ` Arturo Borrero Gonzalez

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.