LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Petr Mladek <pmladek@suse.com>
To: Miroslav Benes <mbenes@suse.cz>
Cc: Joe Lawrence <joe.lawrence@redhat.com>,
	Josh Poimboeuf <jpoimboe@redhat.com>,
	jikos@kernel.org, peterz@infradead.org,
	linux-kernel@vger.kernel.org, live-patching@vger.kernel.org,
	shuah@kernel.org, linux-kselftest@vger.kernel.org
Subject: Re: [PATCH 2/3] livepatch: Allow user to specify functions to search for on a stack
Date: Fri, 3 Dec 2021 17:01:37 +0100	[thread overview]
Message-ID: <Yao/YQlP0kz+lwdN@alley> (raw)
In-Reply-To: <alpine.LSU.2.21.2111251110130.28836@pobox.suse.cz>

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

  reply	other threads:[~2021-12-03 16:01 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-11-19  9:03 [PATCH 0/3] " Miroslav Benes
2021-11-19  9:03 ` [PATCH 1/3] livepatch: Move the initialization of old_func to a new function Miroslav Benes
2021-11-19  9:03 ` [PATCH 2/3] livepatch: Allow user to specify functions to search for on a stack Miroslav Benes
2021-11-19 10:17   ` Peter Zijlstra
2021-11-19 18:20   ` Josh Poimboeuf
2021-11-22  7:57     ` Miroslav Benes
2021-11-22 15:53       ` Joe Lawrence
2021-11-25 10:07         ` Petr Mladek
2021-11-25 10:20           ` Miroslav Benes
2021-12-03 16:01             ` Petr Mladek [this message]
2021-11-19  9:03 ` [PATCH 3/3] selftests/livepatch: Test of the API for specifying " Miroslav Benes
2021-11-25 14:39   ` Petr Mladek
2021-11-26  9:20     ` Miroslav Benes
2021-11-26 14:06       ` Petr Mladek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Yao/YQlP0kz+lwdN@alley \
    --to=pmladek@suse.com \
    --cc=jikos@kernel.org \
    --cc=joe.lawrence@redhat.com \
    --cc=jpoimboe@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=live-patching@vger.kernel.org \
    --cc=mbenes@suse.cz \
    --cc=peterz@infradead.org \
    --cc=shuah@kernel.org \
    --subject='Re: [PATCH 2/3] livepatch: Allow user to specify functions to search for on a stack' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).