On Thu, Nov 29, 2018 at 10:44:25AM +0100, Petr Mladek wrote:
> @@ -309,40 +297,33 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
>
> mutex_lock(&klp_mutex);
>
> - if (!klp_is_patch_registered(patch)) {
> - /*
> - * Module with the patch could either disappear meanwhile or is
> - * not properly initialized yet.
> - */
> - ret = -EINVAL;
> - goto err;
> - }
> -
> if (patch->enabled == enabled) {
> /* already in requested state */
> ret = -EINVAL;
> - goto err;
> + goto out;
> }
>
> - if (patch == klp_transition_patch) {
> + /*
> + * Allow to reverse a pending transition in both ways. It might be
> + * necessary to complete the transition without forcing and breaking
> + * the system integrity.
> + *
> + * Do not allow to re-enable a disabled patch because this interface
> + * is being destroyed.
> + */
> + if (patch == klp_transition_patch)
> klp_reverse_transition();
> - } else if (enabled) {
> - ret = __klp_enable_patch(patch);
> - if (ret)
> - goto err;
> - } else {
> + else if (!enabled)
> ret = __klp_disable_patch(patch);
> - if (ret)
> - goto err;
> - }
> + else
> + ret = -EINVAL;
Now that we can't re-enable a patch, I wonder if we really need both the
'patch->enabled' and 'klp_target_state' variables?
A patch is now always enabled, unless it's in transition, in which case
its 'enabled' state is the same as 'klp_target_state'.
For example I wonder if we could get rid of 'klp_target_state', since it
should be the same as 'klp_transition_patch->enabled'.
Or alternatively we could get rid of 'patch->enabled', since it should
be the same as
patch == klp_transition_patch ? klp_target_state : true
Of course this could be a follow-on cleanup patch, which could be done
in the future, so as not to hold up the merging of these patches
anymore.
--
Josh
