[nf-next PATCH v2 0/5] netfilter: bitwise: support boolean operations with variable RHS operands

[nf-next PATCH v2 0/5] netfilter: bitwise: support boolean operations with variable RHS operands

[Jeremy Sowden]

I've resurrected the work I started a couple of years ago.

Currently bitwise boolean operations (AND, OR and XOR) can only have one
variable operand.  They are converted in user space into mask-and-xor
operations on one register and two immediate values which are evaluated
by the kernel.  We add support for evaluating these operations directly
in kernel space on one register and either an immediate value or a
second register.

We also add support for keeping track of the bit-length of boolean
expressions since this can be useful to user space during
delinearization.

* Patch 1 adds support for keeping track of the bit-length of
  boolean expressions.
* Patches 2 & 3 make some small unrelated improvements.
* Patch 4 renames functions and an enum constant related to the current
  mask-and-xor implementation in anticipation of adding support for
  directly evaluating AND, OR and XOR operations.
* Patch 5 adds support for directly evaluating AND, OR and XOR
  operations.

Changes since v1

  * Patch 1 is new.
  * In v1, all boolean operations were still expected to be
    mask-and-xor operations, but the mask and xor values could be
    passed in registers.

Jeremy Sowden (5):
  netfilter: bitwise: keep track of bit-length of expressions
  netfilter: bitwise: replace hard-coded size with `sizeof` expression
  netfilter: bitwise: improve error goto labels
  netfilter: bitwise: rename some boolean operation functions
  netfilter: bitwise: add support for doing AND, OR and XOR directly

 include/uapi/linux/netfilter/nf_tables.h |  21 ++-
 net/netfilter/nft_bitwise.c              | 178 +++++++++++++++++++----
 2 files changed, 164 insertions(+), 35 deletions(-)

-- 
2.35.1

[nf-next PATCH v2 1/5] netfilter: bitwise: keep track of bit-length of expressions

[Jeremy Sowden]

Some bitwise operations are generated in user space when munging paylod
expressions.  During delinearization, user space attempts to eliminate
these operations.  However, it does this before deducing the byte-order
or the correct length in bits of the operands, which means that it
doesn't always handle multi-byte host-endian operations correctly.
Therefore, add support for storing the bit-length of the expression,
even though the kernel doesn't use it, in order to be able to pass it
back to user space.

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

diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index 466fd3f4447c..f3dcc4a34ff1 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -561,6 +561,7 @@ enum nft_bitwise_ops {
  * @NFTA_BITWISE_OP: type of operation (NLA_U32: nft_bitwise_ops)
  * @NFTA_BITWISE_DATA: argument for non-boolean operations
  *                     (NLA_NESTED: nft_data_attributes)
+ * @NFTA_BITWISE_NBITS: length of operation in bits (NLA_U32)
  *
  * The bitwise expression supports boolean and shift operations.  It implements
  * the boolean operations by performing the following operation:
@@ -584,6 +585,7 @@ enum nft_bitwise_attributes {
 	NFTA_BITWISE_XOR,
 	NFTA_BITWISE_OP,
 	NFTA_BITWISE_DATA,
+	NFTA_BITWISE_NBITS,
 	__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 f590ee1c8a1b..cdace40c6ba0 100644
--- a/net/netfilter/nft_bitwise.c
+++ b/net/netfilter/nft_bitwise.c
@@ -23,6 +23,7 @@ struct nft_bitwise {
 	struct nft_data		mask;
 	struct nft_data		xor;
 	struct nft_data		data;
+	u8                      nbits;
 };
 
 static void nft_bitwise_eval_bool(u32 *dst, const u32 *src,
@@ -88,6 +89,7 @@ static const struct nla_policy nft_bitwise_policy[NFTA_BITWISE_MAX + 1] = {
 	[NFTA_BITWISE_XOR]	= { .type = NLA_NESTED },
 	[NFTA_BITWISE_OP]	= { .type = NLA_U32 },
 	[NFTA_BITWISE_DATA]	= { .type = NLA_NESTED },
+	[NFTA_BITWISE_NBITS]	= { .type = NLA_U32 },
 };
 
 static int nft_bitwise_init_bool(struct nft_bitwise *priv,
@@ -193,6 +195,8 @@ static int nft_bitwise_init(const struct nft_ctx *ctx,
 	} else {
 		priv->op = NFT_BITWISE_BOOL;
 	}
+	if (tb[NFTA_BITWISE_NBITS])
+		priv->nbits = ntohl(nla_get_be32(tb[NFTA_BITWISE_NBITS]));
 
 	switch(priv->op) {
 	case NFT_BITWISE_BOOL:
@@ -243,6 +247,8 @@ static int nft_bitwise_dump(struct sk_buff *skb, const struct nft_expr *expr)
 		return -1;
 	if (nla_put_be32(skb, NFTA_BITWISE_OP, htonl(priv->op)))
 		return -1;
+	if (nla_put_be32(skb, NFTA_BITWISE_NBITS, htonl(priv->nbits)))
+		return -1;
 
 	switch (priv->op) {
 	case NFT_BITWISE_BOOL:
-- 
2.35.1

[nf-next PATCH v2 2/5] netfilter: bitwise: replace hard-coded size with `sizeof` expression

[Jeremy Sowden]

When calculating the length of an array, use the appropriate `sizeof`
expression for its type, rather than an integer literal.

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

diff --git a/net/netfilter/nft_bitwise.c b/net/netfilter/nft_bitwise.c
index cdace40c6ba0..38dd4aa8cd63 100644
--- a/net/netfilter/nft_bitwise.c
+++ b/net/netfilter/nft_bitwise.c
@@ -31,7 +31,7 @@ static void nft_bitwise_eval_bool(u32 *dst, const u32 *src,
 {
 	unsigned int i;
 
-	for (i = 0; i < DIV_ROUND_UP(priv->len, 4); i++)
+	for (i = 0; i < DIV_ROUND_UP(priv->len, sizeof(u32)); i++)
 		dst[i] = (src[i] & priv->mask.data[i]) ^ priv->xor.data[i];
 }
 
-- 
2.35.1

[nf-next PATCH v2 3/5] netfilter: bitwise: improve error goto labels

[Jeremy Sowden]

Replace two labels (`err1` and `err2`) with more informative ones.

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

diff --git a/net/netfilter/nft_bitwise.c b/net/netfilter/nft_bitwise.c
index 38dd4aa8cd63..9d3039a4ffa3 100644
--- a/net/netfilter/nft_bitwise.c
+++ b/net/netfilter/nft_bitwise.c
@@ -111,22 +111,23 @@ static int nft_bitwise_init_bool(struct nft_bitwise *priv,
 		return err;
 	if (mask.type != NFT_DATA_VALUE || mask.len != priv->len) {
 		err = -EINVAL;
-		goto err1;
+		goto err_mask_release;
 	}
 
 	err = nft_data_init(NULL, &priv->xor, sizeof(priv->xor), &xor,
 			    tb[NFTA_BITWISE_XOR]);
 	if (err < 0)
-		goto err1;
+		goto err_mask_release;
 	if (xor.type != NFT_DATA_VALUE || xor.len != priv->len) {
 		err = -EINVAL;
-		goto err2;
+		goto err_xor_release;
 	}
 
 	return 0;
-err2:
+
+err_xor_release:
 	nft_data_release(&priv->xor, xor.type);
-err1:
+err_mask_release:
 	nft_data_release(&priv->mask, mask.type);
 	return err;
 }
-- 
2.35.1

[nf-next PATCH v2 4/5] netfilter: bitwise: rename some boolean operation functions

[Jeremy Sowden]

In the next patch we add support for doing AND, OR and XOR operations
directly in the kernel, so rename some functions and an enum constant
related to mask-and-xor boolean operations.

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

diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index f3dcc4a34ff1..1c118a316381 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -539,16 +539,20 @@ enum nft_immediate_attributes {
 /**
  * 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
+ * @NFT_BITWISE_MASK_XOR: 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_MASK_XOR,
 	NFT_BITWISE_LSHIFT,
 	NFT_BITWISE_RSHIFT,
 };
+/*
+ * Old name for NFT_BITWISE_MASK_XOR.  Retained for backwards-compatibility.
+ */
+#define NFT_BITWISE_BOOL NFT_BITWISE_MASK_XOR
 
 /**
  * enum nft_bitwise_attributes - nf_tables bitwise expression netlink attributes
diff --git a/net/netfilter/nft_bitwise.c b/net/netfilter/nft_bitwise.c
index 9d3039a4ffa3..dad39a2f4433 100644
--- a/net/netfilter/nft_bitwise.c
+++ b/net/netfilter/nft_bitwise.c
@@ -26,8 +26,8 @@ struct nft_bitwise {
 	u8                      nbits;
 };
 
-static void nft_bitwise_eval_bool(u32 *dst, const u32 *src,
-				  const struct nft_bitwise *priv)
+static void nft_bitwise_eval_mask_xor(u32 *dst, const u32 *src,
+				      const struct nft_bitwise *priv)
 {
 	unsigned int i;
 
@@ -69,8 +69,8 @@ void nft_bitwise_eval(const struct nft_expr *expr,
 	u32 *dst = &regs->data[priv->dreg];
 
 	switch (priv->op) {
-	case NFT_BITWISE_BOOL:
-		nft_bitwise_eval_bool(dst, src, priv);
+	case NFT_BITWISE_MASK_XOR:
+		nft_bitwise_eval_mask_xor(dst, src, priv);
 		break;
 	case NFT_BITWISE_LSHIFT:
 		nft_bitwise_eval_lshift(dst, src, priv);
@@ -92,8 +92,8 @@ static const struct nla_policy nft_bitwise_policy[NFTA_BITWISE_MAX + 1] = {
 	[NFTA_BITWISE_NBITS]	= { .type = NLA_U32 },
 };
 
-static int nft_bitwise_init_bool(struct nft_bitwise *priv,
-				 const struct nlattr *const tb[])
+static int nft_bitwise_init_mask_xor(struct nft_bitwise *priv,
+				     const struct nlattr *const tb[])
 {
 	struct nft_data_desc mask, xor;
 	int err;
@@ -186,7 +186,7 @@ static int nft_bitwise_init(const struct nft_ctx *ctx,
 	if (tb[NFTA_BITWISE_OP]) {
 		priv->op = ntohl(nla_get_be32(tb[NFTA_BITWISE_OP]));
 		switch (priv->op) {
-		case NFT_BITWISE_BOOL:
+		case NFT_BITWISE_MASK_XOR:
 		case NFT_BITWISE_LSHIFT:
 		case NFT_BITWISE_RSHIFT:
 			break;
@@ -194,14 +194,14 @@ static int nft_bitwise_init(const struct nft_ctx *ctx,
 			return -EOPNOTSUPP;
 		}
 	} else {
-		priv->op = NFT_BITWISE_BOOL;
+		priv->op = NFT_BITWISE_MASK_XOR;
 	}
 	if (tb[NFTA_BITWISE_NBITS])
 		priv->nbits = ntohl(nla_get_be32(tb[NFTA_BITWISE_NBITS]));
 
 	switch(priv->op) {
-	case NFT_BITWISE_BOOL:
-		err = nft_bitwise_init_bool(priv, tb);
+	case NFT_BITWISE_MASK_XOR:
+		err = nft_bitwise_init_mask_xor(priv, tb);
 		break;
 	case NFT_BITWISE_LSHIFT:
 	case NFT_BITWISE_RSHIFT:
@@ -212,8 +212,8 @@ static int nft_bitwise_init(const struct nft_ctx *ctx,
 	return err;
 }
 
-static int nft_bitwise_dump_bool(struct sk_buff *skb,
-				 const struct nft_bitwise *priv)
+static int nft_bitwise_dump_mask_xor(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)
@@ -252,8 +252,8 @@ static int nft_bitwise_dump(struct sk_buff *skb, const struct nft_expr *expr)
 		return -1;
 
 	switch (priv->op) {
-	case NFT_BITWISE_BOOL:
-		err = nft_bitwise_dump_bool(skb, priv);
+	case NFT_BITWISE_MASK_XOR:
+		err = nft_bitwise_dump_mask_xor(skb, priv);
 		break;
 	case NFT_BITWISE_LSHIFT:
 	case NFT_BITWISE_RSHIFT:
@@ -273,7 +273,7 @@ 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)
+	if (priv->op != NFT_BITWISE_MASK_XOR)
 		return -EOPNOTSUPP;
 
 	if (memcmp(&priv->xor, &zero, sizeof(priv->xor)) ||
@@ -410,7 +410,7 @@ nft_bitwise_fast_dump(struct sk_buff *skb, const struct nft_expr *expr)
 		return -1;
 	if (nla_put_be32(skb, NFTA_BITWISE_LEN, htonl(sizeof(u32))))
 		return -1;
-	if (nla_put_be32(skb, NFTA_BITWISE_OP, htonl(NFT_BITWISE_BOOL)))
+	if (nla_put_be32(skb, NFTA_BITWISE_OP, htonl(NFT_BITWISE_MASK_XOR)))
 		return -1;
 
 	data.data[0] = priv->mask;
@@ -505,7 +505,7 @@ nft_bitwise_select_ops(const struct nft_ctx *ctx,
 		return &nft_bitwise_ops;
 
 	if (tb[NFTA_BITWISE_OP] &&
-	    ntohl(nla_get_be32(tb[NFTA_BITWISE_OP])) != NFT_BITWISE_BOOL)
+	    ntohl(nla_get_be32(tb[NFTA_BITWISE_OP])) != NFT_BITWISE_MASK_XOR)
 		return &nft_bitwise_ops;
 
 	return &nft_bitwise_fast_ops;
-- 
2.35.1

[nf-next PATCH v2 5/5] netfilter: bitwise: add support for doing AND, OR and XOR directly

[Jeremy Sowden]

Hitherto, these operations have been converted in user space to
mask-and-xor operations on one register and two immediate values, and it
is the latter which have been evaluated by the kernel.  We add support
for evaluating these operations directly in kernel space on one register
and either an immediate value or a second register.

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

diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index 1c118a316381..cd3e9e4ac646 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -543,14 +543,21 @@ enum nft_immediate_attributes {
  *                        and XOR boolean operations
  * @NFT_BITWISE_LSHIFT: left-shift operation
  * @NFT_BITWISE_RSHIFT: right-shift operation
+ * @NFT_BITWISE_AND: and operation
+ * @NFT_BITWISE_OR: or operation
+ * @NFT_BITWISE_XOR: xor operation
  */
 enum nft_bitwise_ops {
 	NFT_BITWISE_MASK_XOR,
 	NFT_BITWISE_LSHIFT,
 	NFT_BITWISE_RSHIFT,
+	NFT_BITWISE_AND,
+	NFT_BITWISE_OR,
+	NFT_BITWISE_XOR,
 };
 /*
- * Old name for NFT_BITWISE_MASK_XOR.  Retained for backwards-compatibility.
+ * Old name for NFT_BITWISE_MASK_XOR, predating the addition of NFT_BITWISE_AND,
+ * NFT_BITWISE_OR and NFT_BITWISE_XOR.  Retained for backwards-compatibility.
  */
 #define NFT_BITWISE_BOOL NFT_BITWISE_MASK_XOR
 
@@ -566,6 +573,7 @@ enum nft_bitwise_ops {
  * @NFTA_BITWISE_DATA: argument for non-boolean operations
  *                     (NLA_NESTED: nft_data_attributes)
  * @NFTA_BITWISE_NBITS: length of operation in bits (NLA_U32)
+ * @NFTA_BITWISE_SREG2: second source register (NLA_U32: nft_registers)
  *
  * The bitwise expression supports boolean and shift operations.  It implements
  * the boolean operations by performing the following operation:
@@ -590,6 +598,7 @@ enum nft_bitwise_attributes {
 	NFTA_BITWISE_OP,
 	NFTA_BITWISE_DATA,
 	NFTA_BITWISE_NBITS,
+	NFTA_BITWISE_SREG2,
 	__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 dad39a2f4433..eaf6b2766e11 100644
--- a/net/netfilter/nft_bitwise.c
+++ b/net/netfilter/nft_bitwise.c
@@ -17,6 +17,7 @@
 
 struct nft_bitwise {
 	u8			sreg;
+	u8			sreg2;
 	u8			dreg;
 	enum nft_bitwise_ops	op:8;
 	u8			len;
@@ -61,28 +62,72 @@ static void nft_bitwise_eval_rshift(u32 *dst, const u32 *src,
 	}
 }
 
+static void nft_bitwise_eval_and(u32 *dst, const u32 *src, const u32 *src2,
+				 const struct nft_bitwise *priv)
+{
+	unsigned int i, n;
+
+	for (i = 0, n = DIV_ROUND_UP(priv->len, sizeof(u32)); i < n; i++)
+		dst[i] = src[i] & src2[i];
+}
+
+static void nft_bitwise_eval_or(u32 *dst, const u32 *src, const u32 *src2,
+				const struct nft_bitwise *priv)
+{
+	unsigned int i, n;
+
+	for (i = 0, n = DIV_ROUND_UP(priv->len, sizeof(u32)); i < n; i++)
+		dst[i] = src[i] | src2[i];
+}
+
+static void nft_bitwise_eval_xor(u32 *dst, const u32 *src, const u32 *src2,
+				 const struct nft_bitwise *priv)
+{
+	unsigned int i, n;
+
+	for (i = 0, n = DIV_ROUND_UP(priv->len, sizeof(u32)); i < n; i++)
+		dst[i] = src[i] ^ src2[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];
+	const u32 *src = &regs->data[priv->sreg], *src2;
 	u32 *dst = &regs->data[priv->dreg];
 
-	switch (priv->op) {
-	case NFT_BITWISE_MASK_XOR:
+	if (priv->op == NFT_BITWISE_MASK_XOR) {
 		nft_bitwise_eval_mask_xor(dst, src, priv);
-		break;
-	case NFT_BITWISE_LSHIFT:
+		return;
+	}
+	if (priv->op == NFT_BITWISE_LSHIFT) {
 		nft_bitwise_eval_lshift(dst, src, priv);
-		break;
-	case NFT_BITWISE_RSHIFT:
+		return;
+	}
+	if (priv->op == NFT_BITWISE_RSHIFT) {
 		nft_bitwise_eval_rshift(dst, src, priv);
-		break;
+		return;
+	}
+
+	src2 = priv->sreg2 ? &regs->data[priv->sreg2] : priv->data.data;
+
+	if (priv->op == NFT_BITWISE_AND) {
+		nft_bitwise_eval_and(dst, src, src2, priv);
+		return;
+	}
+	if (priv->op == NFT_BITWISE_OR) {
+		nft_bitwise_eval_or(dst, src, src2, priv);
+		return;
+	}
+	if (priv->op == NFT_BITWISE_XOR) {
+		nft_bitwise_eval_xor(dst, src, src2, priv);
+		return;
 	}
 }
 
 static const struct nla_policy nft_bitwise_policy[NFTA_BITWISE_MAX + 1] = {
 	[NFTA_BITWISE_SREG]	= { .type = NLA_U32 },
+	[NFTA_BITWISE_SREG2]	= { .type = NLA_U32 },
 	[NFTA_BITWISE_DREG]	= { .type = NLA_U32 },
 	[NFTA_BITWISE_LEN]	= { .type = NLA_U32 },
 	[NFTA_BITWISE_MASK]	= { .type = NLA_NESTED },
@@ -98,7 +143,8 @@ static int nft_bitwise_init_mask_xor(struct nft_bitwise *priv,
 	struct nft_data_desc mask, xor;
 	int err;
 
-	if (tb[NFTA_BITWISE_DATA])
+	if (tb[NFTA_BITWISE_DATA] ||
+	    tb[NFTA_BITWISE_SREG2])
 		return -EINVAL;
 
 	if (!tb[NFTA_BITWISE_MASK] ||
@@ -139,7 +185,8 @@ static int nft_bitwise_init_shift(struct nft_bitwise *priv,
 	int err;
 
 	if (tb[NFTA_BITWISE_MASK] ||
-	    tb[NFTA_BITWISE_XOR])
+	    tb[NFTA_BITWISE_XOR]  ||
+	    tb[NFTA_BITWISE_SREG2])
 		return -EINVAL;
 
 	if (!tb[NFTA_BITWISE_DATA])
@@ -158,6 +205,40 @@ static int nft_bitwise_init_shift(struct nft_bitwise *priv,
 	return 0;
 }
 
+static int nft_bitwise_init_bool(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] && !tb[NFTA_BITWISE_SREG2]) ||
+	    (tb[NFTA_BITWISE_DATA] && tb[NFTA_BITWISE_SREG2]))
+		return -EINVAL;
+
+	if (tb[NFTA_BITWISE_DATA]) {
+		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;
+		}
+	} else {
+		err = nft_parse_register_load(tb[NFTA_BITWISE_SREG2],
+					      &priv->sreg2, priv->len);
+		if (err < 0)
+			return err;
+	}
+
+	return 0;
+}
+
 static int nft_bitwise_init(const struct nft_ctx *ctx,
 			    const struct nft_expr *expr,
 			    const struct nlattr * const tb[])
@@ -189,6 +270,9 @@ static int nft_bitwise_init(const struct nft_ctx *ctx,
 		case NFT_BITWISE_MASK_XOR:
 		case NFT_BITWISE_LSHIFT:
 		case NFT_BITWISE_RSHIFT:
+		case NFT_BITWISE_AND:
+		case NFT_BITWISE_OR:
+		case NFT_BITWISE_XOR:
 			break;
 		default:
 			return -EOPNOTSUPP;
@@ -207,6 +291,11 @@ static int nft_bitwise_init(const struct nft_ctx *ctx,
 	case NFT_BITWISE_RSHIFT:
 		err = nft_bitwise_init_shift(priv, tb);
 		break;
+	case NFT_BITWISE_AND:
+	case NFT_BITWISE_OR:
+	case NFT_BITWISE_XOR:
+		err = nft_bitwise_init_bool(priv, tb);
+		break;
 	}
 
 	return err;
@@ -235,6 +324,19 @@ static int nft_bitwise_dump_shift(struct sk_buff *skb,
 	return 0;
 }
 
+static int nft_bitwise_dump_bool(struct sk_buff *skb,
+				 const struct nft_bitwise *priv)
+{
+	if (nft_dump_register(skb, NFTA_BITWISE_SREG2, priv->sreg2))
+		return -1;
+
+	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);
@@ -259,6 +361,11 @@ static int nft_bitwise_dump(struct sk_buff *skb, const struct nft_expr *expr)
 	case NFT_BITWISE_RSHIFT:
 		err = nft_bitwise_dump_shift(skb, priv);
 		break;
+	case NFT_BITWISE_AND:
+	case NFT_BITWISE_OR:
+	case NFT_BITWISE_XOR:
+		err = nft_bitwise_dump_bool(skb, priv);
+		break;
 	}
 
 	return err;
@@ -303,6 +410,7 @@ static bool nft_bitwise_reduce(struct nft_regs_track *track,
 	    track->regs[priv->dreg].bitwise &&
 	    track->regs[priv->dreg].bitwise->ops == expr->ops &&
 	    priv->sreg == bitwise->sreg &&
+	    priv->sreg2 == bitwise->sreg2 &&
 	    priv->dreg == bitwise->dreg &&
 	    priv->op == bitwise->op &&
 	    priv->len == bitwise->len &&
-- 
2.35.1

Re: [nf-next PATCH v2 1/5] netfilter: bitwise: keep track of bit-length of expressions

[Florian Westphal]

Jeremy Sowden <jeremy@azazel.net> wrote:
> Some bitwise operations are generated in user space when munging paylod
> expressions.  During delinearization, user space attempts to eliminate
> these operations.  However, it does this before deducing the byte-order
> or the correct length in bits of the operands, which means that it
> doesn't always handle multi-byte host-endian operations correctly.
> Therefore, add support for storing the bit-length of the expression,
> even though the kernel doesn't use it, in order to be able to pass it
> back to user space.

Can rule udata be used for this, or is that too much work?
The udata infra is already used to store comments and it would not need
kernel changes.

Re: [nf-next PATCH v2 1/5] netfilter: bitwise: keep track of bit-length of expressions

[Jeremy Sowden]

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

On 2022-04-05, at 13:28:50 +0200, Florian Westphal wrote:
> Jeremy Sowden <jeremy@azazel.net> wrote:
> > Some bitwise operations are generated in user space when munging
> > paylod expressions.  During delinearization, user space attempts to
> > eliminate these operations.  However, it does this before deducing
> > the byte-order or the correct length in bits of the operands, which
> > means that it doesn't always handle multi-byte host-endian
> > operations correctly.  Therefore, add support for storing the
> > bit-length of the expression, even though the kernel doesn't use it,
> > in order to be able to pass it back to user space.
> 
> Can rule udata be used for this, or is that too much work?
> The udata infra is already used to store comments and it would not
> need kernel changes.

It wouldn't be straightforward.  Expression udata might make more sense
than adding a new bitwise attribute, but that doesn't currently exist.
Would it be worth adding?  I seem to recall considering something along
those lines for passing type information with expressions as a way to
implement casting.

J.

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

Re: [nf-next PATCH v2 1/5] netfilter: bitwise: keep track of bit-length of expressions

[Florian Westphal]

Jeremy Sowden <jeremy@azazel.net> wrote:
> It wouldn't be straightforward.  Expression udata might make more sense
> than adding a new bitwise attribute, but that doesn't currently exist.
> Would it be worth adding?  I seem to recall considering something along
> those lines for passing type information with expressions as a way to
> implement casting.

Had not thought of casting, good point.
Given bitwise needs to be touched anyway to get the second register
operations I think the proposed patch isn't too bad.

For casts and other use cases (including bitlen), I think its
not needed to add special udata for expressions, as userspace can't
zap them selectively.

We already do something similar for sets (to embed 'typeof' info
for key and data).

Probably extend nftnl_udata_rule_types in libnftnl to add a
NFTNL_UDATA_RULE_EXPR_INFO.

NFTNL_UDATA_RULE_EXPR_INFO would be nested and contain
expression specific (nested) attributes.

i.e., if you have something like

meta mark -> reg 1
binop reg1 &= 0x0000ffff
ct mark -> reg 2
binop and reg1 &= reg2  // ulen 16

Then rule udata would have:
NFTNL_UDATA_RULE_EXPR_INFO (nested)
   type 4 (nested, 4 refers to the last expression above,
 	   type '0' is reserved).
      type 1 // nla_u32  -> for binop, 1 is 'len', it would be
               defined privately in src/bitwise.c
END

because only expression 4 needs annotations, so we don't waste
space for expressions that do not need extra data.

We could reserve a few nested numbers for things that might make sense
for all (or many) expresssions, e.g. 'cast to type x'.

We could of course place expr specific structs in there too but so
far we managed to avoid this and it would be not-so-nice to break
nft userspace when listing a ruleset added by an older version.

Probably could extend struct netlink_linearize_ctx with a memory
blob pointer that netlink_linearize_rule()/netlink_gen_stmt() can use
to add extra data.

My problem is that its a lot of (userspace) code for something that can
easily be done by a small kernel patch such as this one and so far we
don't have a real need for something like this.

Re: [nf-next PATCH v2 1/5] netfilter: bitwise: keep track of bit-length of expressions

[Florian Westphal]

Jeremy Sowden <jeremy@azazel.net> wrote:
> diff --git a/net/netfilter/nft_bitwise.c b/net/netfilter/nft_bitwise.c
> index f590ee1c8a1b..cdace40c6ba0 100644
> --- a/net/netfilter/nft_bitwise.c
> +++ b/net/netfilter/nft_bitwise.c
> @@ -23,6 +23,7 @@ struct nft_bitwise {
>  	struct nft_data		mask;
>  	struct nft_data		xor;
>  	struct nft_data		data;
> +	u8                      nbits;
>  };
>  
>  static void nft_bitwise_eval_bool(u32 *dst, const u32 *src,
> @@ -88,6 +89,7 @@ static const struct nla_policy nft_bitwise_policy[NFTA_BITWISE_MAX + 1] = {
>  	[NFTA_BITWISE_XOR]	= { .type = NLA_NESTED },
>  	[NFTA_BITWISE_OP]	= { .type = NLA_U32 },
>  	[NFTA_BITWISE_DATA]	= { .type = NLA_NESTED },
> +	[NFTA_BITWISE_NBITS]	= { .type = NLA_U32 },

NLA_U8?

Atm values > 255 are accepted but silently truncated to u8.

Re: [nf-next PATCH v2 1/5] netfilter: bitwise: keep track of bit-length of expressions

[Jeremy Sowden]

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

On 2022-04-09, at 01:27:03 +0200, Florian Westphal wrote:
> Jeremy Sowden <jeremy@azazel.net> wrote:
> > diff --git a/net/netfilter/nft_bitwise.c b/net/netfilter/nft_bitwise.c
> > index f590ee1c8a1b..cdace40c6ba0 100644
> > --- a/net/netfilter/nft_bitwise.c
> > +++ b/net/netfilter/nft_bitwise.c
> > @@ -23,6 +23,7 @@ struct nft_bitwise {
> >  	struct nft_data		mask;
> >  	struct nft_data		xor;
> >  	struct nft_data		data;
> > +	u8                      nbits;
> >  };
> >  
> >  static void nft_bitwise_eval_bool(u32 *dst, const u32 *src,
> > @@ -88,6 +89,7 @@ static const struct nla_policy nft_bitwise_policy[NFTA_BITWISE_MAX + 1] = {
> >  	[NFTA_BITWISE_XOR]	= { .type = NLA_NESTED },
> >  	[NFTA_BITWISE_OP]	= { .type = NLA_U32 },
> >  	[NFTA_BITWISE_DATA]	= { .type = NLA_NESTED },
> > +	[NFTA_BITWISE_NBITS]	= { .type = NLA_U32 },
> 
> NLA_U8?
> 
> Atm values > 255 are accepted but silently truncated to u8.

Good point.  I imagine I copied and pasted the types from `len`, which
also has `NLA_U32` and `u8`.  It, however, is parsed correctly:

  err = nft_parse_u32_check(tb[NFTA_BITWISE_LEN], U8_MAX, &len);
  if (err < 0)
    return err;

Since `len` is `u8`, `nbits` will need to be `u16`.  My inclination is
to leave the netlink type as NLA_U32 and parse it as follows:

  err = nft_parse_u32_check(tb[NFTA_BITWISE_NBITS], U8_MAX * BITS_PER_BYTE,
                            &nbits);
  if (err < 0)
    return err;

J.

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

Re: [nf-next PATCH v2 1/5] netfilter: bitwise: keep track of bit-length of expressions

[Florian Westphal]

Jeremy Sowden <jeremy@azazel.net> wrote:
> Good point.  I imagine I copied and pasted the types from `len`, which
> also has `NLA_U32` and `u8`.  It, however, is parsed correctly:
> 
>   err = nft_parse_u32_check(tb[NFTA_BITWISE_LEN], U8_MAX, &len);
>   if (err < 0)
>     return err;
> 
> Since `len` is `u8`, `nbits` will need to be `u16`.  My inclination is
> to leave the netlink type as NLA_U32 and parse it as follows:
> 
>   err = nft_parse_u32_check(tb[NFTA_BITWISE_NBITS], U8_MAX * BITS_PER_BYTE,
>                             &nbits);
>   if (err < 0)
>     return err;

Yes, thats fine as well.

Re: [nf-next PATCH v2 2/5] netfilter: bitwise: replace hard-coded size with `sizeof` expression

[Florian Westphal]

Jeremy Sowden <jeremy@azazel.net> wrote:
> When calculating the length of an array, use the appropriate `sizeof`
> expression for its type, rather than an integer literal.

Applied to nf-next

Re: [nf-next PATCH v2 3/5] netfilter: bitwise: improve error goto labels

[Florian Westphal]

Jeremy Sowden <jeremy@azazel.net> wrote:
> Replace two labels (`err1` and `err2`) with more informative ones.

also applied to nf-next.

Re: [nf-next PATCH v2 1/5] netfilter: bitwise: keep track of bit-length of expressions

[Jeremy Sowden]

On 2022-04-06, at 05:12:44 +0200, Florian Westphal wrote:
> Jeremy Sowden <jeremy@azazel.net> wrote:
> > It wouldn't be straightforward.  Expression udata might make more sense
> > than adding a new bitwise attribute, but that doesn't currently exist.
> > Would it be worth adding?  I seem to recall considering something along
> > those lines for passing type information with expressions as a way to
> > implement casting.
> 
> Had not thought of casting, good point.
> Given bitwise needs to be touched anyway to get the second register
> operations I think the proposed patch isn't too bad.

Cool.

> For casts and other use cases (including bitlen), I think its
> not needed to add special udata for expressions, as userspace can't
> zap them selectively.
> 
> We already do something similar for sets (to embed 'typeof' info
> for key and data).
> 
> Probably extend nftnl_udata_rule_types in libnftnl to add a
> NFTNL_UDATA_RULE_EXPR_INFO.
> 
> NFTNL_UDATA_RULE_EXPR_INFO would be nested and contain
> expression specific (nested) attributes.
> 
> i.e., if you have something like
> 
> meta mark -> reg 1
> binop reg1 &= 0x0000ffff
> ct mark -> reg 2
> binop and reg1 &= reg2  // ulen 16
> 
> Then rule udata would have:
> NFTNL_UDATA_RULE_EXPR_INFO (nested)
>    type 4 (nested, 4 refers to the last expression above,
>  	   type '0' is reserved).
>       type 1 // nla_u32  -> for binop, 1 is 'len', it would be
>                defined privately in src/bitwise.c
> END
> 
> because only expression 4 needs annotations, so we don't waste
> space for expressions that do not need extra data.
> 
> We could reserve a few nested numbers for things that might make sense
> for all (or many) expresssions, e.g. 'cast to type x'.
> 
> We could of course place expr specific structs in there too but so
> far we managed to avoid this and it would be not-so-nice to break
> nft userspace when listing a ruleset added by an older version.
> 
> Probably could extend struct netlink_linearize_ctx with a memory
> blob pointer that netlink_linearize_rule()/netlink_gen_stmt() can use
> to add extra data.
> 
> My problem is that its a lot of (userspace) code for something that can
> easily be done by a small kernel patch such as this one and so far we
> don't have a real need for something like this.

Yes, quite.

If you're happy passing the bit-length through the kernel, how about the
rest of the series?  I'm pretty sure there's some debatable choices in
there too. :)

J.