Re: [PATCH 17/17] module: Prevent module removal racing with text_poke()

On Thu, 17 Jan 2019 17:15:27 -0800
Nadav Amit [off-list ref] wrote:

quoted

On Jan 17, 2019, at 3:58 PM, H. Peter Anvin [off-list ref] wrote:

On 1/16/19 11:54 PM, Masami Hiramatsu wrote:

quoted

On Wed, 16 Jan 2019 16:32:59 -0800
Rick Edgecombe [off-list ref] wrote:

quoted

From: Nadav Amit <redacted>

It seems dangerous to allow code modifications to take place
concurrently with module unloading. So take the text_mutex while the
memory of the module is freed.

At that point, since the module itself is removed from module list,
it seems no actual harm. Or would you have any concern?

The issue isn't the module list, but rather when it is safe to free the
contents, so we don't clobber anything. We absolutely need to enforce
that we can't text_poke() something that might have already been freed.

That being said, we *also* really would prefer to enforce that we can't
text_poke() memory that doesn't actually contain code; as far as I can
tell we don't currently do that check.

Yes, that what the mutex was supposed to achieve. It’s not supposed just
to check whether it is a code page, but also that it is the same code
page that you wanted to patch. 

quoted

This, again, is a good use for a separate mm context. We can enforce
that that context will only ever contain valid page mappings for actual
code pages.

This will not tell you that you have the *right* code-page. The module
notifiers help to do so, since they synchronize the text poking with
the module removal.

quoted

(Note: in my proposed algorithm, with a separate mm, replace INVLPG with
switching CR3 if we have to do a rollback or roll forward in the
breakpoint handler.)

I really need to read your patches more carefully to see what you mean.

Anyhow, so what do you prefer? I’m ok with either one:
	1. Keep this patch
	2. Remove this patch and change into a comment on text_poke()
	3. Just drop the patch

I would prefer 2. so at least we should add a comment to text_poke().

Thank you,


-- 
Masami Hiramatsu [off-list ref]