[PATCH nf-next v4 00/10] netfilter: nft_bitwise: shift support

[PATCH nf-next v4 00/10] netfilter: nft_bitwise: shift support

[Jeremy Sowden]

The connmark xtables extension supports bit-shifts.  Add support for
shifts to nft_bitwise in order to allow nftables to do likewise, e.g.:

  nft add rule t c oif lo ct mark set meta mark << 8 | 0xab
  nft add rule t c iif lo meta mark & 0xff 0xab ct mark set meta mark >> 8

Changes since v3:

  * the length of shift values sent by nft may be less than sizeof(u32).

Changes since v2:

  * convert NFTA_BITWISE_DATA from u32 to nft_data;
  * add check that shift value is not too large;
  * use BITS_PER_TYPE to get the size of u32, rather than hard-coding it
    when evaluating shifts.

Changes since v1:

  * more white-space fixes;
  * move bitwise op enum to UAPI;
  * add NFTA_BITWISE_OP and NFTA_BITWISE_DATA;
  * remove NFTA_BITWISE_LSHIFT and NFTA_BITWISE_RSHIFT;
  * add helpers for initializaing, evaluating and dumping different
    types of operation.

Jeremy Sowden (10):
  netfilter: nf_tables: white-space fixes.
  netfilter: bitwise: remove NULL comparisons from attribute checks.
  netfilter: bitwise: replace gotos with returns.
  netfilter: bitwise: add NFTA_BITWISE_OP attribute.
  netfilter: bitwise: add helper for initializing boolean operations.
  netfilter: bitwise: add helper for evaluating boolean operations.
  netfilter: bitwise: add helper for dumping boolean operations.
  netfilter: bitwise: only offload boolean operations.
  netfilter: bitwise: add NFTA_BITWISE_DATA attribute.
  netfilter: bitwise: add support for shifts.

 include/uapi/linux/netfilter/nf_tables.h |  24 ++-
 net/netfilter/nft_bitwise.c              | 217 ++++++++++++++++++-----
 net/netfilter/nft_set_bitmap.c           |   4 +-
 net/netfilter/nft_set_hash.c             |   2 +-
 4 files changed, 200 insertions(+), 47 deletions(-)

-- 
2.24.1

[PATCH nf-next v4 01/10] netfilter: nf_tables: white-space fixes.

[Jeremy Sowden]

Indentation fixes for the parameters of a few nft functions.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 net/netfilter/nft_bitwise.c    | 4 ++--
 net/netfilter/nft_set_bitmap.c | 4 ++--
 net/netfilter/nft_set_hash.c   | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/netfilter/nft_bitwise.c b/net/netfilter/nft_bitwise.c
