[PATCH 0/2] livepatch: Add compiler optimization disclaimer/docs

[PATCH 0/2] livepatch: Add compiler optimization disclaimer/docs

[Joe Lawrence]

In light of [PATCH] Revert "kbuild: use -flive-patching when
CONFIG_LIVEPATCH is enabled" [1], we should add some loud disclaimers
and explanation of the impact compiler optimizations have on
livepatching.

The first commit provides detailed explanations and examples.  The list
was taken mostly from Miroslav's LPC talk a few years back.  This is a
bit rough, so corrections and additional suggestions welcome.  Expanding
upon the source-based patching approach would be helpful, too.

The second commit adds a small README.rst file in the livepatch samples
directory pointing the reader to the doc introduced in the first commit.

I didn't touch the livepatch kselftests yet as I'm still unsure about
how to best account for IPA here.  We could add the same README.rst
disclaimer here, too, but perhaps we have a chance to do something more.
Possibilities range from checking for renamed functions as part of their
build, or the selftest scripts, or even adding something to the kernel
API.  I think we'll have a better idea after reviewing the compiler
considerations doc.

[1] https://lore.kernel.org/lkml/696262e997359666afa053fe7d1a9fb2bb373964.1595010490.git.jpoimboe@redhat.com/

Joe Lawrence (2):
  docs/livepatch: Add new compiler considerations doc
  samples/livepatch: Add README.rst disclaimer

 .../livepatch/compiler-considerations.rst     | 220 ++++++++++++++++++
 Documentation/livepatch/index.rst             |   1 +
 Documentation/livepatch/livepatch.rst         |   7 +
 samples/livepatch/README.rst                  |  15 ++
 4 files changed, 243 insertions(+)
 create mode 100644 Documentation/livepatch/compiler-considerations.rst
 create mode 100644 samples/livepatch/README.rst

-- 
2.21.3

[PATCH 1/2] docs/livepatch: Add new compiler considerations doc

[Joe Lawrence]

Compiler optimizations can have serious implications on livepatching.
Create a document that outlines common optimization patterns and safe
ways to livepatch them.

Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
---
 .../livepatch/compiler-considerations.rst     | 220 ++++++++++++++++++
 Documentation/livepatch/index.rst             |   1 +
 Documentation/livepatch/livepatch.rst         |   7 +
 3 files changed, 228 insertions(+)
 create mode 100644 Documentation/livepatch/compiler-considerations.rst

