Re: linux-next: build failure after merge of the akpm-current tree
On Mon, 2020-12-21 at 13:55 +1100, Stephen Rothwell wrote:
> Hi Kuan-Ying,
>
> On Mon, 21 Dec 2020 10:31:38 +0800 Kuan-Ying Lee
> wrote:
> >
> > On Mon, 2020-12-21 at 13:10 +1100, Stephen Rothwell wrote:
> > >
> > > After merging the akpm-current tree, today's linux-next build (x86_64
> > > allmodconfig) failed like this:
> > >
> > > mm/kasan/quarantine.c: In function 'quarantine_put':
> > > mm/kasan/quarantine.c:207:15: error: 'info' undeclared (first use in this
> > > function)
> > > 207 | qlink_free(>quarantine_link, cache);
> > > | ^~~~
> > >
> > > Caused by commit
> > >
> > > 120d593a8650 ("kasan: fix memory leak of kasan quarantine")
> > >
> > > interacting with commit
> > >
> > > cfbc92088e1d ("kasan: rename get_alloc/free_info")
> > >
> > > Can we please get this sorted out once and for all?
> > >
> > > I have applied the following patch for today:
> > >
> > > From: Stephen Rothwell
> > > Date: Mon, 21 Dec 2020 13:07:42 +1100
> > > Subject: [PATCH] kasan: fix memory leak of kasan quarantine fix
> > >
> > > Signed-off-by: Stephen Rothwell
> > > ---
> > > mm/kasan/quarantine.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
> > > index 3f3b3d902c18..091a57f942b3 100644
> > > --- a/mm/kasan/quarantine.c
> > > +++ b/mm/kasan/quarantine.c
> > > @@ -204,7 +204,7 @@ bool quarantine_put(struct kmem_cache *cache, void
> > > *object)
> > >
> > > q = this_cpu_ptr(_quarantine);
> > > if (q->offline) {
> > > - qlink_free(>quarantine_link, cache);
> > > + qlink_free(>quarantine_link, cache); // free once
> > > local_irq_restore(flags);
> > > return false; // free twice
> > > }
> > > --
> > > 2.29.2
> > >
> >
> > Please just drop this patch "kasan: fix memory leak of kasan quarantine"
> > from linux-next. Otherwise, it would cause double free issue.
>
> OK, so for today I have reverted my fix patch and 120d593a8650 ("kasan:
> fix memory leak of kasan quarantine").
>
Dear Andrew,
I am sorry. I didn't mean to.
This patch has build error.
https://www.ozlabs.org/~akpm/mmotm/broken-out/kasan-fix-memory-leak-of-kasan-quarantine.patch
Sorry to make the build errors and merge issues repeatedly.
My fix has dependency issues with Andrey's patches [1, 2], and I think
it's better to merge Andrey's patches first and I will push a fix after
Andrey's patch.
Please just drop this patch from akpm-tree directly.
[1]https://www.ozlabs.org/~akpm/mmotm/broken-out/kasan-sanitize-objects-when-metadata-doesnt-fit.patch
[2]https://www.ozlabs.org/~akpm/mmotm/broken-out/kasan-rename-get_alloc-free_info.patch
Thanks.
Re: linux-next: build failure after merge of the akpm-current tree
On Mon, 2020-12-21 at 13:10 +1100, Stephen Rothwell wrote:
> Hi all,
>
> After merging the akpm-current tree, today's linux-next build (x86_64
> allmodconfig) failed like this:
>
> mm/kasan/quarantine.c: In function 'quarantine_put':
> mm/kasan/quarantine.c:207:15: error: 'info' undeclared (first use in this
> function)
> 207 | qlink_free(>quarantine_link, cache);
> | ^~~~
>
> Caused by commit
>
> 120d593a8650 ("kasan: fix memory leak of kasan quarantine")
>
> interacting with commit
>
> cfbc92088e1d ("kasan: rename get_alloc/free_info")
>
> Can we please get this sorted out once and for all?
>
> I have applied the following patch for today:
>
> From: Stephen Rothwell
> Date: Mon, 21 Dec 2020 13:07:42 +1100
> Subject: [PATCH] kasan: fix memory leak of kasan quarantine fix
>
> Signed-off-by: Stephen Rothwell
> ---
> mm/kasan/quarantine.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
> index 3f3b3d902c18..091a57f942b3 100644
> --- a/mm/kasan/quarantine.c
> +++ b/mm/kasan/quarantine.c
> @@ -204,7 +204,7 @@ bool quarantine_put(struct kmem_cache *cache, void
> *object)
>
> q = this_cpu_ptr(_quarantine);
> if (q->offline) {
> - qlink_free(>quarantine_link, cache);
> + qlink_free(>quarantine_link, cache); // free once
> local_irq_restore(flags);
> return false; // free twice
> }
> --
> 2.29.2
>
Hi Stephen,
Please just drop this patch "kasan: fix memory leak of kasan quarantine"
from linux-next. Otherwise, it would cause double free issue.
Hi Andrew,
Sorry to bother.
I upload the v2 standalone fixup patch to fix the memory leak issue on
kernel-5.10 stable as below.
https://marc.info/?l=linux-mm=160820751825252=2
I think this slab memory leak issue is important. It's because when we
do kmem_cache_destroy, it will report object remaining error.
Add this v2 patch to mm-tree, it will have conflicts with
Andrey's patches as below.
"kasan: rename get_alloc/free_info"
"kasan: sanitize objects when metadata doesnt fit"
I think this standalone fixup patch should be added ""before"" Andrey's
patch in mm-tree. Because only merging this standalone fix patch to 5.10
stable, we can resolve this leak issue instead of merging the whole
patchset of Andrey's patch to 5.10 stable.
However, merging the fixup patch into mm-tree will cause some conflicts
in mm-tree.
Please help to fix the conflicts.
And I think the conflict between standalone fixup patch and
Andrey's patches will be fixed as below.
I think this patch "kasan: rename get_alloc/free_info" need to rename
the "info" to "meta" as below.
- qlink_free(>quarantine_link, cache);
+ qlink_free(>quarantine_link, cache);
This patch "kasan: sanitize objects when metadata doesnt fit" need to
remove the qlink_free() and add return false as below.
q = this_cpu_ptr(_quarantine);
if (q->offline) {
- qlink_free(>quarantine_link, cache);
local_irq_restore(flags);
- return;
+ return false;
}
Thanks a lot.
[PATCH v2 0/1] kasan: fix memory leak of kasan quarantine
When cpu is going offline, set q->offline as true and interrupt happened. The interrupt may call the quarantine_put. But quarantine_put do not free the the object. The object will cause memory leak. Add qlink_free() to free the object. Changes since v2: - Add Fixes in the commit message Kuan-Ying Lee (1): kasan: fix memory leak of kasan quarantine mm/kasan/quarantine.c | 1 + 1 file changed, 1 insertion(+) -- 2.18.0
[PATCH v2 1/1] kasan: fix memory leak of kasan quarantine
When cpu is going offline, set q->offline as true
and interrupt happened. The interrupt may call the
quarantine_put. But quarantine_put do not free the
the object. The object will cause memory leak.
Add qlink_free() to free the object.
Fixes: 6c82d45c7f03 (kasan: fix object remaining in offline per-cpu quarantine)
Signed-off-by: Kuan-Ying Lee
Cc: Andrey Ryabinin
Cc: Alexander Potapenko
Cc: Dmitry Vyukov
Cc: Andrew Morton
Cc: Matthias Brugger
Cc: [5.10-]
---
mm/kasan/quarantine.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
index 0e3f8494628f..cac7c617df72 100644
--- a/mm/kasan/quarantine.c
+++ b/mm/kasan/quarantine.c
@@ -191,6 +191,7 @@ void quarantine_put(struct kasan_free_meta *info, struct
kmem_cache *cache)
q = this_cpu_ptr(_quarantine);
if (q->offline) {
+ qlink_free(>quarantine_link, cache);
local_irq_restore(flags);
return;
}
--
2.18.0
Re: [PATCH 1/1] kasan: fix memory leak of kasan quarantine
On Tue, 2020-12-15 at 19:28 +0800, Kuan-Ying Lee wrote:
> When cpu is going offline, set q->offline as true
> and interrupt happened. The interrupt may call the
> quarantine_put. But quarantine_put do not free the
> the object. The object will cause memory leak.
>
> Add qlink_free() to free the object.
>
> Signed-off-by: Kuan-Ying Lee
> Cc: Andrey Ryabinin
> Cc: Alexander Potapenko
> Cc: Dmitry Vyukov
> Cc: Andrew Morton
> Cc: Matthias Brugger
> Cc: [5.10-]
> ---
> mm/kasan/quarantine.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
> index 0e3f8494628f..cac7c617df72 100644
> --- a/mm/kasan/quarantine.c
> +++ b/mm/kasan/quarantine.c
> @@ -191,6 +191,7 @@ void quarantine_put(struct kasan_free_meta *info, struct
> kmem_cache *cache)
>
> q = this_cpu_ptr(_quarantine);
> if (q->offline) {
> + qlink_free(>quarantine_link, cache);
> local_irq_restore(flags);
> return;
> }
Sorry.
Please ignore this patch.
Thanks.
[PATCH 1/1] kasan: fix memory leak of kasan quarantine
When cpu is going offline, set q->offline as true
and interrupt happened. The interrupt may call the
quarantine_put. But quarantine_put do not free the
the object. The object will cause memory leak.
Add qlink_free() to free the object.
Signed-off-by: Kuan-Ying Lee
Cc: Andrey Ryabinin
Cc: Alexander Potapenko
Cc: Dmitry Vyukov
Cc: Andrew Morton
Cc: Matthias Brugger
Cc: [5.10-]
---
mm/kasan/quarantine.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
index 0e3f8494628f..cac7c617df72 100644
--- a/mm/kasan/quarantine.c
+++ b/mm/kasan/quarantine.c
@@ -191,6 +191,7 @@ void quarantine_put(struct kasan_free_meta *info, struct
kmem_cache *cache)
q = this_cpu_ptr(_quarantine);
if (q->offline) {
+ qlink_free(>quarantine_link, cache);
local_irq_restore(flags);
return;
}
--
2.18.0
[PATCH 0/1] kasan: fix memory leak of kasan quarantine
When cpu is going offline, set q->offline as true and interrupt happened. The interrupt may call the quarantine_put. But quarantine_put do not free the the object. The object will cause memory leak. Add qlink_free() to free the object. Kuan-Ying Lee (1): kasan: fix memory leak of kasan quarantine mm/kasan/quarantine.c | 1 + 1 file changed, 1 insertion(+) -- 2.18.0
[PATCH v4 0/1] Fix object remain in offline per-cpu quarantine
This patch fixes object remain in the offline per-cpu quarantine as describe below. Free objects will get into per-cpu quarantine if enable generic KASAN. If a cpu is offline and users use kmem_cache_destroy, kernel will detect objects still remain in the offline per-cpu quarantine and report error. 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. Changes since v4: - Rebase to linux-next - Remove the qlist_free for double free issue - Thanks Qiang, Qian Changes since v3: - Add a barrier to ensure the ordering - Rename the init function Changes since v2: - Thanks for Dmitry suggestion - Remove unnecessary code - Put offline variable into cpu_quarantine - Use single qlist_free_all call instead of iteration over all slabs - Add bug reporter in commit message Kuan-Ying Lee (1): kasan: fix object remain in offline per-cpu quarantine mm/kasan/quarantine.c | 39 +++ 1 file changed, 39 insertions(+) -- 2.18.0
[PATCH v4 1/1] kasan: fix object remain 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=0x20010200
[ 74.986638] CPU: 3 PID: 176 Comm: cat Tainted: GB
5.10.0-rc1-7-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.
Signed-off-by: Kuan-Ying Lee
Signed-off-by: Zqiang
Suggested-by: Dmitry Vyukov
Reported-by: Guangye Yang
Reviewed-by: Dmitry Vyukov
Cc: Andrey Ryabinin
Cc: Alexander Potapenko
Cc: Andrew Morton
Cc: Matthias Brugger
Cc: Andrey Konovalov
Cc: Nicholas Tang
Cc: Miles Chen
Cc: Qian Cai
Cc: Stephen Rothwell
---
mm/kasan/quarantine.c | 39 +++
1 file changed, 39 insertions(+)
diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
index a598c3514e1a..55783125a767 100644
--- a/mm/kasan/quarantine.c
+++ b/mm/kasan/quarantine.c
@@ -19,6 +19,7 @@
#include
#include
#include
+#include
#include "../slab.h"
#include "kasan.h"
@@ -33,6 +34,7 @@ struct qlist_head {
struct qlist_node *head;
struct qlist_node *tail;
size_t bytes;
+ bool offline;
};
#define QLIST_INIT { NULL, NULL, 0 }
@@ -191,6 +193,10 @@ bool quarantine_put(struct kmem_cache *cache, void *object)
local_irq_save(flags);
q = this_cpu_ptr(_quarantine);
+ if (q->offline) {
+ local_irq_restore(flags);
+ return false;
+ }
qlist_put(q, >quarantine_link, cache->size);
if (unlikely(q->bytes > QUARANTINE_PERCPU_SIZE)) {
qlist_move_all(q, );
@@ -333,3 +339,36 @@ void quarantine_remove_cache(struct kmem_cache *cache)
synchronize_srcu(_cache_srcu);
}
+
+static int kasan_cpu_online(unsigned int cpu)
+{
+ this_cpu_ptr(_quarantine)->offline = false;
+ return 0;
+}
+
+static int kasan_cpu_offline(unsigned int cpu)
+{
+ struct qlist_head *q;
+
+ q = this_cpu_ptr(_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/kasa
Re: [PATCH] kasan: fix slab double free when cpu-hotplug
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
> > 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
> > > > 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(_quarantine);
> > > > > > if (q->offline) {
> > > > > > - qlink_free(>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
> > 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
Re: [PATCH] kasan: fix slab double free when cpu-hotplug
On Sat, 2020-12-05 at 17:09 -0800, Andrew Morton wrote:
> On Sun, 6 Dec 2020 00:17:15 +0800 Kuan-Ying Lee
> 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
> > > 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(_quarantine);
> > > > > if (q->offline) {
> > > > > - qlink_free(>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
> 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=0x20010200
> [ 74.986638] CPU: 3 PID: 176 Comm: cat Tainted: GB
> 5.10.0-rc1-7-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.
Re: [PATCH] kasan: fix slab double free when cpu-hotplug
On Fri, 2020-12-04 at 17:25 -0800, Andrew Morton wrote:
> On Fri, 4 Dec 2020 20:01:35 +0800 Kuan-Ying Lee
> 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(_quarantine);
> > > if (q->offline) {
> > > - qlink_free(>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
Re: [PATCH] kasan: fix slab double free when cpu-hotplug
On Fri, 2020-12-04 at 18:22 +0800, qiang.zh...@windriver.com wrote:
> From: Zqiang
>
> 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
> ---
> 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(_quarantine);
> if (q->offline) {
> - qlink_free(>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
Re: [PATCH v3 1/1] kasan: fix object remain in offline per-cpu quarantine
On Thu, 2020-12-03 at 12:28 -0800, Andrew Morton wrote:
> On Thu, 3 Dec 2020 13:46:59 +0100 Andrey Konovalov
> wrote:
>
> > > #define QLIST_INIT { NULL, NULL, 0 }
> > > @@ -188,6 +190,11 @@ void quarantine_put(struct kasan_free_meta *info,
> > > struct kmem_cache *cache)
> > > local_irq_save(flags);
> > >
> > > q = this_cpu_ptr(_quarantine);
> > > + if (q->offline) {
> > > + qlink_free(>quarantine_link, cache);
> >
> > Hi Kuan-Ying,
> >
> > This needs to be rebased onto the mm tree: it has some KASAN patches
> > that touch this code and rename the info variable to meta.
>
> Yup. I'm taking care of that.
Hi Andrew,
Sorry about that.
I will fix that conflict.
Thanks.
[PATCH v3 0/1] Fix object remain in offline per-cpu quarantine
This patch fixes object remain in the offline per-cpu quarantine as describe below. Free objects will get into per-cpu quarantine if enable generic KASAN. If a cpu is offline and users use kmem_cache_destroy, kernel will detect objects still remain in the offline per-cpu quarantine and report error. 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. Changes since v3: - Add a barrier to ensure the ordering - Rename the init function Changes since v2: - Thanks for Dmitry suggestion - Remove unnecessary code - Put offline variable into cpu_quarantine - Use single qlist_free_all call instead of iteration over all slabs - Add bug reporter in commit message Kuan-Ying Lee (1): kasan: fix object remain in offline per-cpu quarantine mm/kasan/quarantine.c | 40 1 file changed, 40 insertions(+) -- 2.18.0
[PATCH v3 1/1] kasan: fix object remain 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=0x20010200
[ 74.986638] CPU: 3 PID: 176 Comm: cat Tainted: GB
5.10.0-rc1-7-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.
Signed-off-by: Kuan-Ying Lee
Suggested-by: Dmitry Vyukov
Reported-by: Guangye Yang
Cc: Andrey Ryabinin
Cc: Alexander Potapenko
Cc: Andrew Morton
Cc: Matthias Brugger
---
mm/kasan/quarantine.c | 40
1 file changed, 40 insertions(+)
diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
index 4c5375810449..cac7c617df72 100644
--- a/mm/kasan/quarantine.c
+++ b/mm/kasan/quarantine.c
@@ -29,6 +29,7 @@
#include
#include
#include
+#include
#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,11 @@ void quarantine_put(struct kasan_free_meta *info, struct
kmem_cache *cache)
local_irq_save(flags);
q = this_cpu_ptr(_quarantine);
+ if (q->offline) {
+ qlink_free(>quarantine_link, cache);
+ local_irq_restore(flags);
+ return;
+ }
qlist_put(q, >quarantine_link, cache->size);
if (unlikely(q->bytes > QUARANTINE_PERCPU_SIZE)) {
qlist_move_all(q, );
@@ -328,3 +335,36 @@ void quarantine_remove_cache(struct kmem_cache *cache)
synchronize_srcu(_cache_srcu);
}
+
+static int kasan_cpu_online(unsigned int cpu)
+{
+ this_cpu_ptr(_quarantine)->offline = false;
+ return 0;
+}
+
+static int kasan_cpu_offline(unsigned int cpu)
+{
+ struct qlist_head *q;
+
+ q = this_cpu_ptr(_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)
Re: [PATCH v2 1/1] kasan: fix object remain in offline per-cpu quarantine
On Tue, 2020-11-17 at 08:13 +0100, Dmitry Vyukov wrote: > On Tue, Nov 17, 2020 at 7:46 AM Kuan-Ying Lee > wrote: > > > > On Mon, 2020-11-16 at 10:26 +0100, Dmitry Vyukov wrote: > > > On Mon, Nov 16, 2020 at 7:30 AM Kuan-Ying Lee > > > wrote: > > > > > > > > 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=0x20010200 > > > > [ 74.986638] CPU: 3 PID: 176 Comm: cat Tainted: GB > > > > 5.10.0-rc1-7-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. > > > > > > > > Signed-off-by: Kuan-Ying Lee > > > > Suggested-by: Dmitry Vyukov > > > > Reported-by: Guangye Yang > > > > Cc: Andrey Ryabinin > > > > Cc: Alexander Potapenko > > > > Cc: Andrew Morton > > > > Cc: Matthias Brugger > > > > --- > > > > mm/kasan/quarantine.c | 35 +++ > > > > 1 file changed, 35 insertions(+) > > > > > > > > diff --git a/mm/kasan/quarantin
Re: [PATCH v2 1/1] kasan: fix object remain in offline per-cpu quarantine
On Mon, 2020-11-16 at 10:26 +0100, Dmitry Vyukov wrote:
> On Mon, Nov 16, 2020 at 7:30 AM Kuan-Ying Lee
> wrote:
> >
> > 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=0x20010200
> > [ 74.986638] CPU: 3 PID: 176 Comm: cat Tainted: GB
> > 5.10.0-rc1-7-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.
> >
> > Signed-off-by: Kuan-Ying Lee
> > Suggested-by: Dmitry Vyukov
> > Reported-by: Guangye Yang
> > Cc: Andrey Ryabinin
> > Cc: Alexander Potapenko
> > Cc: Andrew Morton
> > Cc: Matthias Brugger
> > ---
> > mm/kasan/quarantine.c | 35 +++
> > 1 file changed, 35 insertions(+)
> >
> > diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
> > index 4c5375810449..16e618ea805e 100644
> > --- a/mm/kasan/quarantine.c
> > +++ b/mm/kasan/quarantine.c
> > @@ -29,6 +29,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #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,11 @@ void quarantine_put(struct kasan_free_meta *info,
> > struct kmem_cache *cache)
> > local_irq_save(flags);
> >
> > q = this_cpu_ptr(_quarantine);
> > + if (q->offline) {
> > + qlink_free(>quarant
[PATCH v2 0/1] Fix object remain in offline per-cpu quarantine
This patch fixes object remain in the offline per-cpu quarantine as describe below. Free objects will get into per-cpu quarantine if enable generic KASAN. If a cpu is offline and users use kmem_cache_destroy, kernel will detect objects still remain in the offline per-cpu quarantine and report error. 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. Changes since v2: - Thanks for Dmitry suggestion - Remove unnecessary code - Put offline variable into cpu_quarantine - Use single qlist_free_all call instead of iteration over all slabs - Add bug reporter in commit message Kuan-Ying Lee (1): kasan: fix object remain in offline per-cpu quarantine mm/kasan/quarantine.c | 35 +++ 1 file changed, 35 insertions(+) -- 2.18.0
[PATCH v2 1/1] kasan: fix object remain 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=0x20010200
[ 74.986638] CPU: 3 PID: 176 Comm: cat Tainted: GB
5.10.0-rc1-7-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.
Signed-off-by: Kuan-Ying Lee
Suggested-by: Dmitry Vyukov
Reported-by: Guangye Yang
Cc: Andrey Ryabinin
Cc: Alexander Potapenko
Cc: Andrew Morton
Cc: Matthias Brugger
---
mm/kasan/quarantine.c | 35 +++
1 file changed, 35 insertions(+)
diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
index 4c5375810449..16e618ea805e 100644
--- a/mm/kasan/quarantine.c
+++ b/mm/kasan/quarantine.c
@@ -29,6 +29,7 @@
#include
#include
#include
+#include
#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,11 @@ void quarantine_put(struct kasan_free_meta *info, struct
kmem_cache *cache)
local_irq_save(flags);
q = this_cpu_ptr(_quarantine);
+ if (q->offline) {
+ qlink_free(>quarantine_link, cache);
+ local_irq_restore(flags);
+ return;
+ }
qlist_put(q, >quarantine_link, cache->size);
if (unlikely(q->bytes > QUARANTINE_PERCPU_SIZE)) {
qlist_move_all(q, );
@@ -328,3 +335,31 @@ void quarantine_remove_cache(struct kmem_cache *cache)
synchronize_srcu(_cache_srcu);
}
+
+static int kasan_cpu_online(unsigned int cpu)
+{
+ this_cpu_ptr(_quarantine)->offline = false;
+ return 0;
+}
+
+static int kasan_cpu_offline(unsigned int cpu)
+{
+ struct qlist_head *q;
+
+ q = this_cpu_ptr(_quarantine);
+ q->offline = true;
+ qlist_free_all(q, NULL);
+ return 0;
+}
+
+static int __init kasan_cpu_offline_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 offline cpu quarantine register failed [%d]\n",
ret);
+ return ret;
+}
+late_initcall(kasan_cpu_offline_quarantine_init);
--
2.18.0
Re: [PATCH 1/1] kasan: fix object remain in offline per-cpu quarantine
On Fri, 2020-11-13 at 08:03 +0100, Dmitry Vyukov wrote: > On Fri, Nov 13, 2020 at 3:32 AM Kuan-Ying Lee > wrote: > > > > On Thu, 2020-11-12 at 09:39 +0100, Dmitry Vyukov wrote: > > > On Thu, Nov 12, 2020 at 7:25 AM Kuan-Ying Lee > > > wrote: > > > > > > > > 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=0x20010200 > > > > [ 74.986638] CPU: 3 PID: 176 Comm: cat Tainted: GB > > > > 5.10.0-rc1-7-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. > > > > > > > > Signed-off-by: Kuan-Ying Lee > > > > --- > > > > mm/kasan/quarantine.c | 59 +-- > > > > 1 file changed, 57 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c > > > > index 4c5375810449..67fb91ae2bd0 100644 > > > > --- a/mm/kasan/quarantine.c > > > > +++ b/mm/kasan/quarantine.c > > > > @@ -29,6 +29,7 @@ > > >
Re: [PATCH 1/1] kasan: fix object remain in offline per-cpu quarantine
On Thu, 2020-11-12 at 09:39 +0100, Dmitry Vyukov wrote: > On Thu, Nov 12, 2020 at 7:25 AM Kuan-Ying Lee > wrote: > > > > 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=0x20010200 > > [ 74.986638] CPU: 3 PID: 176 Comm: cat Tainted: GB > > 5.10.0-rc1-7-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. > > > > Signed-off-by: Kuan-Ying Lee > > --- > > mm/kasan/quarantine.c | 59 +-- > > 1 file changed, 57 insertions(+), 2 deletions(-) > > > > diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c > > index 4c5375810449..67fb91ae2bd0 100644 > > --- a/mm/kasan/quarantine.c > > +++ b/mm/kasan/quarantine.c > > @@ -29,6 +29,7 @@ > > #include > > #include > > #include > > +#include > > > > #include "../slab.h" > > #include "kasan.h" > > @@ -97,6 +98,7 @@ static void qlist_move_all(struct qlist_head *from, > > struct qlist_head *to) > > * guarded by quarantine_lock. > > */ > > Hi Kuan-Ying, > > Thanks for fixing this. > > > static DEFINE_PER_CPU(struct qlist_head, cpu_quarantine); > > +static DEFINE_PER_CPU(int, cpu_quarantine_offline); > > I think cpu_quarantine_offline is better be part of cpu_quarantine > because it logically is and we already obtain a pointer to > cpu_quarantine in quarantine_put, so it will also make the code a bit > shorter. > Ok. Got it. > > > /* Round-robin FIFO array of batches. */ > > static struct qlist_head global_quarantine[QUARANTINE_BATCHES]; > >
[PATCH 0/1] Fix objects remain in the offline per-cpu quarantine
This patch fixes object remain in the offline per-cpu quarantine as describe below. Free objects will get into per-cpu quarantine if enable generic KASAN. If a cpu is offline and users use kmem_cache_destroy, kernel will detect objects still remain in the offline per-cpu quarantine and report error. 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. Kuan-Ying Lee (1): kasan: fix object remain in offline per-cpu quarantine mm/kasan/quarantine.c | 59 +-- 1 file changed, 57 insertions(+), 2 deletions(-) -- 2.18.0
[PATCH 1/1] kasan: fix object remain 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=0x20010200
[ 74.986638] CPU: 3 PID: 176 Comm: cat Tainted: GB
5.10.0-rc1-7-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.
Signed-off-by: Kuan-Ying Lee
---
mm/kasan/quarantine.c | 59 +--
1 file changed, 57 insertions(+), 2 deletions(-)
diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
index 4c5375810449..67fb91ae2bd0 100644
--- a/mm/kasan/quarantine.c
+++ b/mm/kasan/quarantine.c
@@ -29,6 +29,7 @@
#include
#include
#include
+#include
#include "../slab.h"
#include "kasan.h"
@@ -97,6 +98,7 @@ static void qlist_move_all(struct qlist_head *from, struct
qlist_head *to)
* guarded by quarantine_lock.
*/
static DEFINE_PER_CPU(struct qlist_head, cpu_quarantine);
+static DEFINE_PER_CPU(int, cpu_quarantine_offline);
/* Round-robin FIFO array of batches. */
static struct qlist_head global_quarantine[QUARANTINE_BATCHES];
@@ -176,6 +178,8 @@ void quarantine_put(struct kasan_free_meta *info, struct
kmem_cache *cache)
unsigned long flags;
struct qlist_head *q;
struct qlist_head temp = QLIST_INIT;
+ int *offline;
+ struct qlist_head q_offline = QLIST_INIT;
/*
* Note: irq must be disabled until after we move the batch to the
@@ -187,8 +191,16 @@ void quarantine_put(struct kasan_free_meta *info, struct
kmem_cache *cache)
*/
local_irq_save(flags);
- q = this_cpu_ptr(_quarantine);
- qlist_put(q, >quarantine_link, cache->size);
+ offline = this_cpu_ptr(_quarantine_offline);
+ if (*offline == 0) {
+ q = this_cpu_ptr(_quarantine);
+ qlist_put(q, >quarantine_link, cache->size);
+ } else {
+ qlist_put(_offline, >quarantine_link, cache->size);
+ qlist_free_all(_offline, cache);
+ local_irq_restore(flags);
+ return;
+ }
if (unlikely(q->bytes > QUARANTINE_PERCPU_SIZE)) {
qlist_move_all(q, );
@@ -328,3 +340,46 @@ void quarantine_remove_cache(struct kmem_cache *cache)
synchronize_srcu(_cache_srcu);
}
+
+static int kasan_cpu_online(unsigned int cpu)
+{
+ int *offline;
+ unsigned long flags;
+
+
