[PATCH 0/2] lib: Fix bitmap_cut() for overlaps, add test

[PATCH 0/2] lib: Fix bitmap_cut() for overlaps, add test

[Stefano Brivio]

Patch 1/2 addresses the issue Yury reported with partially overlapping
src and dst in bitmap_cut(), and 2/2 adds a test that covers basic
functionality as well as this case.

v2: In 2/2, use macro to verify result, drop bogus Co-Authored-by:
    tag, both suggested by Andy Shevchenko, and avoid stack overflow

Stefano Brivio (2):
  bitmap: Fix bitmap_cut() for partial overlapping case
  bitmap: Add test for bitmap_cut()

 lib/bitmap.c      |  4 ++--
 lib/test_bitmap.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+), 2 deletions(-)

-- 
2.27.0

[PATCH 1/2] bitmap: Fix bitmap_cut() for partial overlapping case

[Stefano Brivio]

Yury Norov reports that bitmap_cut() will not produce the right outcome
if src and dst partially overlap, with src pointing at some location
after dst, because the memmove() affects src before we store the bits
that we need to keep, that is, the bits preceding the cut -- as long as
we the beginning of the cut is not aligned to a long.

Fix this by storing those bits before the memmove().

Note that this is just a theoretical concern so far, as the only user
of this function, pipapo_drop() from the nftables set back-end
implemented in net/netfilter/nft_set_pipapo.c, always supplies entirely
overlapping src and dst.

Reported-by: Yury Norov <yury.norov@gmail.com>
Fixes: 2092767168f0 ("bitmap: Introduce bitmap_cut(): cut bits and shift remaining")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
v2: No changes

 lib/bitmap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/bitmap.c b/lib/bitmap.c
