| Message ID | 20210927021538.155991-1-wangkefeng.wang@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [resend] slub: Add back check for free nonslab objects | expand |
On Mon, Sep 27, 2021 at 10:15:38AM +0800, Kefeng Wang wrote: > Commit ("0937502af7c9 slub: Add check for kfree() of non slab objects.") > add the ability, which should be needed in any configs to catch the > invalid free, they even could be potential issue, eg, memory corruption, > use after free and double-free, so replace VM_BUG_ON_PAGE to WARN_ON, and > add dump_page() to help use to debug the issue. Is dump_page() really the best way to catch such a thing? I would have thought that printing the address of 'object' would be more helpful. > @@ -3522,7 +3522,8 @@ static inline void free_nonslab_page(struct page *page, void *object) > { > unsigned int order = compound_order(page); > > - VM_BUG_ON_PAGE(!PageCompound(page), page); > + if (WARN_ON(!PageCompound(page))) > + dump_page(page, "invalid free nonslab page");
On 2021/9/27 10:42, Matthew Wilcox wrote: > On Mon, Sep 27, 2021 at 10:15:38AM +0800, Kefeng Wang wrote: >> Commit ("0937502af7c9 slub: Add check for kfree() of non slab objects.") >> add the ability, which should be needed in any configs to catch the >> invalid free, they even could be potential issue, eg, memory corruption, >> use after free and double-free, so replace VM_BUG_ON_PAGE to WARN_ON, and >> add dump_page() to help use to debug the issue. > Is dump_page() really the best way to catch such a thing? I would have > thought that printing the address of 'object' would be more helpful. With no vmcore, dump_page() do help us to find a memory corruption issue. We could add 'object ' address print too. > >> @@ -3522,7 +3522,8 @@ static inline void free_nonslab_page(struct page *page, void *object) >> { >> unsigned int order = compound_order(page); >> >> - VM_BUG_ON_PAGE(!PageCompound(page), page); >> + if (WARN_ON(!PageCompound(page))) >> + dump_page(page, "invalid free nonslab page"); > . >
On 9/27/21 04:15, Kefeng Wang wrote: > After commit ("f227f0faf63b slub: fix unreclaimable slab stat for bulk > free"), the check for free nonslab page is replaced by VM_BUG_ON_PAGE, > which only check with CONFIG_DEBUG_VM enabled, but this config may > impact performance, so it only for debug. > > Commit ("0937502af7c9 slub: Add check for kfree() of non slab objects.") > add the ability, which should be needed in any configs to catch the > invalid free, they even could be potential issue, eg, memory corruption, > use after free and double-free, so replace VM_BUG_ON_PAGE to WARN_ON, and > add dump_page() to help use to debug the issue. There are other situations in SLUB (such as with smaller allocations that don't go directly to page allocator) where use after free and double-free are undetected in non-debug configs, and it's expected that anyone debugging them will enable slub_debug or even DEBUG_VM. Why should this special case with nonslab pages be different? > Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> > --- > mm/slub.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/mm/slub.c b/mm/slub.c > index 3095b889fab4..555c2e6ccfca 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -3522,7 +3522,8 @@ static inline void free_nonslab_page(struct page *page, void *object) > { > unsigned int order = compound_order(page); > > - VM_BUG_ON_PAGE(!PageCompound(page), page); > + if (WARN_ON(!PageCompound(page))) > + dump_page(page, "invalid free nonslab page"); > kfree_hook(object); > mod_lruvec_page_state(page, NR_SLAB_UNRECLAIMABLE_B, -(PAGE_SIZE << order)); > __free_pages(page, order); >
On 2021/9/27 15:22, Vlastimil Babka wrote: > On 9/27/21 04:15, Kefeng Wang wrote: >> After commit ("f227f0faf63b slub: fix unreclaimable slab stat for bulk >> free"), the check for free nonslab page is replaced by VM_BUG_ON_PAGE, >> which only check with CONFIG_DEBUG_VM enabled, but this config may >> impact performance, so it only for debug. >> >> Commit ("0937502af7c9 slub: Add check for kfree() of non slab objects.") >> add the ability, which should be needed in any configs to catch the >> invalid free, they even could be potential issue, eg, memory corruption, >> use after free and double-free, so replace VM_BUG_ON_PAGE to WARN_ON, and >> add dump_page() to help use to debug the issue. > There are other situations in SLUB (such as with smaller allocations that > don't go directly to page allocator) where use after free and double-free > are undetected in non-debug configs, and it's expected that anyone debugging > them will enable slub_debug or even DEBUG_VM. Why should this special case > with nonslab pages be different? I want the check back in kfree, this one is used widely in driver, and the probability of problem occurred is bigger in driver, especially in some out of tree drivers. > >> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> >> --- >> mm/slub.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/mm/slub.c b/mm/slub.c >> index 3095b889fab4..555c2e6ccfca 100644 >> --- a/mm/slub.c >> +++ b/mm/slub.c >> @@ -3522,7 +3522,8 @@ static inline void free_nonslab_page(struct page *page, void *object) >> { >> unsigned int order = compound_order(page); >> >> - VM_BUG_ON_PAGE(!PageCompound(page), page); >> + if (WARN_ON(!PageCompound(page))) >> + dump_page(page, "invalid free nonslab page"); >> kfree_hook(object); >> mod_lruvec_page_state(page, NR_SLAB_UNRECLAIMABLE_B, -(PAGE_SIZE << order)); >> __free_pages(page, order); >> > . >
On Mon, Sep 27, 2021 at 03:53:47PM +0800, Kefeng Wang wrote: > On 2021/9/27 15:22, Vlastimil Babka wrote: > > On 9/27/21 04:15, Kefeng Wang wrote: > > > After commit ("f227f0faf63b slub: fix unreclaimable slab stat for bulk > > > free"), the check for free nonslab page is replaced by VM_BUG_ON_PAGE, > > > which only check with CONFIG_DEBUG_VM enabled, but this config may > > > impact performance, so it only for debug. > > > > > > Commit ("0937502af7c9 slub: Add check for kfree() of non slab objects.") > > > add the ability, which should be needed in any configs to catch the > > > invalid free, they even could be potential issue, eg, memory corruption, > > > use after free and double-free, so replace VM_BUG_ON_PAGE to WARN_ON, and > > > add dump_page() to help use to debug the issue. > > There are other situations in SLUB (such as with smaller allocations that > > don't go directly to page allocator) where use after free and double-free > > are undetected in non-debug configs, and it's expected that anyone debugging > > them will enable slub_debug or even DEBUG_VM. Why should this special case > > with nonslab pages be different? > > I want the check back in kfree, this one is used widely in driver, and the > probability > > of problem occurred is bigger in driver, especially in some out of tree > drivers. Why would we want to improve life for out of tree drivers? Drivers should be in-tree. That's been the Linux Way for thirty years. I remain sceptical that dump_page() is actually useful for debugging drivers anyway. dump_stack(), I could see -- that'll tell you which driver called kfree() on a bogus pointer. But how does dump_page() help?
On 2021/9/28 23:43, Matthew Wilcox wrote: > On Mon, Sep 27, 2021 at 03:53:47PM +0800, Kefeng Wang wrote: >> On 2021/9/27 15:22, Vlastimil Babka wrote: >>> On 9/27/21 04:15, Kefeng Wang wrote: >>>> After commit ("f227f0faf63b slub: fix unreclaimable slab stat for bulk >>>> free"), the check for free nonslab page is replaced by VM_BUG_ON_PAGE, >>>> which only check with CONFIG_DEBUG_VM enabled, but this config may >>>> impact performance, so it only for debug. >>>> >>>> Commit ("0937502af7c9 slub: Add check for kfree() of non slab objects.") >>>> add the ability, which should be needed in any configs to catch the >>>> invalid free, they even could be potential issue, eg, memory corruption, >>>> use after free and double-free, so replace VM_BUG_ON_PAGE to WARN_ON, and >>>> add dump_page() to help use to debug the issue. >>> There are other situations in SLUB (such as with smaller allocations that >>> don't go directly to page allocator) where use after free and double-free >>> are undetected in non-debug configs, and it's expected that anyone debugging >>> them will enable slub_debug or even DEBUG_VM. Why should this special case >>> with nonslab pages be different? >> >> I want the check back in kfree, this one is used widely in driver, and the >> probability >> >> of problem occurred is bigger in driver, especially in some out of tree >> drivers. > > Why would we want to improve life for out of tree drivers? Drivers should > be in-tree. That's been the Linux Way for thirty years. > > I remain sceptical that dump_page() is actually useful for debugging > drivers anyway. dump_stack(), I could see -- that'll tell you which > driver called kfree() on a bogus pointer. But how does dump_page() help? Add dump_page here because it could help us debug for memory corruption, if we think it is redundant, I could drop it in the next version. > . >
On 9/28/21 17:43, Matthew Wilcox wrote: > On Mon, Sep 27, 2021 at 03:53:47PM +0800, Kefeng Wang wrote: >> On 2021/9/27 15:22, Vlastimil Babka wrote: >> > On 9/27/21 04:15, Kefeng Wang wrote: >> > > After commit ("f227f0faf63b slub: fix unreclaimable slab stat for bulk >> > > free"), the check for free nonslab page is replaced by VM_BUG_ON_PAGE, >> > > which only check with CONFIG_DEBUG_VM enabled, but this config may >> > > impact performance, so it only for debug. >> > > >> > > Commit ("0937502af7c9 slub: Add check for kfree() of non slab objects.") >> > > add the ability, which should be needed in any configs to catch the >> > > invalid free, they even could be potential issue, eg, memory corruption, >> > > use after free and double-free, so replace VM_BUG_ON_PAGE to WARN_ON, and >> > > add dump_page() to help use to debug the issue. >> > There are other situations in SLUB (such as with smaller allocations that >> > don't go directly to page allocator) where use after free and double-free >> > are undetected in non-debug configs, and it's expected that anyone debugging >> > them will enable slub_debug or even DEBUG_VM. Why should this special case >> > with nonslab pages be different? >> >> I want the check back in kfree, this one is used widely in driver, and the >> probability >> >> of problem occurred is bigger in driver, especially in some out of tree >> drivers. > > Why would we want to improve life for out of tree drivers? Drivers should > be in-tree. That's been the Linux Way for thirty years. Yes, there's a reason we distinguish VM_BUG_ON/VM_WARN_ON and plain BUG_ON/WARN_ON. Picking arbitrarily one VM_ variant check and making it always-enabled makes little sense to me, and doing it because of out of tree drivers is certainly not a convincing argument. Commit f227f0faf63b was correct in making it VM_BUG_ON. > I remain sceptical that dump_page() is actually useful for debugging > drivers anyway. dump_stack(), I could see -- that'll tell you which > driver called kfree() on a bogus pointer. But how does dump_page() help? >
diff --git a/mm/slub.c b/mm/slub.c index 3095b889fab4..555c2e6ccfca 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -3522,7 +3522,8 @@ static inline void free_nonslab_page(struct page *page, void *object) { unsigned int order = compound_order(page); - VM_BUG_ON_PAGE(!PageCompound(page), page); + if (WARN_ON(!PageCompound(page))) + dump_page(page, "invalid free nonslab page"); kfree_hook(object); mod_lruvec_page_state(page, NR_SLAB_UNRECLAIMABLE_B, -(PAGE_SIZE << order)); __free_pages(page, order);
After commit ("f227f0faf63b slub: fix unreclaimable slab stat for bulk free"), the check for free nonslab page is replaced by VM_BUG_ON_PAGE, which only check with CONFIG_DEBUG_VM enabled, but this config may impact performance, so it only for debug. Commit ("0937502af7c9 slub: Add check for kfree() of non slab objects.") add the ability, which should be needed in any configs to catch the invalid free, they even could be potential issue, eg, memory corruption, use after free and double-free, so replace VM_BUG_ON_PAGE to WARN_ON, and add dump_page() to help use to debug the issue. Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> --- mm/slub.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)