index 10e9d50e4e19..e8ca1ec105f8 100644
--- a/net/netfilter/nft_bitwise.c
+++ b/net/netfilter/nft_bitwise.c
@@ -130,8 +130,8 @@ static int nft_bitwise_dump(struct sk_buff *skb, const struct nft_expr *expr)
 static struct nft_data zero;
 
 static int nft_bitwise_offload(struct nft_offload_ctx *ctx,
-                               struct nft_flow_rule *flow,
-                               const struct nft_expr *expr)
+			       struct nft_flow_rule *flow,
+			       const struct nft_expr *expr)
 {
 	const struct nft_bitwise *priv = nft_expr_priv(expr);
 	struct nft_offload_reg *reg = &ctx->regs[priv->dreg];
diff --git a/net/netfilter/nft_set_bitmap.c b/net/netfilter/nft_set_bitmap.c
index 087a056e34d1..87e8d9ba0c9b 100644
--- a/net/netfilter/nft_set_bitmap.c
+++ b/net/netfilter/nft_set_bitmap.c
@@ -259,8 +259,8 @@ static u64 nft_bitmap_privsize(const struct nlattr * const nla[],
 }
 
 static int nft_bitmap_init(const struct nft_set *set,
-			 const struct nft_set_desc *desc,
-			 const struct nlattr * const nla[])
+			   const struct nft_set_desc *desc,
+			   const struct nlattr * const nla[])
 {
 	struct nft_bitmap *priv = nft_set_priv(set);
 
diff --git a/net/netfilter/nft_set_hash.c b/net/netfilter/nft_set_hash.c
index b331a3c9a3a8..d350a7cd3af0 100644
--- a/net/netfilter/nft_set_hash.c
+++ b/net/netfilter/nft_set_hash.c
@@ -645,7 +645,7 @@ static bool nft_hash_estimate(const struct nft_set_desc *desc, u32 features,
 }
 
 static bool nft_hash_fast_estimate(const struct nft_set_desc *desc, u32 features,
-			      struct nft_set_estimate *est)
+				   struct nft_set_estimate *est)
 {
 	if (!desc->size)
 		return false;
-- 
2.24.1

[PATCH nf-next v4 02/10] netfilter: bitwise: remove NULL comparisons from attribute checks.

[Jeremy Sowden]

In later patches, we will be adding more checks.  In order to be
consistent and prevent complaints from checkpatch.pl, replace the
existing comparisons with NULL with logical NOT operators.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 net/netfilter/nft_bitwise.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/netfilter/nft_bitwise.c b/net/netfilter/nft_bitwise.c
index e8ca1ec105f8..85605fb1e360 100644
--- a/net/netfilter/nft_bitwise.c
+++ b/net/netfilter/nft_bitwise.c
@@ -52,11 +52,11 @@ static int nft_bitwise_init(const struct nft_ctx *ctx,
 	u32 len;
 	int err;
 
-	if (tb[NFTA_BITWISE_SREG] == NULL ||
-	    tb[NFTA_BITWISE_DREG] == NULL ||
-	    tb[NFTA_BITWISE_LEN] == NULL ||
-	    tb[NFTA_BITWISE_MASK] == NULL ||
-	    tb[NFTA_BITWISE_XOR] == NULL)
+	if (!tb[NFTA_BITWISE_SREG] ||
+	    !tb[NFTA_BITWISE_DREG] ||
+	    !tb[NFTA_BITWISE_LEN]  ||
+	    !tb[NFTA_BITWISE_MASK] ||
+	    !tb[NFTA_BITWISE_XOR])
 		return -EINVAL;
 
 	err = nft_parse_u32_check(tb[NFTA_BITWISE_LEN], U8_MAX, &len);
-- 
2.24.1

[PATCH nf-next v4 03/10] netfilter: bitwise: replace gotos with returns.

[Jeremy Sowden]

When dumping a bitwise expression, if any of the puts fails, we use goto
to jump to a label.  However, no clean-up is required and the only
statement at the label is a return.  Drop the goto's and return
immediately instead.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 net/netfilter/nft_bitwise.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/net/netfilter/nft_bitwise.c b/net/netfilter/nft_bitwise.c
index 85605fb1e360..c15e9beb5243 100644
--- a/net/netfilter/nft_bitwise.c
+++ b/net/netfilter/nft_bitwise.c
@@ -107,24 +107,21 @@ static int nft_bitwise_dump(struct sk_buff *skb, const struct nft_expr *expr)
 	const struct nft_bitwise *priv = nft_expr_priv(expr);
 
 	if (nft_dump_register(skb, NFTA_BITWISE_SREG, priv->sreg))
-		goto nla_put_failure;
+		return -1;
 	if (nft_dump_register(skb, NFTA_BITWISE_DREG, priv->dreg))
-		goto nla_put_failure;
+		return -1;
 	if (nla_put_be32(skb, NFTA_BITWISE_LEN, htonl(priv->len)))
-		goto nla_put_failure;
+		return -1;
 
 	if (nft_data_dump(skb, NFTA_BITWISE_MASK, &priv->mask,
 			  NFT_DATA_VALUE, priv->len) < 0)
-		goto nla_put_failure;
+		return -1;
 
 	if (nft_data_dump(skb, NFTA_BITWISE_XOR, &priv->xor,
 			  NFT_DATA_VALUE, priv->len) < 0)
-		goto nla_put_failure;
+		return -1;
 
 	return 0;
-
-nla_put_failure:
-	return -1;
 }
 
 static struct nft_data zero;
-- 
2.24.1

[PATCH nf-next v4 04/10] netfilter: bitwise: add NFTA_BITWISE_OP attribute.

[Jeremy Sowden]

Add a new bitwise netlink attribute, NFTA_BITWISE_OP, which is set to a
value of a new enum, nft_bitwise_ops.  It describes the type of
operation an expression contains.  Currently, it only has one value:
NFT_BITWISE_BOOL.  More values will be added later to implement shifts.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 include/uapi/linux/netfilter/nf_tables.h | 12 ++++++++++++
 net/netfilter/nft_bitwise.c              | 16 ++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index e237ecbdcd8a..cfda75725455 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -484,6 +484,16 @@ enum nft_immediate_attributes {
 };
 #define NFTA_IMMEDIATE_MAX	(__NFTA_IMMEDIATE_MAX - 1)
 
+/**
+ * enum nft_bitwise_ops - nf_tables bitwise operations
+ *
+ * @NFT_BITWISE_BOOL: mask-and-xor operation used to implement NOT, AND, OR and
+ *                    XOR boolean operations
+ */
+enum nft_bitwise_ops {
+	NFT_BITWISE_BOOL,
+};
+
 /**
  * enum nft_bitwise_attributes - nf_tables bitwise expression netlink attributes
  *
@@ -492,6 +502,7 @@ enum nft_immediate_attributes {
  * @NFTA_BITWISE_LEN: length of operands (NLA_U32)
  * @NFTA_BITWISE_MASK: mask value (NLA_NESTED: nft_data_attributes)
  * @NFTA_BITWISE_XOR: xor value (NLA_NESTED: nft_data_attributes)
+ * @NFTA_BITWISE_OP: type of operation (NLA_U32: nft_bitwise_ops)
  *
  * The bitwise expression performs the following operation:
  *
@@ -512,6 +523,7 @@ enum nft_bitwise_attributes {
 	NFTA_BITWISE_LEN,
 	NFTA_BITWISE_MASK,
 	NFTA_BITWISE_XOR,
+	NFTA_BITWISE_OP,
 	__NFTA_BITWISE_MAX
 };
 #define NFTA_BITWISE_MAX	(__NFTA_BITWISE_MAX - 1)
diff --git a/net/netfilter/nft_bitwise.c b/net/netfilter/nft_bitwise.c
index c15e9beb5243..4884716d844a 100644
--- a/net/netfilter/nft_bitwise.c
+++ b/net/netfilter/nft_bitwise.c
@@ -18,6 +18,7 @@
 struct nft_bitwise {
 	enum nft_registers	sreg:8;
 	enum nft_registers	dreg:8;
+	enum nft_bitwise_ops	op:8;
 	u8			len;
 	struct nft_data		mask;
 	struct nft_data		xor;
@@ -41,6 +42,7 @@ static const struct nla_policy nft_bitwise_policy[NFTA_BITWISE_MAX + 1] = {
 	[NFTA_BITWISE_LEN]	= { .type = NLA_U32 },
 	[NFTA_BITWISE_MASK]	= { .type = NLA_NESTED },
 	[NFTA_BITWISE_XOR]	= { .type = NLA_NESTED },
+	[NFTA_BITWISE_OP]	= { .type = NLA_U32 },
 };
 
 static int nft_bitwise_init(const struct nft_ctx *ctx,
@@ -76,6 +78,18 @@ static int nft_bitwise_init(const struct nft_ctx *ctx,
 	if (err < 0)
 		return err;
 
+	if (tb[NFTA_BITWISE_OP]) {
+		priv->op = ntohl(nla_get_be32(tb[NFTA_BITWISE_OP]));
+		switch (priv->op) {
+		case NFT_BITWISE_BOOL:
+			break;
+		default:
+			return -EINVAL;
+		}
+	} else {
+		priv->op = NFT_BITWISE_BOOL;
+	}
+
 	err = nft_data_init(NULL, &priv->mask, sizeof(priv->mask), &d1,
 			    tb[NFTA_BITWISE_MASK]);
 	if (err < 0)
@@ -112,6 +126,8 @@ static int nft_bitwise_dump(struct sk_buff *skb, const struct nft_expr *expr)
 		return -1;
 	if (nla_put_be32(skb, NFTA_BITWISE_LEN, htonl(priv->len)))
 		return -1;
+	if (nla_put_be32(skb, NFTA_BITWISE_OP, htonl(priv->op)))
+		return -1;
 
 	if (nft_data_dump(skb, NFTA_BITWISE_MASK, &priv->mask,
 			  NFT_DATA_VALUE, priv->len) < 0)
-- 
2.24.1

[PATCH nf-next v4 05/10] netfilter: bitwise: add helper for initializing boolean operations.

[Jeremy Sowden]

Split the code specific to initializing bitwise boolean operations out
into a separate function.  A similar function will be added later for
shift operations.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 net/netfilter/nft_bitwise.c | 67 +++++++++++++++++++++++--------------
 1 file changed, 41 insertions(+), 26 deletions(-)

diff --git a/net/netfilter/nft_bitwise.c b/net/netfilter/nft_bitwise.c
index 4884716d844a..4a8d37eb43a4 100644
--- a/net/netfilter/nft_bitwise.c
+++ b/net/netfilter/nft_bitwise.c
@@ -45,20 +45,53 @@ static const struct nla_policy nft_bitwise_policy[NFTA_BITWISE_MAX + 1] = {
 	[NFTA_BITWISE_OP]	= { .type = NLA_U32 },
 };
 
+static int nft_bitwise_init_bool(struct nft_bitwise *priv,
+				 const struct nlattr *const tb[])
+{
+	struct nft_data_desc d1, d2;
+	int err;
+
+	if (!tb[NFTA_BITWISE_MASK] ||
+	    !tb[NFTA_BITWISE_XOR])
+		return -EINVAL;
+
+	err = nft_data_init(NULL, &priv->mask, sizeof(priv->mask), &d1,
+			    tb[NFTA_BITWISE_MASK]);
+	if (err < 0)
+		return err;
+	if (d1.type != NFT_DATA_VALUE || d1.len != priv->len) {
+		err = -EINVAL;
+		goto err1;
+	}
+
+	err = nft_data_init(NULL, &priv->xor, sizeof(priv->xor), &d2,
+			    tb[NFTA_BITWISE_XOR]);
+	if (err < 0)
+		goto err1;
+	if (d2.type != NFT_DATA_VALUE || d2.len != priv->len) {
+		err = -EINVAL;
+		goto err2;
+	}
+
+	return 0;
+err2:
+	nft_data_release(&priv->xor, d2.type);
+err1:
+	nft_data_release(&priv->mask, d1.type);
+	return err;
+}
+
 static int nft_bitwise_init(const struct nft_ctx *ctx,
 			    const struct nft_expr *expr,
 			    const struct nlattr * const tb[])
 {
 	struct nft_bitwise *priv = nft_expr_priv(expr);
-	struct nft_data_desc d1, d2;
 	u32 len;
 	int err;
 
 	if (!tb[NFTA_BITWISE_SREG] ||
 	    !tb[NFTA_BITWISE_DREG] ||
-	    !tb[NFTA_BITWISE_LEN]  ||
-	    !tb[NFTA_BITWISE_MASK] ||
-	    !tb[NFTA_BITWISE_XOR])
+	    !tb[NFTA_BITWISE_LEN])
 		return -EINVAL;
 
 	err = nft_parse_u32_check(tb[NFTA_BITWISE_LEN], U8_MAX, &len);
@@ -90,30 +123,12 @@ static int nft_bitwise_init(const struct nft_ctx *ctx,
 		priv->op = NFT_BITWISE_BOOL;
 	}
 
-	err = nft_data_init(NULL, &priv->mask, sizeof(priv->mask), &d1,
-			    tb[NFTA_BITWISE_MASK]);
-	if (err < 0)
-		return err;
-	if (d1.type != NFT_DATA_VALUE || d1.len != priv->len) {
-		err = -EINVAL;
-		goto err1;
-	}
-
-	err = nft_data_init(NULL, &priv->xor, sizeof(priv->xor), &d2,
-			    tb[NFTA_BITWISE_XOR]);
-	if (err < 0)
-		goto err1;
-	if (d2.type != NFT_DATA_VALUE || d2.len != priv->len) {
-		err = -EINVAL;
-		goto err2;
+	switch(priv->op) {
+	case NFT_BITWISE_BOOL:
+		return nft_bitwise_init_bool(priv, tb);
 	}
 
-	return 0;
-err2:
-	nft_data_release(&priv->xor, d2.type);
-err1:
-	nft_data_release(&priv->mask, d1.type);
-	return err;
+	return -EINVAL;
 }
 
 static int nft_bitwise_dump(struct sk_buff *skb, const struct nft_expr *expr)
-- 
2.24.1

[PATCH nf-next v4 06/10] netfilter: bitwise: add helper for evaluating boolean operations.

[Jeremy Sowden]

Split the code specific to evaluating bitwise boolean operations out
into a separate function.  Similar functions will be added later for
shift operations.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 net/netfilter/nft_bitwise.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/net/netfilter/nft_bitwise.c b/net/netfilter/nft_bitwise.c
index 4a8d37eb43a4..5f9d151b7047 100644
--- a/net/netfilter/nft_bitwise.c
+++ b/net/netfilter/nft_bitwise.c
@@ -24,16 +24,27 @@ struct nft_bitwise {
 	struct nft_data		xor;
 };
 
+static void nft_bitwise_eval_bool(u32 *dst, const u32 *src,
+				  const struct nft_bitwise *priv)
+{
+	unsigned int i;
+
+	for (i = 0; i < DIV_ROUND_UP(priv->len, 4); i++)
+		dst[i] = (src[i] & priv->mask.data[i]) ^ priv->xor.data[i];
+}
+
 void nft_bitwise_eval(const struct nft_expr *expr,
 		      struct nft_regs *regs, const struct nft_pktinfo *pkt)
 {
 	const struct nft_bitwise *priv = nft_expr_priv(expr);
 	const u32 *src = &regs->data[priv->sreg];
 	u32 *dst = &regs->data[priv->dreg];
-	unsigned int i;
 
-	for (i = 0; i < DIV_ROUND_UP(priv->len, 4); i++)
-		dst[i] = (src[i] & priv->mask.data[i]) ^ priv->xor.data[i];
+	switch (priv->op) {
+	case NFT_BITWISE_BOOL:
+		nft_bitwise_eval_bool(dst, src, priv);
+		break;
+	}
 }
 
 static const struct nla_policy nft_bitwise_policy[NFTA_BITWISE_MAX + 1] = {
-- 
2.24.1

[PATCH nf-next v4 07/10] netfilter: bitwise: add helper for dumping boolean operations.

[Jeremy Sowden]

Split the code specific to dumping bitwise boolean operations out into a
separate function.  A similar function will be added later for shift
operations.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 net/netfilter/nft_bitwise.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/net/netfilter/nft_bitwise.c b/net/netfilter/nft_bitwise.c
index 5f9d151b7047..40272a45deeb 100644
--- a/net/netfilter/nft_bitwise.c
+++ b/net/netfilter/nft_bitwise.c
@@ -142,6 +142,20 @@ static int nft_bitwise_init(const struct nft_ctx *ctx,
 	return -EINVAL;
 }
 
+static int nft_bitwise_dump_bool(struct sk_buff *skb,
+				 const struct nft_bitwise *priv)
+{
+	if (nft_data_dump(skb, NFTA_BITWISE_MASK, &priv->mask,
+			  NFT_DATA_VALUE, priv->len) < 0)
+		return -1;
+
+	if (nft_data_dump(skb, NFTA_BITWISE_XOR, &priv->xor,
+			  NFT_DATA_VALUE, priv->len) < 0)
+		return -1;
+
+	return 0;
+}
+
 static int nft_bitwise_dump(struct sk_buff *skb, const struct nft_expr *expr)
 {
 	const struct nft_bitwise *priv = nft_expr_priv(expr);
@@ -155,15 +169,12 @@ static int nft_bitwise_dump(struct sk_buff *skb, const struct nft_expr *expr)
 	if (nla_put_be32(skb, NFTA_BITWISE_OP, htonl(priv->op)))
 		return -1;
 
-	if (nft_data_dump(skb, NFTA_BITWISE_MASK, &priv->mask,
-			  NFT_DATA_VALUE, priv->len) < 0)
-		return -1;
-
-	if (nft_data_dump(skb, NFTA_BITWISE_XOR, &priv->xor,
-			  NFT_DATA_VALUE, priv->len) < 0)
-		return -1;
+	switch (priv->op) {
+	case NFT_BITWISE_BOOL:
+		return nft_bitwise_dump_bool(skb, priv);
+	}
 
-	return 0;
+	return -1;
 }
 
 static struct nft_data zero;
-- 
2.24.1

[PATCH nf-next v4 08/10] netfilter: bitwise: only offload boolean operations.

[Jeremy Sowden]

Only boolean operations supports offloading, so check the type of the
operation and return an error for other types.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 net/netfilter/nft_bitwise.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/netfilter/nft_bitwise.c b/net/netfilter/nft_bitwise.c
index 40272a45deeb..582014f696ad 100644
--- a/net/netfilter/nft_bitwise.c
+++ b/net/netfilter/nft_bitwise.c
@@ -186,6 +186,9 @@ static int nft_bitwise_offload(struct nft_offload_ctx *ctx,
 	const struct nft_bitwise *priv = nft_expr_priv(expr);
 	struct nft_offload_reg *reg = &ctx->regs[priv->dreg];
 
+	if (priv->op != NFT_BITWISE_BOOL)
+		return -EOPNOTSUPP;
+
 	if (memcmp(&priv->xor, &zero, sizeof(priv->xor)) ||
 	    priv->sreg != priv->dreg || priv->len != reg->len)
 		return -EOPNOTSUPP;
-- 
2.24.1

[PATCH nf-next v4 09/10] netfilter: bitwise: add NFTA_BITWISE_DATA attribute.

[Jeremy Sowden]

Add a new bitwise netlink attribute that will be used by shift
operations to store the size of the shift.  It is not used by boolean
operations.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 include/uapi/linux/netfilter/nf_tables.h | 3 +++
 net/netfilter/nft_bitwise.c              | 5 +++++
 2 files changed, 8 insertions(+)

diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index cfda75725455..0277ebe30c5c 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -503,6 +503,8 @@ enum nft_bitwise_ops {
  * @NFTA_BITWISE_MASK: mask value (NLA_NESTED: nft_data_attributes)
  * @NFTA_BITWISE_XOR: xor value (NLA_NESTED: nft_data_attributes)
  * @NFTA_BITWISE_OP: type of operation (NLA_U32: nft_bitwise_ops)
+ * @NFTA_BITWISE_DATA: argument for non-boolean operations
+ *                     (NLA_NESTED: nft_data_attributes)
  *
  * The bitwise expression performs the following operation:
  *
@@ -524,6 +526,7 @@ enum nft_bitwise_attributes {
 	NFTA_BITWISE_MASK,
 	NFTA_BITWISE_XOR,
 	NFTA_BITWISE_OP,
+	NFTA_BITWISE_DATA,
 	__NFTA_BITWISE_MAX
 };
 #define NFTA_BITWISE_MAX	(__NFTA_BITWISE_MAX - 1)
diff --git a/net/netfilter/nft_bitwise.c b/net/netfilter/nft_bitwise.c
index 582014f696ad..ba1c0cd332c4 100644
--- a/net/netfilter/nft_bitwise.c
+++ b/net/netfilter/nft_bitwise.c
@@ -22,6 +22,7 @@ struct nft_bitwise {
 	u8			len;
 	struct nft_data		mask;
 	struct nft_data		xor;
+	struct nft_data		data;
 };
 
 static void nft_bitwise_eval_bool(u32 *dst, const u32 *src,
@@ -54,6 +55,7 @@ static const struct nla_policy nft_bitwise_policy[NFTA_BITWISE_MAX + 1] = {
 	[NFTA_BITWISE_MASK]	= { .type = NLA_NESTED },
 	[NFTA_BITWISE_XOR]	= { .type = NLA_NESTED },
 	[NFTA_BITWISE_OP]	= { .type = NLA_U32 },
+	[NFTA_BITWISE_DATA]	= { .type = NLA_NESTED },
 };
 
 static int nft_bitwise_init_bool(struct nft_bitwise *priv,
@@ -62,6 +64,9 @@ static int nft_bitwise_init_bool(struct nft_bitwise *priv,
 	struct nft_data_desc d1, d2;
 	int err;
 
+	if (tb[NFTA_BITWISE_DATA])
+		return -EINVAL;
+
 	if (!tb[NFTA_BITWISE_MASK] ||
 	    !tb[NFTA_BITWISE_XOR])
 		return -EINVAL;
-- 
2.24.1

[PATCH nf-next v4 10/10] netfilter: bitwise: add support for shifts.

[Jeremy Sowden]

Hitherto nft_bitwise has only supported boolean operations: NOT, AND, OR
and XOR.  Extend it to do shifts as well.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 include/uapi/linux/netfilter/nf_tables.h |  9 ++-
 net/netfilter/nft_bitwise.c              | 75 ++++++++++++++++++++++++
 2 files changed, 82 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index 0277ebe30c5c..59455e7fec93 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -489,9 +489,13 @@ enum nft_immediate_attributes {
  *
  * @NFT_BITWISE_BOOL: mask-and-xor operation used to implement NOT, AND, OR and
  *                    XOR boolean operations
+ * @NFT_BITWISE_LSHIFT: left-shift operation
+ * @NFT_BITWISE_RSHIFT: right-shift operation
  */
 enum nft_bitwise_ops {
 	NFT_BITWISE_BOOL,
+	NFT_BITWISE_LSHIFT,
+	NFT_BITWISE_RSHIFT,
 };
 
 /**
@@ -506,11 +510,12 @@ enum nft_bitwise_ops {
  * @NFTA_BITWISE_DATA: argument for non-boolean operations
  *                     (NLA_NESTED: nft_data_attributes)
  *
- * The bitwise expression performs the following operation:
+ * The bitwise expression supports boolean and shift operations.  It implements
+ * the boolean operations by performing the following operation:
  *
  * dreg = (sreg & mask) ^ xor
  *
- * which allow to express all bitwise operations:
+ * with these mask and xor values:
  *
  * 		mask	xor
  * NOT:		1	1
diff --git a/net/netfilter/nft_bitwise.c b/net/netfilter/nft_bitwise.c
index ba1c0cd332c4..a0cba86d5ab4 100644
--- a/net/netfilter/nft_bitwise.c
+++ b/net/netfilter/nft_bitwise.c
@@ -34,6 +34,32 @@ static void nft_bitwise_eval_bool(u32 *dst, const u32 *src,
 		dst[i] = (src[i] & priv->mask.data[i]) ^ priv->xor.data[i];
 }
 
+static void nft_bitwise_eval_lshift(u32 *dst, const u32 *src,
+				    const struct nft_bitwise *priv)
+{
+	u32 shift = priv->data.data[0];
+	unsigned int i;
+	u32 carry = 0;
+
+	for (i = DIV_ROUND_UP(priv->len, sizeof(u32)); i > 0; i--) {
+		dst[i - 1] = (src[i - 1] << shift) | carry;
+		carry = src[i - 1] >> (BITS_PER_TYPE(u32) - shift);
+	}
+}
+
+static void nft_bitwise_eval_rshift(u32 *dst, const u32 *src,
+				    const struct nft_bitwise *priv)
+{
+	u32 shift = priv->data.data[0];
+	unsigned int i;
+	u32 carry = 0;
+
+	for (i = 0; i < DIV_ROUND_UP(priv->len, sizeof(u32)); i++) {
+		dst[i] = carry | (src[i] >> shift);
+		carry = src[i] << (BITS_PER_TYPE(u32) - shift);
+	}
+}
+
 void nft_bitwise_eval(const struct nft_expr *expr,
 		      struct nft_regs *regs, const struct nft_pktinfo *pkt)
 {
@@ -45,6 +71,12 @@ void nft_bitwise_eval(const struct nft_expr *expr,
 	case NFT_BITWISE_BOOL:
 		nft_bitwise_eval_bool(dst, src, priv);
 		break;
+	case NFT_BITWISE_LSHIFT:
+		nft_bitwise_eval_lshift(dst, src, priv);
+		break;
+	case NFT_BITWISE_RSHIFT:
+		nft_bitwise_eval_rshift(dst, src, priv);
+		break;
 	}
 }
 
@@ -97,6 +129,32 @@ static int nft_bitwise_init_bool(struct nft_bitwise *priv,
 	return err;
 }
 
+static int nft_bitwise_init_shift(struct nft_bitwise *priv,
+				  const struct nlattr *const tb[])
+{
+	struct nft_data_desc d;
+	int err;
+
+	if (tb[NFTA_BITWISE_MASK] ||
+	    tb[NFTA_BITWISE_XOR])
+		return -EINVAL;
+
+	if (!tb[NFTA_BITWISE_DATA])
+		return -EINVAL;
+
+	err = nft_data_init(NULL, &priv->data, sizeof(priv->data), &d,
+			    tb[NFTA_BITWISE_DATA]);
+	if (err < 0)
+		return err;
+	if (d.type != NFT_DATA_VALUE || d.len > sizeof(u32) ||
+	    priv->data.data[0] >= BITS_PER_TYPE(u32)) {
+		nft_data_release(&priv->data, d.type);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int nft_bitwise_init(const struct nft_ctx *ctx,
 			    const struct nft_expr *expr,
 			    const struct nlattr * const tb[])
@@ -131,6 +189,8 @@ static int nft_bitwise_init(const struct nft_ctx *ctx,
 		priv->op = ntohl(nla_get_be32(tb[NFTA_BITWISE_OP]));
 		switch (priv->op) {
 		case NFT_BITWISE_BOOL:
+		case NFT_BITWISE_LSHIFT:
+		case NFT_BITWISE_RSHIFT:
 			break;
 		default:
 			return -EINVAL;
@@ -142,6 +202,9 @@ static int nft_bitwise_init(const struct nft_ctx *ctx,
 	switch(priv->op) {
 	case NFT_BITWISE_BOOL:
 		return nft_bitwise_init_bool(priv, tb);
+	case NFT_BITWISE_LSHIFT:
+	case NFT_BITWISE_RSHIFT:
+		return nft_bitwise_init_shift(priv, tb);
 	}
 
 	return -EINVAL;
@@ -161,6 +224,15 @@ static int nft_bitwise_dump_bool(struct sk_buff *skb,
 	return 0;
 }
 
+static int nft_bitwise_dump_shift(struct sk_buff *skb,
+				  const struct nft_bitwise *priv)
+{
+	if (nft_data_dump(skb, NFTA_BITWISE_DATA, &priv->data,
+			  NFT_DATA_VALUE, sizeof(u32)) < 0)
+		return -1;
+	return 0;
+}
+
 static int nft_bitwise_dump(struct sk_buff *skb, const struct nft_expr *expr)
 {
 	const struct nft_bitwise *priv = nft_expr_priv(expr);
@@ -177,6 +249,9 @@ static int nft_bitwise_dump(struct sk_buff *skb, const struct nft_expr *expr)
 	switch (priv->op) {
 	case NFT_BITWISE_BOOL:
 		return nft_bitwise_dump_bool(skb, priv);
+	case NFT_BITWISE_LSHIFT:
+	case NFT_BITWISE_RSHIFT:
+		return nft_bitwise_dump_shift(skb, priv);
 	}
 
 	return -1;
-- 
2.24.1

Re: [PATCH nf-next v4 00/10] netfilter: nft_bitwise: shift support

[Jeremy Sowden]

[-- Attachment #1: Type: text/plain, Size: 2771 bytes --]

On 2020-01-15, at 21:32:06 +0000, Jeremy Sowden wrote:
> The connmark xtables extension supports bit-shifts.  Add support for
> shifts to nft_bitwise in order to allow nftables to do likewise, e.g.:
>
>   nft add rule t c oif lo ct mark set meta mark << 8 | 0xab
>   nft add rule t c iif lo meta mark & 0xff 0xab ct mark set meta mark >> 8
>
> Changes since v3:
>
>   * the length of shift values sent by nft may be less than sizeof(u32).

Actually, having thought about this some more, I believe I had it right
in v3.  The difference between v3 and v4 is this:

  @@ -146,7 +146,7 @@ static int nft_bitwise_init_shift(struct nft_bitwise *priv,
                              tb[NFTA_BITWISE_DATA]);
          if (err < 0)
                  return err;
  -       if (d.type != NFT_DATA_VALUE || d.len != sizeof(u32) ||
  +       if (d.type != NFT_DATA_VALUE || d.len > sizeof(u32) ||
              priv->data.data[0] >= BITS_PER_TYPE(u32)) {
                  nft_data_release(&priv->data, d.type);
                  return -EINVAL;

However, I now think the problem is in userspace and nft should always
send four bytes.  If it sends fewer, it makes it more complicated to get
the endianness right.

Unless you think there are other changes needed that will required a v5,
shall we just ignore v4 and stick with v3?

> Changes since v2:
>
>   * convert NFTA_BITWISE_DATA from u32 to nft_data;
>   * add check that shift value is not too large;
>   * use BITS_PER_TYPE to get the size of u32, rather than hard-coding it
>     when evaluating shifts.
>
> Changes since v1:
>
>   * more white-space fixes;
>   * move bitwise op enum to UAPI;
>   * add NFTA_BITWISE_OP and NFTA_BITWISE_DATA;
>   * remove NFTA_BITWISE_LSHIFT and NFTA_BITWISE_RSHIFT;
>   * add helpers for initializaing, evaluating and dumping different
>     types of operation.
>
> Jeremy Sowden (10):
>   netfilter: nf_tables: white-space fixes.
>   netfilter: bitwise: remove NULL comparisons from attribute checks.
>   netfilter: bitwise: replace gotos with returns.
>   netfilter: bitwise: add NFTA_BITWISE_OP attribute.
>   netfilter: bitwise: add helper for initializing boolean operations.
>   netfilter: bitwise: add helper for evaluating boolean operations.
>   netfilter: bitwise: add helper for dumping boolean operations.
>   netfilter: bitwise: only offload boolean operations.
>   netfilter: bitwise: add NFTA_BITWISE_DATA attribute.
>   netfilter: bitwise: add support for shifts.
>
>  include/uapi/linux/netfilter/nf_tables.h |  24 ++-
>  net/netfilter/nft_bitwise.c              | 217 ++++++++++++++++++-----
>  net/netfilter/nft_set_bitmap.c           |   4 +-
>  net/netfilter/nft_set_hash.c             |   2 +-
>  4 files changed, 200 insertions(+), 47 deletions(-)
>
> --
> 2.24.1

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH nf-next v4 00/10] netfilter: nft_bitwise: shift support

[Pablo Neira Ayuso]

On Thu, Jan 16, 2020 at 08:51:33AM +0000, Jeremy Sowden wrote:
> On 2020-01-15, at 21:32:06 +0000, Jeremy Sowden wrote:
> > The connmark xtables extension supports bit-shifts.  Add support for
> > shifts to nft_bitwise in order to allow nftables to do likewise, e.g.:
> >
> >   nft add rule t c oif lo ct mark set meta mark << 8 | 0xab
> >   nft add rule t c iif lo meta mark & 0xff 0xab ct mark set meta mark >> 8
> >
> > Changes since v3:
> >
> >   * the length of shift values sent by nft may be less than sizeof(u32).
> 
> Actually, having thought about this some more, I believe I had it right
> in v3.  The difference between v3 and v4 is this:
> 
>   @@ -146,7 +146,7 @@ static int nft_bitwise_init_shift(struct nft_bitwise *priv,
>                               tb[NFTA_BITWISE_DATA]);
>           if (err < 0)
>                   return err;
>   -       if (d.type != NFT_DATA_VALUE || d.len != sizeof(u32) ||
>   +       if (d.type != NFT_DATA_VALUE || d.len > sizeof(u32) ||
>               priv->data.data[0] >= BITS_PER_TYPE(u32)) {

Why restrict this to 32-bits?

>                   nft_data_release(&priv->data, d.type);
>                   return -EINVAL;
> 
> However, I now think the problem is in userspace and nft should always
> send four bytes.  If it sends fewer, it makes it more complicated to get
> the endianness right.
> 
> Unless you think there are other changes needed that will required a v5,
> shall we just ignore v4 and stick with v3?

Re: [PATCH nf-next v4 00/10] netfilter: nft_bitwise: shift support

[Pablo Neira Ayuso]

On Thu, Jan 16, 2020 at 12:22:47PM +0100, Pablo Neira Ayuso wrote:
> On Thu, Jan 16, 2020 at 08:51:33AM +0000, Jeremy Sowden wrote:
> > On 2020-01-15, at 21:32:06 +0000, Jeremy Sowden wrote:
> > > The connmark xtables extension supports bit-shifts.  Add support for
> > > shifts to nft_bitwise in order to allow nftables to do likewise, e.g.:
> > >
> > >   nft add rule t c oif lo ct mark set meta mark << 8 | 0xab
> > >   nft add rule t c iif lo meta mark & 0xff 0xab ct mark set meta mark >> 8
> > >
> > > Changes since v3:
> > >
> > >   * the length of shift values sent by nft may be less than sizeof(u32).
> > 
> > Actually, having thought about this some more, I believe I had it right
> > in v3.  The difference between v3 and v4 is this:
> > 
> >   @@ -146,7 +146,7 @@ static int nft_bitwise_init_shift(struct nft_bitwise *priv,
> >                               tb[NFTA_BITWISE_DATA]);
> >           if (err < 0)
> >                   return err;
> >   -       if (d.type != NFT_DATA_VALUE || d.len != sizeof(u32) ||
> >   +       if (d.type != NFT_DATA_VALUE || d.len > sizeof(u32) ||
> >               priv->data.data[0] >= BITS_PER_TYPE(u32)) {
> 
> Why restrict this to 32-bits?

Ah, I see, only for the shift case. Indeed, makes sense :-)

Let me have a look at v3.

Re: [PATCH nf-next v4 00/10] netfilter: nft_bitwise: shift support

[Jeremy Sowden]

[-- Attachment #1: Type: text/plain, Size: 2432 bytes --]

On 2020-01-16, at 12:22:47 +0100, Pablo Neira Ayuso wrote:
> On Thu, Jan 16, 2020 at 08:51:33AM +0000, Jeremy Sowden wrote:
> > On 2020-01-15, at 21:32:06 +0000, Jeremy Sowden wrote:
> > > The connmark xtables extension supports bit-shifts.  Add support
> > > for shifts to nft_bitwise in order to allow nftables to do
> > > likewise, e.g.:
> > >
> > >   nft add rule t c oif lo ct mark set meta mark << 8 | 0xab
> > >   nft add rule t c iif lo meta mark & 0xff 0xab ct mark set meta mark >> 8
> > >
> > > Changes since v3:
> > >
> > >   * the length of shift values sent by nft may be less than
> > >   sizeof(u32).
> >
> > Actually, having thought about this some more, I believe I had it
> > right in v3.  The difference between v3 and v4 is this:
> >
> >   @@ -146,7 +146,7 @@ static int nft_bitwise_init_shift(struct nft_bitwise *priv,
> >                               tb[NFTA_BITWISE_DATA]);
> >           if (err < 0)
> >                   return err;
> >   -       if (d.type != NFT_DATA_VALUE || d.len != sizeof(u32) ||
> >   +       if (d.type != NFT_DATA_VALUE || d.len > sizeof(u32) ||
> >               priv->data.data[0] >= BITS_PER_TYPE(u32)) {
>
> Why restrict this to 32-bits?

Because of how I implemented the shifts.  Here's the current rshift:

  static void nft_bitwise_eval_rshift(u32 *dst, const u32 *src,
                                      const struct nft_bitwise *priv)
  {
          u32 shift = priv->data.data[0];
          unsigned int i;
          u32 carry = 0;

          for (i = 0; i < DIV_ROUND_UP(priv->len, sizeof(u32)); i++) {
                  dst[i] = carry | (src[i] >> shift);
                  carry = src[i] << (BITS_PER_TYPE(u32) - shift);
          }
  }

In order to support larger shifts, it would need to look something
like:

  static void nft_bitwise_eval_rshift(u32 *dst, const u32 *src,
                                      const struct nft_bitwise *priv)
  {
          unsigned len = DIV_ROUND_UP(priv->len, sizeof(u32));
          unsigned int d = shift / BITS_PER_TYPE(u32), s = 0;
          u32 shift = priv->data.data[0];
          u32 carry = 0;

          if (d > 0) {
                  memset(dst, '\0', d * sizeof(*dst));
                  shift %= BITS_PER_TYPE(u32);
          }

          for (s = 0; d < len; d++, s++) {
                  dst[d] = carry | (src[s] >> shift);
                  carry = src[s] << (BITS_PER_TYPE(u32) - shift);
          }
  }

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH nf-next v4 00/10] netfilter: nft_bitwise: shift support

[Pablo Neira Ayuso]

On Thu, Jan 16, 2020 at 11:41:53AM +0000, Jeremy Sowden wrote:
> On 2020-01-16, at 12:22:47 +0100, Pablo Neira Ayuso wrote:
> > On Thu, Jan 16, 2020 at 08:51:33AM +0000, Jeremy Sowden wrote:
> > > On 2020-01-15, at 21:32:06 +0000, Jeremy Sowden wrote:
> > > > The connmark xtables extension supports bit-shifts.  Add support
> > > > for shifts to nft_bitwise in order to allow nftables to do
> > > > likewise, e.g.:
> > > >
> > > >   nft add rule t c oif lo ct mark set meta mark << 8 | 0xab
> > > >   nft add rule t c iif lo meta mark & 0xff 0xab ct mark set meta mark >> 8
> > > >
> > > > Changes since v3:
> > > >
> > > >   * the length of shift values sent by nft may be less than
> > > >   sizeof(u32).
> > >
> > > Actually, having thought about this some more, I believe I had it
> > > right in v3.  The difference between v3 and v4 is this:
> > >
> > >   @@ -146,7 +146,7 @@ static int nft_bitwise_init_shift(struct nft_bitwise *priv,
> > >                               tb[NFTA_BITWISE_DATA]);
> > >           if (err < 0)
> > >                   return err;
> > >   -       if (d.type != NFT_DATA_VALUE || d.len != sizeof(u32) ||
> > >   +       if (d.type != NFT_DATA_VALUE || d.len > sizeof(u32) ||
> > >               priv->data.data[0] >= BITS_PER_TYPE(u32)) {
> >
> > Why restrict this to 32-bits?
> 
> Because of how I implemented the shifts.  Here's the current rshift:
> 
>   static void nft_bitwise_eval_rshift(u32 *dst, const u32 *src,
>                                       const struct nft_bitwise *priv)
>   {
>           u32 shift = priv->data.data[0];
>           unsigned int i;
>           u32 carry = 0;
> 
>           for (i = 0; i < DIV_ROUND_UP(priv->len, sizeof(u32)); i++) {
>                   dst[i] = carry | (src[i] >> shift);
>                   carry = src[i] << (BITS_PER_TYPE(u32) - shift);
>           }
>   }
> 
> In order to support larger shifts, it would need to look something
> like:

No need for larger shift indeed, no need for this.

I just wanted to make sure NFTA_BITWISE_DATA can be reused later on in
future new operation that might require larger data.

All good then, I'll review v3, OK?

Re: [PATCH nf-next v4 00/10] netfilter: nft_bitwise: shift support

[Jeremy Sowden]

[-- Attachment #1: Type: text/plain, Size: 2312 bytes --]

On 2020-01-16, at 13:09:25 +0100, Pablo Neira Ayuso wrote:
> On Thu, Jan 16, 2020 at 11:41:53AM +0000, Jeremy Sowden wrote:
> > On 2020-01-16, at 12:22:47 +0100, Pablo Neira Ayuso wrote:
> > > On Thu, Jan 16, 2020 at 08:51:33AM +0000, Jeremy Sowden wrote:
> > > > On 2020-01-15, at 21:32:06 +0000, Jeremy Sowden wrote:
> > > > > The connmark xtables extension supports bit-shifts.  Add support
> > > > > for shifts to nft_bitwise in order to allow nftables to do
> > > > > likewise, e.g.:
> > > > >
> > > > >   nft add rule t c oif lo ct mark set meta mark << 8 | 0xab
> > > > >   nft add rule t c iif lo meta mark & 0xff 0xab ct mark set meta mark >> 8
> > > > >
> > > > > Changes since v3:
> > > > >
> > > > >   * the length of shift values sent by nft may be less than
> > > > >   sizeof(u32).
> > > >
> > > > Actually, having thought about this some more, I believe I had it
> > > > right in v3.  The difference between v3 and v4 is this:
> > > >
> > > >   @@ -146,7 +146,7 @@ static int nft_bitwise_init_shift(struct nft_bitwise *priv,
> > > >                               tb[NFTA_BITWISE_DATA]);
> > > >           if (err < 0)
> > > >                   return err;
> > > >   -       if (d.type != NFT_DATA_VALUE || d.len != sizeof(u32) ||
> > > >   +       if (d.type != NFT_DATA_VALUE || d.len > sizeof(u32) ||
> > > >               priv->data.data[0] >= BITS_PER_TYPE(u32)) {
> > >
> > > Why restrict this to 32-bits?
> >
> > Because of how I implemented the shifts.  Here's the current rshift:
> >
> >   static void nft_bitwise_eval_rshift(u32 *dst, const u32 *src,
> >                                       const struct nft_bitwise *priv)
> >   {
> >           u32 shift = priv->data.data[0];
> >           unsigned int i;
> >           u32 carry = 0;
> >
> >           for (i = 0; i < DIV_ROUND_UP(priv->len, sizeof(u32)); i++) {
> >                   dst[i] = carry | (src[i] >> shift);
> >                   carry = src[i] << (BITS_PER_TYPE(u32) - shift);
> >           }
> >   }
> >
> > In order to support larger shifts, it would need to look something
> > like:
>
> No need for larger shift indeed, no need for this.
>
> I just wanted to make sure NFTA_BITWISE_DATA can be reused later on in
> future new operation that might require larger data.
>
> All good then, I'll review v3, OK?

Yup. :)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH nf-next v4 00/10] netfilter: nft_bitwise: shift support

[Pablo Neira Ayuso]

On Wed, Jan 15, 2020 at 09:32:06PM +0000, Jeremy Sowden wrote:
> The connmark xtables extension supports bit-shifts.  Add support for
> shifts to nft_bitwise in order to allow nftables to do likewise, e.g.:
> 
>   nft add rule t c oif lo ct mark set meta mark << 8 | 0xab
>   nft add rule t c iif lo meta mark & 0xff 0xab ct mark set meta mark >> 8
> 
> Changes since v3:
> 
>   * the length of shift values sent by nft may be less than sizeof(u32).
> 
> Changes since v2:
> 
>   * convert NFTA_BITWISE_DATA from u32 to nft_data;
>   * add check that shift value is not too large;
>   * use BITS_PER_TYPE to get the size of u32, rather than hard-coding it
>     when evaluating shifts.

Series applied, thanks.

I made a few updates:

* Replaced -EINVAL by -EOPNOTSUPP in case NFTA_BITWISE_OP is not
  supported. -EINVAL is usually reserved to missing netlink attribute /
  malformed netlink message (actually, you can find many spots where
  this is a bit overloaded with different "meanings", but just trying
  to stick to those semantics here).

* Replaced:

        return nft_bitwise_init_bool(priv, tb);

  by:

        err = nft_bitwise_init_bool(priv, tb);
        break;
  }

  return err;

  in a few spots, I hope I did not break anything.

I tend to find that easier to read today, minor things like this are
very much debatable.

Thanks.

Re: [PATCH nf-next v4 00/10] netfilter: nft_bitwise: shift support

[Jeremy Sowden]

[-- Attachment #1: Type: text/plain, Size: 883 bytes --]

On 2020-01-16, at 15:48:33 +0100, Pablo Neira Ayuso wrote:
> On Wed, Jan 15, 2020 at 09:32:06PM +0000, Jeremy Sowden wrote:
> > The connmark xtables extension supports bit-shifts.  Add support for
> > shifts to nft_bitwise in order to allow nftables to do likewise,
> > e.g.:
> >
> >   nft add rule t c oif lo ct mark set meta mark << 8 | 0xab
> >   nft add rule t c iif lo meta mark & 0xff 0xab ct mark set meta mark >> 8
> >
> > Changes since v3:
> >
> >   * the length of shift values sent by nft may be less than
> >     sizeof(u32).
> >
> > Changes since v2:
> >
> >   * convert NFTA_BITWISE_DATA from u32 to nft_data;
> >   * add check that shift value is not too large;
> >   * use BITS_PER_TYPE to get the size of u32, rather than
> >     hard-coding it when evaluating shifts.
>
> Series applied, thanks.

Cheers. :) I'll update the userspace changes and send them out.

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH nf-next v4 00/10] netfilter: nft_bitwise: shift support

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 231 bytes --]

Hi Jeremy,

I've been looking into (ab)using bitwise to implement add/sub. I would
like to not add nft_arith for only this, and it seems to me much of
your code can be reused.

Do you think something like this would work?

Thanks.

[-- Attachment #2: arith.patch --]
[-- Type: text/x-diff, Size: 3776 bytes --]

diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index 065218a20bb7..c4078359b6e4 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -508,11 +508,15 @@ enum nft_immediate_attributes {
  *                    XOR boolean operations
  * @NFT_BITWISE_LSHIFT: left-shift operation
  * @NFT_BITWISE_RSHIFT: right-shift operation
+ * @NFT_BITWISE_ADD: add operation
+ * @NFT_BITWISE_SUB: subtract operation
  */
 enum nft_bitwise_ops {
 	NFT_BITWISE_BOOL,
 	NFT_BITWISE_LSHIFT,
 	NFT_BITWISE_RSHIFT,
+	NFT_BITWISE_ADD,
+	NFT_BITWISE_SUB,
 };
 
 /**
diff --git a/net/netfilter/nft_bitwise.c b/net/netfilter/nft_bitwise.c
index 0ed2281f03be..fd0cd2b4722a 100644
--- a/net/netfilter/nft_bitwise.c
+++ b/net/netfilter/nft_bitwise.c
@@ -60,6 +60,38 @@ static void nft_bitwise_eval_rshift(u32 *dst, const u32 *src,
 	}
 }
 
+static void nft_bitwise_eval_add(u32 *dst, const u32 *src,
+				 const struct nft_bitwise *priv)
+{
+	u32 delta = priv->data.data[0];
+	unsigned int i, words;
+	u32 tmp = 0;
+
+	words = DIV_ROUND_UP(priv->len, sizeof(u32));
+	for (i = 0; i < words; i++) {
+		tmp = src[i];
+		dst[i] = src[i] + delta;
+		if (dst[i] < tmp && i + 1 < words)
+			dst[i + 1]++;
+	}
+}
+
+static void nft_bitwise_eval_sub(u32 *dst, const u32 *src,
+				 const struct nft_bitwise *priv)
+{
+	u32 delta = priv->data.data[0];
+	unsigned int i, words;
+	u32 tmp = 0;
+
+	words = DIV_ROUND_UP(priv->len, sizeof(u32));
+	for (i = 0; i < words; i++) {
+		tmp = src[i];
+		dst[i] = src[i] - delta;
+		if (dst[i] > tmp && i + 1 < words)
+			dst[i + 1]--;
+	}
+}
+
 void nft_bitwise_eval(const struct nft_expr *expr,
 		      struct nft_regs *regs, const struct nft_pktinfo *pkt)
 {
@@ -77,6 +109,12 @@ void nft_bitwise_eval(const struct nft_expr *expr,
 	case NFT_BITWISE_RSHIFT:
 		nft_bitwise_eval_rshift(dst, src, priv);
 		break;
+	case NFT_BITWISE_ADD:
+		nft_bitwise_eval_add(dst, src, priv);
+		break;
+	case NFT_BITWISE_SUB:
+		nft_bitwise_eval_sub(dst, src, priv);
+		break;
 	}
 }
 
@@ -129,8 +167,8 @@ static int nft_bitwise_init_bool(struct nft_bitwise *priv,
 	return err;
 }
 
-static int nft_bitwise_init_shift(struct nft_bitwise *priv,
-				  const struct nlattr *const tb[])
+static int nft_bitwise_init_data(struct nft_bitwise *priv,
+				 const struct nlattr *const tb[])
 {
 	struct nft_data_desc d;
 	int err;
@@ -191,6 +229,8 @@ static int nft_bitwise_init(const struct nft_ctx *ctx,
 		case NFT_BITWISE_BOOL:
 		case NFT_BITWISE_LSHIFT:
 		case NFT_BITWISE_RSHIFT:
+		case NFT_BITWISE_ADD:
+		case NFT_BITWISE_SUB:
 			break;
 		default:
 			return -EOPNOTSUPP;
@@ -205,7 +245,9 @@ static int nft_bitwise_init(const struct nft_ctx *ctx,
 		break;
 	case NFT_BITWISE_LSHIFT:
 	case NFT_BITWISE_RSHIFT:
-		err = nft_bitwise_init_shift(priv, tb);
+	case NFT_BITWISE_ADD:
+	case NFT_BITWISE_SUB:
+		err = nft_bitwise_init_data(priv, tb);
 		break;
 	}
 
@@ -226,8 +268,8 @@ static int nft_bitwise_dump_bool(struct sk_buff *skb,
 	return 0;
 }
 
-static int nft_bitwise_dump_shift(struct sk_buff *skb,
-				  const struct nft_bitwise *priv)
+static int nft_bitwise_dump_data(struct sk_buff *skb,
+				 const struct nft_bitwise *priv)
 {
 	if (nft_data_dump(skb, NFTA_BITWISE_DATA, &priv->data,
 			  NFT_DATA_VALUE, sizeof(u32)) < 0)
@@ -255,7 +297,9 @@ static int nft_bitwise_dump(struct sk_buff *skb, const struct nft_expr *expr)
 		break;
 	case NFT_BITWISE_LSHIFT:
 	case NFT_BITWISE_RSHIFT:
-		err = nft_bitwise_dump_shift(skb, priv);
+	case NFT_BITWISE_ADD:
+	case NFT_BITWISE_SUB:
+		err = nft_bitwise_dump_data(skb, priv);
 		break;
 	}
 

Re: [PATCH nf-next v4 00/10] netfilter: nft_bitwise: shift support

[Jeremy Sowden]

[-- Attachment #1: Type: text/plain, Size: 4660 bytes --]

On 2020-01-26, at 12:12:51 +0100, Pablo Neira Ayuso wrote:
> I've been looking into (ab)using bitwise to implement add/sub. I would
> like to not add nft_arith for only this, and it seems to me much of
> your code can be reused.
>
> Do you think something like this would work?

Absolutely.

A couple of questions.  What's the use-case?  I find the combination of
applying the delta to every u32 and having a carry curious.  Do you want
to support bigendian arithmetic (i.e., carrying to the left) as well?

I've suggested a couple of changes below.

J.

> Thanks.
>
> diff --git a/include/uapi/linux/netfilter/nf_tables.h
> b/include/uapi/linux/netfilter/nf_tables.h
> index 065218a20bb7..c4078359b6e4 100644
> --- a/include/uapi/linux/netfilter/nf_tables.h
> +++ b/include/uapi/linux/netfilter/nf_tables.h
> @@ -508,11 +508,15 @@ enum nft_immediate_attributes {
>   *                    XOR boolean operations
>   * @NFT_BITWISE_LSHIFT: left-shift operation
>   * @NFT_BITWISE_RSHIFT: right-shift operation
> + * @NFT_BITWISE_ADD: add operation
> + * @NFT_BITWISE_SUB: subtract operation
>   */
>  enum nft_bitwise_ops {
>  	NFT_BITWISE_BOOL,
>  	NFT_BITWISE_LSHIFT,
>  	NFT_BITWISE_RSHIFT,
> +	NFT_BITWISE_ADD,
> +	NFT_BITWISE_SUB,
>  };
>
>  /**
> diff --git a/net/netfilter/nft_bitwise.c b/net/netfilter/nft_bitwise.c
> index 0ed2281f03be..fd0cd2b4722a 100644
> --- a/net/netfilter/nft_bitwise.c
> +++ b/net/netfilter/nft_bitwise.c
> @@ -60,6 +60,38 @@ static void nft_bitwise_eval_rshift(u32 *dst, const
> u32 *src,
>  	}
>  }
>
> +static void nft_bitwise_eval_add(u32 *dst, const u32 *src,
> +				 const struct nft_bitwise *priv)
> +{
> +	u32 delta = priv->data.data[0];
> +	unsigned int i, words;
> +	u32 tmp = 0;
> +
> +	words = DIV_ROUND_UP(priv->len, sizeof(u32));
> +	for (i = 0; i < words; i++) {
> +		tmp = src[i];
> +		dst[i] = src[i] + delta;
> +		if (dst[i] < tmp && i + 1 < words)
> +			dst[i + 1]++;
> +	}
> +}

for (i = 0; i < words; i++) {
	dst[i] = src[i] + delta + tmp;
	tmp = dst[i] < src[i] ? 1 : 0;
}

> +static void nft_bitwise_eval_sub(u32 *dst, const u32 *src,
> +				 const struct nft_bitwise *priv)
> +{
> +	u32 delta = priv->data.data[0];
> +	unsigned int i, words;
> +	u32 tmp = 0;
> +
> +	words = DIV_ROUND_UP(priv->len, sizeof(u32));
> +	for (i = 0; i < words; i++) {
> +		tmp = src[i];
> +		dst[i] = src[i] - delta;
> +		if (dst[i] > tmp && i + 1 < words)
> +			dst[i + 1]--;
> +	}
> +}

for (i = 0; i < words; i++) {
	dst[i] = src[i] - delta - tmp;
	tmp = dst[i] > src[i] ? 1 : 0;
}

>  void nft_bitwise_eval(const struct nft_expr *expr,
>  		      struct nft_regs *regs, const struct nft_pktinfo *pkt)
>  {
> @@ -77,6 +109,12 @@ void nft_bitwise_eval(const struct nft_expr *expr,
>  	case NFT_BITWISE_RSHIFT:
>  		nft_bitwise_eval_rshift(dst, src, priv);
>  		break;
> +	case NFT_BITWISE_ADD:
> +		nft_bitwise_eval_add(dst, src, priv);
> +		break;
> +	case NFT_BITWISE_SUB:
> +		nft_bitwise_eval_sub(dst, src, priv);
> +		break;
>  	}
>  }
>
> @@ -129,8 +167,8 @@ static int nft_bitwise_init_bool(struct
> nft_bitwise *priv,
>  	return err;
>  }
>
> -static int nft_bitwise_init_shift(struct nft_bitwise *priv,
> -				  const struct nlattr *const tb[])
> +static int nft_bitwise_init_data(struct nft_bitwise *priv,
> +				 const struct nlattr *const tb[])
>  {
>  	struct nft_data_desc d;
>  	int err;
> @@ -191,6 +229,8 @@ static int nft_bitwise_init(const struct nft_ctx
> *ctx,
>  		case NFT_BITWISE_BOOL:
>  		case NFT_BITWISE_LSHIFT:
>  		case NFT_BITWISE_RSHIFT:
> +		case NFT_BITWISE_ADD:
> +		case NFT_BITWISE_SUB:
>  			break;
>  		default:
>  			return -EOPNOTSUPP;
> @@ -205,7 +245,9 @@ static int nft_bitwise_init(const struct nft_ctx
> *ctx,
>  		break;
>  	case NFT_BITWISE_LSHIFT:
>  	case NFT_BITWISE_RSHIFT:
> -		err = nft_bitwise_init_shift(priv, tb);
> +	case NFT_BITWISE_ADD:
> +	case NFT_BITWISE_SUB:
> +		err = nft_bitwise_init_data(priv, tb);
>  		break;
>  	}
>
> @@ -226,8 +268,8 @@ static int nft_bitwise_dump_bool(struct sk_buff
> *skb,
>  	return 0;
>  }
>
> -static int nft_bitwise_dump_shift(struct sk_buff *skb,
> -				  const struct nft_bitwise *priv)
> +static int nft_bitwise_dump_data(struct sk_buff *skb,
> +				 const struct nft_bitwise *priv)
>  {
>  	if (nft_data_dump(skb, NFTA_BITWISE_DATA, &priv->data,
>  			  NFT_DATA_VALUE, sizeof(u32)) < 0)
> @@ -255,7 +297,9 @@ static int nft_bitwise_dump(struct sk_buff *skb,
> const struct nft_expr *expr)
>  		break;
>  	case NFT_BITWISE_LSHIFT:
>  	case NFT_BITWISE_RSHIFT:
> -		err = nft_bitwise_dump_shift(skb, priv);
> +	case NFT_BITWISE_ADD:
> +	case NFT_BITWISE_SUB:
> +		err = nft_bitwise_dump_data(skb, priv);
>  		break;
>  	}
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH nf-next v4 00/10] netfilter: nft_bitwise: shift support

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 1658 bytes --]

On Mon, Jan 27, 2020 at 11:13:14AM +0000, Jeremy Sowden wrote:
> On 2020-01-26, at 12:12:51 +0100, Pablo Neira Ayuso wrote:
> > I've been looking into (ab)using bitwise to implement add/sub. I would
> > like to not add nft_arith for only this, and it seems to me much of
> > your code can be reused.
> >
> > Do you think something like this would work?
> 
> Absolutely.
> 
> A couple of questions.  What's the use-case?

inc/dec ip ttl field.

> I find the combination of applying the delta to every u32 and having
> a carry curious.  Do you want to support bigendian arithmetic (i.e.,
> carrying to the left) as well?

Userspace should convert to host endianess before doing arithmetics.

> I've suggested a couple of changes below.
[...]
> > diff --git a/net/netfilter/nft_bitwise.c b/net/netfilter/nft_bitwise.c
> > index 0ed2281f03be..fd0cd2b4722a 100644
> > --- a/net/netfilter/nft_bitwise.c
> > +++ b/net/netfilter/nft_bitwise.c
> > @@ -60,6 +60,38 @@ static void nft_bitwise_eval_rshift(u32 *dst, const
> > u32 *src,
> >  	}
> >  }
> >
> > +static void nft_bitwise_eval_add(u32 *dst, const u32 *src,
> > +				 const struct nft_bitwise *priv)
> > +{
> > +	u32 delta = priv->data.data[0];
> > +	unsigned int i, words;
> > +	u32 tmp = 0;
> > +
> > +	words = DIV_ROUND_UP(priv->len, sizeof(u32));
> > +	for (i = 0; i < words; i++) {
> > +		tmp = src[i];
> > +		dst[i] = src[i] + delta;
> > +		if (dst[i] < tmp && i + 1 < words)
> > +			dst[i + 1]++;
> > +	}
> > +}
> 
> for (i = 0; i < words; i++) {
> 	dst[i] = src[i] + delta + tmp;
> 	tmp = dst[i] < src[i] ? 1 : 0;
> }

Much simpler indeed, thanks.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH nf-next v4 00/10] netfilter: nft_bitwise: shift support

[Jeremy Sowden]

[-- Attachment #1: Type: text/plain, Size: 1269 bytes --]

On 2020-01-28, at 11:00:35 +0100, Pablo Neira Ayuso wrote:
> On Mon, Jan 27, 2020 at 11:13:14AM +0000, Jeremy Sowden wrote:
> > On 2020-01-26, at 12:12:51 +0100, Pablo Neira Ayuso wrote:
> > > I've been looking into (ab)using bitwise to implement add/sub. I
> > > would like to not add nft_arith for only this, and it seems to me
> > > much of your code can be reused.
> > >
> > > Do you think something like this would work?
> >
> > Absolutely.
> >
> > A couple of questions.  What's the use-case?
>
> inc/dec ip ttl field.

If it's just a simple addition or subtraction on one value, would
this make more sense?

        for (i = 0; i < words; i++) {
	        dst[i] = src[i] + delta;
	        delta = dst[i] < src[i] ? 1 : 0;
        }

> > I find the combination of applying the delta to every u32 and having
> > a carry curious.  Do you want to support bigendian arithmetic (i.e.,
> > carrying to the left) as well?
>
> Userspace should convert to host endianess before doing arithmetics.

Yes, but if the host is bigendian, the least significant bytes will be
on the right, and we need to carry to the left, don't we?

        for (i = words; i > 0; i--) {
	        dst[i - 1] = src[i - 1] + delta;
	        delta = dst[i - 1] < src[i - 1] ? 1 : 0;
        }

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH nf-next v4 00/10] netfilter: nft_bitwise: shift support

[Pablo Neira Ayuso]

On Tue, Jan 28, 2020 at 11:31:39AM +0000, Jeremy Sowden wrote:
> On 2020-01-28, at 11:00:35 +0100, Pablo Neira Ayuso wrote:
> > On Mon, Jan 27, 2020 at 11:13:14AM +0000, Jeremy Sowden wrote:
> > > On 2020-01-26, at 12:12:51 +0100, Pablo Neira Ayuso wrote:
> > > > I've been looking into (ab)using bitwise to implement add/sub. I
> > > > would like to not add nft_arith for only this, and it seems to me
> > > > much of your code can be reused.
> > > >
> > > > Do you think something like this would work?
> > >
> > > Absolutely.
> > >
> > > A couple of questions.  What's the use-case?
> >
> > inc/dec ip ttl field.
> 
> If it's just a simple addition or subtraction on one value, would
> this make more sense?
> 
>         for (i = 0; i < words; i++) {
> 	        dst[i] = src[i] + delta;
> 	        delta = dst[i] < src[i] ? 1 : 0;
>         }

This can be done through _INC / _DEC instead, however...

> > > I find the combination of applying the delta to every u32 and having
> > > a carry curious.  Do you want to support bigendian arithmetic (i.e.,
> > > carrying to the left) as well?
> >
> > Userspace should convert to host endianess before doing arithmetics.
> 
> Yes, but if the host is bigendian, the least significant bytes will be
> on the right, and we need to carry to the left, don't we?
> 
>         for (i = words; i > 0; i--) {
> 	        dst[i - 1] = src[i - 1] + delta;
> 	        delta = dst[i - 1] < src[i - 1] ? 1 : 0;
>         }

I think some simplified version of bignum add/subtract is needed,
something like:

        for (i = len - 1; i >= 0; i--) {
                res[i] = a[i] + b[i] + carry;
                carry = res[i] < a[i] + b[i];
        }

where 'len' is in bytes. Values in a[] and b[] are u8 and data is
represented in big endian.