Re: TLB flushes on fixmap changes

From: Linus Torvalds
Date: Fri Aug 24 2018 - 20:59:20 EST

Adding a few people to the cc.

On Fri, Aug 24, 2018 at 1:24 PM Nadav Amit <nadav.amit@xxxxxxxxx> wrote:
> >
> > Can you actually find something that changes the fixmaps after boot
> > (again, ignoring kmap)?
> At least the alternatives mechanism appears to do so.
> IIUC the following path is possible when adding a module:
> jump_label_add_module()
> ->__jump_label_update()
> ->arch_jump_label_transform()
> ->__jump_label_transform()
> ->text_poke_bp()
> ->text_poke()
> ->set_fixmap()

Yeah, that looks a bit iffy.

But making the tlb flush global wouldn't help. This is running on a
local core, and if there are other CPU's that can do this at the same
time, then they'd just fight about the same mapping.

Honestly, I think it's ok just because I *hope* this is all serialized
anyway (jump_label_lock? But what about other users of text_poke?).

But I'd be a lot happier about it if it either used an explicit lock
to make sure, or used per-cpu fixmap entries.

And the tlb flush is done *after* the address is used, which is bogus anyway.

> And a similar path can happen when static_key_enable/disable() is called.

Same comments.

How about replacing that

... do critical things here ...

in text_poke() with

static DEFINE_SPINLOCK(poke_lock);

spin_lock_irqsave(&poke_lock, flags);
... do critical things here ...
spin_unlock_irqrestore(&poke_lock, flags);

and moving the local_flush_tlb() to after the set_fixmaps, but before
the access through the virtual address.

But changing things to do a global tlb flush would just be wrong.