From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753058AbdHXPjX (ORCPT ); Thu, 24 Aug 2017 11:39:23 -0400 Received: from mx0a-00190b01.pphosted.com ([67.231.149.131]:34912 "EHLO mx0a-00190b01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751348AbdHXPjV (ORCPT ); Thu, 24 Aug 2017 11:39:21 -0400 Subject: Re: [PATCH 1/3] livepatch: Add klp_object and klp_func iterators To: Petr Mladek Cc: linux-kernel@vger.kernel.org, live-patching@vger.kernel.org, jpoimboe@redhat.com, jeyu@kernel.org, jikos@kernel.org, mbenes@suse.cz References: <93efd1d05081843c97ae090333873784f214f17d.1500482335.git.jbaron@akamai.com> <20170824142541.GF30286@pathway.suse.cz> From: Jason Baron Message-ID: Date: Thu, 24 Aug 2017 11:39:08 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: <20170824142541.GF30286@pathway.suse.cz> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-08-24_06:,, signatures=0 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1707230000 definitions=main-1708240245 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-08-24_06:,, signatures=0 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 priorityscore=1501 malwarescore=0 suspectscore=0 phishscore=0 bulkscore=0 spamscore=0 clxscore=1011 lowpriorityscore=0 impostorscore=0 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1707230000 definitions=main-1708240245 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >> #include >> #include >> +#include >> >> #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 >