[PATCH] Revert "kbuild: use -flive-patching when CONFIG_LIVEPATCH is enabled"

[PATCH] Revert "kbuild: use -flive-patching when CONFIG_LIVEPATCH is enabled"

[Josh Poimboeuf]

Use of the new -flive-patching flag was introduced with the following
commit:

  43bd3a95c98e ("kbuild: use -flive-patching when CONFIG_LIVEPATCH is enabled")

This flag has several drawbacks:

- It disables some optimizations, so it can have a negative effect on
  performance.

- According to the GCC documentation it's not compatible with LTO, which
  will become a compatibility issue as LTO support gets upstreamed in
  the kernel.

- It was intended to be used for source-based patch generation tooling,
  as opposed to binary-based patch generation tooling (e.g.,
  kpatch-build).  It probably should have at least been behind a
  separate config option so as not to negatively affect other livepatch
  users.

- Clang doesn't have the flag, so as far as I can tell, this method of
  generating patches is incompatible with Clang, which like LTO is
  becoming more mainstream.

- It breaks GCC's implicit noreturn detection for local functions.  This
  is the cause of several "unreachable instruction" objtool warnings.

- The broken noreturn detection is an obvious GCC regression, but we
  haven't yet gotten GCC developers to acknowledge that, which doesn't
  inspire confidence in their willingness to keep the feature working as
  optimizations are added or changed going forward.

- While there *is* a distro which relies on this flag for their distro
  livepatch module builds, there's not a publicly documented way to
  create safe livepatch modules with it.  Its use seems to be based on
  tribal knowledge.  It serves no benefit to those who don't know how to
  use it.

  (In fact, I believe the current livepatch documentation and samples
  are misleading and dangerous, and should be corrected.  Or at least
  amended with a disclaimer.  But I don't feel qualified to make such
  changes.)

Also, we have an idea for using objtool to detect function changes,
which could potentially obsolete the need for this flag anyway.

At this point the flag has no benefits for upstream which would
counteract the above drawbacks.  Revert it until it becomes more ready.

This reverts commit 43bd3a95c98e1a86b8b55d97f745c224ecff02b9.

Fixes: 43bd3a95c98e ("kbuild: use -flive-patching when CONFIG_LIVEPATCH is enabled")
Reported-by: Randy Dunlap <rdunlap@infradead.org>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---

NOTE: I tried to be objective, factual, and thorough, to the best of my
knowledge.  Any suggestions for corrections to the commit message are
definitely welcome.

 Makefile | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/Makefile b/Makefile
index 0b5f8538bde5..3b37d25aa028 100644
--- a/Makefile
+++ b/Makefile
@@ -876,10 +876,6 @@ KBUILD_CFLAGS_KERNEL += -ffunction-sections -fdata-sections
 LDFLAGS_vmlinux += --gc-sections
 endif
 
-ifdef CONFIG_LIVEPATCH
-KBUILD_CFLAGS += $(call cc-option, -flive-patching=inline-clone)
-endif
-
 ifdef CONFIG_SHADOW_CALL_STACK
 CC_FLAGS_SCS	:= -fsanitize=shadow-call-stack
 KBUILD_CFLAGS	+= $(CC_FLAGS_SCS)
-- 
2.25.4

Re: [PATCH] Revert "kbuild: use -flive-patching when CONFIG_LIVEPATCH is enabled"

[Joe Lawrence]

On 7/17/20 2:29 PM, Josh Poimboeuf wrote:
> Use of the new -flive-patching flag was introduced with the following
> commit:
> 
>    43bd3a95c98e ("kbuild: use -flive-patching when CONFIG_LIVEPATCH is enabled")
> 
> This flag has several drawbacks:
> 
> [ ... snip ... ]
> 
> - While there *is* a distro which relies on this flag for their distro
>    livepatch module builds, there's not a publicly documented way to
>    create safe livepatch modules with it.  Its use seems to be based on
>    tribal knowledge.  It serves no benefit to those who don't know how to
>    use it.
> 
>    (In fact, I believe the current livepatch documentation and samples
>    are misleading and dangerous, and should be corrected.  Or at least
>    amended with a disclaimer.  But I don't feel qualified to make such
>    changes.)

FWIW, I'm not exactly qualified to document source-based creation 
either, however I have written a few of the samples and obviously the 
kselftest modules.

The samples should certainly include a disclaimer (ie, they are only for 
API demonstration purposes!) and eventually it would be great if the 
kselftest modules could guarantee their safety as well.  I don't know 
quite yet how we can automate that, but perhaps some kind of post-build 
sanity check could verify that they are in fact patching what they 
intend to patch.

As for a more general, long-form warning about optimizations, I grabbed 
Miroslav's LPC slides from a few years back and poked around at some 
IPA-optimized disassembly... Here are my notes that attempt to capture 
some common cases:

http://file.bos.redhat.com/~jolawren/klp-compiler-notes/livepatch/compiler-considerations.html

It's not complete and I lost steam about 80% of the way through today. 
:)  But if it looks useful enough to add to Documentation/livepatch, we 
can work on it on-list and try to steer folks into using the automated 
kpatch-build, objtool (eventually) or a source-based safety checklist. 
The source-based steps have been posted on-list a few times, but I think 
it only needs to be formalized in a doc.

-- Joe

Re: [PATCH] Revert "kbuild: use -flive-patching when CONFIG_LIVEPATCH is enabled"

[Kamalesh Babulal]

On 20/07/20 9:05 am, Joe Lawrence wrote:
> On 7/17/20 2:29 PM, Josh Poimboeuf wrote:
>> Use of the new -flive-patching flag was introduced with the following
>> commit:
>>
>>    43bd3a95c98e ("kbuild: use -flive-patching when CONFIG_LIVEPATCH is enabled")
>>
>> This flag has several drawbacks:
>>
>> [ ... snip ... ]
>>
>> - While there *is* a distro which relies on this flag for their distro
>>    livepatch module builds, there's not a publicly documented way to
>>    create safe livepatch modules with it.  Its use seems to be based on
>>    tribal knowledge.  It serves no benefit to those who don't know how to
>>    use it.
>>
>>    (In fact, I believe the current livepatch documentation and samples
>>    are misleading and dangerous, and should be corrected.  Or at least
>>    amended with a disclaimer.  But I don't feel qualified to make such
>>    changes.)
> 
> FWIW, I'm not exactly qualified to document source-based creation either, however I have written a few of the samples and obviously the kselftest modules.
> 
> The samples should certainly include a disclaimer (ie, they are only for API demonstration purposes!) and eventually it would be great if the kselftest modules could guarantee their safety as well.  I don't know quite yet how we can automate that, but perhaps some kind of post-build sanity check could verify that they are in fact patching what they intend to patch.
> 
> As for a more general, long-form warning about optimizations, I grabbed Miroslav's LPC slides from a few years back and poked around at some IPA-optimized disassembly... Here are my notes that attempt to capture some common cases:
> 
> http://file.bos.redhat.com/~jolawren/klp-compiler-notes/livepatch/compiler-considerations.html

Hi Joe,

The notes link you shared is not accessible.

Regards,

-- 
Kamalesh

Re: [PATCH] Revert "kbuild: use -flive-patching when CONFIG_LIVEPATCH is enabled"

[Joe Lawrence]

On 7/20/20 4:50 AM, Kamalesh Babulal wrote:
> On 20/07/20 9:05 am, Joe Lawrence wrote:
>> On 7/17/20 2:29 PM, Josh Poimboeuf wrote:
>>> Use of the new -flive-patching flag was introduced with the following
>>> commit:
>>>
>>>     43bd3a95c98e ("kbuild: use -flive-patching when CONFIG_LIVEPATCH is enabled")
>>>
>>> This flag has several drawbacks:
>>>
>>> [ ... snip ... ]
>>>
>>> - While there *is* a distro which relies on this flag for their distro
>>>     livepatch module builds, there's not a publicly documented way to
>>>     create safe livepatch modules with it.  Its use seems to be based on
>>>     tribal knowledge.  It serves no benefit to those who don't know how to
>>>     use it.
>>>
>>>     (In fact, I believe the current livepatch documentation and samples
>>>     are misleading and dangerous, and should be corrected.  Or at least
>>>     amended with a disclaimer.  But I don't feel qualified to make such
>>>     changes.)
>>
>> FWIW, I'm not exactly qualified to document source-based creation either, however I have written a few of the samples and obviously the kselftest modules.
>>
>> The samples should certainly include a disclaimer (ie, they are only for API demonstration purposes!) and eventually it would be great if the kselftest modules could guarantee their safety as well.  I don't know quite yet how we can automate that, but perhaps some kind of post-build sanity check could verify that they are in fact patching what they intend to patch.
>>
>> As for a more general, long-form warning about optimizations, I grabbed Miroslav's LPC slides from a few years back and poked around at some IPA-optimized disassembly... Here are my notes that attempt to capture some common cases:
>>
>> http://file.bos.redhat.com/~jolawren/klp-compiler-notes/livepatch/compiler-considerations.html
> 
> Hi Joe,
> 
> The notes link you shared is not accessible.
> 

Oops, lets try that again:

http://people.redhat.com/~jolawren/klp-compiler-notes/livepatch/compiler-considerations.html

-- Joe

Re: [PATCH] Revert "kbuild: use -flive-patching when CONFIG_LIVEPATCH is enabled"

[Miroslav Benes]

