Re: [PATCH 1/3] livepatch: Add force sysfs attribute

[Libor Pechacek <lpechacek@suse.com>]

On Thu 18-05-17 14:00:41, Miroslav Benes wrote:
> Add write-only force attribute to livepatch sysfs infrastructure. We can
> use it later to force couple of events during a live patching process.
> Be it a sending of a fake signal or forcing of the tasks' successful
> conversion.
> 
> It does not make sense to use the force facility when there is no
> transaction running (although there is no harm doing that). Therefore we
> limit only to situations when klp_transition_patch variable is set.
> Normally, klp_mutex lock should be grabbed, because the variable is
> shared. However that would hold the action back unnecessarily because of
> waiting for the lock, so we omit the lock here. The resulting race
> window is harmless (using force when there is no transaction running).
> 
> Signed-off-by: Miroslav Benes <mbenes@suse.cz>
> ---
>  Documentation/ABI/testing/sysfs-kernel-livepatch |  9 +++++
>  kernel/livepatch/core.c                          | 45 ++++++++++++++++++++++++
>  2 files changed, 54 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-kernel-livepatch b/Documentation/ABI/testing/sysfs-kernel-livepatch
> index d5d39748382f..26e9f58cea9e 100644
> --- a/Documentation/ABI/testing/sysfs-kernel-livepatch
> +++ b/Documentation/ABI/testing/sysfs-kernel-livepatch
> @@ -8,6 +8,15 @@ Contact:	live-patching@vger.kernel.org
>  		The /sys/kernel/livepatch directory contains subdirectories for
>  		each loaded live patch module.
>  
> +What:		/sys/kernel/livepatch/force
> +Date:		May 2017
> +KernelVersion:	4.13.0
> +Contact:	live-patching@vger.kernel.org
> +Description:
> +		A write-only attribute that allows administrator to affect the
> +		course of an existing transition. A fake signal can be send or
								       ^^^^
								      "sent"

> +		tasks TIF can be cleared.
> +
>  What:		/sys/kernel/livepatch/<patch>
>  Date:		Nov 2014
>  KernelVersion:	3.19.0
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index b9628e43c78f..84f8944704ad 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -437,6 +437,7 @@ EXPORT_SYMBOL_GPL(klp_enable_patch);
>   * Sysfs Interface
>   *
>   * /sys/kernel/livepatch
> + * /sys/kernel/livepatch/force
>   * /sys/kernel/livepatch/<patch>
>   * /sys/kernel/livepatch/<patch>/enabled
>   * /sys/kernel/livepatch/<patch>/transition
> @@ -444,6 +445,43 @@ EXPORT_SYMBOL_GPL(klp_enable_patch);
>   * /sys/kernel/livepatch/<patch>/<object>/<function,sympos>
>   */
>  
> +static ssize_t force_store(struct kobject *kobj, struct kobj_attribute *attr,
> +			   const char *buf, size_t count)
> +{
> +	unsigned long val;
> +	int ret;
> +
> +	ret = kstrtoul(buf, 10, &val);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * klp_mutex lock is not grabbed here intentionally. It is not really
> +	 * needed. The race window is harmless and grabbing the lock would only
> +	 * hold the action back.
> +	 */
> +	if (!klp_transition_patch) {
> +		pr_info("no patching in progress. Force not allowed\n");

proposing smoother wording and information sharing
pr_info("no patching in progress, forced action (%d) ineffective", val);

> +		return -EINVAL;
> +	}
> +
> +	switch (val) {

I felt strong confusion for a while looking at a function what does nothing. A
comment that this is intentionally an empty shell, at this stage, would be
welcome.

> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return count;
> +}
> +
> +static struct kobj_attribute force_kobj_attr = __ATTR_WO(force);
> +static struct attribute *klp_attrs[] = {
> +	&force_kobj_attr.attr,
> +	NULL
> +};
> +static struct attribute_group klp_sysfs_group = {
> +	.attrs = klp_attrs,
> +};
> +
>  static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
>  			     const char *buf, size_t count)
>  {
> @@ -954,6 +992,13 @@ static int __init klp_init(void)
>  	if (!klp_root_kobj)
>  		return -ENOMEM;
>  
> +	ret = sysfs_create_group(klp_root_kobj, &klp_sysfs_group);
> +	if (ret) {
> +		pr_err("cannot create livepatch attributes in sysfs\n");
> +		kobject_put(klp_root_kobj);
> +		return ret;
> +	}
> +
>  	return 0;
>  }
>  

Libor

> -- 
> 2.12.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe live-patching" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Libor Pechacek
SUSE Labs