On Jun 1, 2023, at 1:50 PM, Edgecombe, Rick P [off-list ref] wrote:

On Thu, 2023-06-01 at 14:38 -0400, Kent Overstreet wrote:

quoted

On Thu, Jun 01, 2023 at 06:13:44PM +0000, Edgecombe, Rick P wrote:

quoted

quoted

text_poke() _does_ create a separate RW mapping.

Sorry, I meant a separate RW allocation.

Ah yes, that makes sense

quoted

quoted

The thing that sucks about text_poke() is that it always does a
full
TLB
flush, and AFAICT that's not remotely needed. What it really
wants to
be
doing is conceptually just

kmap_local()
mempcy()
kunmap_loca()
flush_icache();

...except that kmap_local() won't actually create a new mapping
on
non-highmem architectures, so text_poke() open codes it.

Text poke creates only a local CPU RW mapping. It's more secure
because
other threads can't write to it.

*nod*, same as kmap_local

It's only used and flushed locally, but it is accessible to all CPU's,
right?

quoted

quoted

It also only needs to flush the local core when it's done since
it's
not using a shared MM.

 
Ahh! Thanks for that; perhaps the comment in text_poke() about IPIs
could be a bit clearer.

What is it (if anything) you don't like about text_poke() then? It
looks
like it's doing broadly similar things to kmap_local(), so should be
in the same ballpark from a performance POV?

The way text_poke() is used here, it is creating a new writable alias
and flushing it for *each* write to the module (like for each write of
an individual relocation, etc). I was just thinking it might warrant
some batching or something.

I am not advocating to do so, but if you want to have many efficient
writes, perhaps you can just disable CR0.WP. Just saying that if you
are about to write all over the memory, text_poke() does not provide
too much security for the poking thread.