index 89260aa342d6..c5712e8f4c38 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -211,13 +211,13 @@ void bitmap_cut(unsigned long *dst, const unsigned long *src,
 	unsigned long keep = 0, carry;
 	int i;
 
-	memmove(dst, src, len * sizeof(*dst));
-
 	if (first % BITS_PER_LONG) {
 		keep = src[first / BITS_PER_LONG] &
 		       (~0UL >> (BITS_PER_LONG - first % BITS_PER_LONG));
 	}
 
+	memmove(dst, src, len * sizeof(*dst));
+
 	while (cut--) {
 		for (i = first / BITS_PER_LONG; i < len; i++) {
 			if (i < len - 1)
-- 
2.27.0

[PATCH 2/2] bitmap: Add test for bitmap_cut()

[Stefano Brivio]

Inspired by an original patch from Yury Norov: introduce a test for
bitmap_cut() that also makes sure functionality is as described for
partially overlapping src and dst.

Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
v2:
  - use expect_eq_bitmap() instead of open coding result check (Andy
    Shevchenko)
  - don't use uncommon Co-authored-by: tag (Andy Shevchenko), drop
    it altogether as Yury asked me to go ahead with this and I haven't
    heard back in a while. Patch is now rather different anyway
  - avoid stack overflow, buffer needs to be five unsigned longs and
    not four as 'in' is shifted by one, spotted by kernel test robot
    with CONFIG_STACKPROTECTOR_STRONG

 lib/test_bitmap.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c
index 6b13150667f5..df903c53952b 100644
--- a/lib/test_bitmap.c
+++ b/lib/test_bitmap.c
@@ -610,6 +610,63 @@ static void __init test_for_each_set_clump8(void)
 		expect_eq_clump8(start, CLUMP_EXP_NUMBITS, clump_exp, &clump);
 }
 
+struct test_bitmap_cut {
+	unsigned int first;
+	unsigned int cut;
+	unsigned int nbits;
+	unsigned long in[4];
+	unsigned long expected[4];
+};
+
+static struct test_bitmap_cut test_cut[] = {
+	{  0,  0,  8, { 0x0000000aUL, }, { 0x0000000aUL, }, },
+	{  0,  0, 32, { 0xdadadeadUL, }, { 0xdadadeadUL, }, },
+	{  0,  3,  8, { 0x000000aaUL, }, { 0x00000015UL, }, },
+	{  3,  3,  8, { 0x000000aaUL, }, { 0x00000012UL, }, },
+	{  0,  1, 32, { 0xa5a5a5a5UL, }, { 0x52d2d2d2UL, }, },
+	{  0,  8, 32, { 0xdeadc0deUL, }, { 0x00deadc0UL, }, },
+	{  1,  1, 32, { 0x5a5a5a5aUL, }, { 0x2d2d2d2cUL, }, },
+	{  0, 15, 32, { 0xa5a5a5a5UL, }, { 0x00014b4bUL, }, },
+	{  0, 16, 32, { 0xa5a5a5a5UL, }, { 0x0000a5a5UL, }, },
+	{ 15, 15, 32, { 0xa5a5a5a5UL, }, { 0x000125a5UL, }, },
+	{ 15, 16, 32, { 0xa5a5a5a5UL, }, { 0x0000a5a5UL, }, },
+	{ 16, 15, 32, { 0xa5a5a5a5UL, }, { 0x0001a5a5UL, }, },
+
+	{ BITS_PER_LONG, BITS_PER_LONG, BITS_PER_LONG,
+		{ 0xa5a5a5a5UL, 0xa5a5a5a5UL, },
+		{ 0xa5a5a5a5UL, 0xa5a5a5a5UL, },
+	},
+	{ 1, BITS_PER_LONG - 1, BITS_PER_LONG,
+		{ 0xa5a5a5a5UL, 0xa5a5a5a5UL, },
+		{ 0x00000001UL, 0x00000001UL, },
+	},
+
+	{ 0, BITS_PER_LONG * 2, BITS_PER_LONG * 2 + 1,
+		{ 0xa5a5a5a5UL, 0x00000001UL, 0x00000001UL, 0x00000001UL },
+		{ 0x00000001UL, },
+	},
+	{ 16, BITS_PER_LONG * 2 + 1, BITS_PER_LONG * 2 + 1 + 16,
+		{ 0x0000ffffUL, 0x5a5a5a5aUL, 0x5a5a5a5aUL, 0x5a5a5a5aUL },
+		{ 0x2d2dffffUL, },
+	},
+};
+
+static void __init test_bitmap_cut(void)
+{
+	unsigned long b[5], *in = &b[1], *out = &b[0];	/* Partial overlap */
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(test_cut); i++) {
+		struct test_bitmap_cut *t = &test_cut[i];
+
+		memcpy(in, t->in, sizeof(t->in));
+
+		bitmap_cut(out, in, t->first, t->cut, t->nbits);
+
+		expect_eq_bitmap(t->expected, out, t->nbits);
+	}
+}
+
 static void __init selftest(void)
 {
 	test_zero_clear();
@@ -623,6 +680,7 @@ static void __init selftest(void)
 	test_bitmap_parselist_user();
 	test_mem_optimisations();
 	test_for_each_set_clump8();
+	test_bitmap_cut();
 }
 
 KSTM_MODULE_LOADERS(test_bitmap);
-- 
2.27.0

Re: [PATCH 2/2] bitmap: Add test for bitmap_cut()

[Andy Shevchenko]

On Sun, Jun 14, 2020 at 07:40:54PM +0200, Stefano Brivio wrote:
> Inspired by an original patch from Yury Norov: introduce a test for
> bitmap_cut() that also makes sure functionality is as described for
> partially overlapping src and dst.

Taking into account recent fixes for BE 64-bit, do we have test cases for a such?

> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
> v2:
>   - use expect_eq_bitmap() instead of open coding result check (Andy
>     Shevchenko)
>   - don't use uncommon Co-authored-by: tag (Andy Shevchenko), drop
>     it altogether as Yury asked me to go ahead with this and I haven't
>     heard back in a while. Patch is now rather different anyway
>   - avoid stack overflow, buffer needs to be five unsigned longs and
>     not four as 'in' is shifted by one, spotted by kernel test robot
>     with CONFIG_STACKPROTECTOR_STRONG
> 
>  lib/test_bitmap.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 58 insertions(+)
> 
> diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c
> index 6b13150667f5..df903c53952b 100644
> --- a/lib/test_bitmap.c
> +++ b/lib/test_bitmap.c
> @@ -610,6 +610,63 @@ static void __init test_for_each_set_clump8(void)
>  		expect_eq_clump8(start, CLUMP_EXP_NUMBITS, clump_exp, &clump);
>  }
>  
> +struct test_bitmap_cut {
> +	unsigned int first;
> +	unsigned int cut;
> +	unsigned int nbits;
> +	unsigned long in[4];
> +	unsigned long expected[4];
> +};
> +
> +static struct test_bitmap_cut test_cut[] = {
> +	{  0,  0,  8, { 0x0000000aUL, }, { 0x0000000aUL, }, },
> +	{  0,  0, 32, { 0xdadadeadUL, }, { 0xdadadeadUL, }, },
> +	{  0,  3,  8, { 0x000000aaUL, }, { 0x00000015UL, }, },
> +	{  3,  3,  8, { 0x000000aaUL, }, { 0x00000012UL, }, },
> +	{  0,  1, 32, { 0xa5a5a5a5UL, }, { 0x52d2d2d2UL, }, },
> +	{  0,  8, 32, { 0xdeadc0deUL, }, { 0x00deadc0UL, }, },
> +	{  1,  1, 32, { 0x5a5a5a5aUL, }, { 0x2d2d2d2cUL, }, },
> +	{  0, 15, 32, { 0xa5a5a5a5UL, }, { 0x00014b4bUL, }, },
> +	{  0, 16, 32, { 0xa5a5a5a5UL, }, { 0x0000a5a5UL, }, },
> +	{ 15, 15, 32, { 0xa5a5a5a5UL, }, { 0x000125a5UL, }, },
> +	{ 15, 16, 32, { 0xa5a5a5a5UL, }, { 0x0000a5a5UL, }, },
> +	{ 16, 15, 32, { 0xa5a5a5a5UL, }, { 0x0001a5a5UL, }, },
> +
> +	{ BITS_PER_LONG, BITS_PER_LONG, BITS_PER_LONG,
> +		{ 0xa5a5a5a5UL, 0xa5a5a5a5UL, },
> +		{ 0xa5a5a5a5UL, 0xa5a5a5a5UL, },
> +	},
> +	{ 1, BITS_PER_LONG - 1, BITS_PER_LONG,
> +		{ 0xa5a5a5a5UL, 0xa5a5a5a5UL, },
> +		{ 0x00000001UL, 0x00000001UL, },
> +	},
> +
> +	{ 0, BITS_PER_LONG * 2, BITS_PER_LONG * 2 + 1,
> +		{ 0xa5a5a5a5UL, 0x00000001UL, 0x00000001UL, 0x00000001UL },
> +		{ 0x00000001UL, },
> +	},
> +	{ 16, BITS_PER_LONG * 2 + 1, BITS_PER_LONG * 2 + 1 + 16,
> +		{ 0x0000ffffUL, 0x5a5a5a5aUL, 0x5a5a5a5aUL, 0x5a5a5a5aUL },
> +		{ 0x2d2dffffUL, },
> +	},
> +};
> +
> +static void __init test_bitmap_cut(void)
> +{
> +	unsigned long b[5], *in = &b[1], *out = &b[0];	/* Partial overlap */
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(test_cut); i++) {
> +		struct test_bitmap_cut *t = &test_cut[i];
> +
> +		memcpy(in, t->in, sizeof(t->in));
> +
> +		bitmap_cut(out, in, t->first, t->cut, t->nbits);
> +
> +		expect_eq_bitmap(t->expected, out, t->nbits);
> +	}
> +}
> +
>  static void __init selftest(void)
>  {
>  	test_zero_clear();
> @@ -623,6 +680,7 @@ static void __init selftest(void)
>  	test_bitmap_parselist_user();
>  	test_mem_optimisations();
>  	test_for_each_set_clump8();
> +	test_bitmap_cut();
>  }
>  
>  KSTM_MODULE_LOADERS(test_bitmap);
> -- 
> 2.27.0
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] bitmap: Fix bitmap_cut() for partial overlapping case