On Fri, 17 Jul 2020, Josh Poimboeuf wrote:

> Use of the new -flive-patching flag was introduced with the following
> commit:
> 
>   43bd3a95c98e ("kbuild: use -flive-patching when CONFIG_LIVEPATCH is enabled")
> 
> This flag has several drawbacks:
> 
> - It disables some optimizations, so it can have a negative effect on
>   performance.
> 
> - According to the GCC documentation it's not compatible with LTO, which
>   will become a compatibility issue as LTO support gets upstreamed in
>   the kernel.
> 
> - It was intended to be used for source-based patch generation tooling,
>   as opposed to binary-based patch generation tooling (e.g.,
>   kpatch-build).  It probably should have at least been behind a
>   separate config option so as not to negatively affect other livepatch
>   users.
> 
> - Clang doesn't have the flag, so as far as I can tell, this method of
>   generating patches is incompatible with Clang, which like LTO is
>   becoming more mainstream.
> 
> - It breaks GCC's implicit noreturn detection for local functions.  This
>   is the cause of several "unreachable instruction" objtool warnings.
> 
> - The broken noreturn detection is an obvious GCC regression, but we
>   haven't yet gotten GCC developers to acknowledge that, which doesn't
>   inspire confidence in their willingness to keep the feature working as
>   optimizations are added or changed going forward.
> 
> - While there *is* a distro which relies on this flag for their distro
>   livepatch module builds, there's not a publicly documented way to
>   create safe livepatch modules with it.  Its use seems to be based on
>   tribal knowledge.  It serves no benefit to those who don't know how to
>   use it.
> 
>   (In fact, I believe the current livepatch documentation and samples
>   are misleading and dangerous, and should be corrected.  Or at least
>   amended with a disclaimer.  But I don't feel qualified to make such
>   changes.)
> 
> Also, we have an idea for using objtool to detect function changes,
> which could potentially obsolete the need for this flag anyway.
> 
> At this point the flag has no benefits for upstream which would
> counteract the above drawbacks.  Revert it until it becomes more ready.
> 
> This reverts commit 43bd3a95c98e1a86b8b55d97f745c224ecff02b9.
> 
> Fixes: 43bd3a95c98e ("kbuild: use -flive-patching when CONFIG_LIVEPATCH is enabled")
> Reported-by: Randy Dunlap <rdunlap@infradead.org>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>

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

M

Re: [PATCH] Revert "kbuild: use -flive-patching when CONFIG_LIVEPATCH is enabled"

[Miroslav Benes]

On Sun, 19 Jul 2020, Joe Lawrence wrote:

> On 7/17/20 2:29 PM, Josh Poimboeuf wrote:
> > Use of the new -flive-patching flag was introduced with the following
> > commit:
> > 
> >    43bd3a95c98e ("kbuild: use -flive-patching when CONFIG_LIVEPATCH is
> >    enabled")
> > 
> > This flag has several drawbacks:
> > 
> > [ ... snip ... ]
> > 
> > - While there *is* a distro which relies on this flag for their distro
> >    livepatch module builds, there's not a publicly documented way to
> >    create safe livepatch modules with it.  Its use seems to be based on
> >    tribal knowledge.  It serves no benefit to those who don't know how to
> >    use it.
> > 
> >    (In fact, I believe the current livepatch documentation and samples
> >    are misleading and dangerous, and should be corrected.  Or at least
> >    amended with a disclaimer.  But I don't feel qualified to make such
> >    changes.)
> 
> FWIW, I'm not exactly qualified to document source-based creation either,
> however I have written a few of the samples and obviously the kselftest
> modules.
> 
> The samples should certainly include a disclaimer (ie, they are only for API
> demonstration purposes!) and eventually it would be great if the kselftest
> modules could guarantee their safety as well.  I don't know quite yet how we
> can automate that, but perhaps some kind of post-build sanity check could
> verify that they are in fact patching what they intend to patch.

That's a good idea. We should have something like that. I don't know how 
to make it nice. Just horrible post-build hacks that would check that 
modules were compiled as expected
 
> As for a more general, long-form warning about optimizations, I grabbed
> Miroslav's LPC slides from a few years back and poked around at some
> IPA-optimized disassembly... Here are my notes that attempt to capture some
> common cases:
> 
> http://file.bos.redhat.com/~jolawren/klp-compiler-notes/livepatch/compiler-considerations.html
> 
> It's not complete and I lost steam about 80% of the way through today. 
> :)  But if it looks useful enough to add to Documentation/livepatch, we 
> can work on it on-list and try to steer folks into using the automated
> kpatch-build, objtool (eventually) or a source-based safety checklist.

It looks really useful. Could you prepare a patch and submit it, please? 
We could discuss it there.