diff --git a/Documentation/livepatch/compiler-considerations.rst b/Documentation/livepatch/compiler-considerations.rst
new file mode 100644
index 000000000000..23b9cc01bb9c
--- /dev/null
+++ b/Documentation/livepatch/compiler-considerations.rst
@@ -0,0 +1,220 @@
+.. SPDX-License-Identifier: GPL-2.0+
+
+=======================
+Compiler considerations
+=======================
+
+Creating livepatch modules may seem as straightforward as updating a
+few functions in source code and registering them with the livepatch API.
+This idealized method may produce functional livepatch modules in some
+cases.
+
+.. warning::
+
+   A safe and accurate livepatch **must** take into account compiler
+   optimizations and their effect on the binary code that is executed and
+   ultimately livepatched.
+
+Examples
+========
+
+Interprocedural optimization (IPA)
+----------------------------------
+
+Function inlining is probably the most common compiler optimization that
+affects livepatching.  In a simple example, inlining transforms the original
+code::
+
+	foo() { ... [ foo implementation ] ... }
+
+	bar() { ...  foo() ...  }
+
+to::
+
+	bar() { ...  [ foo implementation ] ...  }
+
+Inlining is comparable to macro expansion, however the compiler may inline
+cases which it determines worthwhile (while preserving original call/return
+semantics in others) or even partially inline pieces of functions (see cold
+functions in GCC function suffixes section below).
+
+To safely livepatch ``foo()`` from the previous example, all of its callers
+need to be taken into consideration.  For those callers that the compiler had
+inlined ``foo()``, a livepatch should include a new version of the calling
+function such that it:
+
+  1. Calls a new, patched version of the inlined function, or
+  2. Provides an updated version of the caller that contains its own inlined
+     and updated version of the inlined function
+
+Other interesting IPA examples include:
+
+- *IPA-SRA*: removal of unused parameters, replace parameters passed by
+  referenced by parameters passed by value.  This optimization basically
+  violates ABI.
+
+  .. note::
+     GCC changes the name of function.  See GCC function suffixes
+     section below.
+
+- *IPA-CP*: find values passed to functions are constants and then optimizes
+  accordingly Several clones of a function are possible if a set is limited.
+
+  .. note::
+     GCC changes the name of function.  See GCC function suffixes
+     section below.
+
+- *IPA-PURE-CONST*: discover which functions are pure or constant.  GCC can
+  eliminate calls to such functions, memory accesses can be removed etc.
+
+- *IPA-ICF*: perform identical code folding for functions and read-only
+  variables.  Replaces a function with an equivalent one.
+
+- *IPA-RA*: optimize saving and restoring registers if the compiler considers
+  it safe.
+
+- *Dead code elimination*: omit unused code paths from the resulting binary.
+
+GCC function suffixes
+---------------------
+
+GCC may rename original, copied, and cloned functions depending upon the
+optimizations applied.  Here is a partial list of name suffixes that the
+compiler may apply to kernel functions:
+
+- *Cold subfunctions* : ``.code`` or ``.cold.<N>`` : parts of functions
+  (subfunctions) determined by attribute or optimization to be unlikely
+  executed.
+
+  For example, the unlikely bits of ``irq_do_set_affinity()`` may be moved
+  out to subfunction ``irq_do_set_affinity.cold.49()``.  Starting with GCC 9,
+  the numbered suffix has been removed.  So in the previous example, the cold
+  subfunction is simply ``irq_do_set_affinity.cold()``.
+
+- *Partial inlining* : ``.part.<N>`` : parts of functions when split from
+  their original function body, improves overall inlining decisions.
+
+  The ``cdev_put()`` function provides an interesting example of a partial
+  clone.  GCC builds the source function::
+
+  	void cdev_put(struct cdev *p)
+  	{
+  		if (p) {
+  			struct module *owner = p->owner;
+  			kobject_put(&p->kobj);
+  			module_put(owner);
+  		}
+  	}
+
+  into two functions, the conditional test in ``cdev_put()`` and the
+  ``kobject_put()`` and ``module_put()`` calls in ``cdev_put.part.0()``::
+
+    <cdev_put>:
+           e8 bb 60 73 00          callq  ffffffff81a01a10 <__fentry__>
+           48 85 ff                test   %rdi,%rdi
+           74 05                   je     ffffffff812cb95f <cdev_put+0xf>
+           e9 a1 fc ff ff          jmpq   ffffffff812cb600 <cdev_put.part.0>
+           c3                      retq
+
+    <cdev_put.part.0>:
+           e8 0b 64 73 00          callq  ffffffff81a01a10 <__fentry__>
+           53                      push   %rbx
+           48 8b 5f 60             mov    0x60(%rdi),%rbx
+           e8 a1 54 5a 00          callq  ffffffff81870ab0 <kobject_put>
+           48 89 df                mov    %rbx,%rdi
+           5b                      pop    %rbx
+           e9 b8 5c e8 ff          jmpq   ffffffff811512d0 <module_put>
+           0f 1f 84 00 00 00 00    nopl   0x0(%rax,%rax,1)
+           00
+
+  Some ``cdev_put()`` callers may take advantage of this function splitting
+  to inline one part or another.  Others may also directly call the partial
+  clone.
+
+- *Constant propagation* : ``.constprop.<N>`` : function copies to enable
+  constant propagation when conflicting arguments exist.
+
+  For example, consider ``cpumask_weight()`` and its copies for
+  ``cpumask_weight(cpu_possible_mask)`` and
+  ``cpumask_weight(__cpu_online_mask)``.  Note how the ``.constprop`` copies
+  implicitly assign the function parameter::
+
+    <cpumask_weight>:
+           8b 35 1e 7d 3e 01       mov    0x13e7d1e(%rip),%esi
+           e9 55 6e 3f 00          jmpq   ffffffff8141d2b0 <__bitmap_weight>
+
+    <cpumask_weight.constprop.28>:
+           8b 35 79 cf 1c 01       mov    0x11ccf79(%rip),%esi
+           48 c7 c7 80 db 40 82    mov    $0xffffffff8240db80,%rdi
+                             R_X86_64_32S  __cpu_possible_mask
+           e9 a9 c0 1d 00          jmpq   ffffffff8141d2b0 <__bitmap_weight>
+
+    <cpumask_weight.constprop.108>:
+           8b 35 de 69 32 01       mov    0x13269de(%rip),%esi
+           48 c7 c7 80 d7 40 82    mov    $0xffffffff8240d780,%rdi
+                             R_X86_64_32S  __cpu_online_mask
+           e9 0e 5b 33 00          jmpq   ffffffff8141d2b0 <__bitmap_weight>
+
+- *IPA-SRA* : ``.isra.0`` : TODO
+
+
+Coping with optimizations
+=========================
+
+A livepatch author must take care to consider the consequences of
+interprocedural optimizations that create function clones, ABI changes,
+splitting, etc. A small change to one function may cascade through the
+function call-chain, updating dozens more. A safe livepatch needs to be
+fully compatible with all callers.
+
+kpatch-build
+------------
+
+Given an input .patch file, kpatch-build performs a binary comparison of
+unpatched and patched kernel trees.  This automates the detection of changes
+in compiler-generated code, optimizations included.  It is still important,
+however, for a kpatch developer to learn about compiler transformations in
+order to understand and control the set of modified functions.
+
+kgraft-analysis-tool
+--------------------
+
+With the -fdump-ipa-clones flag, GCC will dump IPA clones that were created
+by all inter-procedural optimizations in ``<source>.000i.ipa-clones`` files.
+
+kgraft-analysis-tool pretty-prints those IPA cloning decisions.  The full
+list of affected functions provides additional updates that the source-based
+livepatch author may need to consider.  For example, for the function
+``scatterwalk_unmap()``:
+
+::
+
+  $ ./kgraft-ipa-analysis.py --symbol=scatterwalk_unmap aesni-intel_glue.i.000i.ipa-clones
+  Function: scatterwalk_unmap/2930 (include/crypto/scatterwalk.h:81:60)
+    isra: scatterwalk_unmap.isra.2/3142 (include/crypto/scatterwalk.h:81:60)
+      inlining to: helper_rfc4106_decrypt/3007 (arch/x86/crypto/aesni-intel_glue.c:1016:12)
+      inlining to: helper_rfc4106_decrypt/3007 (arch/x86/crypto/aesni-intel_glue.c:1016:12)
+      inlining to: helper_rfc4106_encrypt/3006 (arch/x86/crypto/aesni-intel_glue.c:939:12)
+
+    Affected functions: 3
+      scatterwalk_unmap.isra.2/3142 (include/crypto/scatterwalk.h:81:60)
+      helper_rfc4106_decrypt/3007 (arch/x86/crypto/aesni-intel_glue.c:1016:12)
+      helper_rfc4106_encrypt/3006 (arch/x86/crypto/aesni-intel_glue.c:939:12)
+
+kgraft-ipa-analysis notes that it was inlined into function
+``helper_rfc4106_decrypt()`` and was renamed with a ``.isra.<N>`` IPA
+optimization suffix.  A safe livepatch that updates ``scatterwalk_unmap()``
+will of course need to consider updating these functions as well.
+
+References
+==========
+
+[1] GCC optimizations and their impact on livepatch
+    Miroslav Benes, 2016 Linux Plumbers Conferences
+    http://www.linuxplumbersconf.net/2016/ocw//system/presentations/3573/original/pres_gcc.pdf
+
+[2] kpatch-build
+    https://github.com/dynup/kpatch
+
+[3] kgraft-analysis-tool
+    https://github.com/marxin/kgraft-analysis-tool
diff --git a/Documentation/livepatch/index.rst b/Documentation/livepatch/index.rst
index 525944063be7..7fd8a94498a0 100644
--- a/Documentation/livepatch/index.rst
+++ b/Documentation/livepatch/index.rst
@@ -8,6 +8,7 @@ Kernel Livepatching
     :maxdepth: 1
 
     livepatch