[Andy Shevchenko]

On Sun, Jun 14, 2020 at 07:40:53PM +0200, Stefano Brivio wrote:
> Yury Norov reports that bitmap_cut() will not produce the right outcome
> if src and dst partially overlap, with src pointing at some location
> after dst, because the memmove() affects src before we store the bits
> that we need to keep, that is, the bits preceding the cut -- as long as
> we the beginning of the cut is not aligned to a long.
> 
> Fix this by storing those bits before the memmove().
> 
> Note that this is just a theoretical concern so far, as the only user
> of this function, pipapo_drop() from the nftables set back-end
> implemented in net/netfilter/nft_set_pipapo.c, always supplies entirely
> overlapping src and dst.

LGTM as long as test cases are passed,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Reported-by: Yury Norov <yury.norov@gmail.com>
> Fixes: 2092767168f0 ("bitmap: Introduce bitmap_cut(): cut bits and shift remaining")
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
> v2: No changes
> 
>  lib/bitmap.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/bitmap.c b/lib/bitmap.c
> index 89260aa342d6..c5712e8f4c38 100644
> --- a/lib/bitmap.c
> +++ b/lib/bitmap.c
> @@ -211,13 +211,13 @@ void bitmap_cut(unsigned long *dst, const unsigned long *src,
>  	unsigned long keep = 0, carry;
>  	int i;
>  
> -	memmove(dst, src, len * sizeof(*dst));
> -
>  	if (first % BITS_PER_LONG) {
>  		keep = src[first / BITS_PER_LONG] &
>  		       (~0UL >> (BITS_PER_LONG - first % BITS_PER_LONG));
>  	}
>  
> +	memmove(dst, src, len * sizeof(*dst));
> +
>  	while (cut--) {
>  		for (i = first / BITS_PER_LONG; i < len; i++) {
>  			if (i < len - 1)
> -- 
> 2.27.0
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] bitmap: Add test for bitmap_cut()

[Andy Shevchenko]

On Mon, Jun 15, 2020 at 12:41:55PM +0300, Andy Shevchenko wrote:
> On Sun, Jun 14, 2020 at 07:40:54PM +0200, Stefano Brivio wrote:
> > Inspired by an original patch from Yury Norov: introduce a test for
> > bitmap_cut() that also makes sure functionality is as described for
> > partially overlapping src and dst.
> 
> Taking into account recent fixes for BE 64-bit, do we have test cases for a such?

It might be enough to have only these, but perhaps s390 guys can help?

Alexander, can you apply this patch (w/o the first one, which is suppose to
fix) and confirm that you have test case failure, followed by applying first
one and confirm a fix?

> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > ---
> > v2:
> >   - use expect_eq_bitmap() instead of open coding result check (Andy
> >     Shevchenko)
> >   - don't use uncommon Co-authored-by: tag (Andy Shevchenko), drop
> >     it altogether as Yury asked me to go ahead with this and I haven't
> >     heard back in a while. Patch is now rather different anyway
> >   - avoid stack overflow, buffer needs to be five unsigned longs and
> >     not four as 'in' is shifted by one, spotted by kernel test robot
> >     with CONFIG_STACKPROTECTOR_STRONG
> > 
> >  lib/test_bitmap.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 58 insertions(+)
> > 
> > diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c
> > index 6b13150667f5..df903c53952b 100644
> > --- a/lib/test_bitmap.c
> > +++ b/lib/test_bitmap.c
> > @@ -610,6 +610,63 @@ static void __init test_for_each_set_clump8(void)
> >  		expect_eq_clump8(start, CLUMP_EXP_NUMBITS, clump_exp, &clump);
> >  }
> >  
> > +struct test_bitmap_cut {
> > +	unsigned int first;
> > +	unsigned int cut;
> > +	unsigned int nbits;
> > +	unsigned long in[4];
> > +	unsigned long expected[4];
> > +};
> > +
> > +static struct test_bitmap_cut test_cut[] = {
> > +	{  0,  0,  8, { 0x0000000aUL, }, { 0x0000000aUL, }, },
> > +	{  0,  0, 32, { 0xdadadeadUL, }, { 0xdadadeadUL, }, },
> > +	{  0,  3,  8, { 0x000000aaUL, }, { 0x00000015UL, }, },
> > +	{  3,  3,  8, { 0x000000aaUL, }, { 0x00000012UL, }, },
> > +	{  0,  1, 32, { 0xa5a5a5a5UL, }, { 0x52d2d2d2UL, }, },
> > +	{  0,  8, 32, { 0xdeadc0deUL, }, { 0x00deadc0UL, }, },
> > +	{  1,  1, 32, { 0x5a5a5a5aUL, }, { 0x2d2d2d2cUL, }, },
> > +	{  0, 15, 32, { 0xa5a5a5a5UL, }, { 0x00014b4bUL, }, },
> > +	{  0, 16, 32, { 0xa5a5a5a5UL, }, { 0x0000a5a5UL, }, },
> > +	{ 15, 15, 32, { 0xa5a5a5a5UL, }, { 0x000125a5UL, }, },
> > +	{ 15, 16, 32, { 0xa5a5a5a5UL, }, { 0x0000a5a5UL, }, },
> > +	{ 16, 15, 32, { 0xa5a5a5a5UL, }, { 0x0001a5a5UL, }, },
> > +
> > +	{ BITS_PER_LONG, BITS_PER_LONG, BITS_PER_LONG,
> > +		{ 0xa5a5a5a5UL, 0xa5a5a5a5UL, },
> > +		{ 0xa5a5a5a5UL, 0xa5a5a5a5UL, },
> > +	},
> > +	{ 1, BITS_PER_LONG - 1, BITS_PER_LONG,
> > +		{ 0xa5a5a5a5UL, 0xa5a5a5a5UL, },
> > +		{ 0x00000001UL, 0x00000001UL, },
> > +	},
> > +
> > +	{ 0, BITS_PER_LONG * 2, BITS_PER_LONG * 2 + 1,
> > +		{ 0xa5a5a5a5UL, 0x00000001UL, 0x00000001UL, 0x00000001UL },
> > +		{ 0x00000001UL, },
> > +	},
> > +	{ 16, BITS_PER_LONG * 2 + 1, BITS_PER_LONG * 2 + 1 + 16,
> > +		{ 0x0000ffffUL, 0x5a5a5a5aUL, 0x5a5a5a5aUL, 0x5a5a5a5aUL },
> > +		{ 0x2d2dffffUL, },
> > +	},
> > +};
> > +
> > +static void __init test_bitmap_cut(void)
> > +{
> > +	unsigned long b[5], *in = &b[1], *out = &b[0];	/* Partial overlap */
> > +	int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(test_cut); i++) {
> > +		struct test_bitmap_cut *t = &test_cut[i];
> > +
> > +		memcpy(in, t->in, sizeof(t->in));
> > +
> > +		bitmap_cut(out, in, t->first, t->cut, t->nbits);
> > +
> > +		expect_eq_bitmap(t->expected, out, t->nbits);
> > +	}
> > +}
> > +
> >  static void __init selftest(void)
> >  {
> >  	test_zero_clear();
> > @@ -623,6 +680,7 @@ static void __init selftest(void)
> >  	test_bitmap_parselist_user();
> >  	test_mem_optimisations();
> >  	test_for_each_set_clump8();
> > +	test_bitmap_cut();
> >  }
> >  
> >  KSTM_MODULE_LOADERS(test_bitmap);
> > -- 
> > 2.27.0
> > 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] bitmap: Add test for bitmap_cut()

