Message-ID: <ce09b7c8-a2a3-a0d5-eefe-0706d47e2404@akamai.com>
Date:   Thu, 24 Aug 2017 11:39:08 -0400
From:   Jason Baron <jbaron@...mai.com>
To:     Petr Mladek <pmladek@...e.com>
Cc:     linux-kernel@...r.kernel.org, live-patching@...r.kernel.org,
        jpoimboe@...hat.com, jeyu@...nel.org, jikos@...nel.org,
        mbenes@...e.cz
Subject: Re: [PATCH 1/3] livepatch: Add klp_object and klp_func iterators

Hi Petr,

On 08/24/2017 10:25 AM, Petr Mladek wrote:
> 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.
> 
Ok, I can remove the special 'struct klp_func_no_op', I agree it adds
complexity to the code, and perhaps could be viewed as some later
optimization if deemed a size bloat.

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

I think I avoided 'no_op' in the naming because I thought it maybe could
be a more generic list, but in practice I'm only using it for the
dynamic noops, so I like explicitly calling it the no_ops list to make
its usage clear. I also think it can still live in 1/2 with the 'no_op'
name, but if that's not ok, I could just rename it in 2/2.

> 
>>   * @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:
> 

yes, removing the obj_iter would be nice, and I think something like
below could work. Another way to distinguish the objects might be to
simply check if embedded list_head is used. That test would then also be
able to be used for the function iterator (or I guess if its a no_op
function, but I kind of like using the same test). I will try this out.

Thanks,

-Jason

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

Powered by blists - more mailing lists