+    compiler-considerations
     callbacks
     cumulative-patches
     module-elf-format
diff --git a/Documentation/livepatch/livepatch.rst b/Documentation/livepatch/livepatch.rst
index c2c598c4ead8..b6d5beb16a00 100644
--- a/Documentation/livepatch/livepatch.rst
+++ b/Documentation/livepatch/livepatch.rst
@@ -432,6 +432,13 @@ The current Livepatch implementation has several limitations:
     by "notrace".
 
 
+  - Compiler optimizations can complicate livepatching.
+
+    Optimizations may inline, clone and even change a function's calling
+    convention interface. Please consult the
+    Documentation/livepatching/compiler-considerations.rst file before
+    creating any livepatch modules.
+
 
   - Livepatch works reliably only when the dynamic ftrace is located at
     the very beginning of the function.
-- 
2.21.3

[PATCH 2/2] samples/livepatch: Add README.rst disclaimer

[Joe Lawrence]

The livepatch samples aren't very careful with respect to compiler
IPA-optimization of target kernel functions.

Add a quick disclaimer and pointer to the compiler-considerations.rst
file to warn readers.

Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
---
 samples/livepatch/README.rst | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
 create mode 100644 samples/livepatch/README.rst

diff --git a/samples/livepatch/README.rst b/samples/livepatch/README.rst
new file mode 100644
index 000000000000..2f8ef945f2ac
--- /dev/null
+++ b/samples/livepatch/README.rst
@@ -0,0 +1,15 @@
+.. SPDX-License-Identifier: GPL-2.0+
+
+==========
+Disclaimer
+==========
+
+The livepatch sample programs were written with idealized compiler
+output in mind. That is to say that they do not consider ways in which
+optimization may transform target kernel functions.
+
+The samples present only a simple API demonstration and should not be
+considered completely safe.
+
+See the Documentation/livepatching/compiler-considerations.rst file for
+more details on compiler optimizations and how they affect livepatching.
-- 
2.21.3

Re: [PATCH 1/2] docs/livepatch: Add new compiler considerations doc

[Josh Poimboeuf]

On Tue, Jul 21, 2020 at 12:14:06PM -0400, Joe Lawrence wrote:
> Compiler optimizations can have serious implications on livepatching.
> Create a document that outlines common optimization patterns and safe
> ways to livepatch them.
> 
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>

