Hi Yee,
On Tue, Jun 28, 2022 at 1:42 PM <[email protected]> wrote:
> From: Yee Lee <[email protected]>
>
> This patch solves two issues.
>
> (1) The pool allocated by memblock needs to unregister from
> kmemleak scanning. Apply kmemleak_ignore_phys to replace the
> original kmemleak_free as its address now is stored in the phys tree.
>
> (2) The pool late allocated by page-alloc doesn't need to unregister.
> Move out the freeing operation from its call path.
>
> Suggested-by: Catalin Marinas <[email protected]>
> Suggested-by: Marco Elver <[email protected]>
> Signed-off-by: Yee Lee <[email protected]>
Thank you, this fixes the storm of
BUG: KFENCE: invalid read in scan_block+0x78/0x130
BUG: KFENCE: use-after-free read in scan_block+0x78/0x130
BUG: KFENCE: out-of-bounds read in scan_block+0x78/0x130
messages I was seeing on arm64.
Tested-by: Geert Uytterhoeven <[email protected]>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
On Fri, 15 Jul 2022 10:17:43 +0200 Geert Uytterhoeven <[email protected]> wrote:
> On Tue, Jun 28, 2022 at 1:42 PM <[email protected]> wrote:
> > From: Yee Lee <[email protected]>
> >
> > This patch solves two issues.
> >
> > (1) The pool allocated by memblock needs to unregister from
> > kmemleak scanning. Apply kmemleak_ignore_phys to replace the
> > original kmemleak_free as its address now is stored in the phys tree.
> >
> > (2) The pool late allocated by page-alloc doesn't need to unregister.
> > Move out the freeing operation from its call path.
> >
> > Suggested-by: Catalin Marinas <[email protected]>
> > Suggested-by: Marco Elver <[email protected]>
> > Signed-off-by: Yee Lee <[email protected]>
>
> Thank you, this fixes the storm of
>
> BUG: KFENCE: invalid read in scan_block+0x78/0x130
> BUG: KFENCE: use-after-free read in scan_block+0x78/0x130
> BUG: KFENCE: out-of-bounds read in scan_block+0x78/0x130
>
> messages I was seeing on arm64.
Thanks, but...
- It would be great if we could identify a Fixes: for this.
- This patch has been accused of crashing the kernel:
https://lkml.kernel.org/r/YsFeUHkrFTQ7T51Q@xsang-OptiPlex-9020
Do we think that report is bogus?
Hi Andrew,
On Sat, Jul 16, 2022 at 1:33 AM Andrew Morton <[email protected]> wrote:
> On Fri, 15 Jul 2022 10:17:43 +0200 Geert Uytterhoeven <[email protected]> wrote:
> > On Tue, Jun 28, 2022 at 1:42 PM <[email protected]> wrote:
> > > From: Yee Lee <[email protected]>
> > >
> > > This patch solves two issues.
> > >
> > > (1) The pool allocated by memblock needs to unregister from
> > > kmemleak scanning. Apply kmemleak_ignore_phys to replace the
> > > original kmemleak_free as its address now is stored in the phys tree.
> > >
> > > (2) The pool late allocated by page-alloc doesn't need to unregister.
> > > Move out the freeing operation from its call path.
> > >
> > > Suggested-by: Catalin Marinas <[email protected]>
> > > Suggested-by: Marco Elver <[email protected]>
> > > Signed-off-by: Yee Lee <[email protected]>
> >
> > Thank you, this fixes the storm of
> >
> > BUG: KFENCE: invalid read in scan_block+0x78/0x130
> > BUG: KFENCE: use-after-free read in scan_block+0x78/0x130
> > BUG: KFENCE: out-of-bounds read in scan_block+0x78/0x130
> >
> > messages I was seeing on arm64.
>
> Thanks, but...
>
> - It would be great if we could identify a Fixes: for this.
IIRC, I started seeing the issue with "[PATCH v4 3/4] mm:
kmemleak: add rbtree and store physical address for objects
allocated with PA" (i.e. commit 0c24e061196c21d5 ("mm: kmemleak:
add rbtree and store physical address for objects allocated
with PA")) of series "[PATCH v4 0/4] mm: kmemleak: store objects
allocated with physical address separately and check when scan"
(https://lore.kernel.org/all/[email protected]),
in an arm64 config that had enabled kfence.
So I think this patch is sort of a dependency for that series.
I had cherry-picked that series after bisecting a regression to
commit 23c2d497de21f258 ("mm: kmemleak: take a full lowmem check in
kmemleak_*_phys()") in v5.18-rc3, and having a look around.
> - This patch has been accused of crashing the kernel:
>
> https://lkml.kernel.org/r/YsFeUHkrFTQ7T51Q@xsang-OptiPlex-9020
>
> Do we think that report is bogus?
I think all of this is highly architecture-specific...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
On Sat, 16 Jul 2022 at 20:43, Geert Uytterhoeven <[email protected]> wrote:
[...]
> > - This patch has been accused of crashing the kernel:
> >
> > https://lkml.kernel.org/r/YsFeUHkrFTQ7T51Q@xsang-OptiPlex-9020
> >
> > Do we think that report is bogus?
>
> I think all of this is highly architecture-specific...
The report can be reproduced on i386 with CONFIG_X86_PAE=y. But e.g.
mm/memblock.c:memblock_free() is also guilty of using __pa() on
previously memblock_alloc()'d addresses. Looking at the phys addr
before memblock_alloc() does virt_to_phys(), the result of __pa()
looks correct even on PAE, at least for the purpose of passing it on
to kmemleak(). So I don't know what that BUG_ON(slow_virt_to_phys() !=
phys_addr) is supposed to tell us here.
Ideas?
On Sat, Jul 16, 2022 at 08:43:06PM +0200, Geert Uytterhoeven wrote:
> On Sat, Jul 16, 2022 at 1:33 AM Andrew Morton <[email protected]> wrote:
> > On Fri, 15 Jul 2022 10:17:43 +0200 Geert Uytterhoeven <[email protected]> wrote:
> > > On Tue, Jun 28, 2022 at 1:42 PM <[email protected]> wrote:
> > > > From: Yee Lee <[email protected]>
> > > >
> > > > This patch solves two issues.
> > > >
> > > > (1) The pool allocated by memblock needs to unregister from
> > > > kmemleak scanning. Apply kmemleak_ignore_phys to replace the
> > > > original kmemleak_free as its address now is stored in the phys tree.
> > > >
> > > > (2) The pool late allocated by page-alloc doesn't need to unregister.
> > > > Move out the freeing operation from its call path.
> > > >
> > > > Suggested-by: Catalin Marinas <[email protected]>
> > > > Suggested-by: Marco Elver <[email protected]>
> > > > Signed-off-by: Yee Lee <[email protected]>
> > >
> > > Thank you, this fixes the storm of
> > >
> > > BUG: KFENCE: invalid read in scan_block+0x78/0x130
> > > BUG: KFENCE: use-after-free read in scan_block+0x78/0x130
> > > BUG: KFENCE: out-of-bounds read in scan_block+0x78/0x130
> > >
> > > messages I was seeing on arm64.
> >
> > Thanks, but...
> >
> > - It would be great if we could identify a Fixes: for this.
>
> IIRC, I started seeing the issue with "[PATCH v4 3/4] mm:
> kmemleak: add rbtree and store physical address for objects
> allocated with PA" (i.e. commit 0c24e061196c21d5 ("mm: kmemleak:
> add rbtree and store physical address for objects allocated
> with PA")) of series "[PATCH v4 0/4] mm: kmemleak: store objects
> allocated with physical address separately and check when scan"
> (https://lore.kernel.org/all/[email protected]),
> in an arm64 config that had enabled kfence.
Yes, I think it fixes 0c24e061196c21d5 since after that commit, the
kmemleak_free() no longer worked as expected on physically allocated
objects.
--
Catalin
On Mon, 18 Jul 2022 16:26:25 +0200 Marco Elver <[email protected]> wrote:
> On Sat, 16 Jul 2022 at 20:43, Geert Uytterhoeven <[email protected]> wrote:
> [...]
> > > - This patch has been accused of crashing the kernel:
> > >
> > > https://lkml.kernel.org/r/YsFeUHkrFTQ7T51Q@xsang-OptiPlex-9020
> > >
> > > Do we think that report is bogus?
> >
> > I think all of this is highly architecture-specific...
>
> The report can be reproduced on i386 with CONFIG_X86_PAE=y. But e.g.
> mm/memblock.c:memblock_free() is also guilty of using __pa() on
> previously memblock_alloc()'d addresses. Looking at the phys addr
> before memblock_alloc() does virt_to_phys(), the result of __pa()
> looks correct even on PAE, at least for the purpose of passing it on
> to kmemleak(). So I don't know what that BUG_ON(slow_virt_to_phys() !=
> phys_addr) is supposed to tell us here.
>
It's only been nine years, so I'm sure Dave can remember why he added
it ;)
BUG_ON(slow_virt_to_phys((void *)x) != phys_addr);
in arch/x86/mm/physaddr.c:__phys_addr().
This kfence patch does seem to be desirable, but we can't proceed if
it's resulting in kernel crashes.