Adding a few people to the cc.

On Fri, Aug 24, 2018 at 1:24 PM Nadav Amit [off-list ref] wrote:

quoted

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

        local_irq_save(flags);
       ... do critical things here ...
        local_irq_restore(flags);

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.

           Linus