There's a lot of good info here, but I wonder if it should be
reorganized a bit and instead called "how to create a livepatch module",
because that's really the point of it all.

I'm thinking a newcomer reading this might be lost.  It's not
necessarily clear that there are currently two completely different
approaches to creating a livepatch module, each with their own quirks
and benefits/drawbacks.  There is one mention of a "source-based
livepatch author" but no explanation of what that means.

Maybe it could begin with an overview of the two approaches, and then
delve more into the details of each approach, and then delve even more
into the gory details about compiler optimizations.

Also the kpatch-build section can reference the patch author guide which
we have on github.

-- 
Josh

Re: [PATCH 1/2] docs/livepatch: Add new compiler considerations doc

[Joe Lawrence]

On 7/21/20 7:04 PM, Josh Poimboeuf wrote:
> On Tue, Jul 21, 2020 at 12:14:06PM -0400, Joe Lawrence wrote:
>> Compiler optimizations can have serious implications on livepatching.
>> Create a document that outlines common optimization patterns and safe
>> ways to livepatch them.
>>
>> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> 
> There's a lot of good info here, but I wonder if it should be
> reorganized a bit and instead called "how to create a livepatch module",
> because that's really the point of it all.
> 

That would be nice.  Would you consider a stand-alone 
compiler-optimizations doc an incremental step towards that end?  Note 
that the other files (callbacks, shadow-vars, system-state) in their 
current form might be as confusing to the newbie.

> I'm thinking a newcomer reading this might be lost.  It's not
> necessarily clear that there are currently two completely different
> approaches to creating a livepatch module, each with their own quirks
> and benefits/drawbacks.  There is one mention of a "source-based
> livepatch author" but no explanation of what that means.
> 

Yes, the initial draft was light on source-based patching since I only 
really tinker with it for samples/kselftests.  The doc was the result of 
an experienced livepatch developer and Sunday afternoon w/the compiler. 
I'm sure it reads as such. :)

> Maybe it could begin with an overview of the two approaches, and then
> delve more into the details of each approach, and then delve even more
> into the gory details about compiler optimizations.
> 

Up until now, the livepatch documentation has danced around the 
particular creation method and only described the API in abstract.  If a 
compiler considerations doc needs to have that complete context then I'd 
suggest we reorganize the entire lot as a prerequisite.

> Also the kpatch-build section can reference the patch author guide which
> we have on github.
> 

Good point.  I think there are a few kpatch-specific implications 
(sibling call changes maybe) to consider.

-- Joe

Re: [PATCH 1/2] docs/livepatch: Add new compiler considerations doc

[Josh Poimboeuf]

On Wed, Jul 22, 2020 at 01:03:03PM -0400, Joe Lawrence wrote:
> On 7/21/20 7:04 PM, Josh Poimboeuf wrote:
> > On Tue, Jul 21, 2020 at 12:14:06PM -0400, Joe Lawrence wrote:
> > > Compiler optimizations can have serious implications on livepatching.
> > > Create a document that outlines common optimization patterns and safe
> > > ways to livepatch them.
> > > 
> > > Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> > 
> > There's a lot of good info here, but I wonder if it should be
> > reorganized a bit and instead called "how to create a livepatch module",
> > because that's really the point of it all.
> > 
> 
> That would be nice.  Would you consider a stand-alone compiler-optimizations
> doc an incremental step towards that end?  Note that the other files
> (callbacks, shadow-vars, system-state) in their current form might be as
> confusing to the newbie.

It's an incremental step towards _something_.  Whether that's a cohesive
patch creation guide, or just a growing hodgepodge of random documents,
it may be too early to say :-)

> > I'm thinking a newcomer reading this might be lost.  It's not
> > necessarily clear that there are currently two completely different
> > approaches to creating a livepatch module, each with their own quirks
> > and benefits/drawbacks.  There is one mention of a "source-based
> > livepatch author" but no explanation of what that means.
> > 
> 
> Yes, the initial draft was light on source-based patching since I only
> really tinker with it for samples/kselftests.  The doc was the result of an
> experienced livepatch developer and Sunday afternoon w/the compiler. I'm
> sure it reads as such. :)

Are experienced livepatch developers the intended audience?  If so I
question what value this document has in its current form.  Presumably
experienced livepatch developers would already know this stuff.

> > Maybe it could begin with an overview of the two approaches, and then
> > delve more into the details of each approach, and then delve even more
> > into the gory details about compiler optimizations.
> > 
> 
> Up until now, the livepatch documentation has danced around the particular
> creation method and only described the API in abstract.  If a compiler
> considerations doc needs to have that complete context then I'd suggest we
> reorganize the entire lot as a prerequisite.

I wouldn't say it *needs* to have that context.  But it would be a lot
more useful with it.  As you pointed out, the existing documents do need
to be reorganized into a more cohesive whole.

-- 
Josh

Re: [PATCH 1/2] docs/livepatch: Add new compiler considerations doc

[Petr Mladek]

