Re: [PATCH v3] livepatch: add (un)patch callbacks

[Petr Mladek]

On Mon 2017-08-21 17:18:58, Joe Lawrence wrote:
> On Fri, Aug 18, 2017 at 03:58:16PM +0200, Petr Mladek wrote:
> > On Wed 2017-08-16 15:17:04, Joe Lawrence wrote:
> > > Provide livepatch modules a klp_object (un)patching notification
> > > mechanism.  Pre and post-(un)patch callbacks allow livepatch modules to
> > > setup or synchronize changes that would be difficult to support in only
> > > patched-or-unpatched code contexts.
> > > 
> > > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > > index b9628e43c78f..ddb23e18a357 100644
> > > --- a/kernel/livepatch/core.c
> > > +++ b/kernel/livepatch/core.c
> > > @@ -878,6 +890,8 @@ int klp_module_coming(struct module *mod)
> > >  				goto err;
> > >  			}
> > >  
> > > +			klp_post_patch_callback(obj);
> > 
> > This should be called only if (patch != klp_transition_patch).
> > Otherwise, it would be called too early.
> 
> Can you elaborate a bit on this scenario?  When would the transition
> patch (as I understand it, a livepatch not quite fully (un)patched) hit
> the module coming/going notifier?  Is it possible to load or unload a
> module like this?  I'd like to add this scenario to my test script if
> possible.

if (patch == klp_transition_patch) then the transition for this
patch is still running. klp_post_patch_callback() should and is
called at the end of the transition, see klp_complete_transition().
Note that klp_complete_transition() will see the object/module
loaded because obj->mod is set in klp_module_coming().

The (up)patch transition might take a while. It might be even
infinite if a process is blocked in a patched function. klp_mutex
is available most of the time. Therefore it is perfectly fine to
load/remove modules into/from the system and run their
klp_module_coming()/going() callbacks when a livepatch
patch transition is running.


> > > +
> > >  			break;
> > >  		}
> > >  	}
> > > @@ -929,7 +943,10 @@ void klp_module_going(struct module *mod)
> > >  			if (patch->enabled || patch == klp_transition_patch) {
> > >  				pr_notice("reverting patch '%s' on unloading module '%s'\n",
> > >  					  patch->mod->name, obj->mod->name);
> > > +
> > > +				klp_pre_unpatch_callback(obj);
> > 
> > Also the pre_unpatch() callback should be called only
> > if (patch != klp_transition_patch). Otherwise, it should have
> > already been called. It is not the current case but see below.
> 
> Ditto.

This is related to the other comment where I and Josh suggested
to call klp_pre_unpatch_callback() in __klp_disable_patch()
before the transition started. It means that the callback
has already been called for klp_transition_patch.

Best Regards,
Petr