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

[Michael Schmitz <schmitzmic@gmail.com>]

Hi Finn,

On 09/09/21 21:40, Finn Thain wrote:
> 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.

Yes, but that WARN_ON depends on irqs_disabled() returning true if and 
only if interrupts are fully disabled, i.e. the IPL is 7. Our 
implementation of irqs_disabled() returns true if the IPL is > 0 (in 
general) or < 2 (Atari).

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

It uses spin_trylock() while interrupts are disabled, so someone must 
have worried about deadlocks there.

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

That assertion fails here (IMO because we allow interrupts in code that 
assumes they have been disabled, and tries unsuccessfully to assert 
that), but that's OK in this case, as the code protected by the lock 
isn't called if locking fails.

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

You add a redundant interrupt disable/enable for SMP or PREEMPT arches. 
That might be justified if it fixes a kernel crash, not merely a 
CONFIG_DEBUG_SPINLOCK warning.

If we can add preempt count checking or actual spinlocks (without adding 
too much overhead everywhere else), that might fix this issue without 
need for a patch to random.c?

(But then, so would fixing irqs_disables(), or running interrupt 
handlers with interrupts disabled...)

Cheers,

	Michael