On Wed 2020-07-22 15:51:39, Josh Poimboeuf wrote:
> On Wed, Jul 22, 2020 at 01:03:03PM -0400, Joe Lawrence wrote:
> > On 7/21/20 7:04 PM, Josh Poimboeuf wrote:
> > > On Tue, Jul 21, 2020 at 12:14:06PM -0400, Joe Lawrence wrote:
> > > > Compiler optimizations can have serious implications on livepatching.
> > > > Create a document that outlines common optimization patterns and safe
> > > > ways to livepatch them.
> > > > 
> > > > Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> > > 
> > > There's a lot of good info here, but I wonder if it should be
> > > reorganized a bit and instead called "how to create a livepatch module",
> > > because that's really the point of it all.
> > > 
> > 
> > That would be nice.  Would you consider a stand-alone compiler-optimizations
> > doc an incremental step towards that end?  Note that the other files
> > (callbacks, shadow-vars, system-state) in their current form might be as
> > confusing to the newbie.
> 
> It's an incremental step towards _something_.  Whether that's a cohesive
> patch creation guide, or just a growing hodgepodge of random documents,
> it may be too early to say :-)

Yes, it would be nice to have a cohesive documentation. But scattered
pieces are better than nothing.

> > > I'm thinking a newcomer reading this might be lost.  It's not
> > > necessarily clear that there are currently two completely different
> > > approaches to creating a livepatch module, each with their own quirks
> > > and benefits/drawbacks.  There is one mention of a "source-based
> > > livepatch author" but no explanation of what that means.
> > > 
> > 
> > Yes, the initial draft was light on source-based patching since I only
> > really tinker with it for samples/kselftests.  The doc was the result of an
> > experienced livepatch developer and Sunday afternoon w/the compiler. I'm
> > sure it reads as such. :)
> 
> Are experienced livepatch developers the intended audience?  If so I
> question what value this document has in its current form.  Presumably
> experienced livepatch developers would already know this stuff.

IMHO, this document is useful even for newbies. They might at
least get a clue about these catches. It is better than nothing.

I do not want to discourage Joe from creating even better
documentation. But if he does not have interest or time
to work on it, I am happy even for this piece.

Acked-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH 2/2] samples/livepatch: Add README.rst disclaimer

[Petr Mladek]

On Tue 2020-07-21 12:14:07, Joe Lawrence wrote:
> The livepatch samples aren't very careful with respect to compiler
> IPA-optimization of target kernel functions.
> 
> Add a quick disclaimer and pointer to the compiler-considerations.rst
> file to warn readers.
> 
> Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> ---
>  samples/livepatch/README.rst | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>  create mode 100644 samples/livepatch/README.rst

I am not sure if the RST format makes sense here. But it does not
harm. Either way:

Acked-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

refactoring livepatch documentation was Re: [PATCH 1/2] docs/livepatch: Add new compiler considerations doc

[Joe Lawrence]

On 8/6/20 8:03 AM, Petr Mladek wrote:
> On Wed 2020-07-22 15:51:39, Josh Poimboeuf wrote:
>> On Wed, Jul 22, 2020 at 01:03:03PM -0400, Joe Lawrence wrote:
>>> On 7/21/20 7:04 PM, Josh Poimboeuf wrote:
>>>> On Tue, Jul 21, 2020 at 12:14:06PM -0400, Joe Lawrence wrote:
>>>>> Compiler optimizations can have serious implications on livepatching.
>>>>> Create a document that outlines common optimization patterns and safe
>>>>> ways to livepatch them.
>>>>>
>>>>> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
>>>>
>>>> There's a lot of good info here, but I wonder if it should be
>>>> reorganized a bit and instead called "how to create a livepatch module",
>>>> because that's really the point of it all.
>>>>
>>>
>>> That would be nice.  Would you consider a stand-alone compiler-optimizations
>>> doc an incremental step towards that end?  Note that the other files
>>> (callbacks, shadow-vars, system-state) in their current form might be as
>>> confusing to the newbie.
>>
>> It's an incremental step towards _something_.  Whether that's a cohesive
>> patch creation guide, or just a growing hodgepodge of random documents,
>> it may be too early to say :-)
> 
> Yes, it would be nice to have a cohesive documentation. But scattered
> pieces are better than nothing.
> 
>>>> I'm thinking a newcomer reading this might be lost.  It's not
>>>> necessarily clear that there are currently two completely different
>>>> approaches to creating a livepatch module, each with their own quirks
>>>> and benefits/drawbacks.  There is one mention of a "source-based
>>>> livepatch author" but no explanation of what that means.
>>>>
>>>
>>> Yes, the initial draft was light on source-based patching since I only
>>> really tinker with it for samples/kselftests.  The doc was the result of an
>>> experienced livepatch developer and Sunday afternoon w/the compiler. I'm
>>> sure it reads as such. :)
>>
>> Are experienced livepatch developers the intended audience?  If so I
>> question what value this document has in its current form.  Presumably
>> experienced livepatch developers would already know this stuff.
> 
> IMHO, this document is useful even for newbies. They might at
> least get a clue about these catches. It is better than nothing.
> 
> I do not want to discourage Joe from creating even better
> documentation. But if he does not have interest or time
> to work on it, I am happy even for this piece.
> 