[Stefano Brivio]

On Mon, 15 Jun 2020 12:46:16 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Mon, Jun 15, 2020 at 12:41:55PM +0300, Andy Shevchenko wrote:
> > On Sun, Jun 14, 2020 at 07:40:54PM +0200, Stefano Brivio wrote:  
> > > Inspired by an original patch from Yury Norov: introduce a test for
> > > bitmap_cut() that also makes sure functionality is as described for
> > > partially overlapping src and dst.  
> > 
> > Taking into account recent fixes for BE 64-bit, do we have test cases for a such?  
> 
> It might be enough to have only these, but perhaps s390 guys can help?

There's no behaviour difference due to endianness in this test itself --
just word size was a topic, hence that BITS_PER_LONG usage with
redundant values (checked on i686).

That is, if you have:
	{ 0x0000ffffUL, 0x5a5a5a5aUL, 0x5a5a5a5aUL, 0x5a5a5a5aUL },

then 1 as array subscript always denotes the second item (from the left)
there, it doesn't matter how and where different architectures store it.

Indeed, if bitmap_cut() directly addressed single bytes within the
words, I would need to pay special attention there. The values I picked
for these tests are also meant to show any issue in that sense.

> Alexander, can you apply this patch (w/o the first one, which is suppose to
> fix) and confirm that you have test case failure, followed by applying first
> one and confirm a fix?

