From: Zqiang <[email protected]>
When a CPU offline, the per-cpu quarantine's offline be set true,
after this, if the quarantine_put be called in this CPU, the objects
will be free and return false, free objects doesn't to be done, due
to return false, the slab memory manager will free this objects.
Fixes: 41ab1aae781f ("kasan: fix object remaining in offline per-cpu quarantine")
Signed-off-by: Zqiang <[email protected]>
---
mm/kasan/quarantine.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
index d98b516f372f..55783125a767 100644
--- a/mm/kasan/quarantine.c
+++ b/mm/kasan/quarantine.c
@@ -194,7 +194,6 @@ bool quarantine_put(struct kmem_cache *cache, void *object)
q = this_cpu_ptr(&cpu_quarantine);
if (q->offline) {
- qlink_free(&meta->quarantine_link, cache);
local_irq_restore(flags);
return false;
}
--
2.17.1
On Fri, 2020-12-04 at 18:22 +0800, [email protected] wrote:
> From: Zqiang <[email protected]>
>
> When a CPU offline, the per-cpu quarantine's offline be set true,
> after this, if the quarantine_put be called in this CPU, the objects
> will be free and return false, free objects doesn't to be done, due
> to return false, the slab memory manager will free this objects.
>
> Fixes: 41ab1aae781f ("kasan: fix object remaining in offline per-cpu quarantine")
> Signed-off-by: Zqiang <[email protected]>
> ---
> mm/kasan/quarantine.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
> index d98b516f372f..55783125a767 100644
> --- a/mm/kasan/quarantine.c
> +++ b/mm/kasan/quarantine.c
> @@ -194,7 +194,6 @@ bool quarantine_put(struct kmem_cache *cache, void *object)
>
> q = this_cpu_ptr(&cpu_quarantine);
> if (q->offline) {
> - qlink_free(&meta->quarantine_link, cache);
> local_irq_restore(flags);
> return false;
> }
Hi Qiang,
Thanks for fixing this.
Due to that issue, my commit has been removed by Stephen from
linux-next.
Hi Stephen, Andrew,
Should I directly upload the v4 or Stephen can pick the commit which
has been removed back to the linux-next.
What do you think?
Thanks a lot.
Kuan-Ying
On Fri, 4 Dec 2020 20:01:35 +0800 Kuan-Ying Lee <[email protected]> wrote:
> > diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
> > index d98b516f372f..55783125a767 100644
> > --- a/mm/kasan/quarantine.c
> > +++ b/mm/kasan/quarantine.c
> > @@ -194,7 +194,6 @@ bool quarantine_put(struct kmem_cache *cache, void *object)
> >
> > q = this_cpu_ptr(&cpu_quarantine);
> > if (q->offline) {
> > - qlink_free(&meta->quarantine_link, cache);
> > local_irq_restore(flags);
> > return false;
> > }
>
> Hi Qiang,
>
> Thanks for fixing this.
> Due to that issue, my commit has been removed by Stephen from
> linux-next.
>
>
> Hi Stephen, Andrew,
>
> Should I directly upload the v4 or Stephen can pick the commit which
> has been removed back to the linux-next.
I took care of it. Restored the original patch and added this one as a
-fix.
On Fri, 2020-12-04 at 17:25 -0800, Andrew Morton wrote:
> On Fri, 4 Dec 2020 20:01:35 +0800 Kuan-Ying Lee <[email protected]> wrote:
>
> > > diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
> > > index d98b516f372f..55783125a767 100644
> > > --- a/mm/kasan/quarantine.c
> > > +++ b/mm/kasan/quarantine.c
> > > @@ -194,7 +194,6 @@ bool quarantine_put(struct kmem_cache *cache, void *object)
> > >
> > > q = this_cpu_ptr(&cpu_quarantine);
> > > if (q->offline) {
> > > - qlink_free(&meta->quarantine_link, cache);
> > > local_irq_restore(flags);
> > > return false;
Hi Andrew,
Return false will cause slab allocator to free the object.
Thus, we do not need to qlink_free here to free object twice.
The return value is introduced from Andrey's patch.
"kasan: sanitize objects when metadata doesn't fit"
> > > }
> >
> > Hi Qiang,
> >
> > Thanks for fixing this.
> > Due to that issue, my commit has been removed by Stephen from
> > linux-next.
> >
> >
> > Hi Stephen, Andrew,
> >
> > Should I directly upload the v4 or Stephen can pick the commit which
> > has been removed back to the linux-next.
>
> I took care of it. Restored the original patch and added this one as a
> -fix.
Thanks for taking care of it.
I think there are some problem in the patch you just restored.
I saw the restored patch is not based on Andrey's patch and Stephen's
fix conflict patch.
But the issue Qiang fixed need to be based on the Andrey's patch and
Stephen's fix conflict patch.
"kasan: sanitize objects when metadata doesn't fit"
"kasan-rename-get_alloc-free_info-fix"
If the restored patch is not based on that, it may cause some problems
and conflicts.
I think I can prepare a patch v4 based on Andrey's patch, fix the
conflict and include the Qiang's modification.
Thanks,
Kuan-Ying
On Sun, 6 Dec 2020 00:17:15 +0800 Kuan-Ying Lee <[email protected]> wrote:
> On Fri, 2020-12-04 at 17:25 -0800, Andrew Morton wrote:
> > On Fri, 4 Dec 2020 20:01:35 +0800 Kuan-Ying Lee <[email protected]> wrote:
> >
> > > > diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
> > > > index d98b516f372f..55783125a767 100644
> > > > --- a/mm/kasan/quarantine.c
> > > > +++ b/mm/kasan/quarantine.c
> > > > @@ -194,7 +194,6 @@ bool quarantine_put(struct kmem_cache *cache, void *object)
> > > >
> > > > q = this_cpu_ptr(&cpu_quarantine);
> > > > if (q->offline) {
> > > > - qlink_free(&meta->quarantine_link, cache);
> > > > local_irq_restore(flags);
> > > > return false;
>
> Hi Andrew,
>
> Return false will cause slab allocator to free the object.
> Thus, we do not need to qlink_free here to free object twice.
>
> The return value is introduced from Andrey's patch.
> "kasan: sanitize objects when metadata doesn't fit"
>
>
> > > > }
> > >
> > > Hi Qiang,
> > >
> > > Thanks for fixing this.
> > > Due to that issue, my commit has been removed by Stephen from
> > > linux-next.
> > >
> > >
> > > Hi Stephen, Andrew,
> > >
> > > Should I directly upload the v4 or Stephen can pick the commit which
> > > has been removed back to the linux-next.
> >
> > I took care of it. Restored the original patch and added this one as a
> > -fix.
>
> Thanks for taking care of it.
>
> I think there are some problem in the patch you just restored.
> I saw the restored patch is not based on Andrey's patch and Stephen's
> fix conflict patch.
>
> But the issue Qiang fixed need to be based on the Andrey's patch and
> Stephen's fix conflict patch.
> "kasan: sanitize objects when metadata doesn't fit"
> "kasan-rename-get_alloc-free_info-fix"
>
> If the restored patch is not based on that, it may cause some problems
> and conflicts.
>
> I think I can prepare a patch v4 based on Andrey's patch, fix the
> conflict and include the Qiang's modification.
I'm not sure what you mean here. When appying this fix, yes, I had to
replace "meta" with "info", of course.
So the combined patch, which I'd like to send to Linus next week is as
below. Is there something wrong with it?
From: Kuan-Ying Lee <[email protected]>
Subject: kasan: fix object remaining in offline per-cpu quarantine
We hit this issue in our internal test. When enabling generic kasan, a
kfree()'d object is put into per-cpu quarantine first. If the cpu goes
offline, object still remains in the per-cpu quarantine. If we call
kmem_cache_destroy() now, slub will report "Objects remaining" error.
[ 74.982625] =============================================================================
[ 74.983380] BUG test_module_slab (Not tainted): Objects remaining in test_module_slab on __kmem_cache_shutdown()
[ 74.984145] -----------------------------------------------------------------------------
[ 74.984145]
[ 74.984883] Disabling lock debugging due to kernel taint
[ 74.985561] INFO: Slab 0x(____ptrval____) objects=34 used=1 fp=0x(____ptrval____) flags=0x2ffff00000010200
[ 74.986638] CPU: 3 PID: 176 Comm: cat Tainted: G B 5.10.0-rc1-00007-g4525c8781ec0-dirty #10
[ 74.987262] Hardware name: linux,dummy-virt (DT)
[ 74.987606] Call trace:
[ 74.987924] dump_backtrace+0x0/0x2b0
[ 74.988296] show_stack+0x18/0x68
[ 74.988698] dump_stack+0xfc/0x168
[ 74.989030] slab_err+0xac/0xd4
[ 74.989346] __kmem_cache_shutdown+0x1e4/0x3c8
[ 74.989779] kmem_cache_destroy+0x68/0x130
[ 74.990176] test_version_show+0x84/0xf0
[ 74.990679] module_attr_show+0x40/0x60
[ 74.991218] sysfs_kf_seq_show+0x128/0x1c0
[ 74.991656] kernfs_seq_show+0xa0/0xb8
[ 74.992059] seq_read+0x1f0/0x7e8
[ 74.992415] kernfs_fop_read+0x70/0x338
[ 74.993051] vfs_read+0xe4/0x250
[ 74.993498] ksys_read+0xc8/0x180
[ 74.993825] __arm64_sys_read+0x44/0x58
[ 74.994203] el0_svc_common.constprop.0+0xac/0x228
[ 74.994708] do_el0_svc+0x38/0xa0
[ 74.995088] el0_sync_handler+0x170/0x178
[ 74.995497] el0_sync+0x174/0x180
[ 74.996050] INFO: Object 0x(____ptrval____) @offset=15848
[ 74.996752] INFO: Allocated in test_version_show+0x98/0xf0 age=8188 cpu=6 pid=172
[ 75.000802] stack_trace_save+0x9c/0xd0
[ 75.002420] set_track+0x64/0xf0
[ 75.002770] alloc_debug_processing+0x104/0x1a0
[ 75.003171] ___slab_alloc+0x628/0x648
[ 75.004213] __slab_alloc.isra.0+0x2c/0x58
[ 75.004757] kmem_cache_alloc+0x560/0x588
[ 75.005376] test_version_show+0x98/0xf0
[ 75.005756] module_attr_show+0x40/0x60
[ 75.007035] sysfs_kf_seq_show+0x128/0x1c0
[ 75.007433] kernfs_seq_show+0xa0/0xb8
[ 75.007800] seq_read+0x1f0/0x7e8
[ 75.008128] kernfs_fop_read+0x70/0x338
[ 75.008507] vfs_read+0xe4/0x250
[ 75.008990] ksys_read+0xc8/0x180
[ 75.009462] __arm64_sys_read+0x44/0x58
[ 75.010085] el0_svc_common.constprop.0+0xac/0x228
[ 75.011006] kmem_cache_destroy test_module_slab: Slab cache still has objects
Register a cpu hotplug function to remove all objects in the offline
per-cpu quarantine when cpu is going offline. Set a per-cpu variable to
indicate this cpu is offline.
[[email protected]: fix slab double free when cpu-hotplug]
Link: https://lkml.kernel.org/r/[email protected]
Link: https://lkml.kernel.org/r/[email protected]
Signed-off-by: Kuan-Ying Lee <[email protected]>
Signed-off-by: Zqiang <[email protected]>
Suggested-by: Dmitry Vyukov <[email protected]>
Reported-by: Guangye Yang <[email protected]>
Reviewed-by: Dmitry Vyukov <[email protected]>
Cc: Andrey Ryabinin <[email protected]>
Cc: Alexander Potapenko <[email protected]>
Cc: Matthias Brugger <[email protected]>
Cc: Nicholas Tang <[email protected]>
Cc: Miles Chen <[email protected]>
Cc: Qian Cai <[email protected]>
Cc: Stephen Rothwell <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
---
mm/kasan/quarantine.c | 39 +++++++++++++++++++++++++++++++++++++++
1 file changed, 39 insertions(+)
--- a/mm/kasan/quarantine.c~kasan-fix-object-remain-in-offline-per-cpu-quarantine
+++ a/mm/kasan/quarantine.c
@@ -29,6 +29,7 @@
#include <linux/srcu.h>
#include <linux/string.h>
#include <linux/types.h>
+#include <linux/cpuhotplug.h>
#include "../slab.h"
#include "kasan.h"
@@ -43,6 +44,7 @@ struct qlist_head {
struct qlist_node *head;
struct qlist_node *tail;
size_t bytes;
+ bool offline;
};
#define QLIST_INIT { NULL, NULL, 0 }
@@ -188,6 +190,10 @@ void quarantine_put(struct kasan_free_me
local_irq_save(flags);
q = this_cpu_ptr(&cpu_quarantine);
+ if (q->offline) {
+ local_irq_restore(flags);
+ return;
+ }
qlist_put(q, &info->quarantine_link, cache->size);
if (unlikely(q->bytes > QUARANTINE_PERCPU_SIZE)) {
qlist_move_all(q, &temp);
@@ -328,3 +334,36 @@ void quarantine_remove_cache(struct kmem
synchronize_srcu(&remove_cache_srcu);
}
+
+static int kasan_cpu_online(unsigned int cpu)
+{
+ this_cpu_ptr(&cpu_quarantine)->offline = false;
+ return 0;
+}
+
+static int kasan_cpu_offline(unsigned int cpu)
+{
+ struct qlist_head *q;
+
+ q = this_cpu_ptr(&cpu_quarantine);
+ /* Ensure the ordering between the writing to q->offline and
+ * qlist_free_all. Otherwise, cpu_quarantine may be corrupted
+ * by interrupt.
+ */
+ WRITE_ONCE(q->offline, true);
+ barrier();
+ qlist_free_all(q, NULL);
+ return 0;
+}
+
+static int __init kasan_cpu_quarantine_init(void)
+{
+ int ret = 0;
+
+ ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "mm/kasan:online",
+ kasan_cpu_online, kasan_cpu_offline);
+ if (ret < 0)
+ pr_err("kasan cpu quarantine register failed [%d]\n", ret);
+ return ret;
+}
+late_initcall(kasan_cpu_quarantine_init);
_
On Sat, 2020-12-05 at 17:09 -0800, Andrew Morton wrote:
> On Sun, 6 Dec 2020 00:17:15 +0800 Kuan-Ying Lee <[email protected]> wrote:
>
> > On Fri, 2020-12-04 at 17:25 -0800, Andrew Morton wrote:
> > > On Fri, 4 Dec 2020 20:01:35 +0800 Kuan-Ying Lee <[email protected]> wrote:
> > >
> > > > > diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
> > > > > index d98b516f372f..55783125a767 100644
> > > > > --- a/mm/kasan/quarantine.c
> > > > > +++ b/mm/kasan/quarantine.c
> > > > > @@ -194,7 +194,6 @@ bool quarantine_put(struct kmem_cache *cache, void *object)
> > > > >
> > > > > q = this_cpu_ptr(&cpu_quarantine);
> > > > > if (q->offline) {
> > > > > - qlink_free(&meta->quarantine_link, cache);
> > > > > local_irq_restore(flags);
> > > > > return false;
> >
> > Hi Andrew,
> >
> > Return false will cause slab allocator to free the object.
> > Thus, we do not need to qlink_free here to free object twice.
> >
> > The return value is introduced from Andrey's patch.
> > "kasan: sanitize objects when metadata doesn't fit"
> >
> >
> > > > > }
> > > >
> > > > Hi Qiang,
> > > >
> > > > Thanks for fixing this.
> > > > Due to that issue, my commit has been removed by Stephen from
> > > > linux-next.
> > > >
> > > >
> > > > Hi Stephen, Andrew,
> > > >
> > > > Should I directly upload the v4 or Stephen can pick the commit which
> > > > has been removed back to the linux-next.
> > >
> > > I took care of it. Restored the original patch and added this one as a
> > > -fix.
> >
> > Thanks for taking care of it.
> >
> > I think there are some problem in the patch you just restored.
> > I saw the restored patch is not based on Andrey's patch and Stephen's
> > fix conflict patch.
> >
> > But the issue Qiang fixed need to be based on the Andrey's patch and
> > Stephen's fix conflict patch.
> > "kasan: sanitize objects when metadata doesn't fit"
> > "kasan-rename-get_alloc-free_info-fix"
> >
> > If the restored patch is not based on that, it may cause some problems
> > and conflicts.
> >
> > I think I can prepare a patch v4 based on Andrey's patch, fix the
> > conflict and include the Qiang's modification.
>
> I'm not sure what you mean here. When appying this fix, yes, I had to
> replace "meta" with "info", of course.
>
> So the combined patch, which I'd like to send to Linus next week is as
> below. Is there something wrong with it?
>
Is this combined patch based on Andrey's patch?
If yes, Andrey's patch not only change the "info" to "meta" but also
introduce the return value.
I think we need to add return value or it will build error.
>
> From: Kuan-Ying Lee <[email protected]>
> Subject: kasan: fix object remaining in offline per-cpu quarantine
>
> We hit this issue in our internal test. When enabling generic kasan, a
> kfree()'d object is put into per-cpu quarantine first. If the cpu goes
> offline, object still remains in the per-cpu quarantine. If we call
> kmem_cache_destroy() now, slub will report "Objects remaining" error.
>
> [ 74.982625] =============================================================================
> [ 74.983380] BUG test_module_slab (Not tainted): Objects remaining in test_module_slab on __kmem_cache_shutdown()
> [ 74.984145] -----------------------------------------------------------------------------
> [ 74.984145]
> [ 74.984883] Disabling lock debugging due to kernel taint
> [ 74.985561] INFO: Slab 0x(____ptrval____) objects=34 used=1 fp=0x(____ptrval____) flags=0x2ffff00000010200
> [ 74.986638] CPU: 3 PID: 176 Comm: cat Tainted: G B 5.10.0-rc1-00007-g4525c8781ec0-dirty #10
> [ 74.987262] Hardware name: linux,dummy-virt (DT)
> [ 74.987606] Call trace:
> [ 74.987924] dump_backtrace+0x0/0x2b0
> [ 74.988296] show_stack+0x18/0x68
> [ 74.988698] dump_stack+0xfc/0x168
> [ 74.989030] slab_err+0xac/0xd4
> [ 74.989346] __kmem_cache_shutdown+0x1e4/0x3c8
> [ 74.989779] kmem_cache_destroy+0x68/0x130
> [ 74.990176] test_version_show+0x84/0xf0
> [ 74.990679] module_attr_show+0x40/0x60
> [ 74.991218] sysfs_kf_seq_show+0x128/0x1c0
> [ 74.991656] kernfs_seq_show+0xa0/0xb8
> [ 74.992059] seq_read+0x1f0/0x7e8
> [ 74.992415] kernfs_fop_read+0x70/0x338
> [ 74.993051] vfs_read+0xe4/0x250
> [ 74.993498] ksys_read+0xc8/0x180
> [ 74.993825] __arm64_sys_read+0x44/0x58
> [ 74.994203] el0_svc_common.constprop.0+0xac/0x228
> [ 74.994708] do_el0_svc+0x38/0xa0
> [ 74.995088] el0_sync_handler+0x170/0x178
> [ 74.995497] el0_sync+0x174/0x180
> [ 74.996050] INFO: Object 0x(____ptrval____) @offset=15848
> [ 74.996752] INFO: Allocated in test_version_show+0x98/0xf0 age=8188 cpu=6 pid=172
> [ 75.000802] stack_trace_save+0x9c/0xd0
> [ 75.002420] set_track+0x64/0xf0
> [ 75.002770] alloc_debug_processing+0x104/0x1a0
> [ 75.003171] ___slab_alloc+0x628/0x648
> [ 75.004213] __slab_alloc.isra.0+0x2c/0x58
> [ 75.004757] kmem_cache_alloc+0x560/0x588
> [ 75.005376] test_version_show+0x98/0xf0
> [ 75.005756] module_attr_show+0x40/0x60
> [ 75.007035] sysfs_kf_seq_show+0x128/0x1c0
> [ 75.007433] kernfs_seq_show+0xa0/0xb8
> [ 75.007800] seq_read+0x1f0/0x7e8
> [ 75.008128] kernfs_fop_read+0x70/0x338
> [ 75.008507] vfs_read+0xe4/0x250
> [ 75.008990] ksys_read+0xc8/0x180
> [ 75.009462] __arm64_sys_read+0x44/0x58
> [ 75.010085] el0_svc_common.constprop.0+0xac/0x228
> [ 75.011006] kmem_cache_destroy test_module_slab: Slab cache still has objects
>
> Register a cpu hotplug function to remove all objects in the offline
> per-cpu quarantine when cpu is going offline. Set a per-cpu variable to
> indicate this cpu is offline.
>
> [[email protected]: fix slab double free when cpu-hotplug]
> Link: https://lkml.kernel.org/r/[email protected]
> Link: https://lkml.kernel.org/r/[email protected]
> Signed-off-by: Kuan-Ying Lee <[email protected]>
> Signed-off-by: Zqiang <[email protected]>
> Suggested-by: Dmitry Vyukov <[email protected]>
> Reported-by: Guangye Yang <[email protected]>
> Reviewed-by: Dmitry Vyukov <[email protected]>
> Cc: Andrey Ryabinin <[email protected]>
> Cc: Alexander Potapenko <[email protected]>
> Cc: Matthias Brugger <[email protected]>
> Cc: Nicholas Tang <[email protected]>
> Cc: Miles Chen <[email protected]>
> Cc: Qian Cai <[email protected]>
> Cc: Stephen Rothwell <[email protected]>
> Signed-off-by: Andrew Morton <[email protected]>
> ---
>
> mm/kasan/quarantine.c | 39 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 39 insertions(+)
>
> --- a/mm/kasan/quarantine.c~kasan-fix-object-remain-in-offline-per-cpu-quarantine
> +++ a/mm/kasan/quarantine.c
> @@ -29,6 +29,7 @@
> #include <linux/srcu.h>
> #include <linux/string.h>
> #include <linux/types.h>
> +#include <linux/cpuhotplug.h>
>
> #include "../slab.h"
> #include "kasan.h"
> @@ -43,6 +44,7 @@ struct qlist_head {
> struct qlist_node *head;
> struct qlist_node *tail;
> size_t bytes;
> + bool offline;
> };
>
> #define QLIST_INIT { NULL, NULL, 0 }
> @@ -188,6 +190,10 @@ void quarantine_put(struct kasan_free_me
Andrey's patch changes the return value from "void" to "bool".
We need to replace void with bool.
> local_irq_save(flags);
>
> q = this_cpu_ptr(&cpu_quarantine);
> + if (q->offline) {
> + local_irq_restore(flags);
> + return;
I think we need to return false here. Otherwise, it will lack return
value and cause build error.
> + }
> qlist_put(q, &info->quarantine_link, cache->size);
This "info" may cause conflict because Andrey's patch has already
changed it to "meta".
Thanks.
Kuan-Ying
> if (unlikely(q->bytes > QUARANTINE_PERCPU_SIZE)) {
> qlist_move_all(q, &temp);
> @@ -328,3 +334,36 @@ void quarantine_remove_cache(struct kmem
>
> synchronize_srcu(&remove_cache_srcu);
> }
> +
> +static int kasan_cpu_online(unsigned int cpu)
> +{
> + this_cpu_ptr(&cpu_quarantine)->offline = false;
> + return 0;
> +}
> +
> +static int kasan_cpu_offline(unsigned int cpu)
> +{
> + struct qlist_head *q;
> +
> + q = this_cpu_ptr(&cpu_quarantine);
> + /* Ensure the ordering between the writing to q->offline and
> + * qlist_free_all. Otherwise, cpu_quarantine may be corrupted
> + * by interrupt.
> + */
> + WRITE_ONCE(q->offline, true);
> + barrier();
> + qlist_free_all(q, NULL);
> + return 0;
> +}
> +
> +static int __init kasan_cpu_quarantine_init(void)
> +{
> + int ret = 0;
> +
> + ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "mm/kasan:online",
> + kasan_cpu_online, kasan_cpu_offline);
> + if (ret < 0)
> + pr_err("kasan cpu quarantine register failed [%d]\n", ret);
> + return ret;
> +}
> +late_initcall(kasan_cpu_quarantine_init);
> _
>
On Mon, 2020-12-07 at 10:06 +0800, Kuan-Ying Lee wrote:
> On Sat, 2020-12-05 at 17:09 -0800, Andrew Morton wrote:
> > On Sun, 6 Dec 2020 00:17:15 +0800 Kuan-Ying Lee <[email protected]> wrote:
> >
> > > On Fri, 2020-12-04 at 17:25 -0800, Andrew Morton wrote:
> > > > On Fri, 4 Dec 2020 20:01:35 +0800 Kuan-Ying Lee <[email protected]> wrote:
> > > >
> > > > > > diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
> > > > > > index d98b516f372f..55783125a767 100644
> > > > > > --- a/mm/kasan/quarantine.c
> > > > > > +++ b/mm/kasan/quarantine.c
> > > > > > @@ -194,7 +194,6 @@ bool quarantine_put(struct kmem_cache *cache, void *object)
> > > > > >
> > > > > > q = this_cpu_ptr(&cpu_quarantine);
> > > > > > if (q->offline) {
> > > > > > - qlink_free(&meta->quarantine_link, cache);
> > > > > > local_irq_restore(flags);
> > > > > > return false;
> > >
> > > Hi Andrew,
> > >
> > > Return false will cause slab allocator to free the object.
> > > Thus, we do not need to qlink_free here to free object twice.
> > >
> > > The return value is introduced from Andrey's patch.
> > > "kasan: sanitize objects when metadata doesn't fit"
> > >
> > >
> > > > > > }
> > > > >
> > > > > Hi Qiang,
> > > > >
> > > > > Thanks for fixing this.
> > > > > Due to that issue, my commit has been removed by Stephen from
> > > > > linux-next.
> > > > >
> > > > >
> > > > > Hi Stephen, Andrew,
> > > > >
> > > > > Should I directly upload the v4 or Stephen can pick the commit which
> > > > > has been removed back to the linux-next.
> > > >
> > > > I took care of it. Restored the original patch and added this one as a
> > > > -fix.
> > >
> > > Thanks for taking care of it.
> > >
> > > I think there are some problem in the patch you just restored.
> > > I saw the restored patch is not based on Andrey's patch and Stephen's
> > > fix conflict patch.
> > >
> > > But the issue Qiang fixed need to be based on the Andrey's patch and
> > > Stephen's fix conflict patch.
> > > "kasan: sanitize objects when metadata doesn't fit"
> > > "kasan-rename-get_alloc-free_info-fix"
> > >
> > > If the restored patch is not based on that, it may cause some problems
> > > and conflicts.
> > >
> > > I think I can prepare a patch v4 based on Andrey's patch, fix the
> > > conflict and include the Qiang's modification.
> >
> > I'm not sure what you mean here. When appying this fix, yes, I had to
> > replace "meta" with "info", of course.
> >
> > So the combined patch, which I'd like to send to Linus next week is as
> > below. Is there something wrong with it?
> >
>
> Is this combined patch based on Andrey's patch?
>
> If yes, Andrey's patch not only change the "info" to "meta" but also
> introduce the return value.
> I think we need to add return value or it will build error.
>
Hi Andrew,
Sorry to bother.
Thanks for fixing my patch.
But it still has some problem.
I know you are busy. I can fix that by myself.
I will upload the patch v4 to fix the conflicts and build errors and
include the Qiang's patch.
Thanks.
Kuan-Ying
> >
> > From: Kuan-Ying Lee <[email protected]>
> > Subject: kasan: fix object remaining in offline per-cpu quarantine
> >
> > We hit this issue in our internal test. When enabling generic kasan, a
> > kfree()'d object is put into per-cpu quarantine first. If the cpu goes
> > offline, object still remains in the per-cpu quarantine. If we call
> > kmem_cache_destroy() now, slub will report "Objects remaining" error.
> >
> > [ 74.982625] =============================================================================
> > [ 74.983380] BUG test_module_slab (Not tainted): Objects remaining in test_module_slab on __kmem_cache_shutdown()
> > [ 74.984145] -----------------------------------------------------------------------------
> > [ 74.984145]
> > [ 74.984883] Disabling lock debugging due to kernel taint
> > [ 74.985561] INFO: Slab 0x(____ptrval____) objects=34 used=1 fp=0x(____ptrval____) flags=0x2ffff00000010200
> > [ 74.986638] CPU: 3 PID: 176 Comm: cat Tainted: G B 5.10.0-rc1-00007-g4525c8781ec0-dirty #10
> > [ 74.987262] Hardware name: linux,dummy-virt (DT)
> > [ 74.987606] Call trace:
> > [ 74.987924] dump_backtrace+0x0/0x2b0
> > [ 74.988296] show_stack+0x18/0x68
> > [ 74.988698] dump_stack+0xfc/0x168
> > [ 74.989030] slab_err+0xac/0xd4
> > [ 74.989346] __kmem_cache_shutdown+0x1e4/0x3c8
> > [ 74.989779] kmem_cache_destroy+0x68/0x130
> > [ 74.990176] test_version_show+0x84/0xf0
> > [ 74.990679] module_attr_show+0x40/0x60
> > [ 74.991218] sysfs_kf_seq_show+0x128/0x1c0
> > [ 74.991656] kernfs_seq_show+0xa0/0xb8
> > [ 74.992059] seq_read+0x1f0/0x7e8
> > [ 74.992415] kernfs_fop_read+0x70/0x338
> > [ 74.993051] vfs_read+0xe4/0x250
> > [ 74.993498] ksys_read+0xc8/0x180
> > [ 74.993825] __arm64_sys_read+0x44/0x58
> > [ 74.994203] el0_svc_common.constprop.0+0xac/0x228
> > [ 74.994708] do_el0_svc+0x38/0xa0
> > [ 74.995088] el0_sync_handler+0x170/0x178
> > [ 74.995497] el0_sync+0x174/0x180
> > [ 74.996050] INFO: Object 0x(____ptrval____) @offset=15848
> > [ 74.996752] INFO: Allocated in test_version_show+0x98/0xf0 age=8188 cpu=6 pid=172
> > [ 75.000802] stack_trace_save+0x9c/0xd0
> > [ 75.002420] set_track+0x64/0xf0
> > [ 75.002770] alloc_debug_processing+0x104/0x1a0
> > [ 75.003171] ___slab_alloc+0x628/0x648
> > [ 75.004213] __slab_alloc.isra.0+0x2c/0x58
> > [ 75.004757] kmem_cache_alloc+0x560/0x588
> > [ 75.005376] test_version_show+0x98/0xf0
> > [ 75.005756] module_attr_show+0x40/0x60
> > [ 75.007035] sysfs_kf_seq_show+0x128/0x1c0
> > [ 75.007433] kernfs_seq_show+0xa0/0xb8
> > [ 75.007800] seq_read+0x1f0/0x7e8
> > [ 75.008128] kernfs_fop_read+0x70/0x338
> > [ 75.008507] vfs_read+0xe4/0x250
> > [ 75.008990] ksys_read+0xc8/0x180
> > [ 75.009462] __arm64_sys_read+0x44/0x58
> > [ 75.010085] el0_svc_common.constprop.0+0xac/0x228
> > [ 75.011006] kmem_cache_destroy test_module_slab: Slab cache still has objects
> >
> > Register a cpu hotplug function to remove all objects in the offline
> > per-cpu quarantine when cpu is going offline. Set a per-cpu variable to
> > indicate this cpu is offline.
> >
> > [[email protected]: fix slab double free when cpu-hotplug]
> > Link: https://lkml.kernel.org/r/[email protected]
> > Link: https://lkml.kernel.org/r/[email protected]
> > Signed-off-by: Kuan-Ying Lee <[email protected]>
> > Signed-off-by: Zqiang <[email protected]>
> > Suggested-by: Dmitry Vyukov <[email protected]>
> > Reported-by: Guangye Yang <[email protected]>
> > Reviewed-by: Dmitry Vyukov <[email protected]>
> > Cc: Andrey Ryabinin <[email protected]>
> > Cc: Alexander Potapenko <[email protected]>
> > Cc: Matthias Brugger <[email protected]>
> > Cc: Nicholas Tang <[email protected]>
> > Cc: Miles Chen <[email protected]>
> > Cc: Qian Cai <[email protected]>
> > Cc: Stephen Rothwell <[email protected]>
> > Signed-off-by: Andrew Morton <[email protected]>
> > ---
> >
> > mm/kasan/quarantine.c | 39 +++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 39 insertions(+)
> >
> > --- a/mm/kasan/quarantine.c~kasan-fix-object-remain-in-offline-per-cpu-quarantine
> > +++ a/mm/kasan/quarantine.c
> > @@ -29,6 +29,7 @@
> > #include <linux/srcu.h>
> > #include <linux/string.h>
> > #include <linux/types.h>
> > +#include <linux/cpuhotplug.h>
> >
> > #include "../slab.h"
> > #include "kasan.h"
> > @@ -43,6 +44,7 @@ struct qlist_head {
> > struct qlist_node *head;
> > struct qlist_node *tail;
> > size_t bytes;
> > + bool offline;
> > };
> >
> > #define QLIST_INIT { NULL, NULL, 0 }
> > @@ -188,6 +190,10 @@ void quarantine_put(struct kasan_free_me
>
> Andrey's patch changes the return value from "void" to "bool".
> We need to replace void with bool.
>
> > local_irq_save(flags);
> >
> > q = this_cpu_ptr(&cpu_quarantine);
> > + if (q->offline) {
> > + local_irq_restore(flags);
> > + return;
>
> I think we need to return false here. Otherwise, it will lack return
> value and cause build error.
>
> > + }
> > qlist_put(q, &info->quarantine_link, cache->size);
>
> This "info" may cause conflict because Andrey's patch has already
> changed it to "meta".
>
> Thanks.
> Kuan-Ying
>
> > if (unlikely(q->bytes > QUARANTINE_PERCPU_SIZE)) {
> > qlist_move_all(q, &temp);
> > @@ -328,3 +334,36 @@ void quarantine_remove_cache(struct kmem
> >
> > synchronize_srcu(&remove_cache_srcu);
> > }
> > +
> > +static int kasan_cpu_online(unsigned int cpu)
> > +{
> > + this_cpu_ptr(&cpu_quarantine)->offline = false;
> > + return 0;
> > +}
> > +
> > +static int kasan_cpu_offline(unsigned int cpu)
> > +{
> > + struct qlist_head *q;
> > +
> > + q = this_cpu_ptr(&cpu_quarantine);
> > + /* Ensure the ordering between the writing to q->offline and
> > + * qlist_free_all. Otherwise, cpu_quarantine may be corrupted
> > + * by interrupt.
> > + */
> > + WRITE_ONCE(q->offline, true);
> > + barrier();
> > + qlist_free_all(q, NULL);
> > + return 0;
> > +}
> > +
> > +static int __init kasan_cpu_quarantine_init(void)
> > +{
> > + int ret = 0;
> > +
> > + ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "mm/kasan:online",
> > + kasan_cpu_online, kasan_cpu_offline);
> > + if (ret < 0)
> > + pr_err("kasan cpu quarantine register failed [%d]\n", ret);
> > + return ret;
> > +}
> > +late_initcall(kasan_cpu_quarantine_init);
> > _
> >
>
Hi folks,
Andrew Morton writes:
>@@ -188,6 +190,10 @@ void quarantine_put(struct kasan_free_me
> local_irq_save(flags);
>
> q = this_cpu_ptr(&cpu_quarantine);
>+ if (q->offline) {
>+ local_irq_restore(flags);
>+ return;
>+ }
> qlist_put(q, &info->quarantine_link, cache->size);
> if (unlikely(q->bytes > QUARANTINE_PERCPU_SIZE)) {
> qlist_move_all(q, &temp);
I'm afraid as well as the issues already identified, this also fails, because
`quarantine_put` now returns a bool after "kasan: sanitize objects when
metadata doesn't fit":
mm/kasan/quarantine.c: In function ‘quarantine_put’:
mm/kasan/quarantine.c:198:3: error: ‘return’ with no value, in function returning non-void [-Werror=return-type]
198 | return;
| ^~~~~~
mm/kasan/quarantine.c:171:6: note: declared here
171 | bool quarantine_put(struct kmem_cache *cache, void *object)
On Fri, 11 Dec 2020 13:43:39 +0000 Chris Down <[email protected]> wrote:
> Hi folks,
>
> Andrew Morton writes:
> >@@ -188,6 +190,10 @@ void quarantine_put(struct kasan_free_me
> > local_irq_save(flags);
> >
> > q = this_cpu_ptr(&cpu_quarantine);
> >+ if (q->offline) {
> >+ local_irq_restore(flags);
> >+ return;
> >+ }
> > qlist_put(q, &info->quarantine_link, cache->size);
> > if (unlikely(q->bytes > QUARANTINE_PERCPU_SIZE)) {
> > qlist_move_all(q, &temp);
>
> I'm afraid as well as the issues already identified, this also fails, because
> `quarantine_put` now returns a bool after "kasan: sanitize objects when
> metadata doesn't fit":
>
> mm/kasan/quarantine.c: In function ‘quarantine_put’:
> mm/kasan/quarantine.c:198:3: error: ‘return’ with no value, in function returning non-void [-Werror=return-type]
> 198 | return;
> | ^~~~~~
> mm/kasan/quarantine.c:171:6: note: declared here
> 171 | bool quarantine_put(struct kmem_cache *cache, void *object)
Yup, thanks. I think I have this all fixed now, using the old
apply-one-patch-compile-everything-repeat approach.