[PATCH nft 0/2] Fix display of < 64 bits IPv6 masks in concatenated elements

[PATCH nft 0/2] Fix display of < 64 bits IPv6 masks in concatenated elements

[Stefano Brivio]

Patch 1/2 fixes the issue reported by PetrB, and patch 2/2 introduces a
test case for IPv4 and IPv6 ranges of different lengths in concatenated
set elements.

Stefano Brivio (2):
  segtree: Fix range_mask_len() for subnet ranges exceeding unsigned int
  tests: Introduce 0043_concatenated_ranges_1 for subnets of different
    sizes

 src/segtree.c                                 |  4 ++--
 .../testcases/sets/0043concatenated_ranges_1  | 23 +++++++++++++++++++
 2 files changed, 25 insertions(+), 2 deletions(-)
 create mode 100755 tests/shell/testcases/sets/0043concatenated_ranges_1

-- 
2.30.2

[PATCH nft 1/2] segtree: Fix range_mask_len() for subnet ranges exceeding unsigned int

[Stefano Brivio]

As concatenated ranges are fetched from kernel sets and displayed to
the user, range_mask_len() evaluates whether the range is suitable for
display as netmask, and in that case it calculates the mask length by
right-shifting the endpoints until no set bits are left, but in the
existing version the temporary copies of the endpoints are derived by
copying their unsigned int representation, which doesn't suffice for
IPv6 netmask lengths, in general.

PetrB reports that, after inserting a /56 subnet in a concatenated set
element, it's listed as a /64 range. In fact, this happens for any
IPv6 mask shorter than 64 bits.

Fix this issue by simply sourcing the range endpoints provided by the
caller and setting the temporary copies with mpz_init_set(), instead
of fetching the unsigned int representation. The issue only affects
displaying of the masks, setting elements already works as expected.

Reported-by: PetrB <petr.boltik@gmail.com>
Bugzilla: https://bugzilla.netfilter.org/show_bug.cgi?id=1520
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 src/segtree.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/segtree.c b/src/segtree.c
index ad199355532e..353a0053ebc0 100644
--- a/src/segtree.c
+++ b/src/segtree.c
@@ -838,8 +838,8 @@ static int range_mask_len(const mpz_t start, const mpz_t end, unsigned int len)
 	mpz_t tmp_start, tmp_end;
 	int ret;
 
-	mpz_init_set_ui(tmp_start, mpz_get_ui(start));
-	mpz_init_set_ui(tmp_end, mpz_get_ui(end));
+	mpz_init_set(tmp_start, start);
+	mpz_init_set(tmp_end, end);
 
 	while (mpz_cmp(tmp_start, tmp_end) <= 0 &&
 		!mpz_tstbit(tmp_start, 0) && mpz_tstbit(tmp_end, 0) &&
-- 
2.30.2

[PATCH nft 2/2] tests: Introduce 0043_concatenated_ranges_1 for subnets of different sizes

[Stefano Brivio]

The report from https://bugzilla.netfilter.org/show_bug.cgi?id=1520
showed a display issue with particular IPv6 mask lengths in elements
of sets with concatenations. Make sure we cover insertion and listing
of different mask lengths in concatenated set elements for IPv4 and
IPv6.

Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 .../testcases/sets/0043concatenated_ranges_1  | 23 +++++++++++++++++++
 1 file changed, 23 insertions(+)
 create mode 100755 tests/shell/testcases/sets/0043concatenated_ranges_1

diff --git a/tests/shell/testcases/sets/0043concatenated_ranges_1 b/tests/shell/testcases/sets/0043concatenated_ranges_1
new file mode 100755
index 000000000000..bab189c56d8c
--- /dev/null
+++ b/tests/shell/testcases/sets/0043concatenated_ranges_1
@@ -0,0 +1,23 @@
+#!/bin/sh -e
+#
+# 0043concatenated_ranges_1 - Insert and list subnets of different sizes
+
+check() {
+	$NFT add element "${1}" t s "{ ${2} . ${3} }"
+	[ "$( $NFT list set "${1}" t s | grep -c "${2} . ${3}" )" = 1 ]
+}
+
+$NFT add table ip6 t
+$NFT add table ip  t
+
+$NFT add set ip6 t s '{ type ipv6_addr . ipv6_addr ; flags interval ; }'
+$NFT add set ip  t s '{ type ipv4_addr . ipv4_addr ; flags interval ; }'
+
+for n in $(seq 32 127); do
+	h="$(printf %x "${n}")"
+	check ip6 "2001:db8::/${n}" "2001:db8:${h}::-2001:db8:${h}::${h}:1"
+done
+
+for n in $(seq 24 31); do
+	check ip  "192.0.2.0/${n}"  "192.0.2.$((n * 3))-192.0.2.$((n * 3 + 2))"
+done
-- 
2.30.2

Re: [PATCH nft 1/2] segtree: Fix range_mask_len() for subnet ranges exceeding unsigned int

[Phil Sutter]

On Thu, May 06, 2021 at 12:23:13AM +0200, Stefano Brivio wrote:
> As concatenated ranges are fetched from kernel sets and displayed to
> the user, range_mask_len() evaluates whether the range is suitable for
> display as netmask, and in that case it calculates the mask length by
> right-shifting the endpoints until no set bits are left, but in the
> existing version the temporary copies of the endpoints are derived by
> copying their unsigned int representation, which doesn't suffice for
> IPv6 netmask lengths, in general.
> 
> PetrB reports that, after inserting a /56 subnet in a concatenated set
> element, it's listed as a /64 range. In fact, this happens for any
> IPv6 mask shorter than 64 bits.
> 
> Fix this issue by simply sourcing the range endpoints provided by the
> caller and setting the temporary copies with mpz_init_set(), instead
> of fetching the unsigned int representation. The issue only affects
> displaying of the masks, setting elements already works as expected.
> 

Fixes: 8ac2f3b2fca38 ("src: Add support for concatenated set ranges")

> Reported-by: PetrB <petr.boltik@gmail.com>
> Bugzilla: https://bugzilla.netfilter.org/show_bug.cgi?id=1520
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
>  src/segtree.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/segtree.c b/src/segtree.c
> index ad199355532e..353a0053ebc0 100644
> --- a/src/segtree.c
> +++ b/src/segtree.c
> @@ -838,8 +838,8 @@ static int range_mask_len(const mpz_t start, const mpz_t end, unsigned int len)
>  	mpz_t tmp_start, tmp_end;
>  	int ret;
>  
> -	mpz_init_set_ui(tmp_start, mpz_get_ui(start));
> -	mpz_init_set_ui(tmp_end, mpz_get_ui(end));
> +	mpz_init_set(tmp_start, start);
> +	mpz_init_set(tmp_end, end);

The old code is a bit funny, was there a specific reason why you
exported the values into a C variable intermediately?

Cheers, Phil

Re: [PATCH nft 1/2] segtree: Fix range_mask_len() for subnet ranges exceeding unsigned int

[Stefano Brivio]

On Thu, 6 May 2021 11:18:14 +0200
Phil Sutter <phil@nwl.cc> wrote:

> On Thu, May 06, 2021 at 12:23:13AM +0200, Stefano Brivio wrote:
> > As concatenated ranges are fetched from kernel sets and displayed to
> > the user, range_mask_len() evaluates whether the range is suitable for
> > display as netmask, and in that case it calculates the mask length by
> > right-shifting the endpoints until no set bits are left, but in the
> > existing version the temporary copies of the endpoints are derived by
> > copying their unsigned int representation, which doesn't suffice for
> > IPv6 netmask lengths, in general.
> > 
> > PetrB reports that, after inserting a /56 subnet in a concatenated set
> > element, it's listed as a /64 range. In fact, this happens for any
> > IPv6 mask shorter than 64 bits.
> > 
> > Fix this issue by simply sourcing the range endpoints provided by the
> > caller and setting the temporary copies with mpz_init_set(), instead
> > of fetching the unsigned int representation. The issue only affects
> > displaying of the masks, setting elements already works as expected.
> 
> Fixes: 8ac2f3b2fca38 ("src: Add support for concatenated set ranges")

Thanks Phil! I even looked it up and forgot to paste it ;)