Hi Petr, Josh,

The compiler optimization pitfall document can wait for refactored 
livepatch documentation if that puts it into better context, 
particularly for newbies.  I don't mind either way.  FWIW, I don't 
profess to be an authoritative source its content -- we've dealt some of 
these issues in kpatch, so it was interesting to see how they affect 
livepatches that don't rely on binary comparison.


Toward the larger goal, I've changed the thread subject to talk about 
how we may rearrange and supplement our current documentation.  This is 
a first pass at a possible refactoring...


1. Provide a better index page to connect the other files/docs, like
https://www.kernel.org/doc/html/latest/core-api/index.html but obviously 
not that extensive.  Right now we have only a Table of Contents tree 
without any commentary.

2. Rearrange and refactor sections:

livepatch.rst
   Keep just about everything
   Add a history section to explain ksplice, kgraft, kpatch for the
     uninitiated?
   Add a section on source based vs. binary diff livepatch creation,
     this may be worth its own top-level section

Livepatch API
   Basic API
   Callbacks
   Shadow variables
   Cumulative patches
   System state

KLP Relocations
   Right now this is a bit academic AFAIK kpatch is the only tool
   currently making use of them.  So maybe this document becomes a
   more general purpose doc explaining how to reference unexported
   symbols?  (ie, how does kgraft currently do it, particularly
   w/kallsyms going unexported?)

   Eventually this could contain klp-convert howto if it ever gets
   merged.

Compiler considerations
   TBD

I suppose this doesn't create a "Livepatching creation for dummies" 
guide, but my feeling is that there are so many potential (hidden) 
pitfalls that such guide would be dangerous.

If someone were to ask me today how to start building a livepatch, I 
would probably point them at the samples to demonstrate the basic 
concept and API, but then implore them to read through the documentation 
to understand how quickly complicated it can become.

-- Joe

Re: refactoring livepatch documentation was Re: [PATCH 1/2] docs/livepatch: Add new compiler considerations doc

[Josh Poimboeuf]

On Mon, Aug 10, 2020 at 03:46:46PM -0400, Joe Lawrence wrote:
> > > > > I'm thinking a newcomer reading this might be lost.  It's not
> > > > > necessarily clear that there are currently two completely different
> > > > > approaches to creating a livepatch module, each with their own quirks
> > > > > and benefits/drawbacks.  There is one mention of a "source-based
> > > > > livepatch author" but no explanation of what that means.
> > > > > 
> > > > 
> > > > Yes, the initial draft was light on source-based patching since I only
> > > > really tinker with it for samples/kselftests.  The doc was the result of an
> > > > experienced livepatch developer and Sunday afternoon w/the compiler. I'm
> > > > sure it reads as such. :)
> > > 
> > > Are experienced livepatch developers the intended audience?  If so I
> > > question what value this document has in its current form.  Presumably
> > > experienced livepatch developers would already know this stuff.
> > 
> > IMHO, this document is useful even for newbies. They might at
> > least get a clue about these catches. It is better than nothing.
> > 
> > I do not want to discourage Joe from creating even better
> > documentation. But if he does not have interest or time
> > to work on it, I am happy even for this piece.

Agreed.  Joe, sorry for instigating and then disappearing :-)

I know we're all busy and I didn't intend to block the patch until we
reach Documentation Nirvana.  Though it would be _really_ nice to get
more input from those who have more experience with the subject matter
(source-based patch generation).

It's part of my job as a maintainer to push back, question, and
sometimes even complain.  I was just wondering where this is heading,
because as our documentation grows (a good thing), the overall state is
getting less cohesive (a bad thing).

Anyway, ACK to the original patch.

> 1. Provide a better index page to connect the other files/docs, like
> https://www.kernel.org/doc/html/latest/core-api/index.html but obviously not
> that extensive.  Right now we have only a Table of Contents tree without any
> commentary.
> 
> 2. Rearrange and refactor sections:
> 
> livepatch.rst
>   Keep just about everything
>   Add a history section to explain ksplice, kgraft, kpatch for the
>     uninitiated?
>   Add a section on source based vs. binary diff livepatch creation,
>     this may be worth its own top-level section
> 
> Livepatch API
>   Basic API
>   Callbacks
>   Shadow variables
>   Cumulative patches
>   System state
> 
> KLP Relocations
>   Right now this is a bit academic AFAIK kpatch is the only tool
>   currently making use of them.  So maybe this document becomes a
>   more general purpose doc explaining how to reference unexported
>   symbols?  (ie, how does kgraft currently do it, particularly
>   w/kallsyms going unexported?)
> 
>   Eventually this could contain klp-convert howto if it ever gets
>   merged.
> 
> Compiler considerations
>   TBD

