linux-next oops in __lock_acquire for process_one_work

PATCH In review Hugh Dickins 5 participants 18 messages May 7, 2012 – May 15, 2012

Subsystems (2)

Locking primitives Workqueue

lore

Share & tools

lore

Hugh Dickins

author

2012-05-07 17:19 UTC #0 lore raw

Hi Tejun,

Running MM load on recent linux-nexts (e.g. 3.4.0-rc5-next-20120504), with CONFIG_PROVE_LOCKING=y, I've been hitting an oops in __lock_acquire called from lock_acquire called from process_one_work: serving mm/swap.c's lru_add_drain_all - schedule_on_each_cpu(lru_add_drain_per_cpu).

In each case the oopsing address has been ffffffff00000198, and the oopsing instruction is the "atomic_inc((atomic_t *)&class->ops)" in __lock_acquire: so class is ffffffff00000000.

I notice Stephen's commit 0976dfc1d0cd80a4e9dfaf87bd8744612bde475a
workqueue: Catch more locking problems with flush_work() in linux-next but not 3.4-rc, adding

	lock_map_acquire(&work->lockdep_map);
	lock_map_release(&work->lockdep_map);

to flush_work.

I believe that occasionally races with your

	struct lockdep_map lockdep_map = work->lockdep_map;

in process_one_work, putting an entry into the class_cache just as you're copying it, so you end up with half a pointer. yes, the structure copy is using "rep movsl" not "rep movsq".

I've reverted Stephen's commit from my testing, and indeed it's now run that MM load much longer than I've seen since this bug first appeared. Though I suspect that strictly it's your unlocked copying of the lockdep_map that's to blame. Probably easily fixed by someone who understands lockdep - not me!

Hugh

Tejun Heo

maintainer

2012-05-07 17:57 UTC #1 lore raw

