Problem with WARN_ON in mutex_trylock() and rxrpc

Problem with WARN_ON in mutex_trylock() and rxrpc

[David Howells]

Hi Davidlohr,

commit a0855d24fc22d49cdc25664fb224caee16998683 ("locking/mutex: Complain upon
mutex API misuse in IRQ contexts") is a bit of a problem for rxrpc, though
nothing that shouldn't be reasonably easy to solve, I think.

What happens is that rxrpc_new_incoming_call(), which is called in softirq
context, calls mutex_trylock() to prelock a new incoming call:

	/* Lock the call to prevent rxrpc_kernel_send/recv_data() and
	 * sendmsg()/recvmsg() inconveniently stealing the mutex once the
	 * notification is generated.
	 *
	 * The BUG should never happen because the kernel should be well
	 * behaved enough not to access the call before the first notification
	 * event and userspace is prevented from doing so until the state is
	 * appropriate.
	 */
	if (!mutex_trylock(&call->user_mutex))
		BUG();

before publishing it.  This used to work fine, but now there are big splashy
warnings every time a new call comes in.

No one else can see the lock at this point, but I need to lock it so that
lockdep doesn't complain later.  However, I can't lock it in the preallocator
- again because that upsets lockdep.

David

Re: Problem with WARN_ON in mutex_trylock() and rxrpc

[Peter Zijlstra]

On Thu, Dec 05, 2019 at 12:02:24PM +0000, David Howells wrote:
> Hi Davidlohr,
> 
> commit a0855d24fc22d49cdc25664fb224caee16998683 ("locking/mutex: Complain upon
> mutex API misuse in IRQ contexts") is a bit of a problem for rxrpc, though
> nothing that shouldn't be reasonably easy to solve, I think.
> 
> What happens is that rxrpc_new_incoming_call(), which is called in softirq
> context, calls mutex_trylock() to prelock a new incoming call:
> 
> 	/* Lock the call to prevent rxrpc_kernel_send/recv_data() and
> 	 * sendmsg()/recvmsg() inconveniently stealing the mutex once the
> 	 * notification is generated.
> 	 *
> 	 * The BUG should never happen because the kernel should be well
> 	 * behaved enough not to access the call before the first notification
> 	 * event and userspace is prevented from doing so until the state is
> 	 * appropriate.
> 	 */
> 	if (!mutex_trylock(&call->user_mutex))
> 		BUG();
> 
> before publishing it.  This used to work fine, but now there are big splashy
> warnings every time a new call comes in.
> 
> No one else can see the lock at this point, but I need to lock it so that
> lockdep doesn't complain later.  However, I can't lock it in the preallocator
> - again because that upsets lockdep.

To recap the IRC discussion; the intended mutex semantics are such to
allow Priority Inheritance. This means that the mutex must be locked and
unlocked in the same (task) context. Otherwise there is no distinct
owner to boost for contending mutex_lock() operations.

Since (soft)irq context doesn't (necessarily) have a task context, these
operations don't strictly make sense, and that is what the patch in
question tries to WARN about.

As it happens, you do mutex_unlock() from the very same softirq context
you do that mutex_trylock() in, so lockdep will never have had cause to
complain, 'current' is the same at acquire and release.

Now, either we're in non-preemptible softirq context and a contending
mutex_lock() would spuriously boost a random task, which is harmless due
to the non-preemptive nature of softirq, or we're running in ksoftirqd
and that gets boosted, which actually makes some sense.

For PREEMPT_RT (the only case that really matters, since that actually
replaces struct mutex with rt_mutex) this would result in boosting
whatever (soft)irq thread ended up running the thing.

(Also, I'm not entire sure on the current softirq model for -RT)

Is this something we want to allow?

At the very least I'm going to do a lockdep patch that verifies the lock
stack is 'empty' for the current irq_context when it changes.

Re: Problem with WARN_ON in mutex_trylock() and rxrpc

[Peter Zijlstra]

On Thu, Dec 05, 2019 at 02:22:12PM +0100, Peter Zijlstra wrote:

> At the very least I'm going to do a lockdep patch that verifies the lock
> stack is 'empty' for the current irq_context when it changes.

Something like the below..

diff --git a/include/linux/irqflags.h b/include/linux/irqflags.h
index 21619c92c377..c0a314dc9969 100644
--- a/include/linux/irqflags.h
+++ b/include/linux/irqflags.h
@@ -21,11 +21,13 @@
   extern void trace_softirqs_off(unsigned long ip);
   extern void lockdep_hardirqs_on(unsigned long ip);
   extern void lockdep_hardirqs_off(unsigned long ip);
+  extern void lockdep_leave_irq_context(void);
 #else
   static inline void trace_softirqs_on(unsigned long ip) { }
   static inline void trace_softirqs_off(unsigned long ip) { }
   static inline void lockdep_hardirqs_on(unsigned long ip) { }
   static inline void lockdep_hardirqs_off(unsigned long ip) { }
+  static inline void lockdep_leave_irq_context(void) { }
 #endif
 
 #ifdef CONFIG_TRACE_IRQFLAGS
@@ -41,6 +43,8 @@ do {						\
 } while (0)
 # define trace_hardirq_exit()			\
 do {						\
+	if (current->hardirq_context == 1)	\
+		lockdep_leave_irq_context();	\
 	current->hardirq_context--;		\
 } while (0)
 # define lockdep_softirq_enter()		\
@@ -49,6 +53,8 @@ do {						\
 } while (0)
 # define lockdep_softirq_exit()			\
 do {						\
+	if (current->softirq_context == 1)	\
+		lockdep_leave_irq_context();	\
 	current->softirq_context--;		\
 } while (0)
 #else
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 32282e7112d3..5c1102967927 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -3600,6 +3600,43 @@ static inline unsigned int task_irq_context(struct task_struct *task)
 	return 2 * !!task->hardirq_context + !!task->softirq_context;
 }
 
+/*
+ * Validate the current irqcontext holds no locks.
+ */
+void lockdep_leave_irq_context(void)
+{
+	struct task_struct *curr = current;
+	unsigned int irq_context = task_irq_context(curr);
+	int depth = curr->lockdep_depth;
+	struct held_lock *hlock;
+
+	if (unlikely(!debug_locks || curr->lockdep_recursion))
+		return;
+
+	if (!depth)
+		return;
+
+	if (curr->held_locks[depth-1].irq_context != irq_context)
+		return;
+
+	pr_warn("\n");
+	pr_warn("========================================================\n");
+	pr_warn("WARNING: Leaving (soft/hard) IRQ context with locks held\n");
+	print_kernel_ident();
+	pr_warn("--------------------------------------------------------\n");
+
+	for (; depth; depth--) {
+		hlock = curr->held_locks + depth - 1;
+		if (hlock->irq_context != irq_context)
+			break;
+		print_lock(hlock);
+	}
+
+	pr_warn("\nstack backtrace:\n");
+	dump_stack();
+}
+NOKPROBE_SYMBOL(lockdep_leave_irq_context);
+
 static int separate_irq_context(struct task_struct *curr,
 		struct held_lock *hlock)
 {

Re: Problem with WARN_ON in mutex_trylock() and rxrpc

[Thomas Gleixner]

Peter,

Peter Zijlstra <peterz@infradead.org> writes:
> On Thu, Dec 05, 2019 at 12:02:24PM +0000, David Howells wrote:
>> commit a0855d24fc22d49cdc25664fb224caee16998683 ("locking/mutex: Complain upon
>> mutex API misuse in IRQ contexts") is a bit of a problem for rxrpc, though
>> nothing that shouldn't be reasonably easy to solve, I think.
>> 
>> What happens is that rxrpc_new_incoming_call(), which is called in softirq
>> context, calls mutex_trylock() to prelock a new incoming call:
>> 
>> 	/* Lock the call to prevent rxrpc_kernel_send/recv_data() and
>> 	 * sendmsg()/recvmsg() inconveniently stealing the mutex once the
>> 	 * notification is generated.
>> 	 *
>> 	 * The BUG should never happen because the kernel should be well
>> 	 * behaved enough not to access the call before the first notification
>> 	 * event and userspace is prevented from doing so until the state is
>> 	 * appropriate.
>> 	 */
>> 	if (!mutex_trylock(&call->user_mutex))
>> 		BUG();
>> 
>> before publishing it.  This used to work fine, but now there are big splashy
>> warnings every time a new call comes in.
>> 
>> No one else can see the lock at this point, but I need to lock it so that
>> lockdep doesn't complain later.  However, I can't lock it in the preallocator
>> - again because that upsets lockdep.
>
> To recap the IRC discussion; the intended mutex semantics are such to
> allow Priority Inheritance. This means that the mutex must be locked and
> unlocked in the same (task) context. Otherwise there is no distinct
> owner to boost for contending mutex_lock() operations.
>
> Since (soft)irq context doesn't (necessarily) have a task context, these
> operations don't strictly make sense, and that is what the patch in
> question tries to WARN about.

Not only that. Acquiring something which is _NOT_ designed for non
thread context works by chance not by design. IOW it makes assumptions
about the underlying mutex implementation and any change to that which
actually assumes thread context will break that. So, no we don't want
I'm clever and can do that as the implementation allows, simply because
this is a blatant layering violation.

> As it happens, you do mutex_unlock() from the very same softirq context
> you do that mutex_trylock() in, so lockdep will never have had cause to
> complain, 'current' is the same at acquire and release.
>
> Now, either we're in non-preemptible softirq context and a contending
> mutex_lock() would spuriously boost a random task, which is harmless due
> to the non-preemptive nature of softirq, or we're running in ksoftirqd
> and that gets boosted, which actually makes some sense.
>
> For PREEMPT_RT (the only case that really matters, since that actually
> replaces struct mutex with rt_mutex) this would result in boosting
> whatever (soft)irq thread ended up running the thing.

Well, that'd "work". Actually in RT this makes even sense as the
contending waiter wants the owner out of the critical region ASAP>

> (Also, I'm not entire sure on the current softirq model for -RT)
>
> Is this something we want to allow?

I'm not a fan. See above.

Thanks,

        tglx

Re: Problem with WARN_ON in mutex_trylock() and rxrpc

[Peter Zijlstra]

On Tue, Dec 10, 2019 at 07:33:17PM +0100, Thomas Gleixner wrote:
> Peter Zijlstra <peterz@infradead.org> writes:
> > On Thu, Dec 05, 2019 at 12:02:24PM +0000, David Howells wrote:

> > To recap the IRC discussion; the intended mutex semantics are such to
> > allow Priority Inheritance. This means that the mutex must be locked and
> > unlocked in the same (task) context. Otherwise there is no distinct
> > owner to boost for contending mutex_lock() operations.
> >
> > Since (soft)irq context doesn't (necessarily) have a task context, these
> > operations don't strictly make sense, and that is what the patch in
> > question tries to WARN about.
> 
> Not only that. Acquiring something which is _NOT_ designed for non
> thread context works by chance not by design. IOW it makes assumptions
> about the underlying mutex implementation and any change to that which
> actually assumes thread context will break that. So, no we don't want
> I'm clever and can do that as the implementation allows, simply because
> this is a blatant layering violation.

AFAICT the only assumption it relies on are:

 - that the softirq will cleanly preempt a task. That is, the task
   context must not change under the softirq execution.

 - that the softirq runs non-preemptible.

Now, both these properties are rather fundamental to how our softirqs
work. And can, therefore, be relied upon, irrespective of the mutex
implementation.

> > As it happens, you do mutex_unlock() from the very same softirq context
> > you do that mutex_trylock() in, so lockdep will never have had cause to
> > complain, 'current' is the same at acquire and release.
> >
> > Now, either we're in non-preemptible softirq context and a contending
> > mutex_lock() would spuriously boost a random task, which is harmless due
> > to the non-preemptive nature of softirq, or we're running in ksoftirqd
> > and that gets boosted, which actually makes some sense.
> >
> > For PREEMPT_RT (the only case that really matters, since that actually
> > replaces struct mutex with rt_mutex) this would result in boosting
> > whatever (soft)irq thread ended up running the thing.
> 
> Well, that'd "work". Actually in RT this makes even sense as the
> contending waiter wants the owner out of the critical region ASAP>

The only funny I could come up with is if current == idle, because in
that case we'll attempt to boost idle. And that is a major no-no. The
proxy execution patches will actually run into this :/

> > (Also, I'm not entire sure on the current softirq model for -RT)
> >
> > Is this something we want to allow?
> 
> I'm not a fan. See above.

Yeah, I'm pretty adverse to it too. But I'm not sure what to suggest
David do instead. Clearly semaphores are an option, but perhaps there's
something better; I've not yet tried to understand his code.

Re: Problem with WARN_ON in mutex_trylock() and rxrpc

[Sebastian Andrzej Siewior]

On 2019-12-10 20:25:38 [+0100], Peter Zijlstra wrote:
> AFAICT the only assumption it relies on are:
> 
>  - that the softirq will cleanly preempt a task. That is, the task
>    context must not change under the softirq execution.
> 
>  - that the softirq runs non-preemptible.
> 
> Now, both these properties are rather fundamental to how our softirqs
> work. And can, therefore, be relied upon, irrespective of the mutex
> implementation.

softirq is preemptible on -RT (I think you know that already but just in
case).

Sebastian

Re: Problem with WARN_ON in mutex_trylock() and rxrpc

[Peter Zijlstra]

On Tue, Dec 10, 2019 at 09:32:25PM +0100, Sebastian Andrzej Siewior wrote:
> On 2019-12-10 20:25:38 [+0100], Peter Zijlstra wrote:
> > AFAICT the only assumption it relies on are:
> > 
> >  - that the softirq will cleanly preempt a task. That is, the task
> >    context must not change under the softirq execution.
> > 
> >  - that the softirq runs non-preemptible.
> > 
> > Now, both these properties are rather fundamental to how our softirqs
> > work. And can, therefore, be relied upon, irrespective of the mutex
> > implementation.
> 
> softirq is preemptible on -RT (I think you know that already but just in
> case).

Indeed, but there it also runs in task context and then it all works
naturally. The !preempt thing is required for when it runs on top of a
task; then it functions as a priority ceiling like construct.