linux-kernel - Re: m68k boot failure in -next bisected to 'xarray: Replace exceptional entries'

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180623232033.GF18630@bombadil.infradead.org>
Date:   Sat, 23 Jun 2018 16:20:33 -0700
From:   Matthew Wilcox <willy@...radead.org>
To:     Guenter Roeck <linux@...ck-us.net>
Cc:     Josef Bacik <jbacik@...com>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        linux-m68k@...ts.linux-m68k.org, linux-kernel@...r.kernel.org
Subject: Re: m68k boot failure in -next bisected to 'xarray: Replace
 exceptional entries'

On Sat, Jun 23, 2018 at 09:43:41AM -0700, Guenter Roeck wrote:
> On 06/23/2018 12:46 AM, Matthew Wilcox wrote:
> > There actually is a rule that pointers passed to the IDR be aligned.
> > It might not be written down anywhere ;-)  And I'm quite happy to lift
> > that restriction; after all I don't want to force everybody to decorate
> > definitions with __aligned(4).
> 
> I am not sure if that is really correct, or at least I was unable to
> find such a restriction documented or even mentioned anywhere. It is
> true for radix trees, but even there the restriction used to be "even".
> This was changed to "word aligned" with commit 3bcadd6fa6c4f, and the
> offending commit here moves the "INTERNAL" bit from position 0 to 1.
> This is quite a subtle change that was introduced over time. I'd be
> not surprised if there are other more severe problems lurking in
> the radix tree code and/or its users because of that.
> 
> Either case, I think the check for radix_tree_is_internal_node() in
> idr_alloc_u32() is wrong. If anything, it should be radix_tree_exception().
> If that was the case, the problem (and maybe other similar problems)
> would have been found with commit 3bcadd6fa6c4f, not only now.

Oh, but we want people to be able to store exceptional entries as well
as pointers.  So this was the right solution at the time.  Now that I'm
trying to expand the range of exceptional entries, it's time to make all
(*) values storable in the IDR.

(*) Still not all.  The radix tree/IDR/XArray reserve some values for
its own use.  Various values below 4096 and above -4095, for example.

Here's the test-case I'm currently working on:

+static void idr_align_test(struct idr *idr)
+{
+       char name[] = "Motorola 68000";
+       int i;
+
+       for (i = 0; i < 9; i++)
+               BUG_ON(idr_alloc(idr, &name[i], 0, 0, GFP_KERNEL) != i);
+       idr_destroy(idr);
+
+       for (i = 1; i < 10; i++)
+               BUG_ON(idr_alloc(idr, &name[i], 0, 0, GFP_KERNEL) != i - 1);
+       idr_destroy(idr);
+
+       for (i = 2; i < 11; i++)
+               BUG_ON(idr_alloc(idr, &name[i], 0, 0, GFP_KERNEL) != i - 2);
+       idr_destroy(idr);
+
+       for (i = 3; i < 12; i++)
+               BUG_ON(idr_alloc(idr, &name[i], 0, 0, GFP_KERNEL) != i - 3);
+       idr_destroy(idr);
+
+       for (i = 0; i < 8; i++) {
+               BUG_ON(idr_alloc(idr, &name[i], 0, 0, GFP_KERNEL) != 0);
+               BUG_ON(idr_alloc(idr, &name[i + 1], 0, 0, GFP_KERNEL) != 1);
+               idr_remove(idr, 1);
+               idr_remove(idr, 0);
+               BUG_ON(!idr_is_empty(idr));
+       }
+}