(cc'ing Peter and Ingo and quoting whole body)

Quoted 22 lines · Hugh Dickins

Running MM load on recent linux-nexts (e.g. 3.4.0-rc5-next-20120504), with CONFIG_PROVE_LOCKING=y, I've been hitting an oops in __lock_acquire called from lock_acquire called from process_one_work: serving mm/swap.c's lru_add_drain_all - schedule_on_each_cpu(lru_add_drain_per_cpu).
In each case the oopsing address has been ffffffff00000198, and the oopsing instruction is the "atomic_inc((atomic_t *)&class->ops)" in __lock_acquire: so class is ffffffff00000000.
I notice Stephen's commit 0976dfc1d0cd80a4e9dfaf87bd8744612bde475a
workqueue: Catch more locking problems with flush_work() in linux-next but not 3.4-rc, adding
	lock_map_acquire(&work->lockdep_map);
	lock_map_release(&work->lockdep_map);
to flush_work.
I believe that occasionally races with your
	struct lockdep_map lockdep_map = work->lockdep_map;
in process_one_work, putting an entry into the class_cache just as you're copying it, so you end up with half a pointer. yes, the structure copy is using "rep movsl" not "rep movsq".

Quoted 5 lines

I've reverted Stephen's commit from my testing, and indeed it's now run that MM load much longer than I've seen since this bug first appeared. Though I suspect that strictly it's your unlocked copying of the lockdep_map that's to blame. Probably easily fixed by someone who understands lockdep - not me!

The offending commit is 0976dfc1d0cd80a4e9dfaf87bd8744612bde475a "workqueue: Catch more locking problems with flush_work()". It sounds fancy but all it does is adding the following to flush_work().

	lock_map_acquire(&work->lockdep_map);
	lock_map_release(&work->lockdep_map);

Which seems correct to me and more importantly not different from what wait_on_work() does, so if this is broken, flush_work_sync() and cancel_work_sync() are broken too - probably masked by lower usage frequency.

It seems the problem stems from how process_one_work() "caches" lockdep_map. This part predates cmwq changes but it seems necessary because the work item may be freed during execution but lockdep_map should be released after execution is complete. Peter, do you remember how this lockdep_map copying is added? Is (or was) this correct? If it's broken, how do we fix it? Add a lockdep_map copy API which does some magic lockdep locking dancing?

Thanks.

Signature · 1 line

tejun

Peter Zijlstra

maintainer

2012-05-08 13:03 UTC #2 lore raw

Quoted 24 lines · Tejun Heo

(cc'ing Peter and Ingo and quoting whole body)
Quoted 21 lines · Hugh Dickins
Running MM load on recent linux-nexts (e.g. 3.4.0-rc5-next-20120504), with CONFIG_PROVE_LOCKING=y, I've been hitting an oops in __lock_acquire called from lock_acquire called from process_one_work: serving mm/swap.c's lru_add_drain_all - schedule_on_each_cpu(lru_add_drain_per_cpu).
In each case the oopsing address has been ffffffff00000198, and the oopsing instruction is the "atomic_inc((atomic_t *)&class->ops)" in __lock_acquire: so class is ffffffff00000000.
I notice Stephen's commit 0976dfc1d0cd80a4e9dfaf87bd8744612bde475a
workqueue: Catch more locking problems with flush_work() in linux-next but not 3.4-rc, adding
	lock_map_acquire(&work->lockdep_map);
	lock_map_release(&work->lockdep_map);
to flush_work.
I believe that occasionally races with your
	struct lockdep_map lockdep_map = work->lockdep_map;
in process_one_work, putting an entry into the class_cache just as you're copying it, so you end up with half a pointer. yes, the structure copy is using "rep movsl" not "rep movsq".

But the copy is copying from work->lockdep_map, not to, so it doesn't matter does it? If anything would explode it would be the:

  lock_map_acquire(&lockdep_map);

because that's the target of the copy and could indeed observe a partial update (assuming the reported but silly "rep movsl").

Quoted 12 lines

The offending commit is 0976dfc1d0cd80a4e9dfaf87bd8744612bde475a "workqueue: Catch more locking problems with flush_work()". It sounds fancy but all it does is adding the following to flush_work().
	lock_map_acquire(&work->lockdep_map);
	lock_map_release(&work->lockdep_map);
Which seems correct to me and more importantly not different from what wait_on_work() does, so if this is broken, flush_work_sync() and cancel_work_sync() are broken too - probably masked by lower usage frequency.

Quoted 4 lines

It seems the problem stems from how process_one_work() "caches" lockdep_map. This part predates cmwq changes but it seems necessary because the work item may be freed during execution but lockdep_map should be released after execution is complete.

Exactly.

Quoted 4 lines

Peter, do you
remember how this lockdep_map copying is added? Is (or was) this correct? If it's broken, how do we fix it? Add a lockdep_map copy API which does some magic lockdep locking dancing?

I think there's a problem if indeed we do silly things like small copies like Hugh saw (why would gcc ever generate small copies for objects that are naturally aligned and naturally sized?).

Something like the below should fix that problem, but it doesn't explain the observed issue..

---
 include/linux/lockdep.h |   19 +++++++++++++++++++
 kernel/timer.c          |    2 +-
 kernel/workqueue.c      |    2 +-
 3 files changed, 21 insertions(+), 2 deletions(-)

include/linux/lockdep.h +19 -0

index d36619e..dc6661b 100644
@@ -157,6 +157,25 @@ struct lockdep_map {
 #endif
 };
 
+static inline struct lockdep_map lockdep_copy_map(struct lockdep_map *lock)
+{
+	struct lockdep_map _lock = *lock;
+	int i;
+
+	/*
+	 * Since the class cache can be modified concurrently we could observe
+	 * half pointers (64bit arch using 32bit copy insns). Therefore clear
+	 * the caches and take the performance hit.
+	 *
+	 * XXX it doesn't work well with lockdep_set_class_and_subclass(), since
+	 *     that relies on cache abuse.
+	 */
+	for (i = 0; i < NR_LOCKDEP_CACHING_CLASSES; i++)
+		_lock.class_cache[i] = NULL;
+
+	return _lock;
+}
+
 /*
  * Every lock has a list of other locks that were taken after it.
  * We only grow the list, never remove from it:

kernel/timer.c +1 -1

index a297ffc..fa98821 100644
@@ -1102,7 +1102,7 @@ static void call_timer_fn(struct timer_list *timer, void (*fn)(unsigned long),
 	 * warnings as well as problems when looking into
 	 * timer->lockdep_map, make a copy and use that here.
 	 */
-	struct lockdep_map lockdep_map = timer->lockdep_map;
+	struct lockdep_map lockdep_map = lockdep_map_copy(&timer->lockdep_map);
 #endif
 	/*
 	 * Couple the lock chain with the lock chain at

kernel/workqueue.c +1 -1

index 5abf42f..5d92b43 100644
@@ -1810,7 +1810,7 @@ __acquires(&gcwq->lock)
 	 * lock freed" warnings as well as problems when looking into
 	 * work->lockdep_map, make a copy and use that here.
 	 */
-	struct lockdep_map lockdep_map = work->lockdep_map;
+	struct lockdep_map lockdep_map = lockdep_copy_map(&work->lockdep_map);
 #endif
 	/*
 	 * A single work shouldn't be executed concurrently by

Tejun Heo

maintainer

2012-05-08 16:58 UTC #3 lore raw

Quoted 6 lines · Peter Zijlstra

I think there's a problem if indeed we do silly things like small copies like Hugh saw (why would gcc ever generate small copies for objects that are naturally aligned and naturally sized?).
Something like the below should fix that problem, but it doesn't explain the observed issue..

Hmmm.... Hugh, can you please verify whether this patch makes the problem go away somehow?

kernel/workqueue.c __acquires(&gcwq->lock) · line 1813

Quoted 6 lines

kernel/workqueue.c +1 -1

@@ -1810,7 +1810,7 @@ __acquires(&gcwq->lock)
 	 * lock freed" warnings as well as problems when looking into
 	 * work->lockdep_map, make a copy and use that here.
 	 */
-	struct lockdep_map lockdep_map = work->lockdep_map;
+	struct lockdep_map lockdep_map = lockdep_copy_map(&work->lockdep_map);

If this is the correct fix for whatever reason, maybe we want the copy interface to be a bit more conventional? lockdep_copy_map(to, from)?

Thanks.

Signature · 1 line

tejun

Peter Zijlstra

maintainer

2012-05-08 17:03 UTC #4 lore raw

Quoted 11 lines · Tejun Heo

Quoted 6 lines · Peter Zijlstra
I think there's a problem if indeed we do silly things like small copies like Hugh saw (why would gcc ever generate small copies for objects that are naturally aligned and naturally sized?).
Something like the below should fix that problem, but it doesn't explain the observed issue..
Hmmm.... Hugh, can you please verify whether this patch makes the problem go away somehow?

Quoted 9 lines

kernel/workqueue.c __acquires(&gcwq->lock) · line 1813
Quoted 6 lines
kernel/workqueue.c +1 -1
@@ -1810,7 +1810,7 @@ __acquires(&gcwq->lock)
 	 * lock freed" warnings as well as problems when looking into
 	 * work->lockdep_map, make a copy and use that here.
 	 */
-	struct lockdep_map lockdep_map = work->lockdep_map;
+	struct lockdep_map lockdep_map = lockdep_copy_map(&work->lockdep_map);
If this is the correct fix for whatever reason, maybe we want the copy interface to be a bit more conventional? lockdep_copy_map(to, from)?

Sure why not.. still not quite understanding the whole issue though.

Hugh Dickins

author

2012-05-08 18:12 UTC #5 lore raw

In reply to Tejun Heo

Quoted 10 lines · Tejun Heo

Quoted 6 lines · Peter Zijlstra
I think there's a problem if indeed we do silly things like small copies like Hugh saw (why would gcc ever generate small copies for objects that are naturally aligned and naturally sized?).
Something like the below should fix that problem, but it doesn't explain the observed issue..
Hmmm.... Hugh, can you please verify whether this patch makes the problem go away somehow?

Sure, but I won't start the run until tonight, and it'll then take a couple of days for us to be sure (sometimes it hit within the hour, sometimes it would take half a day).

Certainly the principle, cleaning out the cache, looked sound.

Quoted 10 lines

kernel/workqueue.c __acquires(&gcwq->lock) · line 1813
Quoted 7 lines
kernel/workqueue.c +1 -1
@@ -1810,7 +1810,7 @@ __acquires(&gcwq->lock)
 	 * lock freed" warnings as well as problems when looking into
 	 * work->lockdep_map, make a copy and use that here.
 	 */
-	struct lockdep_map lockdep_map = work->lockdep_map;
+	struct lockdep_map lockdep_map = lockdep_copy_map(&work->lockdep_map);
If this is the correct fix for whatever reason, maybe we want the copy interface to be a bit more conventional? lockdep_copy_map(to, from)?

Please send me the version of patch you'd like to put in (lest I make it up myself and you don't like the result).

Hugh

Peter Zijlstra

maintainer

2012-05-08 22:31 UTC #6 lore raw

Quoted 2 lines · Hugh Dickins

Please send me the version of patch you'd like to put in (lest I make it up myself and you don't like the result).

