[PATCH] arm: vfp: always clear vfp_current_hw_state when forcing reload

[Yuanyuan ZHONG]

On Fri, Oct 11, 2013 at 7:07 AM, Russell King - ARM Linux
<linux at arm.linux.org.uk> wrote:
> On Thu, Oct 10, 2013 at 11:00:18AM -0500, Yuanyuan ZHONG wrote:
>> Hi Russell, et al
>>
>> If there is no further comments, I'll submit it to patch system.
>> Thanks.
>
> No, I still don't agree with your patch as being the correct fix.  Look
> at the code that you're creating:
>
>         if (vfp_state_in_hw(cpu, thread)) {
>                 fmxr(FPEXC, fmrx(FPEXC) & ~FPEXC_EN);
>                 vfp_current_hw_state[cpu] = NULL;
>         }
> #ifdef CONFIG_SMP
>         thread->vfpstate.hard.cpu = NR_CPUS;
>         vfp_current_hw_state[cpu] = NULL;
> #endif
>
> So when SMP is enabled we unconditionally blat out the vfp_current_hw_state
> pointer for this CPU?

I think ensuring local vfp_current_hw_state is invalidated is what
vfp_force_reload() need to do when SMP is enabled.

>
> What actually needs fixing is the call to this function in the hotplug
> thread:
>
>                 vfp_force_reload((long)hcpu, current_thread_info());
>
> I'd suggest this becomes:
>
>                 unsigned cpu = (long)hcpu;
>                 if (vfp_current_hw_state[cpu])
>                         vfp_force_reload(cpu, vfp_current_hw_state[cpu]);
>
> That will force the right hw_state to be forced to reload, rather than
> passing in the wrong thread state.

No. This is not right. Look at the other places vfp_force_reload() are
called, they are all using current_thread_info(). I think only
current_thread_info() is safe for vfp_force_reload().
With your suggestion:
(1) vfp_current_hw_state[cpu] is &thread->vfpstate, not thread.
(2) If the thread exited on another CPU and its stack has been
reclaimed and reallocated, the "thread->vfpstate.hard.cpu = NR_CPUS;"
in vfp_force_reload() will cause memory corruption.

Thanks.