Live-Patching Archive on lore.kernel.org
 help / color / Atom feed
From: Miroslav Benes <mbenes@suse.cz>
To: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: jikos@kernel.org, pmladek@suse.com, joe.lawrence@redhat.com,
	live-patching@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH 2/2] livepatch: Clear relocation targets on a module removal
Date: Wed, 14 Aug 2019 13:06:09 +0200 (CEST)
Message-ID: <alpine.LSU.2.21.1908141256150.16696@pobox.suse.cz> (raw)
In-Reply-To: <20190728200427.dbrojgu7hafphia7@treble>

On Sun, 28 Jul 2019, Josh Poimboeuf wrote:

> On Fri, Jul 19, 2019 at 02:28:40PM +0200, Miroslav Benes wrote:
> > Josh reported a bug:
> > 
> >   When the object to be patched is a module, and that module is
> >   rmmod'ed and reloaded, it fails to load with:
> > 
> >   module: x86/modules: Skipping invalid relocation target, existing value is nonzero for type 2, loc 00000000ba0302e9, val ffffffffa03e293c
> >   livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8)
> >   livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd'
> > 
> >   The livepatch module has a relocation which references a symbol
> >   in the _previous_ loading of nfsd. When apply_relocate_add()
> >   tries to replace the old relocation with a new one, it sees that
> >   the previous one is nonzero and it errors out.
> > 
> >   On ppc64le, we have a similar issue:
> > 
> >   module_64: livepatch_nfsd: Expected nop after call, got e8410018 at e_show+0x60/0x548 [livepatch_nfsd]
> >   livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8)
> >   livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd'
> > 
> > He also proposed three different solutions. We could remove the error
> > check in apply_relocate_add() introduced by commit eda9cec4c9a1
> > ("x86/module: Detect and skip invalid relocations"). However the check
> > is useful for detecting corrupted modules.
> > 
> > We could also deny the patched modules to be removed. If it proved to be
> > a major drawback for users, we could still implement a different
> > approach. The solution would also complicate the existing code a lot.
> > 
> > We thus decided to reverse the relocation patching (clear all relocation
> > targets on x86_64, or return back nops on powerpc). The solution is not
> > universal and is too much arch-specific, but it may prove to be simpler
> > in the end.
> 
> Thanks for the patch Miroslav.
> 
> However, I really don't like it.  All this extra convoluted
> arch-specific code, just so users can unload a patched module.

Yes, it is unfortunate.
 
> Remind me why we didn't do the "deny the patched modules to be removed"
> option?

Petr came with a couple of issues in the patch. Nothing unfixable, but it 
would complicate the code a bit, so we wanted to explore arch-specific 
approach first. I'll return to it, fix it and we'll see the outcome.

> Really, we should be going in the opposite direction, by creating module
> dependencies, like all other kernel modules do, ensuring that a module
> is loaded *before* we patch it.  That would also eliminate this bug.

Yes, but it is not ideal either with cumulative one-fixes-all patch 
modules. It would load also modules which are not necessary for a 
customer and I know that at least some customers care about this. They 
want to deploy only things which are crucial for their systems.

We could split patch modules as you proposed in the past, but that have 
issues as well.

Anyway, that is why I proposed "Rethinking late module patching" talk at 
LPC and we should try to come up with a solution there.

> And I think it would also help us remove a lot of nasty code, like the
> coming/going notifiers and the .klp.arch mess.  Which, BTW, seem to be
> the sources of most of our bugs...

Yes.

> Yes, there's the "but it's less flexible!" argument.  Does anybody
> really need the flexibility?  I strongly doubt it.  I would love to see
> an RFC patch which enforces that restriction, to see all the nasty code
> we could remove.  I would much rather live patching be stable than
> flexible.

I agree that unloading a module does not make sense much (famous last 
words), so we could try.

Miroslav

  reply index

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-19 12:28 [RFC PATCH 0/2] " Miroslav Benes
2019-07-19 12:28 ` [PATCH 1/2] livepatch: Nullify obj->mod in klp_module_coming()'s error path Miroslav Benes
2019-07-28 19:45   ` Josh Poimboeuf
2019-08-19 11:26     ` Petr Mladek
2019-07-19 12:28 ` [RFC PATCH 2/2] livepatch: Clear relocation targets on a module removal Miroslav Benes
2019-07-22  9:33   ` Petr Mladek
2019-08-14 12:33     ` Miroslav Benes
2019-07-28 20:04   ` Josh Poimboeuf
2019-08-14 11:06     ` Miroslav Benes [this message]
2019-08-14 15:12       ` Josh Poimboeuf
2019-08-16  9:46         ` Petr Mladek
2019-08-22 22:36           ` Josh Poimboeuf
2019-08-23  8:13             ` Petr Mladek
2019-08-26 14:54               ` Josh Poimboeuf
2019-08-27 15:05                 ` Joe Lawrence
2019-08-27 15:37                   ` Josh Poimboeuf
2019-09-02 16:13                 ` Miroslav Benes
2019-09-02 17:05                   ` Joe Lawrence
2019-09-03 13:02                     ` Miroslav Benes
2019-09-04  8:49                       ` Petr Mladek
2019-09-04 16:26                         ` Joe Lawrence
2019-09-05  2:50                         ` Josh Poimboeuf
2019-09-05 11:09                           ` Petr Mladek
2019-09-05 11:19                             ` Jiri Kosina
2019-09-05 13:23                               ` Josh Poimboeuf
2019-09-05 13:31                                 ` Jiri Kosina
2019-09-05 13:42                                   ` Josh Poimboeuf
2019-09-05 11:39                             ` Joe Lawrence
2019-09-05 13:08                             ` Josh Poimboeuf
2019-09-05 13:15                               ` Josh Poimboeuf
2019-09-05 13:52                                 ` Petr Mladek
2019-09-05 14:28                                   ` Josh Poimboeuf
2019-09-05 12:03                           ` Miroslav Benes
2019-09-05 12:35                             ` Josh Poimboeuf
2019-09-05 12:49                               ` Miroslav Benes
2019-09-05 11:52                         ` Miroslav Benes
2019-09-05  2:32                       ` Josh Poimboeuf
2019-09-05 12:16                         ` Miroslav Benes
2019-09-05 12:54                           ` Josh Poimboeuf
2019-09-06 12:51                             ` Miroslav Benes
2019-09-06 15:38                               ` Joe Lawrence
2019-09-06 16:45                               ` Josh Poimboeuf
2019-08-26 13:44         ` Nicolai Stange
2019-08-26 15:02           ` Josh Poimboeuf

Reply instructions:

You may reply publically 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=alpine.LSU.2.21.1908141256150.16696@pobox.suse.cz \
    --to=mbenes@suse.cz \
    --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=pmladek@suse.com \
    /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

Live-Patching Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/live-patching/0 live-patching/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 live-patching live-patching/ https://lore.kernel.org/live-patching \
		live-patching@vger.kernel.org
	public-inbox-index live-patching

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.live-patching


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git