I did that already on s390x (of course, I thought :)), I can confirm
that. Without patch 1/2 the test also fails there:

[   20.917848] test_bitmap: [lib/test_bitmap.c:666] bitmaps contents differ: expected "0-16,18-19,21,24,26-27,29", got "1,3-4,6,9,11-12,14,16,18-19,21,24,26-27,29"

If Alexander wants to test this on a z14 or z15, sure, it won't harm.

By the way, tests for 'parse', 'parse_user' and 'parselist' report
issues:

[   20.390401] test_bitmap: loaded.
[   20.394839] test_bitmap: parse: 4: input is 1, result is 0x100000000, expected 0x1
[   20.395011] test_bitmap: parse: 5: input is deadbeef, result is 0xdeadbeef00000000, expected 0xdeadbeef
[   20.395059] test_bitmap: parse: 6: input is 1,0, result is 0x1, expected 0x100000000
[   20.395099] test_bitmap: parse: 7: input is deadbeef,
               ,0,1, result is 0x1, expected 0xdeadbeef
[   20.396696] test_bitmap: parse: 8: input is deadbeef,1,0, result is 0x1, expected 0x100000000
[   20.396735] test_bitmap: parse: 9: input is baadf00d,deadbeef,1,0, result is 0x1, expected 0x100000000
[   20.396835] test_bitmap: parse: 10: input is badf00d,deadbeef,1,0, errno is -75, expected 0
[   20.396878] test_bitmap: parse: 11: input is badf00d,deadbeef,1,0, errno is -75, expected 0
[   20.396913] test_bitmap: parse: 12: input is   badf00d,deadbeef,1,0  , errno is -75, expected 0
[   20.396957] test_bitmap: parse: 13: input is  , badf00d,deadbeef,1,0 , , errno is -75, expected 0
[   20.396983] test_bitmap: parse: 14: input is  , badf00d, ,, ,,deadbeef,1,0 , , errno is -75, expected 0
[   20.397052] test_bitmap: parse: 16: input is 3,0, errno is 0, expected -75
[   20.397712] test_bitmap: parse_user: 4: input is 1, result is 0x100000000, expected 0x1
[   20.397832] test_bitmap: parse_user: 5: input is deadbeef, result is 0xdeadbeef00000000, expected 0xdeadbeef
[   20.397928] test_bitmap: parse_user: 6: input is 1,0, result is 0x1, expected 0x100000000
[   20.398022] test_bitmap: parse_user: 7: input is deadbeef,
               ,0,1, result is 0x1, expected 0xdeadbeef