> The
> source-based steps have been posted on-list a few times, but I think it only
> needs to be formalized in a doc.

Yes, I think they were. We discussed it with Nicolai to (better) document 
our workflow. It is currently based on klp-ccp 
(https://github.com/SUSE/klp-ccp), but we need a proper documentation how 
to prepare a live patch starting with an ordinary patch.

Thanks
Miroslav

Re: [PATCH] Revert "kbuild: use -flive-patching when CONFIG_LIVEPATCH is enabled"

[Petr Mladek]

On Tue 2020-07-21 13:17:00, Miroslav Benes wrote:
> On Fri, 17 Jul 2020, Josh Poimboeuf wrote:
> 
> > Use of the new -flive-patching flag was introduced with the following
> > commit:
> > 
> >   43bd3a95c98e ("kbuild: use -flive-patching when CONFIG_LIVEPATCH is enabled")
> > 
> > This reverts commit 43bd3a95c98e1a86b8b55d97f745c224ecff02b9.
> > 
> > Fixes: 43bd3a95c98e ("kbuild: use -flive-patching when CONFIG_LIVEPATCH is enabled")
> > Reported-by: Randy Dunlap <rdunlap@infradead.org>
> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> 
> Acked-by: Miroslav Benes <mbenes@suse.cz>

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

Hmm, the patch has not been pushed into livepatching.git and is not
available in the pull request for 5.9.

Is it OK to leave it for 5.10?
Or would you prefer to get it into 5.9 even on this stage?

I personally do not mind. It depends how urgent it is for others.

Best Regards,
Petr

Re: [PATCH] Revert "kbuild: use -flive-patching when CONFIG_LIVEPATCH is enabled"

[Josh Poimboeuf]

On Thu, Aug 06, 2020 at 11:24:26AM +0200, Petr Mladek wrote:
> On Tue 2020-07-21 13:17:00, Miroslav Benes wrote:
> > On Fri, 17 Jul 2020, Josh Poimboeuf wrote:
> > 
> > > Use of the new -flive-patching flag was introduced with the following
> > > commit:
> > > 
> > >   43bd3a95c98e ("kbuild: use -flive-patching when CONFIG_LIVEPATCH is enabled")
> > > 
> > > This reverts commit 43bd3a95c98e1a86b8b55d97f745c224ecff02b9.
> > > 
> > > Fixes: 43bd3a95c98e ("kbuild: use -flive-patching when CONFIG_LIVEPATCH is enabled")
> > > Reported-by: Randy Dunlap <rdunlap@infradead.org>
> > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > 
> > Acked-by: Miroslav Benes <mbenes@suse.cz>
> 
> Acked-by: Petr Mladek <pmladek@suse.com>
> 
> Hmm, the patch has not been pushed into livepatching.git and is not
> available in the pull request for 5.9.
> 
> Is it OK to leave it for 5.10?
> Or would you prefer to get it into 5.9 even on this stage?
> 
> I personally do not mind. It depends how urgent it is for others.

Sorry for leaving this question hanging.  Let's go with 5.10 ;-)

-- 
Josh

Re: [PATCH] Revert "kbuild: use -flive-patching when CONFIG_LIVEPATCH is enabled"

[Petr Mladek]

On Tue 2020-09-01 12:24:51, Josh Poimboeuf wrote:
> On Thu, Aug 06, 2020 at 11:24:26AM +0200, Petr Mladek wrote:
> > On Tue 2020-07-21 13:17:00, Miroslav Benes wrote:
> > > On Fri, 17 Jul 2020, Josh Poimboeuf wrote:
> > > 
> > > > Use of the new -flive-patching flag was introduced with the following
> > > > commit:
> > > > 
> > > >   43bd3a95c98e ("kbuild: use -flive-patching when CONFIG_LIVEPATCH is enabled")
> > > > 
> > > > This reverts commit 43bd3a95c98e1a86b8b55d97f745c224ecff02b9.
> > > > 
> > > > Fixes: 43bd3a95c98e ("kbuild: use -flive-patching when CONFIG_LIVEPATCH is enabled")
> > > > Reported-by: Randy Dunlap <rdunlap@infradead.org>
> > > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > > 
> > > Acked-by: Miroslav Benes <mbenes@suse.cz>
> > 
> > Acked-by: Petr Mladek <pmladek@suse.com>
> > 
> > Hmm, the patch has not been pushed into livepatching.git and is not
> > available in the pull request for 5.9.
> > 
> > Is it OK to leave it for 5.10?
> > Or would you prefer to get it into 5.9 even on this stage?
> > 
> > I personally do not mind. It depends how urgent it is for others.
> 
> Sorry for leaving this question hanging.  Let's go with 5.10 ;-)

The patch is committed in livepatch.git, branch
for-5.10/flive-patching.

Best Regards,
Petr