* [PATCH] debugobjects: Fix various data races
@ 2020-01-16 18:55 Marco Elver
2020-01-17 14:50 ` [tip: core/debugobjects] " tip-bot2 for Marco Elver
0 siblings, 1 reply; 2+ messages in thread
From: Marco Elver @ 2020-01-16 18:55 UTC (permalink / raw)
To: elver; +Cc: linux-kernel, kasan-dev, tglx, longman, gregkh, Qian Cai
The counters obj_pool_free, and obj_nr_tofree, and the flag obj_freeing
are read locklessly outside the pool_lock critical sections. If read
with plain accesses, this would result in data races.
This is addressed as follows:
* reads outside critical sections become READ_ONCE()s (pairing with
WRITE_ONCE()s added);
* writes become WRITE_ONCE()s (pairing with READ_ONCE()s added); since
writes happen inside critical sections, only the write and not the
read of RMWs needs to be atomic, thus WRITE_ONCE(var, var +/- X) is
sufficient.
The data races were reported by KCSAN:
BUG: KCSAN: data-race in __free_object / fill_pool
write to 0xffffffff8beb04f8 of 4 bytes by interrupt on cpu 1:
__free_object+0x1ee/0x8e0 lib/debugobjects.c:404
__debug_check_no_obj_freed+0x199/0x330 lib/debugobjects.c:969
debug_check_no_obj_freed+0x3c/0x44 lib/debugobjects.c:994
slab_free_hook mm/slub.c:1422 [inline]
read to 0xffffffff8beb04f8 of 4 bytes by task 1 on cpu 2:
fill_pool+0x3d/0x520 lib/debugobjects.c:135
__debug_object_init+0x3c/0x810 lib/debugobjects.c:536
debug_object_init lib/debugobjects.c:591 [inline]
debug_object_activate+0x228/0x320 lib/debugobjects.c:677
debug_rcu_head_queue kernel/rcu/rcu.h:176 [inline]
BUG: KCSAN: data-race in __debug_object_init / fill_pool
read to 0xffffffff8beb04f8 of 4 bytes by task 10 on cpu 6:
fill_pool+0x3d/0x520 lib/debugobjects.c:135
__debug_object_init+0x3c/0x810 lib/debugobjects.c:536
debug_object_init_on_stack+0x39/0x50 lib/debugobjects.c:606
init_timer_on_stack_key kernel/time/timer.c:742 [inline]
write to 0xffffffff8beb04f8 of 4 bytes by task 1 on cpu 3:
alloc_object lib/debugobjects.c:258 [inline]
__debug_object_init+0x717/0x810 lib/debugobjects.c:544
debug_object_init lib/debugobjects.c:591 [inline]
debug_object_activate+0x228/0x320 lib/debugobjects.c:677
debug_rcu_head_queue kernel/rcu/rcu.h:176 [inline]
BUG: KCSAN: data-race in free_obj_work / free_object
read to 0xffffffff9140c190 of 4 bytes by task 10 on cpu 6:
free_object+0x4b/0xd0 lib/debugobjects.c:426
debug_object_free+0x190/0x210 lib/debugobjects.c:824
destroy_timer_on_stack kernel/time/timer.c:749 [inline]
write to 0xffffffff9140c190 of 4 bytes by task 93 on cpu 1:
free_obj_work+0x24f/0x480 lib/debugobjects.c:313
process_one_work+0x454/0x8d0 kernel/workqueue.c:2264
worker_thread+0x9a/0x780 kernel/workqueue.c:2410
Signed-off-by: Marco Elver <elver@google.com>
Reported-by: Qian Cai <cai@lca.pw>
---
lib/debugobjects.c | 46 +++++++++++++++++++++++++---------------------
1 file changed, 25 insertions(+), 21 deletions(-)
diff --git a/lib/debugobjects.c b/lib/debugobjects.c
index 61261195f5b6..48054dbf1b51 100644
--- a/lib/debugobjects.c
+++ b/lib/debugobjects.c
@@ -132,14 +132,18 @@ static void fill_pool(void)
struct debug_obj *obj;
unsigned long flags;
- if (likely(obj_pool_free >= debug_objects_pool_min_level))
+ if (likely(READ_ONCE(obj_pool_free) >= debug_objects_pool_min_level))
return;
/*
* Reuse objs from the global free list; they will be reinitialized
* when allocating.
+ *
+ * Both obj_nr_tofree and obj_pool_free are checked locklessly; the
+ * READ_ONCE()s pair with the WRITE_ONCE()s in pool_lock critical
+ * sections.
*/
- while (obj_nr_tofree && (obj_pool_free < obj_pool_min_free)) {
+ while (READ_ONCE(obj_nr_tofree) && (READ_ONCE(obj_pool_free) < obj_pool_min_free)) {
raw_spin_lock_irqsave(&pool_lock, flags);
/*
* Recheck with the lock held as the worker thread might have
@@ -148,9 +152,9 @@ static void fill_pool(void)
while (obj_nr_tofree && (obj_pool_free < obj_pool_min_free)) {
obj = hlist_entry(obj_to_free.first, typeof(*obj), node);
hlist_del(&obj->node);
- obj_nr_tofree--;
+ WRITE_ONCE(obj_nr_tofree, obj_nr_tofree - 1);
hlist_add_head(&obj->node, &obj_pool);
- obj_pool_free++;
+ WRITE_ONCE(obj_pool_free, obj_pool_free + 1);
}
raw_spin_unlock_irqrestore(&pool_lock, flags);
}
@@ -158,7 +162,7 @@ static void fill_pool(void)
if (unlikely(!obj_cache))
return;
- while (obj_pool_free < debug_objects_pool_min_level) {
+ while (READ_ONCE(obj_pool_free) < debug_objects_pool_min_level) {
struct debug_obj *new[ODEBUG_BATCH_SIZE];
int cnt;
@@ -174,7 +178,7 @@ static void fill_pool(void)
while (cnt) {
hlist_add_head(&new[--cnt]->node, &obj_pool);
debug_objects_allocated++;
- obj_pool_free++;
+ WRITE_ONCE(obj_pool_free, obj_pool_free + 1);
}
raw_spin_unlock_irqrestore(&pool_lock, flags);
}
@@ -236,7 +240,7 @@ alloc_object(void *addr, struct debug_bucket *b, struct debug_obj_descr *descr)
obj = __alloc_object(&obj_pool);
if (obj) {
obj_pool_used++;
- obj_pool_free--;
+ WRITE_ONCE(obj_pool_free, obj_pool_free - 1);
/*
* Looking ahead, allocate one batch of debug objects and
@@ -255,7 +259,7 @@ alloc_object(void *addr, struct debug_bucket *b, struct debug_obj_descr *descr)
&percpu_pool->free_objs);
percpu_pool->obj_free++;
obj_pool_used++;
- obj_pool_free--;
+ WRITE_ONCE(obj_pool_free, obj_pool_free - 1);
}
}
@@ -309,8 +313,8 @@ static void free_obj_work(struct work_struct *work)
obj = hlist_entry(obj_to_free.first, typeof(*obj), node);
hlist_del(&obj->node);
hlist_add_head(&obj->node, &obj_pool);
- obj_pool_free++;
- obj_nr_tofree--;
+ WRITE_ONCE(obj_pool_free, obj_pool_free + 1);
+ WRITE_ONCE(obj_nr_tofree, obj_nr_tofree - 1);
}
raw_spin_unlock_irqrestore(&pool_lock, flags);
return;
@@ -324,7 +328,7 @@ static void free_obj_work(struct work_struct *work)
if (obj_nr_tofree) {
hlist_move_list(&obj_to_free, &tofree);
debug_objects_freed += obj_nr_tofree;
- obj_nr_tofree = 0;
+ WRITE_ONCE(obj_nr_tofree, 0);
}
raw_spin_unlock_irqrestore(&pool_lock, flags);
@@ -375,10 +379,10 @@ static void __free_object(struct debug_obj *obj)
obj_pool_used--;
if (work) {
- obj_nr_tofree++;
+ WRITE_ONCE(obj_nr_tofree, obj_nr_tofree + 1);
hlist_add_head(&obj->node, &obj_to_free);
if (lookahead_count) {
- obj_nr_tofree += lookahead_count;
+ WRITE_ONCE(obj_nr_tofree, obj_nr_tofree + lookahead_count);
obj_pool_used -= lookahead_count;
while (lookahead_count) {
hlist_add_head(&objs[--lookahead_count]->node,
@@ -396,15 +400,15 @@ static void __free_object(struct debug_obj *obj)
for (i = 0; i < ODEBUG_BATCH_SIZE; i++) {
obj = __alloc_object(&obj_pool);
hlist_add_head(&obj->node, &obj_to_free);
- obj_pool_free--;
- obj_nr_tofree++;
+ WRITE_ONCE(obj_pool_free, obj_pool_free - 1);
+ WRITE_ONCE(obj_nr_tofree, obj_nr_tofree + 1);
}
}
} else {
- obj_pool_free++;
+ WRITE_ONCE(obj_pool_free, obj_pool_free + 1);
hlist_add_head(&obj->node, &obj_pool);
if (lookahead_count) {
- obj_pool_free += lookahead_count;
+ WRITE_ONCE(obj_pool_free, obj_pool_free + lookahead_count);
obj_pool_used -= lookahead_count;
while (lookahead_count) {
hlist_add_head(&objs[--lookahead_count]->node,
@@ -423,7 +427,7 @@ static void __free_object(struct debug_obj *obj)
static void free_object(struct debug_obj *obj)
{
__free_object(obj);
- if (!obj_freeing && obj_nr_tofree) {
+ if (!READ_ONCE(obj_freeing) && READ_ONCE(obj_nr_tofree)) {
WRITE_ONCE(obj_freeing, true);
schedule_delayed_work(&debug_obj_work, ODEBUG_FREE_WORK_DELAY);
}
@@ -982,7 +986,7 @@ static void __debug_check_no_obj_freed(const void *address, unsigned long size)
debug_objects_maxchecked = objs_checked;
/* Schedule work to actually kmem_cache_free() objects */
- if (!obj_freeing && obj_nr_tofree) {
+ if (!READ_ONCE(obj_freeing) && READ_ONCE(obj_nr_tofree)) {
WRITE_ONCE(obj_freeing, true);
schedule_delayed_work(&debug_obj_work, ODEBUG_FREE_WORK_DELAY);
}
@@ -1008,12 +1012,12 @@ static int debug_stats_show(struct seq_file *m, void *v)
seq_printf(m, "max_checked :%d\n", debug_objects_maxchecked);
seq_printf(m, "warnings :%d\n", debug_objects_warnings);
seq_printf(m, "fixups :%d\n", debug_objects_fixups);
- seq_printf(m, "pool_free :%d\n", obj_pool_free + obj_percpu_free);
+ seq_printf(m, "pool_free :%d\n", READ_ONCE(obj_pool_free) + obj_percpu_free);
seq_printf(m, "pool_pcp_free :%d\n", obj_percpu_free);
seq_printf(m, "pool_min_free :%d\n", obj_pool_min_free);
seq_printf(m, "pool_used :%d\n", obj_pool_used - obj_percpu_free);
seq_printf(m, "pool_max_used :%d\n", obj_pool_max_used);
- seq_printf(m, "on_free_list :%d\n", obj_nr_tofree);
+ seq_printf(m, "on_free_list :%d\n", READ_ONCE(obj_nr_tofree));
seq_printf(m, "objs_allocated:%d\n", debug_objects_allocated);
seq_printf(m, "objs_freed :%d\n", debug_objects_freed);
return 0;
--
2.25.0.rc1.283.g88dfdc4193-goog
^ permalink raw reply related [flat|nested] 2+ messages in thread
* [tip: core/debugobjects] debugobjects: Fix various data races
2020-01-16 18:55 [PATCH] debugobjects: Fix various data races Marco Elver
@ 2020-01-17 14:50 ` tip-bot2 for Marco Elver
0 siblings, 0 replies; 2+ messages in thread
From: tip-bot2 for Marco Elver @ 2020-01-17 14:50 UTC (permalink / raw)
To: linux-tip-commits; +Cc: Qian Cai, Marco Elver, Thomas Gleixner, x86, LKML
The following commit has been merged into the core/debugobjects branch of tip:
Commit-ID: 35fd7a637c42bb54ba4608f4d40ae6e55fc88781
Gitweb: https://git.kernel.org/tip/35fd7a637c42bb54ba4608f4d40ae6e55fc88781
Author: Marco Elver <elver@google.com>
AuthorDate: Thu, 16 Jan 2020 19:55:29 +01:00
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Fri, 17 Jan 2020 15:45:01 +01:00
debugobjects: Fix various data races
The counters obj_pool_free, and obj_nr_tofree, and the flag obj_freeing are
read locklessly outside the pool_lock critical sections. If read with plain
accesses, this would result in data races.
This is addressed as follows:
* reads outside critical sections become READ_ONCE()s (pairing with
WRITE_ONCE()s added);
* writes become WRITE_ONCE()s (pairing with READ_ONCE()s added); since
writes happen inside critical sections, only the write and not the read
of RMWs needs to be atomic, thus WRITE_ONCE(var, var +/- X) is
sufficient.
The data races were reported by KCSAN:
BUG: KCSAN: data-race in __free_object / fill_pool
write to 0xffffffff8beb04f8 of 4 bytes by interrupt on cpu 1:
__free_object+0x1ee/0x8e0 lib/debugobjects.c:404
__debug_check_no_obj_freed+0x199/0x330 lib/debugobjects.c:969
debug_check_no_obj_freed+0x3c/0x44 lib/debugobjects.c:994
slab_free_hook mm/slub.c:1422 [inline]
read to 0xffffffff8beb04f8 of 4 bytes by task 1 on cpu 2:
fill_pool+0x3d/0x520 lib/debugobjects.c:135
__debug_object_init+0x3c/0x810 lib/debugobjects.c:536
debug_object_init lib/debugobjects.c:591 [inline]
debug_object_activate+0x228/0x320 lib/debugobjects.c:677
debug_rcu_head_queue kernel/rcu/rcu.h:176 [inline]
BUG: KCSAN: data-race in __debug_object_init / fill_pool
read to 0xffffffff8beb04f8 of 4 bytes by task 10 on cpu 6:
fill_pool+0x3d/0x520 lib/debugobjects.c:135
__debug_object_init+0x3c/0x810 lib/debugobjects.c:536
debug_object_init_on_stack+0x39/0x50 lib/debugobjects.c:606
init_timer_on_stack_key kernel/time/timer.c:742 [inline]
write to 0xffffffff8beb04f8 of 4 bytes by task 1 on cpu 3:
alloc_object lib/debugobjects.c:258 [inline]
__debug_object_init+0x717/0x810 lib/debugobjects.c:544
debug_object_init lib/debugobjects.c:591 [inline]
debug_object_activate+0x228/0x320 lib/debugobjects.c:677
debug_rcu_head_queue kernel/rcu/rcu.h:176 [inline]
BUG: KCSAN: data-race in free_obj_work / free_object
read to 0xffffffff9140c190 of 4 bytes by task 10 on cpu 6:
free_object+0x4b/0xd0 lib/debugobjects.c:426
debug_object_free+0x190/0x210 lib/debugobjects.c:824
destroy_timer_on_stack kernel/time/timer.c:749 [inline]
write to 0xffffffff9140c190 of 4 bytes by task 93 on cpu 1:
free_obj_work+0x24f/0x480 lib/debugobjects.c:313
process_one_work+0x454/0x8d0 kernel/workqueue.c:2264
worker_thread+0x9a/0x780 kernel/workqueue.c:2410
Reported-by: Qian Cai <cai@lca.pw>
Signed-off-by: Marco Elver <elver@google.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Link: https://lore.kernel.org/r/20200116185529.11026-1-elver@google.com
---
lib/debugobjects.c | 46 ++++++++++++++++++++++++---------------------
1 file changed, 25 insertions(+), 21 deletions(-)
diff --git a/lib/debugobjects.c b/lib/debugobjects.c
index 6126119..48054db 100644
--- a/lib/debugobjects.c
+++ b/lib/debugobjects.c
@@ -132,14 +132,18 @@ static void fill_pool(void)
struct debug_obj *obj;
unsigned long flags;
- if (likely(obj_pool_free >= debug_objects_pool_min_level))
+ if (likely(READ_ONCE(obj_pool_free) >= debug_objects_pool_min_level))
return;
/*
* Reuse objs from the global free list; they will be reinitialized
* when allocating.
+ *
+ * Both obj_nr_tofree and obj_pool_free are checked locklessly; the
+ * READ_ONCE()s pair with the WRITE_ONCE()s in pool_lock critical
+ * sections.
*/
- while (obj_nr_tofree && (obj_pool_free < obj_pool_min_free)) {
+ while (READ_ONCE(obj_nr_tofree) && (READ_ONCE(obj_pool_free) < obj_pool_min_free)) {
raw_spin_lock_irqsave(&pool_lock, flags);
/*
* Recheck with the lock held as the worker thread might have
@@ -148,9 +152,9 @@ static void fill_pool(void)
while (obj_nr_tofree && (obj_pool_free < obj_pool_min_free)) {
obj = hlist_entry(obj_to_free.first, typeof(*obj), node);
hlist_del(&obj->node);
- obj_nr_tofree--;
+ WRITE_ONCE(obj_nr_tofree, obj_nr_tofree - 1);
hlist_add_head(&obj->node, &obj_pool);
- obj_pool_free++;
+ WRITE_ONCE(obj_pool_free, obj_pool_free + 1);
}
raw_spin_unlock_irqrestore(&pool_lock, flags);
}
@@ -158,7 +162,7 @@ static void fill_pool(void)
if (unlikely(!obj_cache))
return;
- while (obj_pool_free < debug_objects_pool_min_level) {
+ while (READ_ONCE(obj_pool_free) < debug_objects_pool_min_level) {
struct debug_obj *new[ODEBUG_BATCH_SIZE];
int cnt;
@@ -174,7 +178,7 @@ static void fill_pool(void)
while (cnt) {
hlist_add_head(&new[--cnt]->node, &obj_pool);
debug_objects_allocated++;
- obj_pool_free++;
+ WRITE_ONCE(obj_pool_free, obj_pool_free + 1);
}
raw_spin_unlock_irqrestore(&pool_lock, flags);
}
@@ -236,7 +240,7 @@ alloc_object(void *addr, struct debug_bucket *b, struct debug_obj_descr *descr)
obj = __alloc_object(&obj_pool);
if (obj) {
obj_pool_used++;
- obj_pool_free--;
+ WRITE_ONCE(obj_pool_free, obj_pool_free - 1);
/*
* Looking ahead, allocate one batch of debug objects and
@@ -255,7 +259,7 @@ alloc_object(void *addr, struct debug_bucket *b, struct debug_obj_descr *descr)
&percpu_pool->free_objs);
percpu_pool->obj_free++;
obj_pool_used++;
- obj_pool_free--;
+ WRITE_ONCE(obj_pool_free, obj_pool_free - 1);
}
}
@@ -309,8 +313,8 @@ static void free_obj_work(struct work_struct *work)
obj = hlist_entry(obj_to_free.first, typeof(*obj), node);
hlist_del(&obj->node);
hlist_add_head(&obj->node, &obj_pool);
- obj_pool_free++;
- obj_nr_tofree--;
+ WRITE_ONCE(obj_pool_free, obj_pool_free + 1);
+ WRITE_ONCE(obj_nr_tofree, obj_nr_tofree - 1);
}
raw_spin_unlock_irqrestore(&pool_lock, flags);
return;
@@ -324,7 +328,7 @@ free_objs:
if (obj_nr_tofree) {
hlist_move_list(&obj_to_free, &tofree);
debug_objects_freed += obj_nr_tofree;
- obj_nr_tofree = 0;
+ WRITE_ONCE(obj_nr_tofree, 0);
}
raw_spin_unlock_irqrestore(&pool_lock, flags);
@@ -375,10 +379,10 @@ free_to_obj_pool:
obj_pool_used--;
if (work) {
- obj_nr_tofree++;
+ WRITE_ONCE(obj_nr_tofree, obj_nr_tofree + 1);
hlist_add_head(&obj->node, &obj_to_free);
if (lookahead_count) {
- obj_nr_tofree += lookahead_count;
+ WRITE_ONCE(obj_nr_tofree, obj_nr_tofree + lookahead_count);
obj_pool_used -= lookahead_count;
while (lookahead_count) {
hlist_add_head(&objs[--lookahead_count]->node,
@@ -396,15 +400,15 @@ free_to_obj_pool:
for (i = 0; i < ODEBUG_BATCH_SIZE; i++) {
obj = __alloc_object(&obj_pool);
hlist_add_head(&obj->node, &obj_to_free);
- obj_pool_free--;
- obj_nr_tofree++;
+ WRITE_ONCE(obj_pool_free, obj_pool_free - 1);
+ WRITE_ONCE(obj_nr_tofree, obj_nr_tofree + 1);
}
}
} else {
- obj_pool_free++;
+ WRITE_ONCE(obj_pool_free, obj_pool_free + 1);
hlist_add_head(&obj->node, &obj_pool);
if (lookahead_count) {
- obj_pool_free += lookahead_count;
+ WRITE_ONCE(obj_pool_free, obj_pool_free + lookahead_count);
obj_pool_used -= lookahead_count;
while (lookahead_count) {
hlist_add_head(&objs[--lookahead_count]->node,
@@ -423,7 +427,7 @@ free_to_obj_pool:
static void free_object(struct debug_obj *obj)
{
__free_object(obj);
- if (!obj_freeing && obj_nr_tofree) {
+ if (!READ_ONCE(obj_freeing) && READ_ONCE(obj_nr_tofree)) {
WRITE_ONCE(obj_freeing, true);
schedule_delayed_work(&debug_obj_work, ODEBUG_FREE_WORK_DELAY);
}
@@ -982,7 +986,7 @@ repeat:
debug_objects_maxchecked = objs_checked;
/* Schedule work to actually kmem_cache_free() objects */
- if (!obj_freeing && obj_nr_tofree) {
+ if (!READ_ONCE(obj_freeing) && READ_ONCE(obj_nr_tofree)) {
WRITE_ONCE(obj_freeing, true);
schedule_delayed_work(&debug_obj_work, ODEBUG_FREE_WORK_DELAY);
}
@@ -1008,12 +1012,12 @@ static int debug_stats_show(struct seq_file *m, void *v)
seq_printf(m, "max_checked :%d\n", debug_objects_maxchecked);
seq_printf(m, "warnings :%d\n", debug_objects_warnings);
seq_printf(m, "fixups :%d\n", debug_objects_fixups);
- seq_printf(m, "pool_free :%d\n", obj_pool_free + obj_percpu_free);
+ seq_printf(m, "pool_free :%d\n", READ_ONCE(obj_pool_free) + obj_percpu_free);
seq_printf(m, "pool_pcp_free :%d\n", obj_percpu_free);
seq_printf(m, "pool_min_free :%d\n", obj_pool_min_free);
seq_printf(m, "pool_used :%d\n", obj_pool_used - obj_percpu_free);
seq_printf(m, "pool_max_used :%d\n", obj_pool_max_used);
- seq_printf(m, "on_free_list :%d\n", obj_nr_tofree);
+ seq_printf(m, "on_free_list :%d\n", READ_ONCE(obj_nr_tofree));
seq_printf(m, "objs_allocated:%d\n", debug_objects_allocated);
seq_printf(m, "objs_freed :%d\n", debug_objects_freed);
return 0;
^ permalink raw reply related [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-01-17 14:50 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-16 18:55 [PATCH] debugobjects: Fix various data races Marco Elver
2020-01-17 14:50 ` [tip: core/debugobjects] " tip-bot2 for Marco Elver
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.