> > Reported-by: PetrB <petr.boltik@gmail.com>
> > Bugzilla: https://bugzilla.netfilter.org/show_bug.cgi?id=1520
> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > ---
> >  src/segtree.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/segtree.c b/src/segtree.c
> > index ad199355532e..353a0053ebc0 100644
> > --- a/src/segtree.c
> > +++ b/src/segtree.c
> > @@ -838,8 +838,8 @@ static int range_mask_len(const mpz_t start, const mpz_t end, unsigned int len)
> >  	mpz_t tmp_start, tmp_end;
> >  	int ret;
> >  
> > -	mpz_init_set_ui(tmp_start, mpz_get_ui(start));
> > -	mpz_init_set_ui(tmp_end, mpz_get_ui(end));
> > +	mpz_init_set(tmp_start, start);
> > +	mpz_init_set(tmp_end, end);  
> 
> The old code is a bit funny, was there a specific reason why you
> exported the values into a C variable intermediately?

Laziness, ultimately: I didn't remember the name of gmp_printf(),
didn't look it up, and used a fprintf() instead to check 'start' and
'end'... and then whoops, I left the mpz_get_ui() calls there.

-- 
Stefano

Re: [PATCH nft 1/2] segtree: Fix range_mask_len() for subnet ranges exceeding unsigned int

[Phil Sutter]

On Thu, May 06, 2021 at 12:00:21PM +0200, Stefano Brivio wrote:
> On Thu, 6 May 2021 11:18:14 +0200
> Phil Sutter <phil@nwl.cc> wrote:
> 
> > On Thu, May 06, 2021 at 12:23:13AM +0200, Stefano Brivio wrote:
[...]
> > > diff --git a/src/segtree.c b/src/segtree.c
> > > index ad199355532e..353a0053ebc0 100644
> > > --- a/src/segtree.c
> > > +++ b/src/segtree.c
> > > @@ -838,8 +838,8 @@ static int range_mask_len(const mpz_t start, const mpz_t end, unsigned int len)
> > >  	mpz_t tmp_start, tmp_end;
> > >  	int ret;
> > >  
> > > -	mpz_init_set_ui(tmp_start, mpz_get_ui(start));
> > > -	mpz_init_set_ui(tmp_end, mpz_get_ui(end));
> > > +	mpz_init_set(tmp_start, start);
> > > +	mpz_init_set(tmp_end, end);  
> > 
> > The old code is a bit funny, was there a specific reason why you
> > exported the values into a C variable intermediately?
> 
> Laziness, ultimately: I didn't remember the name of gmp_printf(),
> didn't look it up, and used a fprintf() instead to check 'start' and
> 'end'... and then whoops, I left the mpz_get_ui() calls there.

OK, so this patch not just fixes a bug but also streamlines the code for
performance (and something with refactoring, too). ;)

Cheers, Phil