Re: [PATCH 1/3] livepatch: Add klp_object and klp_func iterators

[Petr Mladek]

On Wed 2017-07-19 13:18:05, Jason Baron wrote:
> In preparation to introducing atomic replace, introduce iterators for klp_func
> and klp_object, such that objects and functions can be dynmically allocated
> (needed for atomic replace). Note that this patch is careful, not to grow the
> size of klp_func as that's the most common data structure. This patch is
> intended to effectively be a no-op until atomic replace is
> introduced.

We need to be careful here. The 3-level hiearachy is already complex
enough. This patch adds several asymetric things:

	+ the dynamically alocated structures are put into liked
	  lists while the static ones are in arrays

	+ the dynamically allocated func lists are linked using
	  an external struct klp_func_no_op while
	  the dynamically allocated obj lists are linked using
	  additional list_head in struct klp_object

Well, I agree that the combination of the iterators and extra no-op funcs
allow to implement the atomic replace relatively easy and transparent
way.

Another possibility would be to add some special flags to the
affected patches on the stack. But this would require hacks
in the ftrace handler, going through all patches in
klp_try_complete_transition() and other functions.
It looks more complicated.

I just wonder if we could clean this patch a bit.
Please, find some thoughts below.


> 
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 194991e..5038337 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -24,6 +24,7 @@
>  #include <linux/module.h>
>  #include <linux/ftrace.h>
>  #include <linux/completion.h>
> +#include <linux/list.h>
>  
>  #if IS_ENABLED(CONFIG_LIVEPATCH)
>  
> @@ -88,10 +89,23 @@ struct klp_func {
>  };
>  
>  /**
> + * struct klp_func_no_op - internal object used to link no_op functions, which
> +			   avoids the need to bloat struct klp_func
> + * @orig_func:	embeds struct klp_func
> + * @func_entry:	used link struct klp_func_no_op to struct klp_object
> + */
> +struct klp_func_no_op {
> +	struct klp_func orig_func;
> +	struct list_head func_entry;
> +};

I am not sure that this is worth it. It saves two pointers in
struct klp-func that are mostly unused. But it adds one more
asymmetry that would complicate maintaining the code.

struct klp_func is already quite big. Note that struct kobj
is hidden there. I think that we could afford one more
struct list_head there.

If people are concerned about the size, we could make this
feature configurable at build time. People using this feature
will benefit from the the ability to remove the replaced patches.

I also thought about using arrays also for the dynamic structures.
But we would need to compute the size first. It might require
another crazy code, especially when we allow to replace all
patches and need to look for duplicates.

> +/**
>   * struct klp_object - kernel object structure for live patching
>   * @name:	module name (or NULL for vmlinux)
>   * @funcs:	function entries for functions to be patched in the object
>   * @kobj:	kobject for sysfs resources
> + * @func_list:	head of list for struct klp_func_no_op
> + * @obj_entry:	used to link struct klp_object to struct klp_patch

I would prefer to make the difference between the static
and dynamic stuff more obvious. The generic names for both
variants are confusing.

I would personally use "no_op" in the names of list_heads related
to the no_op stuff. But then it would make more sense to add this
in the next patch.

Well, I do not have strong opinion about it.


>   * @mod:	kernel module associated with the patched object
>   *		(NULL for vmlinux)
>   * @patched:	the object's funcs have been added to the klp_ops list
> @@ -103,6 +117,8 @@ struct klp_object {
>  
>  	/* internal */
>  	struct kobject kobj;
> +	struct list_head func_list;
> +	struct list_head obj_entry;
>  	struct module *mod;
>  	bool patched;
>  };
> @@ -114,6 +130,7 @@ struct klp_object {
>   * @immediate:  patch all funcs immediately, bypassing safety mechanisms
>   * @list:	list node for global list of registered patches
>   * @kobj:	kobject for sysfs resources
> + * @obj_list:	head of list for dynamically allocated struct klp_object
>   * @enabled:	the patch is enabled (but operation may be incomplete)
>   * @finish:	for waiting till it is safe to remove the patch module
>   */
> @@ -126,17 +143,96 @@ struct klp_patch {
>  	/* internal */
>  	struct list_head list;
>  	struct kobject kobj;
> +	struct list_head obj_list;
>  	bool enabled;
>  	struct completion finish;
>  };
>  
> -#define klp_for_each_object(patch, obj) \
> +struct obj_iter {
> +	struct klp_object *obj;
> +	struct list_head *obj_list_head;
> +	struct list_head *obj_list_pos;
> +};
> +
> +static inline struct klp_object *obj_iter_next(struct obj_iter *iter)
> +{
> +	struct klp_object *obj;
> +
> +	if (iter->obj->funcs || iter->obj->name) {
> +		obj = iter->obj;
> +		iter->obj++;
> +	} else {
> +		if (iter->obj_list_pos == iter->obj_list_head) {
> +			obj = NULL;
> +		} else {
> +			obj = list_entry(iter->obj_list_pos, struct klp_object,
> +					 obj_entry);
> +			iter->obj_list_pos = iter->obj_list_pos->next;
> +		}
> +	}

This code might deserve some comments. Well, I wonder if we
could make it working without the struct obj_iter.

If we could somehow distinguish the statically and dynamically
allocated struct klp_object then we would know how to find the next
one. I mean something like:

static inline struct klp_object *klp_next_object(struct klp_patch *patch,
						struct klp_object *obj)
{
	struct klp_object *next_obj = NULL;

	/* Only statically defined objects has funcs array. */
	if (obj->funcs) {
		next_obj = obj + 1;
		if (next_obj)
			goto out;

		/* Is there any dynamically allocated object? */ 
		if (!list_empty(patch->obj_list)) {
			next_obj = container_of(patch->obj_list.next,
					struct klp_object,
					obj_entry);
			goto out;
	}

	/* This must be a dynamically allocated object. Is it the last one? */
	if (obj->obj_entry.next != patch->obj_list)
		next_obj = container_of(obj->obj_entry.next,
					struct klp_object,
					obj_entry);

out:
	return next_obj;
}

Note that this is not even compile tested.

Best Regards,
Petr