On Mon, Nov 20, 2017 at 11:51 AM, Nadav Amit <namit@xxxxxxxxxx> wrote:
> Andy Lutomirski <luto@xxxxxxxxxx> wrote:
>
>> On Fri, Nov 17, 2017 at 10:19 AM, Nadav Amit <namit@xxxxxxxxxx> wrote:
>>
>>> +static inline void update_cr4(unsigned long cr4)
>>> +{
>>> +#ifdef CONFIG_LOCKDEP
>>> + WARN_ON(!irqs_disabled());
>>> +#endif
>>> + this_cpu_write(cpu_tlbstate.cr4, cr4);
>>> + __write_cr4(cr4);
>>> +}
>>
>> Let's call this __cr4_set() instead of update_cr4(). The __ is to
>> remind people that it's not really for use and starting with cr4_ is
>> consistent with the rest of the functions.
>>
>> Also, can you split this whole thing into two patches? The
>> introduction of the separate update function is a great cleanup, but I
>> think it's orthogonal.
>
> Sure. Will do. Thanks for reviewing it.
>
> Just one small clarification: I do not see an issue of correctness right now
> if __native_flush_tlb_global_irq_disabled() is the only one that modifies
> CR4 in an interrupt handler. Yet, the current scheme seems error prone.
If we ever do cr4_whatever() with preempt *on*, then we could be
preempted and we're screwed, probably exploitably. I haven't looked
carefully enough to decide whether I think that the IPI could cause a
real problem.