something like so?

---
 include/linux/lockdep.h |   17 +++++++++++++++++
 kernel/timer.c          |    4 +++-
 kernel/workqueue.c      |    4 +++-
 3 files changed, 23 insertions(+), 2 deletions(-)

include/linux/lockdep.h +17 -0

index d36619e..968c3e2 100644
@@ -157,6 +157,23 @@ struct lockdep_map {
 #endif
 };
 
+static inline void lockdep_copy_map(struct lockdep_map *to struct lockdep_map *from)
+{
+	int i;
+
+	*to = *from;
+	/*
+	 * Since the class cache can be modified concurrently we could observe
+	 * half pointers (64bit arch using 32bit copy insns). Therefore clear
+	 * the caches and take the performance hit.
+	 *
+	 * XXX it doesn't work well with lockdep_set_class_and_subclass(), since
+	 *     that relies on cache abuse.
+	 */
+	for (i = 0; i < NR_LOCKDEP_CACHING_CLASSES; i++)
+		to->class_cache[i] = NULL;
+}
+
 /*
  * Every lock has a list of other locks that were taken after it.
  * We only grow the list, never remove from it:

kernel/timer.c +3 -1

index a297ffc..6aa7ad8 100644
@@ -1102,7 +1102,9 @@ static void call_timer_fn(struct timer_list *timer, void (*fn)(unsigned long),
 	 * warnings as well as problems when looking into
 	 * timer->lockdep_map, make a copy and use that here.
 	 */