[   20.398116] test_bitmap: parse_user: 8: input is deadbeef,1,0, result is 0x1, expected 0x100000000
[   20.398209] test_bitmap: parse_user: 9: input is baadf00d,deadbeef,1,0, result is 0x1, expected 0x100000000
[   20.398301] test_bitmap: parse_user: 10: input is badf00d,deadbeef,1,0, errno is -75, expected 0
[   20.398393] test_bitmap: parse_user: 11: input is badf00d,deadbeef,1,0, errno is -75, expected 0
[   20.398484] test_bitmap: parse_user: 12: input is   badf00d,deadbeef,1,0  , errno is -75, expected 0
[   20.398574] test_bitmap: parse_user: 13: input is  , badf00d,deadbeef,1,0 , , errno is -75, expected 0
[   20.398666] test_bitmap: parse_user: 14: input is  , badf00d, ,, ,,deadbeef,1,0 , , errno is -75, expected 0
[   20.398794] test_bitmap: parse_user: 16: input is 3,0, errno is 0, expected -75
[   20.399906] test_bitmap: parselist: 14: input is '0-2047:128/256' OK, Time: 2641
[   20.400914] test_bitmap: parselist_user: 14: input is '0-2047:128/256' OK, Time: 19961
[   20.421406] test_bitmap: all 1679 tests passed

and at a glance those *seem* to be bugs in the tests themselves, not in
the actual functions they test. Sure, they should be fixed, but I can't
take care of that right now.

