DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 97D2291EA3
Date: Fri, 11 Aug 2017 15:44:21 -0500
From: Josh Poimboeuf <jpoimboe@redhat.com>
To: Joe Lawrence <joe.lawrence@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>
Subject: Re: [PATCH v2 1/1] livepatch: add (un)patch callbacks
Message-ID: <20170811204421.ua2du65bmt3jdbgw@treble>
References: <1502220967-21410-1-git-send-email-joe.lawrence@redhat.com>
 <1502220967-21410-2-git-send-email-joe.lawrence@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <1502220967-21410-2-git-send-email-joe.lawrence@redhat.com>
User-Agent: Mutt/1.6.0.1 (2016-04-01)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4670
Lines: 139

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

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.

> +
> +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" ?

> +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.

> --- 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

> +			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);

> + * 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 ^

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

s/unpatching/patching/

-- 
Josh