-	struct lockdep_map lockdep_map = timer->lockdep_map;
+	struct lockdep_map lockdep_map;
+       
+	lockdep_copy_map(&lockdep_map, &timer->lockdep_map);
 #endif
 	/*
 	 * Couple the lock chain with the lock chain at

kernel/workqueue.c +3 -1

index 5abf42f..7d77d1f 100644
@@ -1810,7 +1810,9 @@ __acquires(&gcwq->lock)
 	 * lock freed" warnings as well as problems when looking into
 	 * work->lockdep_map, make a copy and use that here.
 	 */
-	struct lockdep_map lockdep_map = work->lockdep_map;
+	struct lockdep_map lockdep_map;
+       
+	lockdep_copy_map(&lockdep_map, &work->lockdep_map);
 #endif
 	/*
 	 * A single work shouldn't be executed concurrently by

Hugh Dickins

author

2012-05-08 22:59 UTC #7 lore raw

Quoted 5 lines · Peter Zijlstra

Quoted 2 lines · Hugh Dickins
Please send me the version of patch you'd like to put in (lest I make it up myself and you don't like the result).
something like so?

More like the one below: I'm not alone in preferring a comma between args!

And you're not a believer in checkpatch.pl, I see: I've removed trailing spaces; but left the 85-col line, that's not a fight I'll have with you.

I'll set it going when I get home later - thanks.

Hugh

---
 include/linux/lockdep.h |   17 +++++++++++++++++
 kernel/timer.c          |    4 +++-
 kernel/workqueue.c      |    4 +++-
 3 files changed, 23 insertions(+), 2 deletions(-)

include/linux/lockdep.h +17 -0

index d36619e..968c3e2 100644
@@ -157,6 +157,23 @@ struct lockdep_map {
 #endif
 };
 
+static inline void lockdep_copy_map(struct lockdep_map *to, struct lockdep_map *from)
+{
+	int i;
+
+	*to = *from;
+	/*
+	 * Since the class cache can be modified concurrently we could observe
+	 * half pointers (64bit arch using 32bit copy insns). Therefore clear
+	 * the caches and take the performance hit.
+	 *
+	 * XXX it doesn't work well with lockdep_set_class_and_subclass(), since
+	 *     that relies on cache abuse.
+	 */
+	for (i = 0; i < NR_LOCKDEP_CACHING_CLASSES; i++)
+		to->class_cache[i] = NULL;
+}
+
 /*
  * Every lock has a list of other locks that were taken after it.
  * We only grow the list, never remove from it:

kernel/timer.c +3 -1

index a297ffc..6aa7ad8 100644
@@ -1102,7 +1102,9 @@ static void call_timer_fn(struct timer_list *timer, void (*fn)(unsigned long),
 	 * warnings as well as problems when looking into
 	 * timer->lockdep_map, make a copy and use that here.
 	 */
-	struct lockdep_map lockdep_map = timer->lockdep_map;
+	struct lockdep_map lockdep_map;
+
+	lockdep_copy_map(&lockdep_map, &timer->lockdep_map);
 #endif
 	/*
 	 * Couple the lock chain with the lock chain at

kernel/workqueue.c +3 -1

index 5abf42f..7d77d1f 100644
@@ -1810,7 +1810,9 @@ __acquires(&gcwq->lock)
 	 * lock freed" warnings as well as problems when looking into
 	 * work->lockdep_map, make a copy and use that here.
 	 */
