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.
Stefano Brivio (2):
lib: Fix bitmap_cut() for partial overlapping case
lib: Add test for bitmap_cut()
lib/bitmap.c | 4 ++--
lib/test_bitmap.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 62 insertions(+), 2 deletions(-)
--
2.26.2
Based on an original patch by Yury Norov: introduce a test for
bitmap_cut() that also makes sure functionality is as described for
partially overlapping src and dst.
Co-authored-by: Yury Norov <[email protected]>
Signed-off-by: Stefano Brivio <[email protected]>
---
lib/test_bitmap.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 60 insertions(+)
diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c
index 6b13150667f5..2c16adec5922 100644
--- a/lib/test_bitmap.c
+++ b/lib/test_bitmap.c
@@ -610,6 +610,65 @@ 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[4], *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);
+ if (!bitmap_equal(out, t->expected, t->nbits)) {
+ pr_err("bitmap_cut failed: expected %*pb, got %*pb\n",
+ t->nbits, t->expected, t->nbits, out);
+ }
+ }
+}
+
static void __init selftest(void)
{
test_zero_clear();
@@ -623,6 +682,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.26.2
On Mon, Jun 08, 2020 at 11:13:29AM +0200, Stefano Brivio wrote:
> Based on an original patch by Yury Norov: introduce a test for
> bitmap_cut() that also makes sure functionality is as described for
> partially overlapping src and dst.
> Co-authored-by: Yury Norov <[email protected]>
Co-developed-by (and it requires Yury's SoB as well).
> Signed-off-by: Stefano Brivio <[email protected]>
...
> +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 },
Perhaps leave comma as well?
> + { 0x00000001UL, },
> + },
> + { 16, BITS_PER_LONG * 2 + 1, BITS_PER_LONG * 2 + 1 + 16,
> + { 0x0000ffffUL, 0x5a5a5a5aUL, 0x5a5a5a5aUL, 0x5a5a5a5aUL },
Ditto.
> + { 0x2d2dffffUL, },
> + },
> +};
> +
> +static void __init test_bitmap_cut(void)
> +{
> + unsigned long b[4], *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);
> + if (!bitmap_equal(out, t->expected, t->nbits)) {
> + pr_err("bitmap_cut failed: expected %*pb, got %*pb\n",
> + t->nbits, t->expected, t->nbits, out);
> + }
Perhaps
if (bitmap_equal(...))
continue;
...
?
> + }
> +}
--
With Best Regards,
Andy Shevchenko
On Mon, Jun 08, 2020 at 11:13:27AM +0200, Stefano Brivio wrote:
> 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.
>
>
FWIW, after addressing comments per test case,
Reviewed-by: Andy Shevchenko <[email protected]>
> Stefano Brivio (2):
> lib: Fix bitmap_cut() for partial overlapping case
> lib: Add test for bitmap_cut()
>
> lib/bitmap.c | 4 ++--
> lib/test_bitmap.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 62 insertions(+), 2 deletions(-)
>
> --
> 2.26.2
>
--
With Best Regards,
Andy Shevchenko
On Mon, 8 Jun 2020 13:12:14 +0300
Andy Shevchenko <[email protected]> wrote:
> On Mon, Jun 08, 2020 at 11:13:29AM +0200, Stefano Brivio wrote:
> > Based on an original patch by Yury Norov: introduce a test for
> > bitmap_cut() that also makes sure functionality is as described for
> > partially overlapping src and dst.
>
> > Co-authored-by: Yury Norov <[email protected]>
>
> Co-developed-by (and it requires Yury's SoB as well).
Oops, sorry, I didn't remember this part from submitting-patches.rst
correctly. Thanks for pointing this out.
Yury, let me know if I should re-post with both Co-authored-by: and
Signed-off-by: you -- otherwise I'll repost without both.
> > Signed-off-by: Stefano Brivio <[email protected]>
>
> ...
>
> > +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 },
>
> Perhaps leave comma as well?
I have a full explicit initialiser for this one, hence the "missing"
comma, I find it clearer. Any specific reason why I should add it?
>
> > + { 0x00000001UL, },
> > + },
> > + { 16, BITS_PER_LONG * 2 + 1, BITS_PER_LONG * 2 + 1 + 16,
>
> > + { 0x0000ffffUL, 0x5a5a5a5aUL, 0x5a5a5a5aUL, 0x5a5a5a5aUL },
>
> Ditto.
>
> > + { 0x2d2dffffUL, },
> > + },
> > +};
> > +
> > +static void __init test_bitmap_cut(void)
> > +{
> > + unsigned long b[4], *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);
>
> > + if (!bitmap_equal(out, t->expected, t->nbits)) {
> > + pr_err("bitmap_cut failed: expected %*pb, got %*pb\n",
> > + t->nbits, t->expected, t->nbits, out);
> > + }
>
> Perhaps
>
> if (bitmap_equal(...))
> continue;
>
> ...
>
> ?
That's five lines instead of four (I can't get pr_err() on one line
anyway) and it looks less straightforward: "if it doesn't match we have
an error" vs. "if it matches go to next case. We have an error". Any
specific reason I'm missing?
--
Stefano
On Mon, Jun 8, 2020 at 1:29 PM Stefano Brivio <[email protected]> wrote:
> On Mon, 8 Jun 2020 13:12:14 +0300
> Andy Shevchenko <[email protected]> wrote:
> > On Mon, Jun 08, 2020 at 11:13:29AM +0200, Stefano Brivio wrote:
> > > Based on an original patch by Yury Norov: introduce a test for
> > > bitmap_cut() that also makes sure functionality is as described for
> > > partially overlapping src and dst.
> >
> > > Co-authored-by: Yury Norov <[email protected]>
> >
> > Co-developed-by (and it requires Yury's SoB as well).
>
> Oops, sorry, I didn't remember this part from submitting-patches.rst
> correctly. Thanks for pointing this out.
>
> Yury, let me know if I should re-post with both Co-authored-by: and
Co-developed-by: :-)
> Signed-off-by: you -- otherwise I'll repost without both.
...
> > > + if (!bitmap_equal(out, t->expected, t->nbits)) {
> > > + pr_err("bitmap_cut failed: expected %*pb, got %*pb\n",
> > > + t->nbits, t->expected, t->nbits, out);
> > > + }
> >
> > Perhaps
> >
> > if (bitmap_equal(...))
> > continue;
> >
> > ...
> >
> > ?
>
> That's five lines instead of four (I can't get pr_err() on one line
> anyway) and it looks less straightforward: "if it doesn't match we have
> an error" vs. "if it matches go to next case. We have an error". Any
> specific reason I'm missing?
Actually, please use one of suitable expect_eq_*() macro or add your
own. Because above has an inconsistent format with the rest.
--
With Best Regards,
Andy Shevchenko
On Mon, 8 Jun 2020 14:31:02 +0300
Andy Shevchenko <[email protected]> wrote:
> On Mon, Jun 8, 2020 at 1:29 PM Stefano Brivio <[email protected]> wrote:
> > On Mon, 8 Jun 2020 13:12:14 +0300
> > Andy Shevchenko <[email protected]> wrote:
> > > On Mon, Jun 08, 2020 at 11:13:29AM +0200, Stefano Brivio wrote:
> > > > Based on an original patch by Yury Norov: introduce a test for
> > > > bitmap_cut() that also makes sure functionality is as described for
> > > > partially overlapping src and dst.
> > >
> > > > Co-authored-by: Yury Norov <[email protected]>
> > >
> > > Co-developed-by (and it requires Yury's SoB as well).
> >
> > Oops, sorry, I didn't remember this part from submitting-patches.rst
> > correctly. Thanks for pointing this out.
> >
> > Yury, let me know if I should re-post with both Co-authored-by: and
>
> Co-developed-by: :-)
Grrr. That! :)
> > Signed-off-by: you -- otherwise I'll repost without both.
>
> ...
>
> > > > + if (!bitmap_equal(out, t->expected, t->nbits)) {
> > > > + pr_err("bitmap_cut failed: expected %*pb, got %*pb\n",
> > > > + t->nbits, t->expected, t->nbits, out);
> > > > + }
> > >
> > > Perhaps
> > >
> > > if (bitmap_equal(...))
> > > continue;
> > >
> > > ...
> > >
> > > ?
> >
> > That's five lines instead of four (I can't get pr_err() on one line
> > anyway) and it looks less straightforward: "if it doesn't match we have
> > an error" vs. "if it matches go to next case. We have an error". Any
> > specific reason I'm missing?
>
> Actually, please use one of suitable expect_eq_*() macro or add your
> own. Because above has an inconsistent format with the rest.
Whoops, I see now. Yes, expect_eq_bitmap() will do, I'll change this in
v2.
--
Stefano
Greeting,
FYI, we noticed the following commit (built with gcc-9):
commit: 0a1d3d4a3c5b6ac940e93041df8ad78430f0df95 ("[PATCH 2/2] lib: Add test for bitmap_cut()")
url: https://github.com/0day-ci/linux/commits/Stefano-Brivio/lib-Fix-bitmap_cut-for-overlaps-add-test/20200608-171544
in testcase: boot
on test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
+----------------------------------------------------------------------------------------+------------+------------+
| | 28010335d0 | 0a1d3d4a3c |
+----------------------------------------------------------------------------------------+------------+------------+
| boot_successes | 6 | 0 |
| boot_failures | 2 | 8 |
| Kernel_panic-not_syncing:VFS:Unable_to_mount_root_fs_on_unknown-block(#,#) | 2 | |
| Kernel_panic-not_syncing:stack-protector:Kernel_stack_is_corrupted_in:test_bitmap_init | 0 | 8 |
+----------------------------------------------------------------------------------------+------------+------------+
If you fix the issue, kindly add following tag
Reported-by: kernel test robot <[email protected]>
[ 33.028685] test_printf: all 364 tests passed
[ 33.030451] test_bitmap: loaded.
[ 33.031050] test_bitmap: parselist: 14: input is '0-2047:128/256' OK, Time: 721
[ 33.031968] test_bitmap: parselist_user: 14: input is '0-2047:128/256' OK, Time: 1161
[ 33.034559] test_bitmap: all 1663 tests passed
[ 33.035085] Kernel panic - not syncing: stack-protector: Kernel stack is corrupted in: test_bitmap_init+0x202/0x20b
[ 33.036182] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.7.0-12705-g0a1d3d4a3c5b6 #2
[ 33.036182] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014
[ 33.036182] Call Trace:
[ 33.036182] dump_stack+0x50/0x63
[ 33.036182] panic+0x100/0x2b4
[ 33.036182] ? printk+0x53/0x6a
[ 33.036182] ? test_bitmap_init+0x202/0x20b
[ 33.036182] __stack_chk_fail+0x10/0x10
[ 33.036182] test_bitmap_init+0x202/0x20b
[ 33.036182] ? test_zero_clear+0x1ab/0x1ab
[ 33.036182] do_one_initcall+0x6e/0x158
[ 33.036182] kernel_init_freeable+0x202/0x29f
[ 33.036182] ? rest_init+0xf7/0xf7
[ 33.036182] kernel_init+0x5/0xf0
[ 33.036182] ret_from_fork+0x1f/0x30
[ 33.036182] Kernel Offset: disabled
Elapsed time: 60
qemu-img create -f qcow2 disk-vm-snb-24-0 256G
To reproduce:
# build kernel
cd linux
cp config-5.7.0-12705-g0a1d3d4a3c5b6 .config
make HOSTCC=gcc-9 CC=gcc-9 ARCH=x86_64 olddefconfig prepare modules_prepare bzImage
git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
bin/lkp qemu -k <bzImage> job-script # job-script is attached in this email
Thanks,
lkp