Re: BUG: spinlock bad magic on CPU#0, migration/0/9

[Nicholas Mc Guire]

On Thu, 12 Feb 2015, Oleg Nesterov wrote:

> Nicholas, sorry, I sent the patch but forgot to CC you.
> See https://lkml.org/lkml/2015/2/12/587
> 
> And please note that "completion" was specially designed to guarantee
> that complete() can't play with this memory after wait_for_completion/etc
> returns.
>

hmmm.... I guess that "falling out of context" can happen in a number of cases 
with completion - any of the timeout/interruptible variants e.g:

	void xxx(void)
	{
		struct completion c;

		init_completion(&c);

		expose_this_completion(&c);

		wait_for_completion_timeout(&c,A_FEW_JIFFIES);
	}

and if the other side did not call complete() within A_FEW_JIFFIES then
it would result in the same failure - I don't think the API can prevent 
this type of bug. Tt has to be ensured by additional locking e.g:
drivers/misc/tifm_7xx1.c:tifm_7xx1_resume() resolve this issue by resetting
the completion to NULL and testing for !NULL before calling complete() 
with appropriate locking protection access.

Never the less of course the proposed change in completion_done() was a bug - 
many thanks for catching that so quickly !
 
> On 02/12, Oleg Nesterov wrote:
> >
> > On 02/12, Nicholas Mc Guire wrote:
> > >
> > > On Thu, 12 Feb 2015, Oleg Nesterov wrote:
> > >
> > > > No, sorry, only the 2nd one.
> > > >
> > > > > Unless at least document how
> > > > > you can use these helpers.
> > > > >
> > > > > Consider this code:
> > > > >
> > > > > 	void xxx(void)
> > > > > 	{
> > > > > 		struct completion c;
> > > > >
> > > > > 		init_completion(&c);
> > > > >
> > > > > 		expose_this_completion(&c);
> > > > >
> > > > > 		while (!completion_done(&c)
> > > > > 			schedule_timeout_uninterruptible(1);
> > >
> > > But that would not break due to the change - even if completion_done() had a
> > > problem - complete_done() is not consuming x->done it is only checking it?
> > 
> > Nicholas, looks like you didn't read the text below:
> > 
> > > > > Before that change this code was correct, now it is not. Hmm and note that
> > > > > this is what stop_machine_from_inactive_cpu() does although I do not know
> > > > > if this is related or not.
> > > > >
> > > > > Because completion_done() can now race with complete(), the final
> > > > > spin_unlock() can write to the memory after it was freed/reused. In this
> > > > > case it can write to the stack after return.
> > 
> > Or I misunderstood you.
> > 
> > > >  bool completion_done(struct completion *x)
> > > >  {
> > > > -	return !!ACCESS_ONCE(x->done);
> > > > +	if (!READ_ONCE(x->done))
> > > > +		return false;
> > > > +
> > > > +	smp_rmb();
> > > > +	spin_unlock_wait(&x->wait.lock);
> > > > +	return true;
> > >
> > > what would be the sense of the spin_unlock_wait here ?
> > > all you are interested in here is the state of x->done
> > 
> > No. Please see above.
> > 
> > > regarding the smp_rmb() where would the counterpart to that be ?
> > 
> > to avoid the reordering, we should not read ->wait.lock before ->done.
> > 
> > Oleg.
>

thx!
hofrat 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/