DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 674E6C001CF1
From: Joe Lawrence <joe.lawrence@redhat.com>
Subject: Re: [PATCH v2 1/1] livepatch: add (un)patch callbacks
To: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: live-patching@vger.kernel.org, linux-kernel@vger.kernel.org,
        Jessica Yu <jeyu@kernel.org>, Jiri Kosina <jikos@kernel.org>,
        Miroslav Benes <mbenes@suse.cz>, Petr Mladek <pmladek@suse.com>,
        Chris J Arges <chris.j.arges@canonical.com>
References: <1502220967-21410-1-git-send-email-joe.lawrence@redhat.com>
 <1502220967-21410-2-git-send-email-joe.lawrence@redhat.com>
 <20170811204421.ua2du65bmt3jdbgw@treble>
Organization: Red Hat
Message-ID: <23103cac-6f01-bb3c-f318-d273c2f1066b@redhat.com>
Date: Mon, 14 Aug 2017 10:53:08 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.2.0
MIME-Version: 1.0
In-Reply-To: <20170811204421.ua2du65bmt3jdbgw@treble>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 6582
Lines: 189

On 08/11/2017 04:44 PM, Josh Poimboeuf wrote:
> On Tue, Aug 08, 2017 at 03:36:07PM -0400, Joe Lawrence wrote:
>> +++ b/Documentation/livepatch/callbacks.txt
>> @@ -0,0 +1,75 @@
>> +(Un)patching Callbacks
>> +======================
>> +
>> +Livepatch (un)patch-callbacks provide a mechanism for livepatch modules
>> +to execute callback functions when a kernel object is (un)patched.
> 
> I think it would be helpful to put a little blurb here about why
> callbacks are needed and when they might be used.  Maybe steal some of
> the description from the first two bullet points here:
> 
>   https://lkml.kernel.org/r/20170720041723.35r6qk2fia7xix3t@treble

Ok -- btw, can you explain this point: "patching otherwise unpatchable
code (i.e., assembly)".  I wasn't sure if you were referring to the
actual code, or modifying the machine state as setup by some init time
assembly.

> Also, I tested stop_machine() in the callbacks and it seemed to work
> fine.  It might be worth mentioning in the docs that it's an option.

I'll file that under the "you better know what you're doing" section. :)
If your test would be a better use-case example or sample module than
what's currently in the patchset, feel free to send it over and I can
incorporate it.

>> +
>> +These callbacks differ from existing kernel facilities:
>> +
>> +  - Module init/exit code doesn't run when disabling and re-enabling a
>> +    patch.
>> +
>> +  - A module notifier can't stop the to-be-patched module from loading.
>> +
>> +Callbacks are part of the klp_object structure and their implementation
>> +is specific to the given object.  Other livepatch objects may or may not
>> +be patched, irrespective of the target klp_object's current state.
>> +
>> +Callbacks can be registered for the following livepatch actions:
>> +
>> +  * Pre-patch    - before klp_object is patched
>> +
>> +  * Post-patch   - after klp_object has been patched and is active
>> +                   across all tasks
>> +
>> +  * Pre-unpatch  - before klp_object is unpatched, patched code is active
>> +
>> +  * Post-unpatch - after klp_object has been patched, all code has been
>> +		   restored and no tasks are running patched code
>> +
>> +Callbacks are only executed if its host klp_object is loaded.  For
> 
> "Callbacks are" -> "A callback is" ?

Okay.  What about the preceding plural-case instances?

> 
>> +static inline int klp_pre_patch_callback(struct klp_object *obj)
>> +{
>> +	if (!obj->patched && obj->callbacks.pre_patch)
>> +		return (*obj->callbacks.pre_patch)(obj);
>> +	return 0;
>> +}
>> +static inline void klp_post_patch_callback(struct klp_object *obj)
>> +{
>> +	if (obj->patched && obj->callbacks.post_patch)
>> +		(*obj->callbacks.post_patch)(obj);
>> +}
>> +static inline void klp_pre_unpatch_callback(struct klp_object *obj)
>> +{
>> +	if (obj->patched && obj->callbacks.pre_unpatch)
>> +		(*obj->callbacks.pre_unpatch)(obj);
>> +}
>> +static inline void klp_post_unpatch_callback(struct klp_object *obj)
>> +{
>> +	if (!obj->patched && obj->callbacks.post_unpatch)
>> +		(*obj->callbacks.post_unpatch)(obj);
>> +}
>> +
> 
> Do these need the obj->patched checks?  As far as I can tell they seem
> to be called in the right places and the checks are superfluous.

