On Thu 2021-11-25 11:20:04, Miroslav Benes wrote:
> On Thu, 25 Nov 2021, Petr Mladek wrote:
> > On Mon 2021-11-22 10:53:21, Joe Lawrence wrote:
> > > On 11/22/21 2:57 AM, Miroslav Benes wrote:
> > > > On Fri, 19 Nov 2021, Josh Poimboeuf wrote:
> > > >>> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> > > >>> index 2614247a9781..89df578af8c3 100644
> > > >>> --- a/include/linux/livepatch.h
> > > >>> +++ b/include/linux/livepatch.h
> > > >>> @@ -106,9 +106,11 @@ struct klp_callbacks {
> > > >>> * 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
> > > >>> + * @funcs_stack: function entries for functions to be stack checked
> > > >>
> > > >> So there are two arrays/lists of 'klp_func', and two implied meanings of
> > > >> what a 'klp_func' is and how it's initialized.
> > > >>
> > > >> Might it be simpler and more explicit to just add a new external field
> > > >> to 'klp_func' and continue to have a single 'funcs' array? Similar to
> > > >> what we already do with the special-casing of 'nop', except it would be
> > > >> an external field, e.g. 'no_patch' or 'stack_only'.
> > >
> > > I'll add that the first thing that came to mind when you raised this
> > > feature idea in the other thread was to support existing klp_funcs array
> > > with NULL new_func's.
> >
> > Please, solve this with the extra flag, e.g. .stack_only, as
> > already suggested. It will help to distinguish mistakes and
> > intentions. Also it will allow to find these symbols by grep.
>
> Indeed, that is what I did for v2. I used new_func being NULL fact even in
> v1, but I do not like it much. Extra flag is definitely more robust.
>
> > > I didn't go look to see how invasive it would be,
> > > but it will be interesting to see if a single list approach turns out
> > > any simpler for v2.
> >
> > I am not sure either. But I expect that it will be easier than
> > the extra array.
>
> So, extra flag and one array makes certain things (initialization)
> definitely easier. On the other hand, there are suddenly more problems to
> think about (and I haven't solved them yet):
>
> - I need to make it work with nops functions. Especially if we allow a
> scenario where there could be klp_object instance with just stack_only
> functions. Would that be useful? For example, to patch something in a
> module and add a stack_only for a function in vmlinux.
My naive expectation is that the struct klp_func with @stack_only
flag might be handled the same way on most locations, except when:
+ func->new_func is handled
+ ftrace handler is added/removed
Something like:
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -724,7 +724,7 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
* NOPs get the address later. The patched module must be loaded,
* see klp_init_object_loaded().
*/
- if (!func->new_func && !func->nop)
+ if (!func->new_func && !func->nop && !func->stack_only)
return -EINVAL;
if (strlen(func->old_name) >= KSYM_NAME_LEN)
@@ -801,6 +801,9 @@ static int klp_init_object_loaded(struct klp_patch *patch,
return -ENOENT;
}
+ if (func->stack_only)
+ continue;
+
if (func->nop)
func->new_func = func->old_func;
--- a/kernel/livepatch/patch.c
+++ b/kernel/livepatch/patch.c
@@ -247,6 +247,9 @@ static void __klp_unpatch_object(struct klp_object *obj, bool nops_only)
struct klp_func *func;
klp_for_each_func(obj, func) {
+ if (func->stack_only)
+ continue;
+
if (nops_only && !func->nop)
continue;
@@ -273,6 +276,8 @@ int klp_patch_object(struct klp_object *obj)
return -EINVAL;
klp_for_each_func(obj, func) {
+ if (func->stack_only)
+ continue;
ret = klp_patch_func(func);
if (ret) {
klp_unpatch_object(obj);
> If yes, then the interaction with nops is not completely
> straightforward and also some parts of the code would have to be
> changed (for example how obj->patched flag is handled).
I would keep func->patched disabled for @stack_only entries.
But they should not affect obj->patched state. I mean that
obj->patched should be set when the patch is enabled even when
there are only "stack_only" funcs.
It might look a bit unclear. A solution might be to rename the flags:
+ func->patched -> func->active (and set even for @stack_only)
+ obj->patched -> obj->active (same as func)
But I am not sure if it is worth it.
> - klp_func instances are directly mirrored in sysfs. Do we want to keep
> stack_only functions there too? If not, it makes the whole thing
> slighly more difficult given how we manage kobjects.
I would keep them in sysfs. It will be easier and it does not harm.
> Nothing really difficult to implement if we come up with answers.
I am sorry for not answering this earlier. I have missed the questions :-(
Best Regards,
Petr
