netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] Fix dumping vlan rules
@ 2019-07-13 21:03 michael-dev
  2019-07-13 21:15 ` Laura Garcia
  2019-07-13 21:43 ` Florian Westphal
  0 siblings, 2 replies; 3+ messages in thread
From: michael-dev @ 2019-07-13 21:03 UTC (permalink / raw)
  To: netdev; +Cc: michael-dev

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 ++++++++++++
 1 file changed, 12 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
+		 * as this lacks ether type vlan.
+		 * More generally speaking, do not kill protocol type
+		 * for stacked protocols if we only have protcol type matches.
+		 */
+		if (dep->left->etype == EXPR_PAYLOAD && dep->op == OP_EQ &&
+		    expr->flags & EXPR_F_PROTOCOL &&
+		    expr->payload.base == dep->left->payload.base)
+			return false;
 		break;
 	}
 
-- 
2.20.1


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

* Re: [PATCH] Fix dumping vlan rules
  2019-07-13 21:03 [PATCH] Fix dumping vlan rules michael-dev
@ 2019-07-13 21:15 ` Laura Garcia
  2019-07-13 21:43 ` Florian Westphal
  1 sibling, 0 replies; 3+ messages in thread
From: Laura Garcia @ 2019-07-13 21:15 UTC (permalink / raw)
  To: michael-dev; +Cc: netdev, Netfilter Development Mailing list

CC'ing netfilter.

On Sat, Jul 13, 2019 at 11:10 PM <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
>
> 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 ++++++++++++
>  1 file changed, 12 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
> +                * as this lacks ether type vlan.
> +                * More generally speaking, do not kill protocol type
> +                * for stacked protocols if we only have protcol type matches.
> +                */
> +               if (dep->left->etype == EXPR_PAYLOAD && dep->op == OP_EQ &&
> +                   expr->flags & EXPR_F_PROTOCOL &&
> +                   expr->payload.base == dep->left->payload.base)
> +                       return false;
>                 break;
>         }
>
> --
> 2.20.1
>

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

* Re: [PATCH] Fix dumping vlan rules
  2019-07-13 21:03 [PATCH] Fix dumping vlan rules michael-dev
  2019-07-13 21:15 ` Laura Garcia
@ 2019-07-13 21:43 ` Florian Westphal
  1 sibling, 0 replies; 3+ messages in thread
From: Florian Westphal @ 2019-07-13 21:43 UTC (permalink / raw)
  To: michael-dev; +Cc: netdev, netfilter-devel

michael-dev@fami-braun.de <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
> 
> The are currently both dumped by "nft list ruleset" as
> 1. ip protocol icmp accept
> 2. ip protocol icmp accept

Yes, thats a bug, the dependency removal is incorrect.

> +++ 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
> +		 * as this lacks ether type vlan.
> +		 * More generally speaking, do not kill protocol type
> +		 * for stacked protocols if we only have protcol type matches.
> +		 */
> +		if (dep->left->etype == EXPR_PAYLOAD && dep->op == OP_EQ &&
> +		    expr->flags & EXPR_F_PROTOCOL &&
> +		    expr->payload.base == dep->left->payload.base)
> +			return false;

Can you please add a test case for this problem to
tests/py/bridge/vlan.t so we catch this when messing with dependency
handling in the future?

Also, please submit v2 directly to netfilter-devel@.

Thanks!

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

end of thread, other threads:[~2019-07-13 21:43 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-13 21:03 [PATCH] Fix dumping vlan rules michael-dev
2019-07-13 21:15 ` Laura Garcia
2019-07-13 21:43 ` Florian Westphal

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).