That is correct.  I can leave them (defensive coding) or take them out
and perhaps add comments above to explain their use and assumptions.

>> --- a/kernel/livepatch/transition.c
>> +++ b/kernel/livepatch/transition.c
>> @@ -109,9 +109,6 @@ static void klp_complete_transition(void)
>>  		}
>>  	}
>>  
>> -	if (klp_target_state == KLP_UNPATCHED && !immediate_func)
>> -		module_put(klp_transition_patch->mod);
>> -
>>  	/* Prevent klp_ftrace_handler() from seeing KLP_UNDEFINED state */
>>  	if (klp_target_state == KLP_PATCHED)
>>  		klp_synchronize_transition();
>> @@ -130,6 +127,22 @@ static void klp_complete_transition(void)
>>  	}
>>  
>>  done:
>> +	klp_for_each_object(klp_transition_patch, obj) {
>> +		if (klp_target_state == KLP_PATCHED)
>> +			klp_post_patch_callback(obj);
>> +		else if (klp_target_state == KLP_PATCHED)
> 
> s/KLP_PATCHED/KLP_UNPATCHED

Ahh, I was so focused on the loadable module cases in
module_coming/going that I botched this case.  Will fix for v3.

>> +			klp_post_unpatch_callback(obj);
>> +	}
>> +
>> +	/*
>> +	 * See complementary comment in __klp_enable_patch() for why we
>> +	 * keep the module reference for immediate patches.
>> +	 */
>> +	if (!klp_transition_patch->immediate) {
>> +		if (klp_target_state == KLP_UNPATCHED && !immediate_func)
>> +			module_put(klp_transition_patch->mod);
>> +	}
>> +
> 
> Maybe combine these into a single 'if' for clarity:
> 
> 	if (klp_target_state == KLP_UNPATCHED && !immediate_func &&
> 	    !klp_transition_patch->immediate)
> 		module_put(klp_transition_patch->mod);

How about this arrangement:

if (!klp_transition_patch->immediate &&
    klp_target_state == KLP_UNPATCHED && !immediate_func) {
	module_put(klp_transition_patch->mod);
}

1) It leads with the klp_transition_patch->immediate variable, which the
preceding comment and goto is all about and 2) brackets the multiline
conditional part -- a personal preference, but I could drop for
convention sake.

>> + * NOTE: 'pre_patch_ret' is a module parameter that sets the pre-patch
>> + *       callback return status.  Try setting up a non-zero status
>> + *       such as -19 (-ENODEV):
>> + *
>> + *       # Load demo livepatch, vmlinux is patched
>> + *       insmod samples/livepatch/livepatch-callbacks-demo.ko
>> + *
>> + *       # Setup next pre-patch callback to return -ENODEV
>> + *       echo -19 > /sys/module/livepatch_callbacks_demo/parameters/pre_patch_ret 
> 
> Git complained about trailing whitespace here ^
> 
>> + *
>> + *       # Module loader refuses to load the target module
>> + *       insmod samples/livepatch/livepatch-callbacks-mod.ko 
> 
> and here ^

Oh hey, look who was too cool to run checkpatch, again.

>> +/* Executed on object unpatching (ie, patch disablement) */
>> +static void post_patch_callback(struct klp_object *obj)
> 
> s/unpatching/patching/
> 

Good catch.

So v2 was a bit rushed to try and get something out there to talk about:

Are the callback locations accurate to your v1 suggestions?

How do you feel about a pre-patch callback potentially preventing the
loading of a kernel module -or- the patch module itself depending on
which is loaded first?

Is the pre-patch return status sufficient? (ie, I couldn't see how
post-patch, pre-unpatch, post-patch callbacks could affect the actions
already set in motion.)

Thanks,

-- Joe