-	struct lockdep_map lockdep_map = work->lockdep_map;
+	struct lockdep_map lockdep_map;
+
+	lockdep_copy_map(&lockdep_map, &work->lockdep_map);
 #endif
 	/*
 	 * A single work shouldn't be executed concurrently by

Ingo Molnar

2012-05-09 09:25 UTC #8 lore raw

Quoted 9 lines · Hugh Dickins

Quoted 5 lines · Peter Zijlstra
Quoted 2 lines · Hugh Dickins
Please send me the version of patch you'd like to put in (lest I make it up myself and you don't like the result).
something like so?
More like the one below: I'm not alone in preferring a comma between args!

Silly compilers!

Quoted 2 lines

And you're not a believer in checkpatch.pl, I see: I've removed trailing spaces; but left the 85-col line, that's not a fight I'll have with you.

I suspect we could break up the prototype like this:

static inline void
lockdep_copy_map(struct lockdep_map *to, struct lockdep_map *from)

Quoted 1 line

I'll set it going when I get home later - thanks.

Do we still need an explanation about why it's needed and why it makes a difference?

Thanks,

Ingo

Hugh Dickins

author

2012-05-09 20:09 UTC #9 lore raw

Quoted 3 lines · Ingo Molnar

Quoted 1 line · Hugh Dickins
I'll set it going when I get home later - thanks.

Going fine so far, but a more convincing final report tomorrow.

Quoted 3 lines

Do we still need an explanation about why it's needed and why it makes a difference?

I don't see the difficulty in understanding it. Peter didn't comment whether my further explanations convinced him or not. Or perhaps you're asking for some commit description text - I may not be the right person to write it, since I didn't make myself understood very well, but here's a go.

lockdep: fix oops in processing workqueue

Under memory load, on x86_64, with lockdep enabled, the workqueue's process_one_work() has been seen to oops in __lock_acquire(), barfing on a 0xffffffff00000000 pointer in the lockdep_map's class_cache[].

Because it's permissible to free a work_struct from its callout function, the map used is an onstack copy of the map given in the work_struct: and that copy is made without any locking.

Surprisingly, gcc (4.5.1 in Hugh's case) uses "rep movsl" rather than "rep movsq" for that structure copy: which might race with a workqueue user's wait_on_work() doing lock_map_acquire() on the source of the copy, putting a pointer into the class_cache[], but only in time for the top half of that pointer to be copied to the destination map.

Boom when process_one_work() subsequently does lock_map_acquire() on its onstack copy of the lockdep_map.

Fix this, and a similar instance in call_timer_fn(), with a lockdep_copy_map() function which additionally NULLs the class_cache[].

Note: this oops was actually seen on 3.4-next, where flush_work() newly does the racing lock_map_acquire(); but Tejun points out that 3.4 and earlier are already vulnerable to the same through wait_on_work().

Hugh

Hugh Dickins

author

2012-05-10 17:53 UTC #10 lore raw

Quoted 6 lines · Hugh Dickins

Quoted 3 lines · Ingo Molnar
Quoted 1 line · Hugh Dickins
I'll set it going when I get home later - thanks.
Going fine so far, but a more convincing final report tomorrow.

Yes, as expected, Peter's patch (+ ,) is good: that load ran four times longer than the longest it managed before without the patch. Just needs Peter's signoff, I think.

Hugh

Tejun Heo

maintainer

2012-05-14 21:28 UTC #11 lore raw

Quoted 11 lines · Hugh Dickins

Quoted 6 lines · Hugh Dickins
Quoted 3 lines · Ingo Molnar
Quoted 1 line · Hugh Dickins
I'll set it going when I get home later - thanks.
Going fine so far, but a more convincing final report tomorrow.
Yes, as expected, Peter's patch (+ ,) is good: that load ran four times longer than the longest it managed before without the patch. Just needs Peter's signoff, I think.

Peter, can I take your patch with Hugh's description w/ your SOB?

Thanks.

Signature · 1 line

tejun

Peter Zijlstra

maintainer

2012-05-15 11:11 UTC #12 lore raw

Quoted 7 lines · Tejun Heo

Quoted 6 lines · Hugh Dickins
Quoted 5 lines · Hugh Dickins
Quoted 3 lines · Ingo Molnar
Quoted 1 line · Hugh Dickins
I'll set it going when I get home later - thanks.

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
Going fine so far, but a more convincing final report tomorrow.
Yes, as expected, Peter's patch (+ ,) is good: that load ran four times longer than the longest it managed before without the patch. Just needs Peter's signoff, I think.
Peter, can I take your patch with Hugh's description w/ your SOB?

Yes, sorry, I meant to queue it up the lockdep tree but got distracted by other stuff.

If you want it, feel tree to take it.

Tejun Heo

maintainer

2012-05-15 15:11 UTC #13 lore raw

PATCH 1/1 lockdep: fix oops in processing workqueue

Quoted 1 line

From 4d82a1debbffec129cc387aafa8f40b7bbab3297 Mon Sep 17 00:00:00 2001

From: Peter Zijlstra <[email protected]>
Date: Tue, 15 May 2012 08:06:19 -0700

Because it's permissible to free a work_struct from its callout function, the map used is an onstack copy of the map given in the work_struct: and that copy is made without any locking.

Boom when process_one_work() subsequently does lock_map_acquire() on its onstack copy of the lockdep_map.

Fix this, and a similar instance in call_timer_fn(), with a lockdep_copy_map() function which additionally NULLs the class_cache[].

* Patch orginally from Peter. Hugh modified it a bit and wrote the description.

Signed-off-by Peter Zijlstra <[email protected]>

Reported-by Hugh Dickins <[email protected]>

LKML-Reference: <[email protected]>

Signed-off-by Tejun Heo <[email protected]>

---
Applied to wq/for-3.5 with the function decl broken in traditional way.

Thanks.

 include/linux/lockdep.h |   18 ++++++++++++++++++
 kernel/timer.c          |    4 +++-
 kernel/workqueue.c      |    4 +++-
 3 files changed, 24 insertions(+), 2 deletions(-)

include/linux/lockdep.h +18 -0

index d36619e..00e4637 100644
@@ -157,6 +157,24 @@ struct lockdep_map {
 #endif
 };
 
+static inline void lockdep_copy_map(struct lockdep_map *to,
+				    struct lockdep_map *from)
+{
+	int i;
+
+	*to = *from;
+	/*
+	 * Since the class cache can be modified concurrently we could observe
+	 * half pointers (64bit arch using 32bit copy insns). Therefore clear
+	 * the caches and take the performance hit.
+	 *
+	 * XXX it doesn't work well with lockdep_set_class_and_subclass(), since
+	 *     that relies on cache abuse.
+	 */
+	for (i = 0; i < NR_LOCKDEP_CACHING_CLASSES; i++)
+		to->class_cache[i] = NULL;
+}
+
 /*
  * Every lock has a list of other locks that were taken after it.
  * We only grow the list, never remove from it:

kernel/timer.c +3 -1

index a297ffc..b123852 100644
@@ -1102,7 +1102,9 @@ static void call_timer_fn(struct timer_list *timer, void (*fn)(unsigned long),
 	 * warnings as well as problems when looking into
 	 * timer->lockdep_map, make a copy and use that here.
 	 */
