Re: [PATCH 6/7] livepatch: Remove module_disable_ro() usage

[Miroslav Benes]

On Wed, 15 Apr 2020, Josh Poimboeuf wrote:

> On Wed, Apr 15, 2020 at 05:02:16PM +0200, Jessica Yu wrote:
> > +++ Josh Poimboeuf [14/04/20 11:28 -0500]:
> > > With arch_klp_init_object_loaded() gone, and apply_relocate_add() now
> > > using text_poke(), livepatch no longer needs to use module_disable_ro().
> > > 
> > > The text_mutex usage can also be removed -- its purpose was to protect
> > > against module permission change races.
> > > 
> > > Signed-off-by: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
> > > ---
> > > kernel/livepatch/core.c | 8 --------
> > > 1 file changed, 8 deletions(-)
> > > 
> > > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > > index 817676caddee..3a88639b3326 100644
> > > --- a/kernel/livepatch/core.c
> > > +++ b/kernel/livepatch/core.c
> > > @@ -767,10 +767,6 @@ static int klp_init_object_loaded(struct klp_patch *patch,
> > > 	struct klp_modinfo *info = patch->mod->klp_info;
> > > 
> > > 	if (klp_is_module(obj)) {
> > > -
> > > -		mutex_lock(&text_mutex);
> > > -		module_disable_ro(patch->mod);
> > > -
> > 
> > Don't you still need the text_mutex to use text_poke() though?
> > (Through klp_write_relocations -> apply_relocate_add -> text_poke)
> > At least, I see this assertion there:
> > 
> > void *text_poke(void *addr, const void *opcode, size_t len)
> > {
> > 	lockdep_assert_held(&text_mutex);
> > 
> > 	return __text_poke(addr, opcode, len);
> > }
> 
> Hm, guess I should have tested with lockdep ;-)

:)

If I remember correctly, text_mutex must be held whenever text is modified 
to prevent race due to the modification. It is not only about permission 
changes even though it was how it manifested itself in our case.

Miroslav