Re: [PATCHv6 2/3] kernel: add support for live patching

[Josh Poimboeuf <jpoimboe@redhat.com>]

On Fri, Dec 12, 2014 at 05:58:19PM +0100, Miroslav Benes wrote:
> 
> Hi, 
> 
> I think we are really close (or I hope so). I found few suspicious things 
> or nitpicks though. They might have applied also to v5, but I didn't 
> manage to look at that. Sorry about that.
> 
> On Wed, 10 Dec 2014, Josh Poimboeuf wrote:
> 
> > +/* klp_mutex must be held by caller */
> > +static bool klp_patch_is_registered(struct klp_patch *patch)
> 
> Maybe klp_is_patch_registered is more appropriate name (consistent with 
> other predicates in the file).

Ok.

> > +static int klp_disable_func(struct klp_func *func)
> > +{
> > +	int ret;
> > +
> > +	if (WARN_ON(func->state != KLP_ENABLED))
> > +		return -EINVAL;
> > +
> > +	if (WARN_ON(!func->old_addr))
> > +		return -EINVAL;
> > +
> > +	ret = unregister_ftrace_function(func->fops);
> > +	if (ret) {
> > +		pr_err("failed to unregister ftrace handler for function '%s' (%d)\n",
> > +		       func->old_name, ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = ftrace_set_filter_ip(func->fops, func->old_addr, 1, 0);
> > +	if (ret)
> > +		pr_warn("function unregister succeeded but failed to clear the filter\n");
> > +
> > +	func->state = KLP_DISABLED;
> > +
> > +	return 0;
> > +}
> > +
> > +static int klp_enable_func(struct klp_func *func)
> > +{
> > +	int ret;
> > +
> > +	if (WARN_ON(!func->old_addr))
> > +		return -EINVAL;
> > +
> > +	if (WARN_ON(func->state != KLP_DISABLED))
> > +		return -EINVAL;
> > +
> > +	ret = ftrace_set_filter_ip(func->fops, func->old_addr, 0, 0);
> > +	if (ret) {
> > +		pr_err("failed to set ftrace filter for function '%s' (%d)\n",
> > +		       func->old_name, ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = register_ftrace_function(func->fops);
> > +	if (ret) {
> > +		pr_err("failed to register ftrace handler for function '%s' (%d)\n",
> > +		       func->old_name, ret);
> > +		ftrace_set_filter_ip(func->fops, func->old_addr, 1, 0);
> > +	} else {
> > +		func->state = KLP_ENABLED;
> > +	}
> > +
> > +	return ret;
> > +}
> 
> Just to be sure about our policy. We want to be stricter during enabling 
> than in disabling process. Is that correct? Otherwise there is 
> inconsistency in pr_* macros and return values. Also fops could be 
> hypothetically registered back when ftrace_set_filter_ip fails in 
> klp_disable_func. I just want to be sure that we didn't overlook 
> anything...

The asymmetry in the enable/disable error handling is intentional.  In
klp_disable_func(), a ftrace_set_filter_ip() failure isn't a fatal
condition because we've already unregistered the fops and thus removed
the patch.

> > +static int klp_init_func(struct klp_object *obj, struct klp_func *func)
> > +{
> > +	struct ftrace_ops *ops;
> > +	int ret;
> > +
> > +	func->state = KLP_DISABLED;
> > +
> > +	ops = kzalloc(sizeof(*ops), GFP_KERNEL);
> > +	if (!ops)
> > +		ret = -ENOMEM;
> 
> There should be return -ENOMEM.

Agreed.

> > +static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
> > +{
> > +	struct klp_func *func;
> > +	int ret;
> > +	const char *name;
> > +
> > +	if (!obj->funcs)
> > +		return -EINVAL;
> > +
> > +	obj->state = KLP_DISABLED;
> > +
> > +	klp_find_object_module(obj);
> > +
> > +	name = klp_is_module(obj) ? obj->name : "vmlinux";
> > +	obj->kobj = kobject_create_and_add(name, &patch->kobj);
> > +	if (!obj->kobj)
> > +		return -ENOMEM;
> > +
> > +	for (func = obj->funcs; func->old_name; func++) {
> > +		ret = klp_init_func(obj, func);
> > +		if (ret)
> > +			goto free;
> > +	}
> > +
> > +	if (klp_is_object_loaded(obj)) {
> > +		ret = klp_init_object_loaded(patch, obj);
> > +		if (ret)
> > +			goto free;
> > +	}
> > +
> > +	return 0;
> > +
> > +free:
> > +	klp_free_funcs_limited(obj, func);
> > +	return ret;
> > +}
> 
> Shouldn't we call kobject_put(obj->kobj) in free branch? If I am not wrong 
> it is not freed anywhere else. We free only already initialized functions 
> and already initialized objects later in klp_init_patch, but not the 
> kobject of the currently failing object.

Agreed.

> And that is everything. I like it, it has improved a lot. I hope that 
> there are no other problems. I am getting blind looking at it all the 
> time :)

Thanks!  I'll send out the next patch set soon, maybe Monday.

-- 
Josh