[ebtables-compat PATCH 1/2] nft-shared: rework inversion of matches

[ebtables-compat PATCH 1/2] nft-shared: rework inversion of matches

[Arturo Borrero Gonzalez]

Previous to this patch, inversion of matches didn't work in ebtables-compat.

A way to solve this is to refactor the logic to calculate the proper NFT_CMP_*
operation, and let ebtables-compat to use this new logic.

With this patch, rules like this are correctly sent to the kernel:
 % ebtables-compat -A FORWARD -p ! 0x0800 -j ACCEPT

Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
---
 iptables/nft-bridge.c |   21 ++++++---------------
 iptables/nft-shared.c |   44 ++++++++++++++++++--------------------------
 iptables/nft-shared.h |    1 +
 3 files changed, 25 insertions(+), 41 deletions(-)

diff --git a/iptables/nft-bridge.c b/iptables/nft-bridge.c
index 0e21b46..70501ac 100644
--- a/iptables/nft-bridge.c
+++ b/iptables/nft-bridge.c
@@ -99,15 +99,10 @@ static uint16_t ipt_to_ebt_flags(uint8_t invflags)
 static void add_logical_iniface(struct nft_rule *r, char *iface, int invflags)
 {
 	int iface_len;
-	uint32_t op;
+	uint16_t op = invflags2cmp_op(invflags);
 
 	iface_len = strlen(iface);
 
-	if (invflags & EBT_ILOGICALIN)
-		op = NFT_CMP_NEQ;
-	else
-		op = NFT_CMP_EQ;
-
 	add_meta(r, NFT_META_BRI_IIFNAME);
 	if (iface[iface_len - 1] == '+')
 		add_cmp_ptr(r, op, iface, iface_len - 1);
@@ -118,15 +113,10 @@ static void add_logical_iniface(struct nft_rule *r, char *iface, int invflags)
 static void add_logical_outiface(struct nft_rule *r, char *iface, int invflags)
 {
 	int iface_len;
-	uint32_t op;
+	uint16_t op = invflags2cmp_op(invflags);
 
 	iface_len = strlen(iface);
 
-	if (invflags & EBT_ILOGICALOUT)
-		op = NFT_CMP_NEQ;
-	else
-		op = NFT_CMP_EQ;
-
 	add_meta(r, NFT_META_BRI_OIFNAME);
 	if (iface[iface_len - 1] == '+')
 		add_cmp_ptr(r, op, iface, iface_len - 1);
@@ -165,6 +155,7 @@ static int nft_bridge_add(struct nft_rule *r, void *data)
 	struct ebtables_command_state *cs = data;
 	struct ebt_entry *fw = &cs->fw;
 	uint8_t flags = ebt_to_ipt_flags(fw->invflags);
+	uint32_t op = invflags2cmp_op(flags);
 	char *addr;
 
 	if (fw->in[0] != '\0')
@@ -182,18 +173,18 @@ static int nft_bridge_add(struct nft_rule *r, void *data)
 	addr = ether_ntoa((struct ether_addr *) fw->sourcemac);
 	if (strcmp(addr, "0:0:0:0:0:0") != 0) {
 		add_payload(r, offsetof(struct ethhdr, h_source), 6);
-		add_cmp_ptr(r, NFT_CMP_EQ, fw->sourcemac, 6);
+		add_cmp_ptr(r, op, fw->sourcemac, 6);
 	}
 
 	addr = ether_ntoa((struct ether_addr *) fw->destmac);
 	if (strcmp(addr, "0:0:0:0:0:0") != 0) {
 		add_payload(r, offsetof(struct ethhdr, h_dest), 6);
-		add_cmp_ptr(r, NFT_CMP_EQ, fw->destmac, 6);
+		add_cmp_ptr(r, op, fw->destmac, 6);
 	}
 
 	if (fw->ethproto != 0) {
 		add_payload(r, offsetof(struct ethhdr, h_proto), 2);
-		add_cmp_u16(r, fw->ethproto, NFT_CMP_EQ);
+		add_cmp_u16(r, fw->ethproto, op);
 	}
 
 	return _add_action(r, cs);
diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c
index d6f838c..2e6bf74 100644
--- a/iptables/nft-shared.c
+++ b/iptables/nft-shared.c
@@ -34,6 +34,20 @@ extern struct nft_family_ops nft_family_ops_ipv6;
 extern struct nft_family_ops nft_family_ops_arp;
 extern struct nft_family_ops nft_family_ops_bridge;
 
+uint16_t invflags2cmp_op(uint16_t invflags)
+{
+	if ((invflags & XT_INV_PROTO)
+	    || (invflags & IPT_INV_PROTO)
+	    || (invflags & IPT_INV_VIA_IN)
+	    || (invflags & IPT_INV_VIA_OUT)
+	    || (invflags & IPT_INV_SRCIP)
+	    || (invflags & IPT_INV_DSTIP)
+	    || (invflags & IPT_INV_FRAG))
+		return NFT_CMP_NEQ;
+
+	return NFT_CMP_EQ;
+}
+
 void add_meta(struct nft_rule *r, uint32_t key)
 {
 	struct nft_rule_expr *expr;
@@ -134,15 +148,10 @@ void add_cmp_u32(struct nft_rule *r, uint32_t val, uint32_t op)
 void add_iniface(struct nft_rule *r, char *iface, int invflags)
 {
 	int iface_len;
-	uint32_t op;
+	uint32_t op = invflags2cmp_op(invflags);
 
 	iface_len = strlen(iface);
 
-	if (invflags & IPT_INV_VIA_IN)
-		op = NFT_CMP_NEQ;
-	else
-		op = NFT_CMP_EQ;
-
 	add_meta(r, NFT_META_IIFNAME);
 	if (iface[iface_len - 1] == '+')
 		add_cmp_ptr(r, op, iface, iface_len - 1);
@@ -153,15 +162,10 @@ void add_iniface(struct nft_rule *r, char *iface, int invflags)
 void add_outiface(struct nft_rule *r, char *iface, int invflags)
 {
 	int iface_len;
-	uint32_t op;
+	uint32_t op = invflags2cmp_op(invflags);
 
 	iface_len = strlen(iface);
 
-	if (invflags & IPT_INV_VIA_OUT)
-		op = NFT_CMP_NEQ;
-	else
-		op = NFT_CMP_EQ;
-
 	add_meta(r, NFT_META_OIFNAME);
 	if (iface[iface_len - 1] == '+')
 		add_cmp_ptr(r, op, iface, iface_len - 1);
@@ -172,31 +176,19 @@ void add_outiface(struct nft_rule *r, char *iface, int invflags)
 void add_addr(struct nft_rule *r, int offset,
 	      void *data, void *mask, size_t len, int invflags)
 {
-	uint32_t op;
+	uint32_t op = invflags2cmp_op(invflags);
 
 	add_payload(r, offset, len);
 	add_bitwise(r, mask, len);
-
-	if (invflags & IPT_INV_SRCIP || invflags & IPT_INV_DSTIP)
-		op = NFT_CMP_NEQ;
-	else
-		op = NFT_CMP_EQ;
-
 	add_cmp_ptr(r, op, data, len);
 }
 
 void add_proto(struct nft_rule *r, int offset, size_t len,
 	       uint8_t proto, int invflags)
 {
-	uint32_t op;
+	uint32_t op = invflags2cmp_op(invflags);
 
 	add_payload(r, offset, len);
-
-	if (invflags & XT_INV_PROTO)
-		op = NFT_CMP_NEQ;
-	else
-		op = NFT_CMP_EQ;
-
 	add_cmp_u8(r, proto, op);
 }
 
diff --git a/iptables/nft-shared.h b/iptables/nft-shared.h
index f637b6e..1048d3b 100644
--- a/iptables/nft-shared.h
+++ b/iptables/nft-shared.h
@@ -102,6 +102,7 @@ struct nft_family_ops {
 			  void *data);
 };
 
+uint16_t invflags2cmp_op(uint16_t invflags);
 void add_meta(struct nft_rule *r, uint32_t key);
 void add_payload(struct nft_rule *r, int offset, int len);
 void add_bitwise_u16(struct nft_rule *r, int mask, int xor);

[ebtables-compat PATCH 2/2] nft-bridge: fix printing of inverted protocols, addresses

[Arturo Borrero Gonzalez]

Previous to this patch, no '!' is printed in payload comparisions.
This patch solves it, so we can print for example inverted protocols:

 % ebtables-compat -L
[...]
-p ! 0x800 -j ACCEPT

Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
---
 iptables/nft-bridge.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/iptables/nft-bridge.c b/iptables/nft-bridge.c
index 70501ac..891015a 100644
--- a/iptables/nft-bridge.c
+++ b/iptables/nft-bridge.c
@@ -259,15 +259,21 @@ static void nft_bridge_parse_payload(struct nft_xt_ctx *ctx,
 		get_cmp_data(e, addr, sizeof(addr), &inv);
 		for (i = 0; i < ETH_ALEN; i++)
 			fw->destmac[i] = addr[i];
+		if (inv)
+			fw->invflags |= EBT_IDEST;
 		break;
 	case offsetof(struct ethhdr, h_source):
 		get_cmp_data(e, addr, sizeof(addr), &inv);
 		for (i = 0; i < ETH_ALEN; i++)
 			fw->sourcemac[i] = addr[i];
+		if (inv)
+			fw->invflags |= EBT_ISOURCE;
 		break;
 	case offsetof(struct ethhdr, h_proto):
 		get_cmp_data(e, &ethproto, sizeof(ethproto), &inv);
 		fw->ethproto = ethproto;
+		if (inv)
+			fw->invflags |= EBT_IPROTO;
 		break;
 	}
 }

Re: [ebtables-compat PATCH 1/2] nft-shared: rework inversion of matches

[Pablo Neira Ayuso]

On Tue, Oct 28, 2014 at 07:32:59PM +0100, Arturo Borrero Gonzalez wrote:
> Previous to this patch, inversion of matches didn't work in ebtables-compat.
>
> A way to solve this is to refactor the logic to calculate the proper NFT_CMP_*
> operation, and let ebtables-compat to use this new logic.
>
> With this patch, rules like this are correctly sent to the kernel:
> ebtables-compat -A FORWARD -p ! 0x0800 -j ACCEPT
>
> Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
> ---
>  iptables/nft-bridge.c |   21 ++++++---------------
>  iptables/nft-shared.c |   44 ++++++++++++++++++--------------------------
>  iptables/nft-shared.h |    1 +
>  3 files changed, 25 insertions(+), 41 deletions(-)
>
[...]
> diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c
> index d6f838c..2e6bf74 100644
> --- a/iptables/nft-shared.c
> +++ b/iptables/nft-shared.c
> @@ -34,6 +34,20 @@ extern struct nft_family_ops nft_family_ops_ipv6;
>  extern struct nft_family_ops nft_family_ops_arp;
>  extern struct nft_family_ops nft_family_ops_bridge;
>
> +uint16_t invflags2cmp_op(uint16_t invflags)
> +{
> +	if ((invflags & XT_INV_PROTO)
> +	    || (invflags & IPT_INV_PROTO)
> +	    || (invflags & IPT_INV_VIA_IN)
> +	    || (invflags & IPT_INV_VIA_OUT)
> +	    || (invflags & IPT_INV_SRCIP)
> +	    || (invflags & IPT_INV_DSTIP)
> +	    || (invflags & IPT_INV_FRAG))

Coding style nitpick:

        if (a ||
            b ||
            c)
                ...

I know iptables doesn't fulfill this in many cases, fixing coding
style there is not worth, but new code should at least try to do it
right.


> +		return NFT_CMP_NEQ;
> +
> +	return NFT_CMP_EQ;
> +}
> +
>  void add_meta(struct nft_rule *r, uint32_t key)
>  {
>  	struct nft_rule_expr *expr;
> @@ -134,15 +148,10 @@ void add_cmp_u32(struct nft_rule *r, uint32_t val, uint32_t op)
>  void add_iniface(struct nft_rule *r, char *iface, int invflags)
>  {
>  	int iface_len;
> -	uint32_t op;
> +	uint32_t op = invflags2cmp_op(invflags);

add_iniface() is used from all families, but note one important thing.
Let's have a look at the inv flag in all families.

In IPv4:

#define IPT_INV_VIA_IN          0x01    /* Invert the sense of IN IFACE. */
#define IPT_INV_VIA_OUT         0x02    /* Invert the sense of OUT IFACE */
#define IPT_INV_TOS             0x04    /* Invert the sense of TOS. */
#define IPT_INV_SRCIP           0x08    /* Invert the sense of SRC IP. */
#define IPT_INV_DSTIP           0x10    /* Invert the sense of DST OP. */
#define IPT_INV_FRAG            0x20    /* Invert the sense of FRAG. */
#define IPT_INV_PROTO           XT_INV_PROTO
#define IPT_INV_MASK            0x7F    /* All possible flag bits mask. */

In IPv6:

#define IP6T_INV_VIA_IN         0x01    /* Invert the sense of IN IFACE. */
#define IP6T_INV_VIA_OUT        0x02    /* Invert the sense of OUT IFACE */
#define IP6T_INV_TOS            0x04    /* Invert the sense of TOS. */
#define IP6T_INV_SRCIP          0x08    /* Invert the sense of SRC IP.*/
#define IP6T_INV_DSTIP          0x10    /* Invert the sense of DST OP.*/
#define IP6T_INV_FRAG           0x20    /* Invert the sense of FRAG.*/
#define IP6T_INV_PROTO          XT_INV_PROTO
#define IP6T_INV_MASK           0x7F    /* All possible flag bits mask. */


OK, the definitions match, good.

Now arp:

#define ARPT_INV_VIA_IN         0x0001  /* Invert the sense of IN IFACE. */
#define ARPT_INV_VIA_OUT        0x0002  /* Invert the sense of OUT IFACE */
#define ARPT_INV_SRCIP          0x0004  /* Invert the sense of SRC IP. */
#define ARPT_INV_TGTIP          0x0008  /* Invert the sense of TGT IP. */
#define ARPT_INV_SRCDEVADDR     0x0010  /* Invert the sense of SRC DEV ADDR. */
#define ARPT_INV_TGTDEVADDR     0x0020  /* Invert the sense of TGT DEV ADDR. */
#define ARPT_INV_ARPOP          0x0040  /* Invert the sense of ARP OP. */
#define ARPT_INV_ARPHRD         0x0080  /* Invert the sense of ARP HRD. */
#define ARPT_INV_ARPPRO         0x0100  /* Invert the sense of ARP PRO. */
#define ARPT_INV_ARPHLN         0x0200  /* Invert the sense of ARP HLN. */
#define ARPT_INV_MASK           0x03FF  /* All possible flag bits mask. */

Now, these don't match with ipv4 and ipv6.

Similar thing with ebtables.

I'm telling this because add_iniface() needs to be generalized to
something like:

void add_iniface(struct nft_rule *r, char *iface, bool neg)
                                                  ^------^

So we check for the specific flag from the nft-FAMILY.c class to
interpret the flags that we received from the parser and then we can
this with neg true or false depending on what we need.

So it's almost this patch, but your invflags2cmp_op() should be
specific for each family. Well, IPv4 and IPv6 overlap, so you can
probably put that function in nft-shared.

Remember that the idea was to keep common IPv4 and IPv6 code in
nft-shared.c

nft.c contains the functions that should be global to all supported
compat families.

Thanks.