Re: [PATCH 0/7] livepatch,module: Remove .klp.arch and module_disable_ro()

[Josh Poimboeuf <jpoimboe@redhat.com>]

On Wed, Apr 15, 2020 at 04:24:15PM +0200, Peter Zijlstra wrote:
> > It bothers me that both the notifiers and the module init() both see the
> > same MODULE_STATE_COMING state, but only in the former case is the text
> > writable.
> > 
> > I think it's cognitively simpler if MODULE_STATE_COMING always means the
> > same thing, like the comments imply, "fully formed" and thus
> > not-writable:
> > 
> > enum module_state {
> > 	MODULE_STATE_LIVE,	/* Normal state. */
> > 	MODULE_STATE_COMING,	/* Full formed, running module_init. */
> > 	MODULE_STATE_GOING,	/* Going away. */
> > 	MODULE_STATE_UNFORMED,	/* Still setting it up. */
> > };
> > 
> > And, it keeps tighter constraints on what a notifier can do, which is a
> > good thing if we can get away with it.
> 
> Moo! -- but jump_label and static_call are on the notifier chain and I
> was hoping to make it cheaper for them. Should we perhaps weane them off the
> notifier and, like ftrace/klp put in explicit calls?
> 
> It'd make the error handling in prepare_coming_module() a bigger mess,
> but it should work.

So you're wanting to have jump labels and static_call do direct writes
instead of text pokes, right?  Makes sense.

I don't feel strongly about "don't let module notifiers modify text".

But I still not a fan of the fact that COMING has two different
"states".  For example, after your patch, when apply_relocate_add() is
called from klp_module_coming(), it can use memcpy(), but when called
from klp module init() it has to use text poke.  But both are COMING so
there's no way to look at the module state to know which can be used.

I hate to say it, but it almost feels like another module state would be
useful.

-- 
Josh