-- 
Stefano

Re: [PATCH 2/2] bitmap: Add test for bitmap_cut()

[Andy Shevchenko]

On Mon, Jun 15, 2020 at 01:08:25PM +0200, Stefano Brivio wrote:
> On Mon, 15 Jun 2020 12:46:16 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > On Mon, Jun 15, 2020 at 12:41:55PM +0300, Andy Shevchenko wrote:
> > > On Sun, Jun 14, 2020 at 07:40:54PM +0200, Stefano Brivio wrote:  
> > > > Inspired by an original patch from Yury Norov: introduce a test for
> > > > bitmap_cut() that also makes sure functionality is as described for
> > > > partially overlapping src and dst.  
> > > 
> > > Taking into account recent fixes for BE 64-bit, do we have test cases for a such?  
> > 
> > It might be enough to have only these, but perhaps s390 guys can help?
> 
> There's no behaviour difference due to endianness in this test itself --
> just word size was a topic, hence that BITS_PER_LONG usage with
> redundant values (checked on i686).
> 
> That is, if you have:
> 	{ 0x0000ffffUL, 0x5a5a5a5aUL, 0x5a5a5a5aUL, 0x5a5a5a5aUL },
> 
> then 1 as array subscript always denotes the second item (from the left)
> there, it doesn't matter how and where different architectures store it.
> 
> Indeed, if bitmap_cut() directly addressed single bytes within the
> words, I would need to pay special attention there. The values I picked
> for these tests are also meant to show any issue in that sense.
> 
> > Alexander, can you apply this patch (w/o the first one, which is suppose to
> > fix) and confirm that you have test case failure, followed by applying first
> > one and confirm a fix?
> 
> I did that already on s390x (of course, I thought :)), I can confirm
> that. Without patch 1/2 the test also fails there:
> 
> [   20.917848] test_bitmap: [lib/test_bitmap.c:666] bitmaps contents differ: expected "0-16,18-19,21,24,26-27,29", got "1,3-4,6,9,11-12,14,16,18-19,21,24,26-27,29"

Thanks!
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>


> If Alexander wants to test this on a z14 or z15, sure, it won't harm.

Sure.

> By the way, tests for 'parse', 'parse_user' and 'parselist' report
> issues:

I believe this [1] will fix it.

[1]: 81c4f4d924d5 ("lib: fix bitmap_parse() on 64-bit big endian archs")

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] bitmap: Add test for bitmap_cut()

[Alexander Gordeev]

On Mon, Jun 15, 2020 at 12:46:16PM +0300, Andy Shevchenko wrote:
> On Mon, Jun 15, 2020 at 12:41:55PM +0300, Andy Shevchenko wrote:
> > On Sun, Jun 14, 2020 at 07:40:54PM +0200, Stefano Brivio wrote:
> > > Inspired by an original patch from Yury Norov: introduce a test for
> > > bitmap_cut() that also makes sure functionality is as described for
> > > partially overlapping src and dst.
> > 
> > Taking into account recent fixes for BE 64-bit, do we have test cases for a such?
> 
> It might be enough to have only these, but perhaps s390 guys can help?
> 
> Alexander, can you apply this patch (w/o the first one, which is suppose to
> fix) and confirm that you have test case failure, followed by applying first
> one and confirm a fix?

This failure goes away when patch #1 is applied:

test_bitmap: [lib/test_bitmap.c:666] bitmaps contents differ: expected "0-16,18-19,21,24,26-27,29", got "1,3-4,6,9,11-12,14,16,18-19,21,24,26-27,29"

Thus, I confirm.

[...]

> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH 2/2] bitmap: Add test for bitmap_cut()

[Stefano Brivio]

On Mon, 15 Jun 2020 14:43:53 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Mon, Jun 15, 2020 at 01:08:25PM +0200, Stefano Brivio wrote:
> >   
> > [...]
> >   
> > By the way, tests for 'parse', 'parse_user' and 'parselist' report
> > issues:  
> 
> I believe this [1] will fix it.
> 
> [1]: 81c4f4d924d5 ("lib: fix bitmap_parse() on 64-bit big endian archs")

Yes, thanks, that works for me too.

-- 
Stefano