On Mon, 11 Jun 2018, Petr Mladek wrote: > On Thu 2018-06-07 11:29:48, Miroslav Benes wrote: > > We decided to deny the patched modules to be removed. If it proves to be > > a major drawback for users, we can still implement a different approach. > > > > The reference of a patched module has to be taken regardless of a > > patch's state. Thus it is not taken and dropped in enable/disable paths, > > but in register/unregister paths. > > > --- a/kernel/livepatch/core.c > > +++ b/kernel/livepatch/core.c > > @@ -739,6 +746,14 @@ static int klp_init_object_loaded(struct klp_patch *patch, > > } > > } > > > > + /* > > + * Do not allow patched modules to be removed. > > + * > > + * All callers of klp_init_object_loaded() set obj->mod. > > + */ > > + if (klp_is_module(obj) && !try_module_get(obj->mod)) > > + return -ENODEV; > > + > > return 0; > > } > > This looks strange. We try to take the reference after we do all > actions needed for a loaded module. There is nothing that would > prevent obj->mod to get into the going state. Yes, there was an intention to keep the number of necessary modifications of the error paths as low as possible. > Note that it was safe (except for the relocated symbols) because > the module was not able to really go until it called > klp_module_going(). But you removed this last break > by the 3rd patch. Yes, try_module_get() would return an error here. > I suggest another approach: > > I would rename klp_find_object_module() to klp_get_object_module() > and get the reference there. Note that it should be fine to call > it later in klp_init_object() (before klp_init_object_loaded()). > This would solve klp_init_patch(). > > We will also need to get the reference in klp_module_coming(). > It can be called anywhere there. If it fails, we will block > loading the module. I can as well move the above hunk up in klp_init_object_loaded(), no? Not that I'm seeing a problem to have it in klp_find_object_module(). > Note that the mod->klp_alive logic will still be necessary. > It solves various races when both the patch and the patched > module are loaded in parallel. > > Sigh, this also means that we still could call klp_init_object_loaded() > and apply reloacations even when the patched module fails to loaded > later from other reasons. This means that this approach does not > solve the original problem completely. That would be in klp_module_coming(). Hm, you're right. > I wonder how complicated would be the arch-specific part that > would clean up relocations when the module is unloaded. I don't think it would be complicated. We just want to avoid it, because it is arch-specific. It is more difficult to maintain such code. Regards, Miroslav -- To unsubscribe from this list: send the line "unsubscribe live-patching" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
