Re: [RFC 0/2] Reenable might_sleep() checks for might_fault() when atomic

[David Hildenbrand <dahi@linux.vnet.ibm.com>]

> > This is what happened on our side (very recent kernel):
> > 
> > spin_lock(&lock)
> > copy_to_user(...)
> > spin_unlock(&lock)
> 
> That's a deadlock even without copy_to_user - it's
> enough for the thread to be preempted and another one
> to try taking the lock.
> 
> 
> > 1. s390 locks/unlocks a spin lock with a compare and swap, using the _cpu id_
> >    as "old value"
> > 2. we slept during copy_to_user()
> > 3. the thread got scheduled onto another cpu
> > 4. spin_unlock failed as the _cpu id_ didn't match (another cpu that locked
> >    the spinlock tried to unlocked it).
> > 5. lock remained locked -> deadlock
> > 
> > Christian came up with the following explanation:
> > Without preemption, spin_lock() will not touch the preempt counter.
> > disable_pfault() will always touch it.
> > 
> > Therefore, with preemption disabled, copy_to_user() has no idea that it is
> > running in atomic context - and will therefore try to sleep.
> >
> > So copy_to_user() will on s390:
> > 1. run "as atomic" while spin_lock() with preemption enabled.
> > 2. run "as not atomic" while spin_lock() with preemption disabled.
> > 3.  run "as atomic" while pagefault_disabled() with preemption enabled or
> > disabled.
> > 4. run "as not atomic" when really not atomic.

should have been more clear at that point: 
preemption enabled == kernel compiled with preemption support
preemption disabled == kernel compiled without preemption support

> > 
> > And exactly nr 2. is the thing that produced the deadlock in our scenario and
> > the reason why I want a might_sleep() :)
> 
> IMHO it's not copy to user that causes the problem.
> It's the misuse of spinlocks with preemption on.

As I said, preemption was off.

> 
> So might_sleep would make you think copy_to_user is
> the problem, and e.g. let you paper over it by
> moving copy_to_user out.

Actually implementing different way of locking easily fixed the problem for us.
The old might_sleep() checks would have given us the problem within a few
seconds (I tested it).

> 
> Enable lock prover and you will see what the real
> issue is, which is you didn't disable preempt.
> and if you did, copy_to_user would be okay.
> 

Our kernel is compiled without preemption and we turned on all lock/atomic
sleep debugging aid. No problem was detected.

----
But the question is if we shouldn't rather provide a:

  copy_to_user_nosleep() implementation that can be called from
    pagefault_disable() because it won't sleep.
and a
  copy_to_user_sleep() implementation that cannot be called from
    pagefault_disable().

Another way to fix it would be a reworked pagefault_disable() function that
somehow sets "a flag", so copy_to_user() knows that it is in fact called from a
valid context, not just from "some atomic" context. So we could trigger
might_sleep() when detecting a !pagefault_disable context.