Replaced patches are removed from the stack when the transition is finished. It means that Nop structures will never be needed again and can be removed. Why should we care? + Nop structures give the impression that the function is patched even though the ftrace handler has no effect. + Ftrace handlers do not come for free. They cause slowdown that might be visible in some workloads. The ftrace-related slowdown might actually be the reason why the function is no longer patched in the new cumulative patch. One would expect that cumulative patch would help solve these problems as well. + Cumulative patches are supposed to replace any earlier version of the patch. The amount of NOPs depends on which version was replaced. This multiplies the amount of scenarios that might happen. One might say that NOPs are innocent. But there are even optimized NOP instructions for different processors, for example, see arch/x86/kernel/alternative.c. And klp_ftrace_handler() is much more complicated. + It sounds natural to clean up a mess that is no longer needed. It could only be worse if we do not do it. This patch allows to unpatch and free the dynamic structures independently when the transition finishes. The free part is a bit tricky because kobject free callbacks are called asynchronously. We could not wait for them easily. Fortunately, we do not have to. Any further access can be avoided by removing them from the dynamic lists.
Unified · Split
| @@ -611,11 +611,16 @@ static struct kobj_type klp_ktype_func = { · 610 unmodified lines | ||
| 611 | 611 | .sysfs_ops = &kobj_sysfs_ops, |
| 612 | 612 | }; |
| 613 | 613 | |
| 614 | −static void klp_free_funcs(struct klp_object *obj) | |
| 614 | +static void __klp_free_funcs(struct klp_object *obj, bool nops_only) | |
| 615 | 615 | { |
| 616 | 616 | struct klp_func *func, *tmp_func; |
| 617 | 617 | |
| 618 | 618 | klp_for_each_func_safe(obj, func, tmp_func) { |
| 619 | + if (nops_only && !func->nop) | |
| 620 | + continue; | |
| 621 | + | |
| 622 | + list_del(&func->node); | |
| 623 | + | |
| 619 | 624 | /* Might be called from klp_init_patch() error path. */ |
| 620 | 625 | if (func->kobj_added) { |
| 621 | 626 | kobject_put(&func->kobj); |
| @@ -640,12 +645,17 @@ static void klp_free_object_loaded(struct klp_object *obj) · 18 unmodified lines | ||
| 640 | 645 | } |
| 641 | 646 | } |
| 642 | 647 | |
| 643 | −static void klp_free_objects(struct klp_patch *patch) | |
| 648 | +static void __klp_free_objects(struct klp_patch *patch, bool nops_only) | |
| 644 | 649 | { |
| 645 | 650 | struct klp_object *obj, *tmp_obj; |
| 646 | 651 | |
| 647 | 652 | klp_for_each_object_safe(patch, obj, tmp_obj) { |
| 648 | − klp_free_funcs(obj); | |
| 653 | + __klp_free_funcs(obj, nops_only); | |
| 654 | + | |
| 655 | + if (nops_only && !obj->dynamic) | |
| 656 | + continue; | |
| 657 | + | |
| 658 | + list_del(&obj->node); | |
| 649 | 659 | |
| 650 | 660 | /* Might be called from klp_init_patch() error path. */ |
| 651 | 661 | if (obj->kobj_added) { |
| @@ -656,6 +666,16 @@ static void klp_free_objects(struct klp_patch *patch) · 4 unmodified lines | ||
| 656 | 666 | } |
| 657 | 667 | } |
| 658 | 668 | |
| 669 | +static void klp_free_objects(struct klp_patch *patch) | |
| 670 | +{ | |
| 671 | + __klp_free_objects(patch, false); | |
| 672 | +} | |
| 673 | + | |
| 674 | +static void klp_free_objects_dynamic(struct klp_patch *patch) | |
| 675 | +{ | |
| 676 | + __klp_free_objects(patch, true); | |
| 677 | +} | |
| 678 | + | |
| 659 | 679 | /* |
| 660 | 680 | * This function implements the free operations that can be called safely |
| 661 | 681 | * under klp_mutex. |
| @@ -1084,6 +1104,28 @@ void klp_discard_replaced_patches(struct klp_patch *new_patch) · 422 unmodified lines | ||
| 1084 | 1104 | } |
| 1085 | 1105 | } |
| 1086 | 1106 | |
| 1107 | +/* | |
| 1108 | + * This function removes the dynamically allocated 'nop' functions. | |
| 1109 | + * | |
| 1110 | + * We could be pretty aggressive. NOPs do not change the existing | |
| 1111 | + * behavior except for adding unnecessary delay by the ftrace handler. | |
| 1112 | + * | |
| 1113 | + * It is safe even when the transition was forced. The ftrace handler | |
| 1114 | + * will see a valid ops->func_stack entry thanks to RCU. | |
| 1115 | + * | |
| 1116 | + * We could even free the NOPs structures. They must be the last entry | |
| 1117 | + * in ops->func_stack. Therefore unregister_ftrace_function() is called. | |
| 1118 | + * It does the same as klp_synchronize_transition() to make sure that | |
| 1119 | + * nobody is inside the ftrace handler once the operation finishes. | |
| 1120 | + * | |
| 1121 | + * IMPORTANT: It must be called right after removing the replaced patches! | |
| 1122 | + */ | |
| 1123 | +void klp_discard_nops(struct klp_patch *new_patch) | |
| 1124 | +{ | |
| 1125 | + klp_unpatch_objects_dynamic(klp_transition_patch); | |
| 1126 | + klp_free_objects_dynamic(klp_transition_patch); | |
| 1127 | +} | |
| 1128 | + | |
| 1087 | 1129 | /* |
| 1088 | 1130 | * Remove parts of patches that touch a given kernel module. The list of |
| 1089 | 1131 | * patches processed might be limited. When limit is NULL, all patches |
| @@ -9,6 +9,7 @@ extern struct list_head klp_patches; · 8 unmodified lines | ||
| 9 | 9 | |
| 10 | 10 | void klp_free_patch_start(struct klp_patch *patch); |
| 11 | 11 | void klp_discard_replaced_patches(struct klp_patch *new_patch); |
| 12 | +void klp_discard_nops(struct klp_patch *new_patch); | |
| 12 | 13 | |
| 13 | 14 | static inline bool klp_is_object_loaded(struct klp_object *obj) |
| 14 | 15 | { |
| @@ -246,15 +246,26 @@ err: · 245 unmodified lines | ||
| 246 | 246 | return ret; |
| 247 | 247 | } |
| 248 | 248 | |
| 249 | −void klp_unpatch_object(struct klp_object *obj) | |
| 249 | +static void __klp_unpatch_object(struct klp_object *obj, bool nops_only) | |
| 250 | 250 | { |
| 251 | 251 | struct klp_func *func; |
| 252 | 252 | |
| 253 | − klp_for_each_func(obj, func) | |
| 253 | + klp_for_each_func(obj, func) { | |
| 254 | + if (nops_only && !func->nop) | |
| 255 | + continue; | |
| 256 | + | |
| 254 | 257 | if (func->patched) |
| 255 | 258 | klp_unpatch_func(func); |
| 259 | + } | |
| 256 | 260 | |
| 257 | − obj->patched = false; | |
| 261 | + if (obj->dynamic || !nops_only) | |
| 262 | + obj->patched = false; | |
| 263 | +} | |
| 264 | + | |
| 265 | + | |
| 266 | +void klp_unpatch_object(struct klp_object *obj) | |
| 267 | +{ | |
| 268 | + __klp_unpatch_object(obj, false); | |
| 258 | 269 | } |
| 259 | 270 | |
| 260 | 271 | int klp_patch_object(struct klp_object *obj) |
| @@ -277,11 +288,21 @@ int klp_patch_object(struct klp_object *obj) · 16 unmodified lines | ||
| 277 | 288 | return 0; |
| 278 | 289 | } |
| 279 | 290 | |
| 280 | −void klp_unpatch_objects(struct klp_patch *patch) | |
| 291 | +static void __klp_unpatch_objects(struct klp_patch *patch, bool nops_only) | |
| 281 | 292 | { |
| 282 | 293 | struct klp_object *obj; |
| 283 | 294 | |
| 284 | 295 | klp_for_each_object(patch, obj) |
| 285 | 296 | if (obj->patched) |
| 286 | − klp_unpatch_object(obj); | |
| 297 | + __klp_unpatch_object(obj, nops_only); | |
| 298 | +} | |
| 299 | + | |
| 300 | +void klp_unpatch_objects(struct klp_patch *patch) | |
| 301 | +{ | |
| 302 | + __klp_unpatch_objects(patch, false); | |
| 303 | +} | |
| 304 | + | |
| 305 | +void klp_unpatch_objects_dynamic(struct klp_patch *patch) | |
| 306 | +{ | |
| 307 | + __klp_unpatch_objects(patch, true); | |
| 287 | 308 | } |
| @@ -30,5 +30,6 @@ struct klp_ops *klp_find_ops(void *old_func); · 29 unmodified lines | ||
| 30 | 30 | int klp_patch_object(struct klp_object *obj); |
| 31 | 31 | void klp_unpatch_object(struct klp_object *obj); |
| 32 | 32 | void klp_unpatch_objects(struct klp_patch *patch); |
| 33 | +void klp_unpatch_objects_dynamic(struct klp_patch *patch); | |
| 33 | 34 | |
| 34 | 35 | #endif /* _LIVEPATCH_PATCH_H */ |
| @@ -85,8 +85,10 @@ static void klp_complete_transition(void) · 84 unmodified lines | ||
| 85 | 85 | klp_transition_patch->mod->name, |
| 86 | 86 | klp_target_state == KLP_PATCHED ? "patching" : "unpatching"); |
| 87 | 87 | |
| 88 | − if (klp_transition_patch->replace && klp_target_state == KLP_PATCHED) | |
| 88 | + if (klp_transition_patch->replace && klp_target_state == KLP_PATCHED) { | |
| 89 | 89 | klp_discard_replaced_patches(klp_transition_patch); |
| 90 | + klp_discard_nops(klp_transition_patch); | |
| 91 | + } | |
| 90 | 92 | |
| 91 | 93 | if (klp_target_state == KLP_UNPATCHED) { |
| 92 | 94 | /* |