[PATCH nft 1/2] netlink: fix concat range expansion in map case

[PATCH nft 1/2] netlink: fix concat range expansion in map case

[Florian Westphal]

Maps with range + concatenation do not work:

Input to nft -f:
        map map_test_concat_interval {
                type ipv4_addr . ipv4_addr : mark
                flags interval
                elements = { 192.168.0.0/24 . 192.168.0.0/24 : 1,
                     192.168.0.0/24 . 10.0.0.1 : 2,
                             192.168.1.0/24 . 10.0.0.1 : 3,
                             192.168.0.0/24 . 192.168.1.10 : 4,
                }
        }

nft list:
        map map_test_concat_interval {
                type ipv4_addr . ipv4_addr : mark
                flags interval
                elements = { 192.168.0.0 . 192.168.0.0-10.0.0.1 : 0x00000002,
                             192.168.1.0-192.168.0.0 . 10.0.0.1-192.168.1.10 : 0x00000004 }
        }

This is not a display bug, nft sends broken information
to kernel.  Use the correct key expression to fix this.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/netlink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/netlink.c b/src/netlink.c
index f752c3c932aa..b57e1c558501 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -123,7 +123,7 @@ static struct nftnl_set_elem *alloc_nftnl_setelem(const struct expr *set,
 	netlink_gen_data(key, &nld);
 	nftnl_set_elem_set(nlse, NFTNL_SET_ELEM_KEY, &nld.value, nld.len);
 
-	if (set->set_flags & NFT_SET_INTERVAL && expr->key->field_count > 1) {
+	if (set->set_flags & NFT_SET_INTERVAL && key->field_count > 1) {
 		key->flags |= EXPR_F_INTERVAL_END;
 		netlink_gen_data(key, &nld);
 		key->flags &= ~EXPR_F_INTERVAL_END;
-- 
2.26.2

[PATCH nft 2/2] tests: extend 0043concatenated_ranges_0 to cover maps too

[Florian Westphal]

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 .../testcases/sets/0043concatenated_ranges_0  | 78 ++++++++++++-------
 1 file changed, 50 insertions(+), 28 deletions(-)

diff --git a/tests/shell/testcases/sets/0043concatenated_ranges_0 b/tests/shell/testcases/sets/0043concatenated_ranges_0
index a783dacc361c..bb13bdada610 100755
--- a/tests/shell/testcases/sets/0043concatenated_ranges_0
+++ b/tests/shell/testcases/sets/0043concatenated_ranges_0
@@ -63,27 +63,31 @@ cat <<'EOF' > "${tmp}"
 flush ruleset
 
 table inet filter {
-	set test {
-		type ${ta} . ${tb} . ${tc}
+	${setmap} test {
+		type ${ta} . ${tb} . ${tc} ${mapt}
 		flags interval,timeout
-		elements = { ${a1} . ${b1} . ${c1} ,
-			     ${a2} . ${b2} . ${c2} ,
-			     ${a3} . ${b3} . ${c3} }
+		elements = { ${a1} . ${b1} . ${c1} ${mapv},
+			     ${a2} . ${b2} . ${c2} ${mapv},
+			     ${a3} . ${b3} . ${c3} ${mapv}, }
 	}
 
 	chain output {
 		type filter hook output priority 0; policy accept;
-		${sa} . ${sb} . ${sc} @test counter
+		${rule} @test counter
 	}
 }
 EOF
 
 timeout_tested=0
+run_test()
+{
+setmap="$1"
 for ta in ${TYPES}; do
 	eval a=\$ELEMS_${ta}
 	a1=${a%% *}; a2=$(expr "$a" : ".* \(.*\) .*"); a3=${a##* }
 	eval sa=\$RULESPEC_${ta}
 
+	mark=0
 	for tb in ${TYPES}; do
 		[ "${tb}" = "${ta}" ] && continue
 		if [ "${tb}" = "ipv6_addr" ]; then
@@ -107,40 +111,54 @@ for ta in ${TYPES}; do
 				[ "${tb}" = "ipv6_addr" ] && continue
 			fi
 
-			echo "TYPE: ${ta} ${tb} ${tc}"
+			echo "$setmap TYPE: ${ta} ${tb} ${tc}"
 
 			eval c=\$ELEMS_${tc}
 			c1=${c%% *}; c2=$(expr "$c" : ".* \(.*\) .*"); c3=${c##* }
 			eval sc=\$RULESPEC_${tc}
 
+			case "${setmap}" in
+			"set")
+				mapt=""
+				mapv=""
+				rule="${sa} . ${sb} . ${sc}"
+			;;
+			"map")
+				mapt=": mark"
+				mark=$RANDOM
+				mapv=$(printf " : 0x%08x" ${mark})
+				rule="meta mark set ${sa} . ${sb} . ${sc} map"
+			;;
+			esac
+
 			render ${tmp} | ${NFT} -f -
 
-			[ $(${NFT} list set inet filter test |		\
-			   grep -c -e "${a1} . ${b1} . ${c1}"		\
-				   -e "${a2} . ${b2} . ${c2}"		\
-				   -e "${a3} . ${b3} . ${c3}") -eq 3 ]
+			[ $(${NFT} list ${setmap} inet filter test |		\
+			   grep -c -e "${a1} . ${b1} . ${c1}${mapv}"		\
+				   -e "${a2} . ${b2} . ${c2}${mapv}"		\
+				   -e "${a3} . ${b3} . ${c3}${mapv}") -eq 3 ]
 
 			! ${NFT} "add element inet filter test \
-				  { ${a1} . ${b1} . ${c1} };
+				  { ${a1} . ${b1} . ${c1}${mapv} };
 				  add element inet filter test \
-				  { ${a2} . ${b2} . ${c2} };
+				  { ${a2} . ${b2} . ${c2}${mapv} };
 				  add element inet filter test \
-				  { ${a3} . ${b3} . ${c3} }" 2>/dev/null
+				  { ${a3} . ${b3} . ${c3}${mapv} }" 2>/dev/null
 
 			${NFT} delete element inet filter test \
-				"{ ${a1} . ${b1} . ${c1} }"
+				"{ ${a1} . ${b1} . ${c1}${mapv} }"
 			! ${NFT} delete element inet filter test \
-				"{ ${a1} . ${b1} . ${c1} }" 2>/dev/null
+				"{ ${a1} . ${b1} . ${c1}${mapv} }" 2>/dev/null
 
 			eval add_a=\$ADD_${ta}
 			eval add_b=\$ADD_${tb}
 			eval add_c=\$ADD_${tc}
 			${NFT} add element inet filter test \
-				"{ ${add_a} . ${add_b} . ${add_c} timeout 1s}"
-			[ $(${NFT} list set inet filter test |		\
+				"{ ${add_a} . ${add_b} . ${add_c} timeout 1s${mapv}}"
+			[ $(${NFT} list ${setmap} inet filter test |	\
 			   grep -c "${add_a} . ${add_b} . ${add_c}") -eq 1 ]
 			! ${NFT} add element inet filter test \
-				"{ ${add_a} . ${add_b} . ${add_c} timeout 1s}" \
+				"{ ${add_a} . ${add_b} . ${add_c} timeout 1s${mapv}}" \
 				2>/dev/null
 
 			eval get_a=\$GET_${ta}
@@ -150,31 +168,35 @@ for ta in ${TYPES}; do
 			exp_b=${get_b##* }; get_b=${get_b%% *}
 			exp_c=${get_c##* }; get_c=${get_c%% *}
 			[ $(${NFT} get element inet filter test 	\
-			   "{ ${get_a} . ${get_b} . ${get_c} }" |	\
+			   "{ ${get_a} . ${get_b} . ${get_c}${mapv} }" |	\
 			   grep -c "${exp_a} . ${exp_b} . ${exp_c}") -eq 1 ]
 
 			${NFT} "delete element inet filter test \
-				{ ${a2} . ${b2} . ${c2} };
+				{ ${a2} . ${b2} . ${c2}${mapv} };
 				delete element inet filter test \
-				{ ${a3} . ${b3} . ${c3} }"
+				{ ${a3} . ${b3} . ${c3}${mapv} }"
 			! ${NFT} "delete element inet filter test \
-				  { ${a2} . ${b2} . ${c2} };
+				  { ${a2} . ${b2} . ${c2}${mapv} };
 				  delete element inet filter test \
-				  { ${a3} . ${b3} . ${c3} }" 2>/dev/null
+				  { ${a3} . ${b3} . ${c3} ${mapv} }" 2>/dev/null
 
 			if [ ${timeout_tested} -eq 1 ]; then
 				${NFT} delete element inet filter test \
-					"{ ${add_a} . ${add_b} . ${add_c} }"
+					"{ ${add_a} . ${add_b} . ${add_c} ${mapv} }"
 				! ${NFT} delete element inet filter test \
-					"{ ${add_a} . ${add_b} . ${add_c} }" \
+					"{ ${add_a} . ${add_b} . ${add_c} ${mapv} }" \
 					2>/dev/null
 				continue
 			fi
 
 			sleep 1
-			[ $(${NFT} list set inet filter test |		\
-			   grep -c "${add_a} . ${add_b} . ${add_c}") -eq 0 ]
+			[ $(${NFT} list ${setmap} inet filter test |		\
+			   grep -c "${add_a} . ${add_b} . ${add_c} ${mapv}") -eq 0 ]
 			timeout_tested=1
 		done
 	done
 done
+}
+
+run_test "set"
+run_test "map"
-- 
2.26.2

Re: [PATCH nft 2/2] tests: extend 0043concatenated_ranges_0 to cover maps too

[Stefano Brivio]

On Wed, 22 Jul 2020 13:51:26 +0200
Florian Westphal <fw@strlen.de> wrote:

> +			"map")
> +				mapt=": mark"
> +				mark=$RANDOM
> +				mapv=$(printf " : 0x%08x" ${mark})

I don't have $RANDOM in dash :( Can you use $(date +%s) (it's
POSIX.2-1992) or a fixed number instead? The test doesn't fail for me
because printf turns that empty variable into 0x00000000 anyway, but it's
not really specified.

Looks good to me otherwise.

-- 
Stefano

Re: [PATCH nft 1/2] netlink: fix concat range expansion in map case

[Stefano Brivio]

On Wed, 22 Jul 2020 13:51:25 +0200
Florian Westphal <fw@strlen.de> wrote:

> This is not a display bug, nft sends broken information
> to kernel.  Use the correct key expression to fix this.

Thanks for fixing this! I didn't realise it could be so simple. :)

> Signed-off-by: Florian Westphal <fw@strlen.de>

Fixes: 8ac2f3b2fca3 ("src: Add support for concatenated set ranges")
Reviewed-by: Stefano Brivio <sbrivio@redhat.com>

-- 
Stefano