linux-kernel - Re: [PATCH v4 2/5] Xarray: move forward index correctly in xas

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAMuHMdWT05uMjEassapuFr7podb_eH=T14SOyS4yW4Wh7DUcTQ@mail.gmail.com>
Date: Thu, 6 Feb 2025 08:34:17 +0100
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Kemeng Shi <shikemeng@...weicloud.com>
Cc: akpm@...ux-foundation.org, willy@...radead.org, 
	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org, 
	linux-mm@...ck.org, linux-nfs@...r.kernel.org, 
	linux-m68k <linux-m68k@...ts.linux-m68k.org>
Subject: Re: [PATCH v4 2/5] Xarray: move forward index correctly in xas_pause()

Hi Kemeng,

On Thu, 6 Feb 2025 at 07:13, Kemeng Shi <shikemeng@...weicloud.com> wrote:
> on 1/28/2025 12:21 AM, Geert Uytterhoeven wrote:
> > On Wed, 18 Dec 2024 at 07:58, Kemeng Shi <shikemeng@...weicloud.com> wrote:
> >> After xas_load(), xas->index could point to mid of found multi-index entry
> >> and xas->index's bits under node->shift maybe non-zero. The afterward
> >> xas_pause() will move forward xas->index with xa->node->shift with bits
> >> under node->shift un-masked and thus skip some index unexpectedly.
> >>
> >> Consider following case:
> >> Assume XA_CHUNK_SHIFT is 4.
> >> xa_store_range(xa, 16, 31, ...)
> >> xa_store(xa, 32, ...)
> >> XA_STATE(xas, xa, 17);
> >> xas_for_each(&xas,...)
> >> xas_load(&xas)
> >> /* xas->index = 17, xas->xa_offset = 1, xas->xa_node->xa_shift = 4 */
> >> xas_pause()
> >> /* xas->index = 33, xas->xa_offset = 2, xas->xa_node->xa_shift = 4 */
> >> As we can see, index of 32 is skipped unexpectedly.
> >>
> >> Fix this by mask bit under node->xa_shift when move forward index in
> >> xas_pause().
> >>
> >> For now, this will not cause serious problems. Only minor problem
> >> like cachestat return less number of page status could happen.
> >>
> >> Signed-off-by: Kemeng Shi <shikemeng@...weicloud.com>
> >
> > Thanks for your patch, which is now commit c9ba5249ef8b080c ("Xarray:
> > move forward index correctly in xas_pause()") upstream.
> >
> >> --- a/lib/test_xarray.c
> >> +++ b/lib/test_xarray.c
> >> @@ -1448,6 +1448,41 @@ static noinline void check_pause(struct xarray *xa)
> >>         XA_BUG_ON(xa, count != order_limit);
> >>
> >>         xa_destroy(xa);
> >> +
> >> +       index = 0;
> >> +       for (order = XA_CHUNK_SHIFT; order > 0; order--) {
> >> +               XA_BUG_ON(xa, xa_store_order(xa, index, order,
> >> +                                       xa_mk_index(index), GFP_KERNEL));
> >> +               index += 1UL << order;
> >> +       }
> >> +
> >> +       index = 0;
> >> +       count = 0;
> >> +       xas_set(&xas, 0);
> >> +       rcu_read_lock();
> >> +       xas_for_each(&xas, entry, ULONG_MAX) {
> >> +               XA_BUG_ON(xa, entry != xa_mk_index(index));
> >> +               index += 1UL << (XA_CHUNK_SHIFT - count);
> >> +               count++;
> >> +       }
> >> +       rcu_read_unlock();
> >> +       XA_BUG_ON(xa, count != XA_CHUNK_SHIFT);
> >> +
> >> +       index = 0;
> >> +       count = 0;
> >> +       xas_set(&xas, XA_CHUNK_SIZE / 2 + 1);
> >> +       rcu_read_lock();
> >> +       xas_for_each(&xas, entry, ULONG_MAX) {
> >> +               XA_BUG_ON(xa, entry != xa_mk_index(index));
> >> +               index += 1UL << (XA_CHUNK_SHIFT - count);
> >> +               count++;
> >> +               xas_pause(&xas);
> >> +       }
> >> +       rcu_read_unlock();
> >> +       XA_BUG_ON(xa, count != XA_CHUNK_SHIFT);
> >> +
> >> +       xa_destroy(xa);
> >> +
> >>  }
> >
> > On m68k, the last four XA_BUG_ON() checks above are triggered when
> > running the test.  With extra debug prints added:
> >
> >     entry = 00000002 xa_mk_index(index) = 000000c1
> >     entry = 00000002 xa_mk_index(index) = 000000e1
> >     entry = 00000002 xa_mk_index(index) = 000000f1
> >     ...
> >     entry = 000000e2 xa_mk_index(index) = fffff0ff
> >     entry = 000000f9 xa_mk_index(index) = fffff8ff
> >     entry = 000000f2 xa_mk_index(index) = fffffcff
> >     count = 63 XA_CHUNK_SHIFT = 6
> >     entry = 00000081 xa_mk_index(index) = 00000001
> >     entry = 00000002 xa_mk_index(index) = 00000081
> >     entry = 00000002 xa_mk_index(index) = 000000c1
> >     ...
> >     entry = 000000e2 xa_mk_index(index) = ffffe0ff
> >     entry = 000000f9 xa_mk_index(index) = fffff0ff
> >     entry = 000000f2 xa_mk_index(index) = fffff8ff
> >      count = 62 XA_CHUNK_SHIFT = 6
> >
> > On arm32, the test succeeds, so it's probably not a 32-vs-64-bit issue.
> > Perhaps a big-endian or alignment issue (alignof(int/long) = 2)?
> Hi Geert,
> Sorry for late reply. After check the debug info and the code, I think
> the test is failed because CONFIG_XARRAY_MULTI is off. I guess
> CONFIG_XARRAY_MULTI is on arm32 and is off on m68k so the test result
> diffs. Luckly it's only a problem of of test code.
> I will send patch to correct the test code soon. Thanks!

You are right: CONFIG_XARRAY_MULTI is enabled in my arm32 build,
but not in my m68k build.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds