[PATCHv2] Fix dumping vlan rules

From: "M. Braun" <michael-dev@fami-braun.de>

Given the following bridge rules:
1. ip protocol icmp accept
2. ether type vlan vlan type ip ip protocol icmp accept

The are currently both dumped by "nft list ruleset" as
1. ip protocol icmp accept
2. ip protocol icmp accept

Though, the netlink code actually is different

bridge filter FORWARD 4
  [ payload load 2b @ link header + 12 => reg 1 ]
  [ cmp eq reg 1 0x00000008 ]
  [ payload load 1b @ network header + 9 => reg 1 ]
  [ cmp eq reg 1 0x00000001 ]
  [ immediate reg 0 accept ]

bridge filter FORWARD 5 4
  [ payload load 2b @ link header + 12 => reg 1 ]
  [ cmp eq reg 1 0x00000081 ]
  [ payload load 2b @ link header + 16 => reg 1 ]
  [ cmp eq reg 1 0x00000008 ]
  [ payload load 1b @ network header + 9 => reg 1 ]
  [ cmp eq reg 1 0x00000001 ]
  [ immediate reg 0 accept ]

Fix this by avoiding the removal of all vlan statements
in the given example.

Signed-off-by: Michael Braun <michael-dev@fami-braun.de>
---
 src/payload.c                         | 12 ++++++++++++
 tests/py/bridge/vlan.t                |  2 ++
 tests/py/bridge/vlan.t.payload        | 10 ++++++++++
 tests/py/bridge/vlan.t.payload.netdev | 12 ++++++++++++
 4 files changed, 36 insertions(+)

On Mon, Jul 15, 2019 at 06:59:01PM +0200, michael-dev@fami-braun.de wrote:
> From: "M. Braun" <michael-dev@fami-braun.de>
> 
> Given the following bridge rules:
> 1. ip protocol icmp accept
> 2. ether type vlan vlan type ip ip protocol icmp accept

No testcase for #2?

> The are currently both dumped by "nft list ruleset" as
> 1. ip protocol icmp accept
> 2. ip protocol icmp accept
> 
> Though, the netlink code actually is different

Good catch.

> bridge filter FORWARD 4
>   [ payload load 2b @ link header + 12 => reg 1 ]
>   [ cmp eq reg 1 0x00000008 ]
>   [ payload load 1b @ network header + 9 => reg 1 ]
>   [ cmp eq reg 1 0x00000001 ]
>   [ immediate reg 0 accept ]
> 
> bridge filter FORWARD 5 4
>   [ payload load 2b @ link header + 12 => reg 1 ]
>   [ cmp eq reg 1 0x00000081 ]
>   [ payload load 2b @ link header + 16 => reg 1 ]
>   [ cmp eq reg 1 0x00000008 ]
>   [ payload load 1b @ network header + 9 => reg 1 ]
>   [ cmp eq reg 1 0x00000001 ]
>   [ immediate reg 0 accept ]
>
> Fix this by avoiding the removal of all vlan statements
> in the given example.
> 
> Signed-off-by: Michael Braun <michael-dev@fami-braun.de>
> ---
>  src/payload.c                         | 12 ++++++++++++
>  tests/py/bridge/vlan.t                |  2 ++
>  tests/py/bridge/vlan.t.payload        | 10 ++++++++++
>  tests/py/bridge/vlan.t.payload.netdev | 12 ++++++++++++
>  4 files changed, 36 insertions(+)
> 
> diff --git a/src/payload.c b/src/payload.c
> index 3bf1ecc..905422a 100644
> --- a/src/payload.c
> +++ b/src/payload.c
> @@ -506,6 +506,18 @@ static bool payload_may_dependency_kill(struct payload_dep_ctx *ctx,
>  		     dep->left->payload.desc == &proto_ip6) &&
>  		    expr->payload.base == PROTO_BASE_TRANSPORT_HDR)
>  			return false;
> +		/* Do not kill
> +		 *  ether type vlan and vlan type ip and ip protocol icmp
> +		 * into
> +		 *  ip protocol icmp

So, what happens here is that:

        #1 vlan type ip kills ether type vlan
        #2 ip protocol icmp kills vlan type ip

right?

> +		 * as this lacks ether type vlan.
> +		 * More generally speaking, do not kill protocol type
> +		 * for stacked protocols if we only have protcol type matches.

s/protcol/protocol

> +		 */
> +		if (dep->left->etype == EXPR_PAYLOAD && dep->op == OP_EQ &&
> +		    expr->flags & EXPR_F_PROTOCOL &&
> +		    expr->payload.base == dep->left->payload.base)

