[PATCH v2] arm64: fpsimd: improve stacking logic in non-interruptible context

[ard.biesheuvel@linaro.org (Ard Biesheuvel)]

On 8 December 2016 at 15:50, Catalin Marinas <catalin.marinas@arm.com> wrote:
> Hi Ard,
>
> On Wed, Dec 07, 2016 at 10:14:08AM +0000, Ard Biesheuvel wrote:
>>  void kernel_neon_begin_partial(u32 num_regs)
>>  {
>> -     if (in_interrupt()) {
>> -             struct fpsimd_partial_state *s = this_cpu_ptr(
>> -                     in_irq() ? &hardirq_fpsimdstate : &softirq_fpsimdstate);
>> +     struct fpsimd_partial_state *s;
>> +     int level;
>> +
>> +     preempt_disable();
>> +
>> +     level = this_cpu_read(kernel_neon_nesting_level);
>> +     BUG_ON(level > 2);
>> +
>> +     if (level > 0) {
>> +             s = this_cpu_ptr(nested_fpsimdstate);
>>
>> -             BUG_ON(num_regs > 32);
>> -             fpsimd_save_partial_state(s, roundup(num_regs, 2));
>> +             WARN_ON_ONCE(num_regs > 32);
>> +             num_regs = min(roundup(num_regs, 2), 32U);
>> +
>> +             fpsimd_save_partial_state(&s[level - 1], num_regs);
>>       } else {
>>               /*
>>                * Save the userland FPSIMD state if we have one and if we
>> @@ -241,24 +256,29 @@ void kernel_neon_begin_partial(u32 num_regs)
>>                * that there is no longer userland FPSIMD state in the
>>                * registers.
>>                */
>> -             preempt_disable();
>>               if (current->mm &&
>>                   !test_and_set_thread_flag(TIF_FOREIGN_FPSTATE))
>>                       fpsimd_save_state(&current->thread.fpsimd_state);
>>               this_cpu_write(fpsimd_last_state, NULL);
>>       }
>> +     this_cpu_write(kernel_neon_nesting_level, level + 1);
>>  }
>
> I'm slightly confused with the potential race with an interrupt here.
> Let's say the above is running in the process context, sets the
> TIF_FOREIGN_FPSTATE but is interrupted before fpsimd_save_state(). The
> interrupt handler calling kernel_neon_begin_partial() is seeing level 0
> and TIF_FOREIGN_FPSTATE and decides that it is safe to corrupt the Neon
> state without any further saving.
>
> I think the kernel_neon_nesting_level should be incremented early on in
> this function.
>

Good point, I hadn't considered that.