• [PATCH] livepatch: unpatch all klp_objects if klp_module_coming fai

    From Petr Mladek@21:1/5 to Joe Lawrence on Mon Oct 2 15:00:02 2017
    On Thu 2017-09-28 15:15:00, Joe Lawrence wrote:
    When an incoming module is considered for livepatching by klp_module_coming(), it iterates over multiple patches and multiple
    kernel objects in this order:

    list_for_each_entry(patch, &klp_patches, list) {
    klp_for_each_object(patch, obj) {

    which means that if one of the kernel objects fails to patch, klp_module_coming()'s error path needs to unpatch and cleanup any kernel objects that were already patched by a previous patch.

    diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
    index b9628e43c78f..3d457e0bbe26 100644
    --- a/kernel/livepatch/core.c
    +++ b/kernel/livepatch/core.c
    @@ -830,6 +830,42 @@ int klp_register_patch(struct klp_patch *patch)
    }
    EXPORT_SYMBOL_GPL(klp_register_patch);

    +/*
    + * Revert patches (up to a given limit) to objects belonging to a given
    + * kernel module. After unpatching such objects, the function also

    "objects belonging to a module" sounds strange. In the livepatch
    world: module == object. I would say something like:

    /* Remove parts of patches that touch a given kernel module.
    * The list of proceed patches might be limited. When limit is
    * NULL, all patches will be handled.
    */


    + * frees them. When limit is NULL, all patches to the given module will
    + * be reverted.
    + */
    +static void klp_cleanup_module_objects_limited(struct module *mod,
    + struct klp_patch *limit)
    +{

    Same here with: "module_objects". What about?

    klp_cleanup_module_patches_limited(struct module *mod,
    struct klp_patch *limit)


    + struct klp_patch *patch;
    + struct klp_object *obj;
    +
    + list_for_each_entry(patch, &klp_patches, list) {
    + if (patch == limit)
    + break;
    +
    + klp_for_each_object(patch, obj) {
    + if (!klp_is_module(obj) || strcmp(obj->name, mod->name))
    + continue;
    +
    + /*
    + * Only unpatch the module if the patch is enabled or
    + * is in transition.
    + */
    + if (patch->enabled || patch == klp_transition_patch) { + pr_notice("reverting patch '%s' on unloading module '%s'\n",
    + patch->mod->name, obj->mod->name);
    + klp_unpatch_object(obj);
    + }
    +
    + klp_free_object_loaded(obj);
    + break;

    Heh, I was about to say OK for this patch. But then I noticed
    this strange "break". It was already in the original code and
    I wondered if it was OK.

    It is OK. It is an optimization because this code handles
    only one struct object in each patch. But it made me
    to complain about the wording above ;-)

    + }
    + }
    +}
    +
    int klp_module_coming(struct module *mod)
    {
    int ret;

    Otherwise the patch looks fine to me. Thanks a lot for handling
    it separately.

    Best Regards,
    Petr

    --- SoupGate-Win32 v1.05
    * Origin: fsxNet Usenet Gateway (21:1/5)