-	struct lockdep_map lockdep_map = timer->lockdep_map;
+	struct lockdep_map lockdep_map;
+
+	lockdep_copy_map(&lockdep_map, &timer->lockdep_map);
 #endif
 	/*
 	 * Couple the lock chain with the lock chain at

kernel/workqueue.c +3 -1

index c36c86c..9a3128d 100644
@@ -1818,7 +1818,9 @@ __acquires(&gcwq->lock)
 	 * lock freed" warnings as well as problems when looking into
 	 * work->lockdep_map, make a copy and use that here.
 	 */
-	struct lockdep_map lockdep_map = work->lockdep_map;
+	struct lockdep_map lockdep_map;
+
+	lockdep_copy_map(&lockdep_map, &work->lockdep_map);
 #endif
 	/*
 	 * A single work shouldn't be executed concurrently by

Signature · 1 line

1.7.7.3

Dave Jones

2012-05-15 15:30 UTC #14 lore raw

Re: [PATCH] lockdep: fix oops in processing workqueue

Quoted 7 lines · Tejun Heo

Quoted 1 line
From 4d82a1debbffec129cc387aafa8f40b7bbab3297 Mon Sep 17 00:00:00 2001
From: Peter Zijlstra <[email protected]>
Date: Tue, 15 May 2012 08:06:19 -0700
Under memory load, on x86_64, with lockdep enabled, the workqueue's process_one_work() has been seen to oops in __lock_acquire(), barfing on a 0xffffffff00000000 pointer in the lockdep_map's class_cache[].

can you elaborate what 'memory load' means here ?
I'm curious if I can add something to my fuzzing tool to shake out bugs like this.

Dave

Tejun Heo

maintainer

2012-05-15 15:31 UTC #15 lore raw

Re: [PATCH] lockdep: fix oops in processing workqueue

Quoted 11 lines · Dave Jones

Quoted 7 lines · Tejun Heo
Quoted 1 line
From 4d82a1debbffec129cc387aafa8f40b7bbab3297 Mon Sep 17 00:00:00 2001
From: Peter Zijlstra <[email protected]>
Date: Tue, 15 May 2012 08:06:19 -0700
Under memory load, on x86_64, with lockdep enabled, the workqueue's process_one_work() has been seen to oops in __lock_acquire(), barfing on a 0xffffffff00000000 pointer in the lockdep_map's class_cache[].
can you elaborate what 'memory load' means here ?
I'm curious if I can add something to my fuzzing tool to shake out bugs like this.

I think Hugh knows and can explain this much better than I do. Hugh?

Thanks.

Signature · 1 line

tejun

Hugh Dickins

author

2012-05-15 20:37 UTC #16 lore raw

Re: [PATCH] lockdep: fix oops in processing workqueue

Quoted 6 lines · Tejun Heo

Quoted 5 lines · Dave Jones
Quoted 4 lines · Tejun Heo
Quoted 1 line
From 4d82a1debbffec129cc387aafa8f40b7bbab3297 Mon Sep 17 00:00:00 2001
From: Peter Zijlstra <[email protected]>
Date: Tue, 15 May 2012 08:06:19 -0700

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
Under memory load, on x86_64, with lockdep enabled, the workqueue's process_one_work() has been seen to oops in __lock_acquire(), barfing on a 0xffffffff00000000 pointer in the lockdep_map's class_cache[].
can you elaborate what 'memory load' means here ?
I'm curious if I can add something to my fuzzing tool to shake out bugs like this.
I think Hugh knows and can explain this much better than I do. Hugh?

Quoting from myself, quoting from myself, on an earlier occasion:

"It's the tmpfs swapping test that I've been running, with variations, for years. System booted with mem=700M and 1.5G swap, two repetitious make -j20 kernel builds (of a 2.6.24 kernel: I stuck with that because the balance of built to unbuilt source grows smaller with later kernels), one directly in a tmpfs, the other in a 1k-block ext2 (that I drive with ext4's CONFIG_EXT4_USE_FOR_EXT23) on /dev/loop0 on a 450MB tmpfs file."

Most of those details will be irrelevant in this case, but it's been a useful test down the years, catching lots of bugs and races. On this occasion I was running a variation which further puts each of the builds in its own 300M mem cgroup, with a concurrent script which cycles around making a new 300M mem cgroup, moving all the tasks from one old into the new (with memory.move_charge_at_immigrate set to 3), then rmdir the old.

It was probably the moving of memcg charges (or the rmdir'ing of memcg, which also involves moving memcg charges) which was making so many calls to lru_add_drain_all() to show the problem - lru_add_drain_all() has to schedule work on each cpu and then flush_work() on each.

(But it only came up as a problem in linux-next, which has added some lockmap accesses to flush_work() - Tejun spotted that upstream could already be vulnerable via other routes, but this was all I ever hit.)

Hugh

Hugh Dickins

author

2012-05-08 18:06 UTC #17 lore raw

In reply to Peter Zijlstra

Quoted 20 lines · Peter Zijlstra

Quoted 19 lines · Tejun Heo
(cc'ing Peter and Ingo and quoting whole body)
Quoted 16 lines · Hugh Dickins
Running MM load on recent linux-nexts (e.g. 3.4.0-rc5-next-20120504), with CONFIG_PROVE_LOCKING=y, I've been hitting an oops in __lock_acquire called from lock_acquire called from process_one_work: serving mm/swap.c's lru_add_drain_all - schedule_on_each_cpu(lru_add_drain_per_cpu).
In each case the oopsing address has been ffffffff00000198, and the oopsing instruction is the "atomic_inc((atomic_t *)&class->ops)" in __lock_acquire: so class is ffffffff00000000.
I notice Stephen's commit 0976dfc1d0cd80a4e9dfaf87bd8744612bde475a
workqueue: Catch more locking problems with flush_work() in linux-next but not 3.4-rc, adding
	lock_map_acquire(&work->lockdep_map);
	lock_map_release(&work->lockdep_map);
to flush_work.

Quoted 8 lines

Quoted 5 lines
Quoted 5 lines
I believe that occasionally races with your
	struct lockdep_map lockdep_map = work->lockdep_map;
in process_one_work, putting an entry into the class_cache just as you're copying it, so you end up with half a pointer. yes, the structure copy is using "rep movsl" not "rep movsq".
But the copy is copying from work->lockdep_map, not to, so it doesn't matter does it?

It doesn't matter to work->lockdep_map, it matters to the lockdep_map on process_one_work()'s stack.

Quoted 3 lines

If anything would explode it would be the:
  lock_map_acquire(&lockdep_map);

Yes, on line 1867 of the rc5-next-2012504 kernel/workqueue.c (line 1864 in rc6). I have not noted down the offset in process_one_work() at which it crashed (calling lock_acquire calling __lock_acquire), so cannot reconfirm, but I believe that's the lock_acquire() I tracked it to.

Quoted 3 lines

because that's the target of the copy and could indeed observe a partial update (assuming the reported but silly "rep movsl").

When the racing copy is done earlier with "rep movsl", the bogus pointer ffffffff00000000 is put in class_cache[N]. Then when it comes to lock_map_acquire(&lockdep_map) here, it oopses on it.

Do you see it now? (It's always hard to understand misunderstandings ;)

Quoted 20 lines

Quoted 13 lines
The offending commit is 0976dfc1d0cd80a4e9dfaf87bd8744612bde475a "workqueue: Catch more locking problems with flush_work()". It sounds fancy but all it does is adding the following to flush_work().
	lock_map_acquire(&work->lockdep_map);
	lock_map_release(&work->lockdep_map);
Which seems correct to me and more importantly not different from what wait_on_work() does, so if this is broken, flush_work_sync() and cancel_work_sync() are broken too - probably masked by lower usage frequency.
Quoted 4 lines
It seems the problem stems from how process_one_work() "caches" lockdep_map. This part predates cmwq changes but it seems necessary because the work item may be freed during execution but lockdep_map should be released after execution is complete.
Exactly.

Quoted 8 lines

Quoted 4 lines
Peter, do you
remember how this lockdep_map copying is added? Is (or was) this correct? If it's broken, how do we fix it? Add a lockdep_map copy API which does some magic lockdep locking dancing?
I think there's a problem if indeed we do silly things like small copies like Hugh saw (why would gcc ever generate small copies for objects that are naturally aligned and naturally sized?).

I don't know. gcc 4.5.1 here. The structure is only 32 bytes, maybe there's some advantage to copying that with movl rather than movq.

Quoted 15 lines

Something like the below should fix that problem, but it doesn't explain the observed issue..
---
 include/linux/lockdep.h |   19 +++++++++++++++++++
 kernel/timer.c          |    2 +-
 kernel/workqueue.c      |    2 +-
 3 files changed, 21 insertions(+), 2 deletions(-)
include/linux/lockdep.h +0 -0
index d36619e..dc6661b 100644

include/linux/lockdep.h struct lockdep_map { · line 160

Quoted 4 lines

include/linux/lockdep.h +1 -0

@@ -157,6 +157,25 @@ struct lockdep_map {
 #endif
 };
 
+static inline struct lockdep_map lockdep_copy_map(struct lockdep_map *lock)

Does that "inline" need to be "__this_will_go_very_horribly_wrong_if_not_inline"?

Ah, looks like Tejun suggests a more conventional interface in his reply.

Quoted 47 lines

include/linux/lockdep.h +18 -0

+{
+	struct lockdep_map _lock = *lock;
+	int i;
+
+	/*
+	 * Since the class cache can be modified concurrently we could observe
+	 * half pointers (64bit arch using 32bit copy insns). Therefore clear
+	 * the caches and take the performance hit.
+	 *
+	 * XXX it doesn't work well with lockdep_set_class_and_subclass(), since
+	 *     that relies on cache abuse.
+	 */
+	for (i = 0; i < NR_LOCKDEP_CACHING_CLASSES; i++)
+		_lock.class_cache[i] = NULL;
+
+	return _lock;
+}
+
 /*
  * Every lock has a list of other locks that were taken after it.
  * We only grow the list, never remove from it:

kernel/timer.c +1 -1

index a297ffc..fa98821 100644
@@ -1102,7 +1102,7 @@ static void call_timer_fn(struct timer_list *timer, void (*fn)(unsigned long),
 	 * warnings as well as problems when looking into
 	 * timer->lockdep_map, make a copy and use that here.
 	 */
-	struct lockdep_map lockdep_map = timer->lockdep_map;
+	struct lockdep_map lockdep_map = lockdep_map_copy(&timer->lockdep_map);
 #endif
 	/*
 	 * Couple the lock chain with the lock chain at

kernel/workqueue.c +1 -1

index 5abf42f..5d92b43 100644
@@ -1810,7 +1810,7 @@ __acquires(&gcwq->lock)
 	 * lock freed" warnings as well as problems when looking into
 	 * work->lockdep_map, make a copy and use that here.
 	 */
-	struct lockdep_map lockdep_map = work->lockdep_map;
+	struct lockdep_map lockdep_map = lockdep_copy_map(&work->lockdep_map);
 #endif
 	/*
 	 * A single work shouldn't be executed concurrently by