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