On 08/29/2017 11:49 AM, Josh Poimboeuf wrote: > On Fri, Aug 25, 2017 at 03:10:00PM -0400, Joe Lawrence wrote: >> +Test 6 >> +------ >> + >> +Test a scenario where a vmlinux pre-patch callback returns a non-zero >> +status (ie, failure): >> + >> +- load target module >> +- load livepatch -ENODEV >> +- unload target module >> + >> +First load a target module: >> + >> + % insmod samples/livepatch/livepatch-callbacks-mod.ko >> + [ 80.740520] livepatch_callbacks_mod: livepatch_callbacks_mod_init >> + >> +Load the livepatch module, setting its 'pre_patch_ret' value to -19 >> +(-ENODEV). When its vmlinux pre-patch callback executed, this status >> +code will propagate back to the module-loading subsystem. The result is >> +that the insmod command refuses to load the livepatch module: >> + >> + % insmod samples/livepatch/livepatch-callbacks-demo.ko pre_patch_ret=-19 >> + [ 82.747326] livepatch: enabling patch 'livepatch_callbacks_demo' >> + [ 82.747743] livepatch: 'livepatch_callbacks_demo': initializing >> unpatching transition >> + [ 82.747767] livepatch_callbacks_demo: pre_patch_callback: vmlinux >> + [ 82.748237] livepatch: pre-patch callback failed for object 'vmlinux' >> + [ 82.748637] livepatch: failed to enable patch >> 'livepatch_callbacks_demo' >> + [ 82.749059] livepatch: 'livepatch_callbacks_demo': canceling >> transition, unpatching >> + [ 82.749060] livepatch: 'livepatch_callbacks_demo': completing >> unpatching transition >> + [ 82.749177] livepatch_callbacks_demo: post_unpatch_callback: >> livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state >> + [ 82.749868] livepatch: 'livepatch_callbacks_demo': unpatching complete >> + [ 82.765809] insmod: ERROR: could not insert module >> samples/livepatch/livepatch-callbacks-demo.ko: No such device >> + >> + % rmmod samples/livepatch/livepatch-callbacks-mod.ko >> + [ 84.774238] livepatch_callbacks_mod: livepatch_callbacks_mod_exit > > First off, this documentation is very nice because it clarifies all the > callback scenarios and edge cases. > > The above situation still seems a little odd to me. If I understand > correctly, the target module was never patched, and its pre_patch > callback was never called. But its post_unpatch callback *was* called. > That doesn't seem right.
Ah, this does look to be a bug. > Maybe we should change the condition a little bit. Currently it's: > > No post-patch, pre-unpatch, or post-unpatch callbacks will be executed > for a given klp_object if its pre-patch callback returned non-zero > status. > > I think that might have been my idea, but seeing the above case makes it > clear that it's not quite right. It could have been correct if the code differentiated between a never-run pre_patch_status of 0 (by kzalloc) and a successful pre_patch_status of 0 (by callback return), I think. > Maybe it should instead be: > > No post-patch, pre-unpatch, or post-unpatch callbacks will be executed > for a given klp_object if the object failed to patch, due to a failed > pre_patch callback or for any other reason. > > If the object did successfully patch, but the patch transition never > started for some reason (e.g., if another object failed to patch), > only the post-unpatch callback will be called. That description sounds correct... > So then, instead of tracking whether the pre-patch callback succeeded, > we just need to track whether the object was patched (which we already > do, with obj->patched). > > What do you think? I think this would only work if there was a sticky "obj->was_ever_patched" variable. We moved the post-unpatch-callback to the very end of klp_complete_transition()... by that point, obj->patched will have already been cleared by klp_unpatch_objects. We could maybe move obj->patched assignments out to encapsulate the pre and post callbacks... but I would need to think about that a while. It seems pretty clear and symmetric as it is today (immediately set in klp_(un)patch_object(). Perhaps a more careful checking of obj->pre_patch_callback_status is all we need? (I can't think of anything more succinct than adding a obj->pre_patch_callback_done variable to the mix.) -- Joe