This is certainly a logical way to organize things.  But again I would
wonder, who's the audience?

> I suppose this doesn't create a "Livepatching creation for dummies" guide,
> but my feeling is that there are so many potential (hidden) pitfalls that
> such guide would be dangerous.

I disagree that a live patching creation guide would be dangerous.  I
think it would be less dangerous than *not* having one.  There are
several companies now delivering (hopefully reliable) livepatches to
customers, and they're all presumably following processes.  We just need
to agree on best practices and document the resulting process.  Over
time I believe that will create much more good than harm.

Sure, there are pitfalls, but the known ones can be highlighted in the
guide.  No document is perfect but it hopefully improves and becomes
more useful over time.

-- 
Josh

Re: [PATCH 1/2] docs/livepatch: Add new compiler considerations doc

[Miroslav Benes]

Hi,

first, I'm sorry for the late reply. Thanks, Josh, for the reminder.

CCing Nicolai. Nicolai, could you take a look at the proposed 
documentation too, please? You have more up-to-date experience.

On Tue, 21 Jul 2020, Joe Lawrence wrote:

> +Examples
> +========
> +
> +Interprocedural optimization (IPA)
> +----------------------------------
> +
> +Function inlining is probably the most common compiler optimization that
> +affects livepatching.  In a simple example, inlining transforms the original
> +code::
> +
> +	foo() { ... [ foo implementation ] ... }
> +
> +	bar() { ...  foo() ...  }
> +
> +to::
> +
> +	bar() { ...  [ foo implementation ] ...  }
> +
> +Inlining is comparable to macro expansion, however the compiler may inline
> +cases which it determines worthwhile (while preserving original call/return
> +semantics in others) or even partially inline pieces of functions (see cold
> +functions in GCC function suffixes section below).
> +
> +To safely livepatch ``foo()`` from the previous example, all of its callers
> +need to be taken into consideration.  For those callers that the compiler had
> +inlined ``foo()``, a livepatch should include a new version of the calling
> +function such that it:
> +
> +  1. Calls a new, patched version of the inlined function, or
> +  2. Provides an updated version of the caller that contains its own inlined
> +     and updated version of the inlined function

I'm afraid the above could cause a confusion...

"1. Calls a new, patched version of the inlined function, or". The 
function is not inlined in this case. Would it be more understandable to 
use function names?

1. Calls a new, patched version of function foo(), or
2. Provides an updated version of bar() that contains its own inlined and 
   updated version of foo() (as seen in the example above).

Not to say that it is again a call of the compiler to decide that, so one 
usually prepares an updated version of foo() and updated version of bar() 
calling to it. Updated foo() has to be there for non-inlined cases anyway.

> +
> +Other interesting IPA examples include:
> +
> +- *IPA-SRA*: removal of unused parameters, replace parameters passed by
> +  referenced by parameters passed by value.  This optimization basically

s/referenced/reference/

> +  violates ABI.
> +
> +  .. note::
> +     GCC changes the name of function.  See GCC function suffixes
> +     section below.
> +
> +- *IPA-CP*: find values passed to functions are constants and then optimizes
> +  accordingly Several clones of a function are possible if a set is limited.

"...accordingly. Several..."

[...]

> +  	void cdev_put(struct cdev *p)
> +  	{
> +  		if (p) {
> +  			struct module *owner = p->owner;
> +  			kobject_put(&p->kobj);
> +  			module_put(owner);
> +  		}
> +  	}

git am complained here about whitespace damage.

[...]

> +kgraft-analysis-tool
> +--------------------
> +
> +With the -fdump-ipa-clones flag, GCC will dump IPA clones that were created
> +by all inter-procedural optimizations in ``<source>.000i.ipa-clones`` files.
> +
> +kgraft-analysis-tool pretty-prints those IPA cloning decisions.  The full
> +list of affected functions provides additional updates that the source-based
> +livepatch author may need to consider.  For example, for the function
> +``scatterwalk_unmap()``:
> +
> +::
> +
> +  $ ./kgraft-ipa-analysis.py --symbol=scatterwalk_unmap aesni-intel_glue.i.000i.ipa-clones
> +  Function: scatterwalk_unmap/2930 (include/crypto/scatterwalk.h:81:60)
> +    isra: scatterwalk_unmap.isra.2/3142 (include/crypto/scatterwalk.h:81:60)
> +      inlining to: helper_rfc4106_decrypt/3007 (arch/x86/crypto/aesni-intel_glue.c:1016:12)
> +      inlining to: helper_rfc4106_decrypt/3007 (arch/x86/crypto/aesni-intel_glue.c:1016:12)
> +      inlining to: helper_rfc4106_encrypt/3006 (arch/x86/crypto/aesni-intel_glue.c:939:12)
> +
> +    Affected functions: 3
> +      scatterwalk_unmap.isra.2/3142 (include/crypto/scatterwalk.h:81:60)
> +      helper_rfc4106_decrypt/3007 (arch/x86/crypto/aesni-intel_glue.c:1016:12)
> +      helper_rfc4106_encrypt/3006 (arch/x86/crypto/aesni-intel_glue.c:939:12)