If the current expression is a key (EXPR_F_PROTOCOL expressions tells
us what it comes in the upper layer) and base of such expression is
the same as the dependency.

I'd prefer this rule is restricted to vlan, and wait for more similar
usecases before this rule can be generalized.

OK?

Thanks.

Am 15. Juli 2019 20:06:39 MESZ schrieb Pablo Neira Ayuso <pablo@netfilter.org>:
>> Given the following bridge rules:
>> 1. ip protocol icmp accept
>> 2. ether type vlan vlan type ip ip protocol icmp accept
>
>No testcase for #2?

The added testcase covers #2 due to the netlink dump check and thus is basically a synonym with respect to the netlink parser.

>
>So, what happens here is that:
>
>        #1 vlan type ip kills ether type vlan
>        #2 ip protocol icmp kills vlan type ip
>
>right?

Right

>> +		 */
>> +		if (dep->left->etype == EXPR_PAYLOAD && dep->op == OP_EQ &&
>> +		    expr->flags & EXPR_F_PROTOCOL &&
>> +		    expr->payload.base == dep->left->payload.base)
>
>If the current expression is a key (EXPR_F_PROTOCOL expressions tells
>us what it comes in the upper layer) and base of such expression is
>the same as the dependency.
>
>I'd prefer this rule is restricted to vlan, and wait for more similar
>usecases before this rule can be generalized.
>
>OK?

I used nft list ruleset to generate /etc/nftables.conf. In case too few statements are killed, nftables.conf becomes a bit longer but it is still correct and parseable although not minimal. In case too many statements are killed, the semantic changes on next reboot or for review with all kinds of implications.
Therefore killing to many statements seems critical too many, kill too few only like a minor issue. I'd therefore prefer to take the risk of being overly broad here rathen than having incorrect information and thus not restrict this to vlan.

Stacked protocols like ipsec, ipip tunnel or vlan tend to have the same upper layer payload protocol, e.g. udp in ip, udp in ipip or udp in esp/ah. Therefore killing protocol type statements for stacked protocols generally does not look safe to me, as the upper layer will not imply any stacked protocol.

Regards,
M. Braun

On Sat, Jul 27, 2019 at 07:24:24PM +0200, michael-dev@fami-braun.de wrote:
[...]
> I used nft list ruleset to generate /etc/nftables.conf. In case too
> few statements are killed, nftables.conf becomes a bit longer but it
> is still correct and parseable although not minimal. In case too
> many statements are killed, the semantic changes on next reboot or
> for review with all kinds of implications.  Therefore killing to
> many statements seems critical too many, kill too few only like a
> minor issue. I'd therefore prefer to take the risk of being overly
> broad here rathen than having incorrect information and thus not
> restrict this to vlan.
> 
> Stacked protocols like ipsec, ipip tunnel or vlan tend to have the
> same upper layer payload protocol, e.g. udp in ip, udp in ipip or
> udp in esp/ah. Therefore killing protocol type statements for
> stacked protocols generally does not look safe to me, as the upper
> layer will not imply any stacked protocol.

OK. We may have to revisit the stacked protocol logic at some point
though.

Patch is applied. Thanks.

BTW, would you follow up with a fix for json tests?

If I run here:

        nft-tests.py -j

it complains here:

ERROR: did not find JSON equivalent for rule 'ether type vlan ip
protocol 1 accept'

Thanks!