diff mbox series

[nft] segtree: Fix get element for little endian ranges

Message ID 20191114142122.13931-1-phil@nwl.cc
State Accepted
Delegated to: Pablo Neira
Headers show
Series [nft] segtree: Fix get element for little endian ranges | expand

Commit Message

Phil Sutter Nov. 14, 2019, 2:21 p.m. UTC
This fixes get element command for interval sets with host byte order
data type, like e.g. mark. During serializing of the range (or element)
to query, data was exported in wrong byteorder and consequently not
found in kernel.

The mystery part is that code seemed correct: When calling
constant_expr_alloc() from set_elem_add(), the set key's byteorder was
passed with correct value of BYTEORDER_HOST_ENDIAN.

Comparison with delete/add element code paths though turned out that in
those use-cases, constant_expr_alloc() is called with BYTEORDER_INVALID:

- seg_tree_init() takes byteorder field value of first element in
  init->expressions (i.e., the elements requested on command line) and
  assigns that to tree->byteorder
- tree->byteorder is passed to constant_expr_alloc() in
  set_insert_interval()
- the elements' byteorder happens to be the default value

This patch may not fix the right side, but at least it aligns get with
add/delete element codes.

Fixes: a43cc8d53096d ("src: support for get element command")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/segtree.c                                 | 12 +++---
 .../sets/0040get_host_endian_elements_0       | 43 +++++++++++++++++++
 2 files changed, 50 insertions(+), 5 deletions(-)
 create mode 100755 tests/shell/testcases/sets/0040get_host_endian_elements_0

Comments

Pablo Neira Ayuso Nov. 14, 2019, 11:34 p.m. UTC | #1
On Thu, Nov 14, 2019 at 03:21:22PM +0100, Phil Sutter wrote:
> This fixes get element command for interval sets with host byte order
> data type, like e.g. mark. During serializing of the range (or element)
> to query, data was exported in wrong byteorder and consequently not
> found in kernel.
> 
> The mystery part is that code seemed correct: When calling
> constant_expr_alloc() from set_elem_add(), the set key's byteorder was
> passed with correct value of BYTEORDER_HOST_ENDIAN.
> 
> Comparison with delete/add element code paths though turned out that in
> those use-cases, constant_expr_alloc() is called with BYTEORDER_INVALID:
> 
> - seg_tree_init() takes byteorder field value of first element in
>   init->expressions (i.e., the elements requested on command line) and
>   assigns that to tree->byteorder
> - tree->byteorder is passed to constant_expr_alloc() in
>   set_insert_interval()
> - the elements' byteorder happens to be the default value
> 
> This patch may not fix the right side, but at least it aligns get with
> add/delete element codes.
> 
> Fixes: a43cc8d53096d ("src: support for get element command")
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>
diff mbox series

Patch

diff --git a/src/segtree.c b/src/segtree.c
index 10c82eed5378f..9f1eecc0ae7e1 100644
--- a/src/segtree.c
+++ b/src/segtree.c
@@ -622,12 +622,12 @@  int set_to_intervals(struct list_head *errs, struct set *set,
 }
 
 static void set_elem_add(const struct set *set, struct expr *init, mpz_t value,
-			 uint32_t flags)
+			 uint32_t flags, enum byteorder byteorder)
 {
 	struct expr *expr;
 
 	expr = constant_expr_alloc(&internal_location, set->key->dtype,
-				   set->key->byteorder, set->key->len, NULL);
+				   byteorder, set->key->len, NULL);
 	mpz_set(expr->value, value);
 	expr = set_elem_expr_alloc(&internal_location, expr);
 	expr->flags = flags;
@@ -649,14 +649,16 @@  struct expr *get_set_intervals(const struct set *set, const struct expr *init)
 	list_for_each_entry(i, &init->expressions, list) {
 		switch (i->key->etype) {
 		case EXPR_VALUE:
-			set_elem_add(set, new_init, i->key->value, i->flags);
+			set_elem_add(set, new_init, i->key->value,
+				     i->flags, i->byteorder);
 			break;
 		default:
 			range_expr_value_low(low, i);
-			set_elem_add(set, new_init, low, 0);
+			set_elem_add(set, new_init, low, 0, i->byteorder);
 			range_expr_value_high(high, i);
 			mpz_add_ui(high, high, 1);
-			set_elem_add(set, new_init, high, EXPR_F_INTERVAL_END);
+			set_elem_add(set, new_init, high,
+				     EXPR_F_INTERVAL_END, i->byteorder);
 			break;
 		}
 	}
diff --git a/tests/shell/testcases/sets/0040get_host_endian_elements_0 b/tests/shell/testcases/sets/0040get_host_endian_elements_0
new file mode 100755
index 0000000000000..caf6a4af326a2
--- /dev/null
+++ b/tests/shell/testcases/sets/0040get_host_endian_elements_0
@@ -0,0 +1,43 @@ 
+#!/bin/bash
+
+RULESET="table ip t {
+	set s {
+		type mark
+		flags interval
+		elements = {
+			0x23-0x42, 0x1337
+		}
+	}
+}"
+
+$NFT -f - <<< "$RULESET" || { echo "can't apply basic ruleset"; exit 1; }
+
+$NFT get element ip t s '{ 0x23-0x42 }' || {
+	echo "can't find existing range 0x23-0x42"
+	exit 1
+}
+
+$NFT get element ip t s '{ 0x26-0x28 }' || {
+	echo "can't find existing sub-range 0x26-0x28"
+	exit 1
+}
+
+$NFT get element ip t s '{ 0x26-0x99 }' && {
+	echo "found non-existing range 0x26-0x99"
+	exit 1
+}
+
+$NFT get element ip t s '{ 0x55-0x99 }' && {
+	echo "found non-existing range 0x55-0x99"
+	exit 1
+}
+
+$NFT get element ip t s '{ 0x55 }' && {
+	echo "found non-existing element 0x55"
+	exit 1
+}
+
+$NFT get element ip t s '{ 0x1337 }' || {
+	echo "can't find existing element 0x1337"
+	exit 1
+}