The example for the github is not up-to-date. The tool now expects 
file_list with *.ipa-clones files and the output is a bit different for 
the recent kernel.

$ echo arch/x86/crypto/aesni-intel_glue.c.000i.ipa-clones | kgraft-ipa-analysis.py --symbol=scatterwalk_unmap /dev/stdin
Parsing file (1/1): arch/x86/crypto/aesni-intel_glue.c.000i.ipa-clones
Function: scatterwalk_unmap/3935 (./include/crypto/scatterwalk.h:59:20) [REMOVED] [object file: arch/x86/crypto/aesni-intel_glue.c.000i.ipa-clones]
  isra: scatterwalk_unmap.isra.8/4117 (./include/crypto/scatterwalk.h:59:20) [REMOVED]
    inlining to: gcmaes_crypt_by_sg/4019 (arch/x86/crypto/aesni-intel_glue.c:682:12) [REMOVED] [edges: 4]
      constprop: gcmaes_crypt_by_sg.constprop.13/4182 (arch/x86/crypto/aesni-intel_glue.c:682:12)

  Affected functions: 3
    scatterwalk_unmap.isra.8/4117 (./include/crypto/scatterwalk.h:59:20) [REMOVED]
    gcmaes_crypt_by_sg/4019 (arch/x86/crypto/aesni-intel_glue.c:682:12) [REMOVED]
    gcmaes_crypt_by_sg.constprop.13/4182 (arch/x86/crypto/aesni-intel_glue.c:682:12)



The rest looks great. Thanks a lot, Joe, for putting it together.

Miroslav

Re: [PATCH 2/2] samples/livepatch: Add README.rst disclaimer

[Miroslav Benes]

On Tue, 21 Jul 2020, Joe Lawrence wrote:

> The livepatch samples aren't very careful with respect to compiler
> IPA-optimization of target kernel functions.
> 
> Add a quick disclaimer and pointer to the compiler-considerations.rst
> file to warn readers.
> 
> Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>

Acked-by: Miroslav Benes <mbenes@suse.cz>

M

Re: refactoring livepatch documentation was Re: [PATCH 1/2] docs/livepatch: Add new compiler considerations doc

[Miroslav Benes]

[side note: So not only that my INBOX is a mess after the summer. I also 
lost some emails apparently. I'm really sorry about that. ]

CCing Nicolai too.

> Hi Petr, Josh,
> 
> The compiler optimization pitfall document can wait for refactored livepatch
> documentation if that puts it into better context, particularly for newbies.
> I don't mind either way.  FWIW, I don't profess to be an authoritative source
> its content -- we've dealt some of these issues in kpatch, so it was
> interesting to see how they affect livepatches that don't rely on binary
> comparison.
> 
> 
> Toward the larger goal, I've changed the thread subject to talk about how we
> may rearrange and supplement our current documentation.  This is a first pass
> at a possible refactoring...
> 
> 
> 1. Provide a better index page to connect the other files/docs, like
> https://www.kernel.org/doc/html/latest/core-api/index.html but obviously not
> that extensive.  Right now we have only a Table of Contents tree without any
> commentary.
> 
> 2. Rearrange and refactor sections:
> 
> livepatch.rst
>   Keep just about everything
>   Add a history section to explain ksplice, kgraft, kpatch for the
>     uninitiated?
>   Add a section on source based vs. binary diff livepatch creation,
>     this may be worth its own top-level section
> 
> Livepatch API
>   Basic API
>   Callbacks
>   Shadow variables
>   Cumulative patches
>   System state
> 
> KLP Relocations
>   Right now this is a bit academic AFAIK kpatch is the only tool
>   currently making use of them.  So maybe this document becomes a
>   more general purpose doc explaining how to reference unexported
>   symbols?  (ie, how does kgraft currently do it, particularly
>   w/kallsyms going unexported?)

Yes, we rely on kallsyms_lookup_name() pretty much right now and once we 
hit the problem with the next kernel version upgrade, we'll have to fix 
it.
 
>   Eventually this could contain klp-convert howto if it ever gets
>   merged.
> 
> Compiler considerations
>   TBD
> 
> I suppose this doesn't create a "Livepatching creation for dummies" guide, but
> my feeling is that there are so many potential (hidden) pitfalls that such
> guide would be dangerous.

It does not create the guide, but it looks like a good basis. I agree with 
Josh here. It might be difficult at the beginning, but the outcome could 
be great even for a newbie and I think we should aim for that.
 
> If someone were to ask me today how to start building a livepatch, I would
> probably point them at the samples to demonstrate the basic concept and API,
> but then implore them to read through the documentation to understand how
> quickly complicated it can become.

True.

We discuss the need to properly document our internal process every once 
in a while and there is always something more important to deal with, but 
it is high time to finally start with that.

Miroslav