Re: [PATCH] lib/rbtree: avoid pointless rb_node alignment

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi David,

On Fri, Jan 10, 2020 at 11:02 PM Davidlohr Bueso <dave@stgolabs.net> wrote:
> Now that Linux no longer supports the CRIS architecture,
> we can drop this fishy alignment. Apparently this was
> need to prevent misalignments in struct address_space.
>
> Signed-off-by: Davidlohr Bueso <dbueso@suse.de>

Thanks for your patch!

> --- a/include/linux/rbtree.h
> +++ b/include/linux/rbtree.h
> @@ -25,8 +25,7 @@ struct rb_node {
>         unsigned long  __rb_parent_color;
>         struct rb_node *rb_right;
>         struct rb_node *rb_left;
> -} __attribute__((aligned(sizeof(long))));
> -    /* The alignment might seem pointless, but allegedly CRIS needs it */
> +};

Unfortunately this is also needed on m68k.

This is now commit 2d39da80d43693a2 ("include/linux/rbtree.h: avoid
pointless rb_node alignment") in linux-next, where Jason Donenfeld
reported the following crash:

> [    3.200000] Unable to handle kernel access at virtual address 6b7cdccf
> [    3.200000] Oops: 00000000
> [    3.200000] PC: [<00237454>] rb_erase+0x1c/0x346
> [    3.200000] SR: 2710  SP: c48c162a  a2: 0fcc5280
> [    3.200000] d0: 00000001    d1: 0fe6a000    d2: 0fd82000    d3: 00000000
> [    3.200000] d4: 00000000    d5: 00000377    a0: 002b28f6    a1: 002b28f6
> [    3.200000] Process ping (pid: 73, task=1e7eecd7)
> [    3.200000] Frame format=7 eff addr=46e60008 ssw=0505 faddr=46e60008
> [    3.200000] wb 1 stat/addr/data: 0000 00000000 00000000
> [    3.200000] wb 2 stat/addr/data: 0000 00000000 00000000
> [    3.200000] wb 3 stat/addr/data: 0000 46e60008 00000000
> [    3.200000] push data: 00000000 00000000 00000000 00000000
> [    3.200000] Stack from 0fe6bf00:
> [    3.200000]         0fd82000 00000000 0fd846e6 002b290a 002b28d0 0fe6bfb0 0023f77c 0fd846e6
> [    3.200000]         002b290a 0fd846e6 002b28f6 00045058 002b290a 0fd846e6 0fd82000 00000000
> [    3.200000]         0fd846e6 0fe6a000 0000fd00 00045956 0fd846e6 002b28f6 00000000 00000001
> [    3.200000]         0fd846e6 000458f4 00045972 0fd846e6 000000ff 0fcc5280 00015e76 0fd846e6
> [    3.200000]         00000000 00000000 00000000 00000377 c49ba615 00000000 80001510 00000000
> [    3.200000]         0000fd00 00000002 00000000 00000000 80009ff8 0001675a 00000000 8000b6c5
> [    3.200000] Call Trace: [<0023f77c>] timerqueue_del+0x20/0x8a
> [    3.200000]  [<00045058>] __remove_hrtimer+0x30/0xaa
> [    3.200000]  [<0000fd00>] unf_e1_exc+0x20/0x48
> [    3.200000]  [<00045956>] hrtimer_try_to_cancel+0x62/0x6c
> [    3.200000]  [<000458f4>] hrtimer_try_to_cancel+0x0/0x6c
> [    3.200000]  [<00045972>] hrtimer_cancel+0x12/0x20
> [    3.200000]  [<00015e76>] do_exit+0x8c/0x91e
> [    3.200000]  [<0000fd00>] unf_e1_exc+0x20/0x48
> [    3.200000]  [<0001675a>] do_group_exit+0x24/0x9c
> [    3.200000]  [<000167e6>] __wake_up_parent+0x0/0x20
> [    3.200000]  [<00002774>] syscall+0x8/0xc
> [    3.200000]  [<0004c002>] timecounter_read+0x16/0xcc
> [    3.200000] Code: 2a6c 0008 4a8d 6700 0202 4a8b 6700 0244 <206b>
> 0008 200b 4a88 6700 0166 224b 2028 0008 670a 2248 2040 2028 0008 66f6 2c68
> [    3.200000] Disabling lock debugging due to kernel taint
> [    3.200000] Fixing recursive fault but reboot is needed!
>
> Seems to happen if a process exits with an hrtimer?

I could easily reproduce it by just booting next-2020120 on ARAnyM:

Unable to handle kernel access at virtual address 7c05a009
Oops: 00000000
Modules linked in:
PC: [<002eb772>] rb_erase+0x1c8/0x25a
SR: 2704  SP: c286c837  a2: 10aab020
d0: 10a7ae67    d1: 00000000    d2: 00000000    d3: 00024000
d4: 00000008    d5: 270011de    a0: fab01081    a1: 00000000
Process sh (pid: 428, task=09f84a54)
Frame format=7 eff addr=fab01084 ssw=0525 faddr=fab01084
wb 1 stat/addr/data: 0000 00000000 00000000
wb 2 stat/addr/data: 0000 00000000 00000000
wb 3 stat/addr/data: 0000 fab01084 00000001
push data: 00000000 00000000 00000000 00000000
Stack from 10ab3ca0:
        00000000 1081bab0 003eb88a 003eb876 003eb876 002f1cc6 1081bab0 003eb88a
        003eb850 1081bab0 0004b0f2 003eb88a 1081bab0 00000000 00024000 1081bab0
        003eb880 003eb870 0004b4d0 1081bab0 003eb876 00000000 00000000 00002700
        0000000d 00000008 270011de 00043c1c 00000001 10803140 003e6408 10ab3dc4
        00024000 003eb88a 003eb874 0004b6dc 00000008 270011de 00002700 0000000f
        00000000 00000005 0004454a 0004ab1c 0004ab4e 10aab020 00000000 00002604
Call Trace: [<002f1cc6>] timerqueue_del+0x58/0x6a
 [<0004b0f2>] __remove_hrtimer+0x2a/0x9c

timerqueue_del() uses rbtree, and

    #define rb_parent(r)   ((struct rb_node *)((r)->__rb_parent_color & ~3))

relies on all objects being 4-byte aligned.  But your patch broke that
assumption on m68k, where the default alignment is 2-byte.

Andrew: please drop this patch.

Thanks!

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-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