Re: [PATCH 2/2] kernel: add support for live patching

[Seth Jennings <sjenning@redhat.com>]

On Thu, Nov 06, 2014 at 04:11:37PM +0100, Jiri Kosina wrote:
> On Thu, 6 Nov 2014, Seth Jennings wrote:
> 
> > This commit introduces code for the live patching core.  It implements
> > an ftrace-based mechanism and kernel interface for doing live patching
> > of kernel and kernel module functions.
> > 
> > It represents the greatest common functionality set between kpatch and
> > kgraft and can accept patches built using either method.
> > 
> > This first version does not implement any consistency mechanism that
> > ensures that old and new code do not run together.  In practice, ~90% of
> > CVEs are safe to apply in this way, since they simply add a conditional
> > check.  However, any function change that can not execute safely with
> > the old version of the function can _not_ be safely applied in this
> > version.
> 
> Thanks a lot for having started the work on this!
> 
> We will be reviewing it carefully in the coming days and will getting back 
> to you (I was surprised to see that that diffstat indicates that it's 
> actually more code than our whole kgraft implementation including the 
> consistency model :) ).

The structure allocation and sysfs stuff is a lot of (mundane) code.
Lots of boring error path handling too.

Plus, I show that kernel/kgraft.c + kernel/kgraft_files.c is
906+193=1099.  I'd say they are about the same size :)

> 
> I have one questions right away though.
> 
> > +/****************************************
> > + * dynamic relocations (load-time linker)
> > + ****************************************/
> > +
> > +/*
> > + * external symbols are located outside the parent object (where the parent
> > + * object is either vmlinux or the kmod being patched).
> > + */
> 
> I have no ideas what dynrela is, and quickly reading the source doesn't 
> really help too much.
> 
> Could you please provide some explanation / pointer to some documentation, 
> explaining what exactly it is, and why should it be part of the common 
> infrastructure?

Yes, I should explain it.

This is something that is currently only used in the kpatch approach.
It allows the patching core to do dynamic relocations on the new
function code, similar to what the kernel module linker does, but this
works for non-exported symbols as well.

This is so the patch module doesn't have to do a kallsyms lookup on
every non-exported symbol that the new functions use.

The fields of the dynrela structure are those of a normal ELF rela
entry, except for the "external" field, which conveys information about
where the core module should go looking for the symbol referenced in the
dynrela entry.

Josh was under the impression that Vojtech was ok with putting the
dynrela stuff in the core.  Is that not correct (misunderstanding)?

Thanks,
Seth

> 
> Thanks,
> 
> -- 
> Jiri Kosina
> SUSE Labs