Re: Mainline kernel crashes, was Re: RFC: remove set_fs for m68k

Finn Thain
2021-09-09T09:40:29Z

On Wed, 8 Sep 2021, Michael Schmitz wrote:

> On 08/09/21 11:50, Finn Thain wrote:
> > On Tue, 7 Sep 2021, Michael Schmitz wrote:
> >
> > > > Does anyone know what causes this?
> > >
> > > Our practice to run interrupt handlers at the IPL of the current
> > > interrupt under service, instead of disabling local interrupts for
> > > the duration of the handler?
> > >
> >
> > Lock contention will happen anyway.
> >
> > If using spin_trylock() outside of "atomic" context was a bug
> > (really?) then it can't be used here.
>
> add_interrupt_randomness() relies on interrupts being disabled in
> __handle_irq_event_percpu(), so this check assumes atomic context.
>

But __handle_irq_event_percpu() also assumes that local interrupts have
been disabled. There is a WARN_ONCE to that effect.

> Our definition of irqs_disabled() invalidates that assumption.
>
> > Perhaps add_interrupt_randomness() should use the lock in irq mode,
> > like the rest of drivers/char/random.c does.
>
> That might deadlock on SMP when a task reading from the random pool
> holding the lock executes on the same CPU as the interrupt.
>

That does not make sense to me. add_interrupt_randomness() effectively
does acquire the lock in irq mode on SMP ports yet it doesn't deadlock.

> In the UP case, the spinlock is optimized away,

Yes and spin_trylock() always succeeds. Hence CONFIG_SPINLOCK_DEBUG
asserts that the lock is never contended.

One thing that bothered me when I raised this issue was the apparent false
positive from CONFIG_SPINLOCK_DEBUG. But after sleeping on it, it makes
more sense to me.

(Perhaps the difficulty here is !SMP && !PREEMPT_COUNT. If PREEMPT_COUNT
was available on m68k then spin_trylock() might be expected to test
preempt_count, and the problem would not arise.)

BTW, the problem went away when I changed to irq mode locking to avoid any
lock contention, like so:

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 605969ed0f96..4e73c87cbdb8 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1248,6 +1248,7 @@ void add_interrupt_randomness(int irq, int irq_flags)
struct fast_pool *fast_pool = this_cpu_ptr(&irq_randomness);
struct pt_regs *regs = get_irq_regs();
unsigned long now = jiffies;
+ unsigned long flags;
cycles_t cycles = random_get_entropy();
__u32 c_high, j_high;
__u64 ip;
@@ -1281,12 +1282,12 @@ void add_interrupt_randomness(int irq, int irq_flags)
return;

r = &input_pool;
- if (!spin_trylock(&r->lock))
+ if (!spin_trylock_irqsave(&r->lock, flags))
return;

fast_pool->last = now;
__mix_pool_bytes(r, &fast_pool->pool, sizeof(fast_pool->pool));
- spin_unlock(&r->lock);
+ spin_unlock_irqrestore(&r->lock, flags);

fast_pool->count = 0;