From: Petr Mladek <pmladek@suse.com>
To: Miroslav Benes <mbenes@suse.cz>
Cc: Jiri Kosina <jikos@kernel.org>,
Josh Poimboeuf <jpoimboe@redhat.com>,
Jason Baron <jbaron@akamai.com>,
Joe Lawrence <joe.lawrence@redhat.com>,
Jessica Yu <jeyu@kernel.org>,
Evgenii Shatokhin <eshatokhin@virtuozzo.com>,
live-patching@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v12 06/12] livepatch: Simplify API by removing registration step
Date: Fri, 12 Oct 2018 15:01:20 +0200 [thread overview]
Message-ID: <20181012130120.f5berowklyccd7lj@pathway.suse.cz> (raw)
In-Reply-To: <alpine.LSU.2.21.1809051108440.7768@pobox.suse.cz>
On Wed 2018-09-05 11:34:06, Miroslav Benes wrote:
> On Tue, 28 Aug 2018, Petr Mladek wrote:
> > Also the API and logic is much easier. It is enough to call
> > klp_enable_patch() in module_init() call. The patch patch can be disabled
> > by writing '0' into /sys/kernel/livepatch/<patch>/enabled. Then the module
> > can be removed once the transition finishes and sysfs interface is freed.
>
> I think it would be good to discuss our sysfs interface here as well.
>
> Writing '1' to enabled attribute now makes sense only when you need to
> reverse an unpatching transition. Writing '0' means "disable" or a
> reversion again.
>
> Wouldn't be better to split it to two different attributes? Something like
> "disable" and "reverse"? It could be more intuitive.
>
> Maybe we'd also find out that even patch->enabled member is not useful
> anymore in such case.
I though about this as well. I kept "enabled" because:
+ It keeps the public interface the same as before. Most people
would not notice any change in the behavior except maybe that
the interface disappears when the patch gets disabled.
+ The reverse operation makes most sense when the transition
cannot get finished. In theory, it might be problem to
finish even the reversed one. People might want to
reverse once again and force it. Then "reverse" file
might be confusing. They might not know in which direction
they do the reverse.
> > @@ -846,17 +740,8 @@ static int __klp_enable_patch(struct klp_patch *patch)
> > if (WARN_ON(patch->enabled))
> > return -EINVAL;
> >
> > - /* enforce stacking: only the first disabled patch can be enabled */
> > - if (patch->list.prev != &klp_patches &&
> > - !list_prev_entry(patch, list)->enabled)
> > - return -EBUSY;
> > -
> > - /*
> > - * A reference is taken on the patch module to prevent it from being
> > - * unloaded.
> > - */
> > - if (!try_module_get(patch->mod))
> > - return -ENODEV;
> > + if (!patch->kobj.state_initialized)
> > + return -EINVAL;
>
> I think the check is not needed here. __klp_enable_patch() is called right after
> klp_init_patch() in klp_enable_patch().
I would keep it. Someone might want to call this also from other
location. Even we used to do it from enable_store() ;-)
Best Regards,
Petr
next prev parent reply other threads:[~2018-10-12 13:01 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-08-28 14:35 [PATCH v12 00/12] Petr Mladek
2018-08-28 14:35 ` [PATCH v12 01/12] livepatch: Change void *new_func -> unsigned long new_addr in struct klp_func Petr Mladek
2018-08-31 8:37 ` Miroslav Benes
2018-08-28 14:35 ` [PATCH v12 02/12] livepatch: Helper macros to define livepatch structures Petr Mladek
2018-08-28 14:35 ` [PATCH v12 03/12] livepatch: Shuffle klp_enable_patch()/klp_disable_patch() code Petr Mladek
2018-08-31 8:38 ` Miroslav Benes
2018-08-28 14:35 ` [PATCH v12 04/12] livepatch: Consolidate klp_free functions Petr Mladek
2018-08-31 10:39 ` Miroslav Benes
2018-10-12 11:43 ` Petr Mladek
2018-08-28 14:35 ` [PATCH v12 05/12] livepatch: Refuse to unload only livepatches available during a forced transition Petr Mladek
2018-08-28 14:35 ` [PATCH v12 06/12] livepatch: Simplify API by removing registration step Petr Mladek
2018-09-05 9:34 ` Miroslav Benes
2018-10-12 13:01 ` Petr Mladek [this message]
2018-10-15 16:01 ` Miroslav Benes
2018-10-18 14:54 ` Petr Mladek
2018-10-18 15:30 ` Josh Poimboeuf
2018-10-19 12:16 ` Miroslav Benes
2018-10-19 14:36 ` Josh Poimboeuf
2018-10-22 13:25 ` Petr Mladek
2018-10-23 16:39 ` Josh Poimboeuf
2018-10-24 2:55 ` Josh Poimboeuf
2018-10-24 11:14 ` Petr Mladek
2018-08-28 14:35 ` [PATCH v12 07/12] livepatch: Use lists to manage patches, objects and functions Petr Mladek
2018-09-03 16:00 ` Miroslav Benes
2018-10-12 12:12 ` Petr Mladek
2018-08-28 14:35 ` [PATCH v12 08/12] livepatch: Add atomic replace Petr Mladek
2018-08-28 14:36 ` [PATCH v12 09/12] livepatch: Remove Nop structures when unused Petr Mladek
2018-09-04 14:50 ` Miroslav Benes
2018-08-28 14:36 ` [PATCH v12 10/12] livepatch: Atomic replace and cumulative patches documentation Petr Mladek
2018-09-04 15:15 ` Miroslav Benes
2018-08-28 14:36 ` [PATCH v12 11/12] livepatch: Remove ordering and refuse loading conflicting patches Petr Mladek
2018-08-28 14:36 ` [PATCH v12 12/12] selftests/livepatch: introduce tests Petr Mladek
2018-08-30 11:58 ` [PATCH v12 00/12] Miroslav Benes
2018-10-11 12:48 ` 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=20181012130120.f5berowklyccd7lj@pathway.suse.cz \
--to=pmladek@suse.com \
--cc=eshatokhin@virtuozzo.com \
--cc=jbaron@akamai.com \
--cc=jeyu@kernel.org \
--cc=jikos@kernel.org \
--cc=joe.lawrence@redhat.com \
--cc=jpoimboe@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=live-patching@vger.kernel.org \
--cc=mbenes@suse.cz \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.