On Mon, 2 May 2016, Josh Poimboeuf wrote: > On Mon, May 02, 2016 at 01:57:22PM +0200, Miroslav Benes wrote: > > Currently we do not allow patch module to unload since there is no > > method to determine if a task is still running in the patched code. > > > > The consistency model gives us the way because when the patching > > finishes we know that all tasks were marked as safe to call a new > > patched function. Thus every new call to the function calls the new > > patched code and at the same time no task can be somewhere in the old > > code, because it had to leave that code to be marked as safe. > > > > We can safely let the patch module go after that. > > I found this a little confusing because it talks about patching, whereas > we really want to remove the patch module after _unpatching_ it. You're right. I'll rephrase that. > > Completion is used for synchronization between module removal and sysfs > > infrastructure. Note that we still do not allow the removal for > > immediate model, that is no consistency model. > > > > With this change a call to try_module_get() is moved to > > __klp_enable_patch from klp_register_patch to make module reference > > counting symmetric (module_put() is in a patch disable path) and to > > allow to take a new reference to a disabled module when being enabled. > > One minor issue here: for patch->immediate, if somebody enabled and > disabled the patch several times, the module ref count would keep > increasing. Probably not a big deal... maybe we don't need to worry > about it. That is unfortunately true. I don't have a solution in my pocket which would be 100% reliable. At the same time I don't see a practical problem. Yes, refcount could overflow, but that shouldn't be a problem, should it? Anyway, I'll note it in the changelog. > > Also all kobject_put(&patch->kobj) calls are moved outside of klp_mutex > > lock protection to prevent a deadlock situation when > > klp_unregister_patch is called and sysfs directories are removed. There > > is no need to do the same for other kobject_put() callsites as we > > currently do not have their sysfs counterparts. > > > > Signed-off-by: Miroslav Benes <mbenes@xxxxxxx> > > --- > > Based on Josh's v2. > > > > There are still two things which need to be discussed. > > > > 1. Do we really need a completion? If I am not missing something > > kobject_del() always waits for sysfs callers to leave thanks to kernfs > > active protection. The only problem is in kobject_release() when > > CONFIG_DEBUG_KOBJECT_RELEASE=y. In this case kobject_delayed_cleanup() > > is delay-scheduled, which is a problem for us. For this we definitely > > need the completion, right? JiriS, is this correct? > > Sysfs and kobjects hurt my brain; I need to take a refresher course, so > I don't have an answer to this yet :-) Maybe Jiri S can enlighten us. :) > > 2. I refuse to remove a module when patch->immediate is set or one of > > its func->immediate is set. We can do slightly better, because we can > > allow the module to go if patch->immediate is false and all func with > > immediate set to true are from some object which is not loaded. This is > > for discussion because it needs more changes in the code (I'd like to > > call klp_is_object_loaded() which is somewhere else and so on). > > I don't think we need to worry about doing that unless somebody > complains about it. It's kind of obscure and patch removal isn't very > important anyway... Agreed. Thanks, 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
