linux-next oops in __lock_acquire for process_one_work

linux-next oops in __lock_acquire for process_one_work

[Hugh Dickins]

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

Re: linux-next oops in __lock_acquire for process_one_work

[Tejun Heo]

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

On Mon, May 07, 2012 at 10:19:09AM -0700, Hugh Dickins wrote:
> 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!

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.

-- 
tejun

Re: linux-next oops in __lock_acquire for process_one_work

[Peter Zijlstra]

On Mon, 2012-05-07 at 10:57 -0700, Tejun Heo wrote:
> (cc'ing Peter and Ingo and quoting whole body)
> 
> On Mon, May 07, 2012 at 10:19:09AM -0700, Hugh Dickins wrote:
> > 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").

> 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.

Exactly.

>   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(-)

diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index d36619e..dc6661b 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -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:
diff --git a/kernel/timer.c b/kernel/timer.c
index a297ffc..fa98821 100644
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -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
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 5abf42f..5d92b43 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -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

Re: linux-next oops in __lock_acquire for process_one_work

[Tejun Heo]

On Tue, May 08, 2012 at 03:03:22PM +0200, Peter Zijlstra wrote:
> 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?

> @@ -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.

-- 
tejun

Re: linux-next oops in __lock_acquire for process_one_work

[Peter Zijlstra]

On Tue, 2012-05-08 at 09:58 -0700, Tejun Heo wrote:
> On Tue, May 08, 2012 at 03:03:22PM +0200, Peter Zijlstra wrote:
> > 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?
> 
> > @@ -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.

Re: linux-next oops in __lock_acquire for process_one_work

[Hugh Dickins]

On Tue, 8 May 2012, Peter Zijlstra wrote:
> On Mon, 2012-05-07 at 10:57 -0700, Tejun Heo wrote:
> > (cc'ing Peter and Ingo and quoting whole body)
> > 
> > On Mon, May 07, 2012 at 10:19:09AM -0700, Hugh Dickins wrote:
> > > 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?

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

> 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.

> 
> 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 ;)

> 
> > 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.
> 
> Exactly.
> 
> >   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.

> 
> 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(-)
> 
> diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
> index d36619e..dc6661b 100644
> --- a/include/linux/lockdep.h
> +++ b/include/linux/lockdep.h
> @@ -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.

> +{
> +	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:
> diff --git a/kernel/timer.c b/kernel/timer.c
> index a297ffc..fa98821 100644
> --- a/kernel/timer.c
> +++ b/kernel/timer.c
> @@ -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
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 5abf42f..5d92b43 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -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

Re: linux-next oops in __lock_acquire for process_one_work

[Hugh Dickins]

On Tue, 8 May 2012, Tejun Heo wrote:
> On Tue, May 08, 2012 at 03:03:22PM +0200, Peter Zijlstra wrote:
> > 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.

> 
> > @@ -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

Re: linux-next oops in __lock_acquire for process_one_work

[Peter Zijlstra]

On Tue, 2012-05-08 at 11:11 -0700, Hugh Dickins wrote:
> 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(-)

diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index d36619e..968c3e2 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -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:
diff --git a/kernel/timer.c b/kernel/timer.c
index a297ffc..6aa7ad8 100644
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -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
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 5abf42f..7d77d1f 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -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

Re: linux-next oops in __lock_acquire for process_one_work

[Hugh Dickins]

On Wed, 9 May 2012, Peter Zijlstra wrote:
> On Tue, 2012-05-08 at 11:11 -0700, Hugh Dickins wrote:
> > 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(-)

diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index d36619e..968c3e2 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -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:
diff --git a/kernel/timer.c b/kernel/timer.c
index a297ffc..6aa7ad8 100644
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -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
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 5abf42f..7d77d1f 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -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

Re: linux-next oops in __lock_acquire for process_one_work

[Ingo Molnar]

* Hugh Dickins <hughd@google.com> wrote:

> On Wed, 9 May 2012, Peter Zijlstra wrote:
> > On Tue, 2012-05-08 at 11:11 -0700, Hugh Dickins wrote:
> > > 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!

> 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)

> 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

Re: linux-next oops in __lock_acquire for process_one_work

[Hugh Dickins]

On Wed, 9 May 2012, Ingo Molnar wrote:
> * Hugh Dickins <hughd@google.com> wrote:
> 
> > I'll set it going when I get home later - thanks.

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

> 
> 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

Re: linux-next oops in __lock_acquire for process_one_work

[Hugh Dickins]

On Wed, 9 May 2012, Hugh Dickins wrote:
> On Wed, 9 May 2012, Ingo Molnar wrote:
> > * Hugh Dickins <hughd@google.com> wrote:
> > 
> > > 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

Re: linux-next oops in __lock_acquire for process_one_work

[Tejun Heo]

On Thu, May 10, 2012 at 10:52:46AM -0700, Hugh Dickins wrote:
> On Wed, 9 May 2012, Hugh Dickins wrote:
> > On Wed, 9 May 2012, Ingo Molnar wrote:
> > > * Hugh Dickins <hughd@google.com> wrote:
> > > 
> > > > 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.

-- 
tejun

Re: linux-next oops in __lock_acquire for process_one_work

[Peter Zijlstra]

On Mon, 2012-05-14 at 14:27 -0700, Tejun Heo wrote:
> On Thu, May 10, 2012 at 10:52:46AM -0700, Hugh Dickins wrote:
> > On Wed, 9 May 2012, Hugh Dickins wrote:
> > > On Wed, 9 May 2012, Ingo Molnar wrote:
> > > > * Hugh Dickins <hughd@google.com> wrote:
> > > > 
> > > > > 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?

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.

[PATCH] lockdep: fix oops in processing workqueue

[Tejun Heo]

>From 4d82a1debbffec129cc387aafa8f40b7bbab3297 Mon Sep 17 00:00:00 2001
From: Peter Zijlstra <peterz@infradead.org>
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[].

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().

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

Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Reported-by: Hugh Dickins <hughd@google.com>
LKML-Reference: <alpine.LSU.2.00.1205070951170.1544@eggly.anvils>
Signed-off-by: Tejun Heo <tj@kernel.org>
---
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(-)

diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index d36619e..00e4637 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -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:
diff --git a/kernel/timer.c b/kernel/timer.c
index a297ffc..b123852 100644
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -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
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index c36c86c..9a3128d 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -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
-- 
1.7.7.3

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

[Dave Jones]

On Tue, May 15, 2012 at 08:10:48AM -0700, Tejun Heo wrote:
 > >From 4d82a1debbffec129cc387aafa8f40b7bbab3297 Mon Sep 17 00:00:00 2001
 > From: Peter Zijlstra <peterz@infradead.org>
 > 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

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

[Tejun Heo]

On Tue, May 15, 2012 at 11:29:52AM -0400, Dave Jones wrote:
> On Tue, May 15, 2012 at 08:10:48AM -0700, Tejun Heo wrote:
>  > >From 4d82a1debbffec129cc387aafa8f40b7bbab3297 Mon Sep 17 00:00:00 2001
>  > From: Peter Zijlstra <peterz@infradead.org>
>  > 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.

-- 
tejun

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

[Hugh Dickins]

On Tue, 15 May 2012, Tejun Heo wrote:
> On Tue, May 15, 2012 at 11:29:52AM -0400, Dave Jones wrote:
> > On Tue, May 15, 2012 at 08:10:48AM -0700, Tejun Heo wrote:
> >  > >From 4d82a1debbffec129cc387aafa8f40b7bbab3297 Mon Sep 17 00:00:00 2001
> >  > From: Peter Zijlstra <peterz@infradead.org>
> >  > 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?

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