Live-Patching Archive on lore.kernel.org
 help / color / Atom feed
From: Josh Poimboeuf <jpoimboe@redhat.com>
To: Petr Mladek <pmladek@suse.com>
Cc: jikos@kernel.org, joe.lawrence@redhat.com,
	Miroslav Benes <mbenes@suse.cz>,
	linux-kernel@vger.kernel.org, live-patching@vger.kernel.org
Subject: Re: [RFC PATCH 2/2] livepatch: Clear relocation targets on a module removal
Date: Mon, 26 Aug 2019 09:54:49 -0500
Message-ID: <20190826145449.wyo7avwpqyriem46@treble> (raw)
In-Reply-To: <20190823081306.kbkm7b4deqrare2v@pathway.suse.cz>

On Fri, Aug 23, 2019 at 10:13:06AM +0200, Petr Mladek wrote:
> On Thu 2019-08-22 17:36:49, Josh Poimboeuf wrote:
> > On Fri, Aug 16, 2019 at 11:46:08AM +0200, Petr Mladek wrote:
> > > On Wed 2019-08-14 10:12:44, Josh Poimboeuf wrote:
> > > > On Wed, Aug 14, 2019 at 01:06:09PM +0200, Miroslav Benes wrote:
> > > > > > 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.
> > > > 
> > > > We should look at whether it makes sense to destabilize live patching
> > > > for everybody, for a small minority of people who care about a small
> > > > minority of edge cases.
> > > 
> > > I do not see it that simple. Forcing livepatched modules to be
> > > loaded would mean loading "random" new modules when updating
> > > livepatches:
> > 
> > I don't want to start a long debate on this, because this idea isn't
> > even my first choice.  But we shouldn't dismiss it outright.
> 
> I am glad to hear that this is not your first choice.
> 
> 
> > >   + It means more actions and higher risk to destabilize
> > >     the system. Different modules have different quality.
> > 
> > Maybe the distro shouldn't ship modules which would destabilize the
> > system.
> 
> Is this realistic? Even the best QA could not check all scenarios.
> My point is that the more actions we do the bigger the risk is.

Sure, it introduces risk.  But we have to compare that risk (which only
affects rare edge cases) with the ones introduced by the late module
patching code.  I get the feeling that "late module patching" introduces
risk to a broader range of use cases than "occasional loading of unused
modules".

The latter risk could be minimized by introducing a disabled state for
modules - load it in memory, but don't expose it to users until
explicitly loaded.  Just a brainstormed idea; not sure whether it would
work in practice.

> Anyway, this approach might cause loading modules that are never
> or rarely loaded together. Real life systems have limited number of
> peripherals.
> 
> I wonder if it might actually break certification of some
> hardware. It is just an idea. I do not know how certifications
> are done and what is the scope or limits.
> 
> 
> > >   + It might open more security holes that are not fixed by
> > >     the livepatch.
> > 
> > Following the same line of thinking, the livepatch infrastructure might
> > open security holes because of the inherent complexity of late module
> > patching.
> 
> Could you be more specific, please?
> Has there been any known security hole in the late module
> livepatching code?

Just off the top of my head, I can think of two recent bugs which can be
blamed on late module patching:

1) There was a RHEL-only bug which caused arch_klp_init_object_loaded()
   to not be loaded.  This resulted in a panic when certain patched code
   was executed.

2) arch_klp_init_object_loaded() currently doesn't have any jump label
   specific code.  This has recently caused panics for patched code
   which relies on static keys.  The workaround is to not use jump
   labels in patched code.  The real fix is to add support for them in
   arch_klp_init_object_loaded().

I can easily foresee more problems like those in the future.  Going
forward we have to always keep track of which special sections are
needed for which architectures.  Those special sections can change over
time, or can simply be overlooked for a given architecture.  It's
fragile.

Not to mention that most of the bugs we've fixed over the years seem to
be related to klp_init_object_loaded() and klp_module_coming/going().
I would expect that to continue given the hackish nature of late module
loading.  With live patching, almost any bug can be a security bug.


> > >   + It might require some extra configuration actions to handle
> > >     the newly opened interfaces (devices). For example, updating
> > >     SELinux policies.
> > 
> > I assume you mean user-created policies, not distro ones?  Is this even
> > a realistic concern?
> 
> Honestly, I do not know. I am not familiar with this area. There are
> also containers. They are going to be everywhere. They also need a lot
> of rules to keep stuff separated. And it is another area where I have
> no idea if newly loaded and unexpectedly modules might need special
> handling.
> 
> 
> > >   + Are there conflicting modules that might need to get
> > >     livepatched?
> > 
> > Again is this realistic?
> 
> I do not know. But I could imagine it.
> 
> 
> > > This approach has a strong no-go from my side.
> > 
> > </devils-advocate>
> > 
> > I agree it's not ideal, but nothing is ideal at this point.  Let's not
> > to rule it out prematurely.  I do feel that our current approach is not
> > the best.  It will continue to create problems for us until we fix it.
> 
> I am sure that we could do better. I just think that forcibly loading
> modules is opening too huge can of worms. Basically all other
> approaches have more limited or better defined effects.
> 
> For example, the newly added code that clears the relocations
> is something that can be tested. Behavior of "random" mix of
> loaded modules opens possibilities that have never been
> discovered before.

I'd just ask that you not be so quick to shut down ideas.  Ideas can be
iterated.  If you're sure we can do better, propose something better.
Shooting down ideas without trying to improve them (or find better ones)
doesn't help.

Our late module patching architecture is too fragile to be acceptable.
It's time to find something better.

> > > > - Changing 'atomic replace' to allow patch modules to be per-object.
> > > 
> > > The problem might be how to transition all loaded objects atomically
> > > when the needed code is loaded from different modules.
> > 
> > I'm not sure what you mean.
> > 
> > My idea was that each patch module would be specific to an object, with
> > no inter-module change dependencies.  So when using atomic replace, if
> > the patch module is only targeted to vmlinux, then only vmlinux-targeted
> > patch modules would be replaced.
> > 
> > In other words, 'atomic replace' would be object-specific.
> > 
> > > Alternative would be to support only per-object consitency. But it
> > > might reduce the number of supported scenarios too much. Also it
> > > would make livepatching more error-prone.
> 
> By per-object consistency I mean the same as you with "each patch
> module would be specific to an object, with no inter-module change
> dependencies".
> 
> My concern is that it would prevent semantic changes in a shared code.
> Semantic changes are rare. But changes in shared code are not.
> 
> If we reduce the consistency to per-object consistency. Will the
> consistency still make sense then? We might want to go back to
> trees, I mean immediate mode.

I still don't follow your logic.  Why wouldn't per-object consistency
make sense?  Most patches are per-object anyway.

We just have to make sure not to change exported interfaces.  (But
that's already an issue for our distros anyway because of kABI.)

-- 
Josh

  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
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 [this message]
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=20190826145449.wyo7avwpqyriem46@treble \
    --to=jpoimboe@redhat.com \
    --cc=jikos@kernel.org \
    --cc=joe.lawrence@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=live-patching@vger.kernel.org \
    --cc=mbenes@suse.cz \
    --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