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 <[email protected]>
Signed-off-by: Josh Poimboeuf <[email protected]>
---
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
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
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
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
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 <[email protected]>
> Signed-off-by: Josh Poimboeuf <[email protected]>
Acked-by: Miroslav Benes <[email protected]>
M
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
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 <[email protected]>
> > Signed-off-by: Josh Poimboeuf <[email protected]>
>
> Acked-by: Miroslav Benes <[email protected]>
Acked-by: Petr Mladek <[email protected]>
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
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 <[email protected]>
> > > Signed-off-by: Josh Poimboeuf <[email protected]>
> >
> > Acked-by: Miroslav Benes <[email protected]>
>
> Acked-by: Petr Mladek <[email protected]>
>
> 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
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 <[email protected]>
> > > > Signed-off-by: Josh Poimboeuf <[email protected]>
> > >
> > > Acked-by: Miroslav Benes <[email protected]>
> >
> > Acked-by: Petr Mladek <[email protected]>
> >
> > 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