Re: [PATCH 0/2] Fix issue with alternatives/paravirt patches
On Tue, Jul 12, 2016 at 01:55:54PM +0200, Miroslav Benes wrote:
> On Thu, 7 Jul 2016, Josh Poimboeuf wrote:
>
> > On Thu, Jul 07, 2016 at 05:56:33PM +0200, Petr Mladek wrote:
> > > On Tue 2016-07-05 22:34:58, Jessica Yu wrote:
> > > > Hi,
> > > >
> > > > A few months ago, Chris Arges reported a bug involving
> > > > alternatives/paravirt
> > > > patching that was discussed here [1] and here [2]. To briefly summarize
> > > > the
> > > > bug, patch modules that contained .altinstructions or .parainstructions
> > > > sections would break because these alternative/paravirt patches would be
> > > > applied first by the module loader (see x86 module_finalize()), then
> > > > livepatch would later clobber these patches when applying per-object
> > > > relocations. This lead to crashes and unpredictable behavior.
> > > >
> > > > One conclusion we reached from our last discussion was that we will
> > > > need to introduce some arch-specific code to address this problem.
> > > > This patchset presents a possible fix for the bug by adding a new
> > > > arch-specific arch_klp_init_object_loaded() function that by default
> > > > does nothing but can be overridden by different arches.
> > > >
> > > > To fix this issue for x86, since we can access a patch module's Elf
> > > > sections through mod->klp_info, we can simply delay the calls to
> > > > apply_paravirt() and apply_alternatives() to
> > > > arch_klp_init_object_loaded(),
> > > > which is called after relocations have been written for an object.
> > > > In addition, for patch modules, .parainstructions and .altinstructions
> > > > are
> > > > prefixed by ".klp.arch.${objname}" so that the module loader ignores
> > > > them
> > > > and livepatch can apply them manually.
> > >
> > > The solution looks correct to me. The fun will be how to generate
> > > the sections. If I get this correctly, it is not enough to rename
> > > the existing ones. Instead, we need to split .parainstructions
> > > and .altinstructions sections into per-object ones.
> > >
> > > I wonder if there is a plan for this. Especially I am interested
> > > into the patches created from sources ;-) I wonder if we could add
> > > a tag somewhere and improve the build infrastructure.
> >
> > Yeah. I'd like to reiterate[1] that this would all be a lot easier if
> > we weren't circumventing module dependencies.
> >
> > [1]
> > https://lkml.kernel.org/r/20160404161428.3qap2i4vpgda6...@treble.redhat.com
>
> Oh, we haven't come to any conclusion. I think it would be a great topic
> for Plumbers conf. It is always better to discuss such things personally.
> What do you think? Any volunteer to propose it? :)
Well, it's somewhat related to my "Livepatch module creation tooling"
proposed talk, because I suspect the tooling could be *much* simpler if
we didn't circumvent module dependencies. So I'll probably talk about
that aspect of it.
But it would be great if somebody wanted to submit a separate talk to
explore the pros and cons of our current "load patches to modules before
the modules themselves have been loaded" approach and if there are any
viable alternatives.
--
Josh
Re: [PATCH 0/2] Fix issue with alternatives/paravirt patches
On Tue, Jul 12, 2016 at 01:55:54PM +0200, Miroslav Benes wrote:
> On Thu, 7 Jul 2016, Josh Poimboeuf wrote:
>
> > On Thu, Jul 07, 2016 at 05:56:33PM +0200, Petr Mladek wrote:
> > > On Tue 2016-07-05 22:34:58, Jessica Yu wrote:
> > > > Hi,
> > > >
> > > > A few months ago, Chris Arges reported a bug involving
> > > > alternatives/paravirt
> > > > patching that was discussed here [1] and here [2]. To briefly summarize
> > > > the
> > > > bug, patch modules that contained .altinstructions or .parainstructions
> > > > sections would break because these alternative/paravirt patches would be
> > > > applied first by the module loader (see x86 module_finalize()), then
> > > > livepatch would later clobber these patches when applying per-object
> > > > relocations. This lead to crashes and unpredictable behavior.
> > > >
> > > > One conclusion we reached from our last discussion was that we will
> > > > need to introduce some arch-specific code to address this problem.
> > > > This patchset presents a possible fix for the bug by adding a new
> > > > arch-specific arch_klp_init_object_loaded() function that by default
> > > > does nothing but can be overridden by different arches.
> > > >
> > > > To fix this issue for x86, since we can access a patch module's Elf
> > > > sections through mod->klp_info, we can simply delay the calls to
> > > > apply_paravirt() and apply_alternatives() to
> > > > arch_klp_init_object_loaded(),
> > > > which is called after relocations have been written for an object.
> > > > In addition, for patch modules, .parainstructions and .altinstructions
> > > > are
> > > > prefixed by ".klp.arch.${objname}" so that the module loader ignores
> > > > them
> > > > and livepatch can apply them manually.
> > >
> > > The solution looks correct to me. The fun will be how to generate
> > > the sections. If I get this correctly, it is not enough to rename
> > > the existing ones. Instead, we need to split .parainstructions
> > > and .altinstructions sections into per-object ones.
> > >
> > > I wonder if there is a plan for this. Especially I am interested
> > > into the patches created from sources ;-) I wonder if we could add
> > > a tag somewhere and improve the build infrastructure.
> >
> > Yeah. I'd like to reiterate[1] that this would all be a lot easier if
> > we weren't circumventing module dependencies.
> >
> > [1]
> > https://lkml.kernel.org/r/20160404161428.3qap2i4vpgda6...@treble.redhat.com
>
> Oh, we haven't come to any conclusion. I think it would be a great topic
> for Plumbers conf. It is always better to discuss such things personally.
> What do you think? Any volunteer to propose it? :)
Well, it's somewhat related to my "Livepatch module creation tooling"
proposed talk, because I suspect the tooling could be *much* simpler if
we didn't circumvent module dependencies. So I'll probably talk about
that aspect of it.
But it would be great if somebody wanted to submit a separate talk to
explore the pros and cons of our current "load patches to modules before
the modules themselves have been loaded" approach and if there are any
viable alternatives.
--
Josh
Re: [PATCH 0/2] Fix issue with alternatives/paravirt patches
On Tue, 5 Jul 2016, Jessica Yu wrote:
> Hi,
>
> A few months ago, Chris Arges reported a bug involving alternatives/paravirt
> patching that was discussed here [1] and here [2]. To briefly summarize the
> bug, patch modules that contained .altinstructions or .parainstructions
> sections would break because these alternative/paravirt patches would be
> applied first by the module loader (see x86 module_finalize()), then
> livepatch would later clobber these patches when applying per-object
> relocations. This lead to crashes and unpredictable behavior.
>
> One conclusion we reached from our last discussion was that we will
> need to introduce some arch-specific code to address this problem.
> This patchset presents a possible fix for the bug by adding a new
> arch-specific arch_klp_init_object_loaded() function that by default
> does nothing but can be overridden by different arches.
>
> To fix this issue for x86, since we can access a patch module's Elf
> sections through mod->klp_info, we can simply delay the calls to
> apply_paravirt() and apply_alternatives() to arch_klp_init_object_loaded(),
> which is called after relocations have been written for an object.
> In addition, for patch modules, .parainstructions and .altinstructions are
> prefixed by ".klp.arch.${objname}" so that the module loader ignores them
> and livepatch can apply them manually.
>
> Currently for kpatch, we don't support including jump table sections in
> the patch module, and supporting .smp_locks is currently broken, so we
> don't consider those sections (for now).
>
> I did some light testing with some patches to kvm and verified that the
> original issue reported in [2] was fixed.
>
> Based on linux-next.
>
> [1] http://thread.gmane.org/gmane.linux.kernel/2185604/
> [2] https://github.com/dynup/kpatch/issues/580
>
> Jessica Yu (2):
> livepatch: use arch_klp_init_object_loaded() to finish arch-specific tasks
> livepatch/x86: apply alternatives and paravirt patches after relocations
>
> arch/x86/kernel/Makefile| 1 +
> arch/x86/kernel/livepatch.c | 66
> +
> include/linux/livepatch.h | 3 +++
> kernel/livepatch/core.c | 12 +++--
> 4 files changed, 80 insertions(+), 2 deletions(-)
> create mode 100644 arch/x86/kernel/livepatch.c
Hi,
thanks Jessica for implementing it. It does not look as bad as I was
afraid, which is great. Few remarks...
Is there a problem when you need to generate a dynrela for paravirt code?
I mean one does not know during the build of a patch module if paravirt
would or would not be applied. If such code needs to be relocated it could
be a problem for kpatch-build. Is this correct or am I missing something?
Now the other architectures we support. s390 should be ok. There is
nothing much in module_finalize() there. powerpc is different though. It
is quite similar to x86_64 case. And also arm64 needs to be handled in
future.
Thanks,
Miroslav
Re: [PATCH 0/2] Fix issue with alternatives/paravirt patches
On Tue, 5 Jul 2016, Jessica Yu wrote:
> Hi,
>
> A few months ago, Chris Arges reported a bug involving alternatives/paravirt
> patching that was discussed here [1] and here [2]. To briefly summarize the
> bug, patch modules that contained .altinstructions or .parainstructions
> sections would break because these alternative/paravirt patches would be
> applied first by the module loader (see x86 module_finalize()), then
> livepatch would later clobber these patches when applying per-object
> relocations. This lead to crashes and unpredictable behavior.
>
> One conclusion we reached from our last discussion was that we will
> need to introduce some arch-specific code to address this problem.
> This patchset presents a possible fix for the bug by adding a new
> arch-specific arch_klp_init_object_loaded() function that by default
> does nothing but can be overridden by different arches.
>
> To fix this issue for x86, since we can access a patch module's Elf
> sections through mod->klp_info, we can simply delay the calls to
> apply_paravirt() and apply_alternatives() to arch_klp_init_object_loaded(),
> which is called after relocations have been written for an object.
> In addition, for patch modules, .parainstructions and .altinstructions are
> prefixed by ".klp.arch.${objname}" so that the module loader ignores them
> and livepatch can apply them manually.
>
> Currently for kpatch, we don't support including jump table sections in
> the patch module, and supporting .smp_locks is currently broken, so we
> don't consider those sections (for now).
>
> I did some light testing with some patches to kvm and verified that the
> original issue reported in [2] was fixed.
>
> Based on linux-next.
>
> [1] http://thread.gmane.org/gmane.linux.kernel/2185604/
> [2] https://github.com/dynup/kpatch/issues/580
>
> Jessica Yu (2):
> livepatch: use arch_klp_init_object_loaded() to finish arch-specific tasks
> livepatch/x86: apply alternatives and paravirt patches after relocations
>
> arch/x86/kernel/Makefile| 1 +
> arch/x86/kernel/livepatch.c | 66
> +
> include/linux/livepatch.h | 3 +++
> kernel/livepatch/core.c | 12 +++--
> 4 files changed, 80 insertions(+), 2 deletions(-)
> create mode 100644 arch/x86/kernel/livepatch.c
Hi,
thanks Jessica for implementing it. It does not look as bad as I was
afraid, which is great. Few remarks...
Is there a problem when you need to generate a dynrela for paravirt code?
I mean one does not know during the build of a patch module if paravirt
would or would not be applied. If such code needs to be relocated it could
be a problem for kpatch-build. Is this correct or am I missing something?
Now the other architectures we support. s390 should be ok. There is
nothing much in module_finalize() there. powerpc is different though. It
is quite similar to x86_64 case. And also arm64 needs to be handled in
future.
Thanks,
Miroslav
Re: [PATCH 0/2] Fix issue with alternatives/paravirt patches
On Thu, 7 Jul 2016, Josh Poimboeuf wrote:
> On Thu, Jul 07, 2016 at 05:56:33PM +0200, Petr Mladek wrote:
> > On Tue 2016-07-05 22:34:58, Jessica Yu wrote:
> > > Hi,
> > >
> > > A few months ago, Chris Arges reported a bug involving
> > > alternatives/paravirt
> > > patching that was discussed here [1] and here [2]. To briefly summarize
> > > the
> > > bug, patch modules that contained .altinstructions or .parainstructions
> > > sections would break because these alternative/paravirt patches would be
> > > applied first by the module loader (see x86 module_finalize()), then
> > > livepatch would later clobber these patches when applying per-object
> > > relocations. This lead to crashes and unpredictable behavior.
> > >
> > > One conclusion we reached from our last discussion was that we will
> > > need to introduce some arch-specific code to address this problem.
> > > This patchset presents a possible fix for the bug by adding a new
> > > arch-specific arch_klp_init_object_loaded() function that by default
> > > does nothing but can be overridden by different arches.
> > >
> > > To fix this issue for x86, since we can access a patch module's Elf
> > > sections through mod->klp_info, we can simply delay the calls to
> > > apply_paravirt() and apply_alternatives() to
> > > arch_klp_init_object_loaded(),
> > > which is called after relocations have been written for an object.
> > > In addition, for patch modules, .parainstructions and .altinstructions are
> > > prefixed by ".klp.arch.${objname}" so that the module loader ignores them
> > > and livepatch can apply them manually.
> >
> > The solution looks correct to me. The fun will be how to generate
> > the sections. If I get this correctly, it is not enough to rename
> > the existing ones. Instead, we need to split .parainstructions
> > and .altinstructions sections into per-object ones.
> >
> > I wonder if there is a plan for this. Especially I am interested
> > into the patches created from sources ;-) I wonder if we could add
> > a tag somewhere and improve the build infrastructure.
>
> Yeah. I'd like to reiterate[1] that this would all be a lot easier if
> we weren't circumventing module dependencies.
>
> [1]
> https://lkml.kernel.org/r/20160404161428.3qap2i4vpgda6...@treble.redhat.com
Oh, we haven't come to any conclusion. I think it would be a great topic
for Plumbers conf. It is always better to discuss such things personally.
What do you think? Any volunteer to propose it? :)
Miroslav
Re: [PATCH 0/2] Fix issue with alternatives/paravirt patches
On Thu, 7 Jul 2016, Josh Poimboeuf wrote:
> On Thu, Jul 07, 2016 at 05:56:33PM +0200, Petr Mladek wrote:
> > On Tue 2016-07-05 22:34:58, Jessica Yu wrote:
> > > Hi,
> > >
> > > A few months ago, Chris Arges reported a bug involving
> > > alternatives/paravirt
> > > patching that was discussed here [1] and here [2]. To briefly summarize
> > > the
> > > bug, patch modules that contained .altinstructions or .parainstructions
> > > sections would break because these alternative/paravirt patches would be
> > > applied first by the module loader (see x86 module_finalize()), then
> > > livepatch would later clobber these patches when applying per-object
> > > relocations. This lead to crashes and unpredictable behavior.
> > >
> > > One conclusion we reached from our last discussion was that we will
> > > need to introduce some arch-specific code to address this problem.
> > > This patchset presents a possible fix for the bug by adding a new
> > > arch-specific arch_klp_init_object_loaded() function that by default
> > > does nothing but can be overridden by different arches.
> > >
> > > To fix this issue for x86, since we can access a patch module's Elf
> > > sections through mod->klp_info, we can simply delay the calls to
> > > apply_paravirt() and apply_alternatives() to
> > > arch_klp_init_object_loaded(),
> > > which is called after relocations have been written for an object.
> > > In addition, for patch modules, .parainstructions and .altinstructions are
> > > prefixed by ".klp.arch.${objname}" so that the module loader ignores them
> > > and livepatch can apply them manually.
> >
> > The solution looks correct to me. The fun will be how to generate
> > the sections. If I get this correctly, it is not enough to rename
> > the existing ones. Instead, we need to split .parainstructions
> > and .altinstructions sections into per-object ones.
> >
> > I wonder if there is a plan for this. Especially I am interested
> > into the patches created from sources ;-) I wonder if we could add
> > a tag somewhere and improve the build infrastructure.
>
> Yeah. I'd like to reiterate[1] that this would all be a lot easier if
> we weren't circumventing module dependencies.
>
> [1]
> https://lkml.kernel.org/r/20160404161428.3qap2i4vpgda6...@treble.redhat.com
Oh, we haven't come to any conclusion. I think it would be a great topic
for Plumbers conf. It is always better to discuss such things personally.
What do you think? Any volunteer to propose it? :)
Miroslav
Re: [PATCH 0/2] Fix issue with alternatives/paravirt patches
On Tue, Jul 05, 2016 at 10:34:58PM -0400, Jessica Yu wrote:
> Hi,
>
> A few months ago, Chris Arges reported a bug involving alternatives/paravirt
> patching that was discussed here [1] and here [2]. To briefly summarize the
> bug, patch modules that contained .altinstructions or .parainstructions
> sections would break because these alternative/paravirt patches would be
> applied first by the module loader (see x86 module_finalize()), then
> livepatch would later clobber these patches when applying per-object
> relocations. This lead to crashes and unpredictable behavior.
>
> One conclusion we reached from our last discussion was that we will
> need to introduce some arch-specific code to address this problem.
> This patchset presents a possible fix for the bug by adding a new
> arch-specific arch_klp_init_object_loaded() function that by default
> does nothing but can be overridden by different arches.
>
> To fix this issue for x86, since we can access a patch module's Elf
> sections through mod->klp_info, we can simply delay the calls to
> apply_paravirt() and apply_alternatives() to arch_klp_init_object_loaded(),
> which is called after relocations have been written for an object.
> In addition, for patch modules, .parainstructions and .altinstructions are
> prefixed by ".klp.arch.${objname}" so that the module loader ignores them
> and livepatch can apply them manually.
>
> Currently for kpatch, we don't support including jump table sections in
> the patch module, and supporting .smp_locks is currently broken, so we
> don't consider those sections (for now).
>
> I did some light testing with some patches to kvm and verified that the
> original issue reported in [2] was fixed.
>
> Based on linux-next.
>
Jessica,
I was able to test these patches on top of linux-next. I took your kpatch
branch and hacked it a bit to get it working and was able to
apply a patch to 'kvm_arch_vm_ioctl' while running a VM workload.
Great job!
Tested-by: Chris J Arges
--chris
> [1] http://thread.gmane.org/gmane.linux.kernel/2185604/
> [2] https://github.com/dynup/kpatch/issues/580
>
> Jessica Yu (2):
> livepatch: use arch_klp_init_object_loaded() to finish arch-specific tasks
> livepatch/x86: apply alternatives and paravirt patches after relocations
>
> arch/x86/kernel/Makefile| 1 +
> arch/x86/kernel/livepatch.c | 66
> +
> include/linux/livepatch.h | 3 +++
> kernel/livepatch/core.c | 12 +++--
> 4 files changed, 80 insertions(+), 2 deletions(-)
> create mode 100644 arch/x86/kernel/livepatch.c
>
> --
> 2.4.3
>
Re: [PATCH 0/2] Fix issue with alternatives/paravirt patches
On Tue, Jul 05, 2016 at 10:34:58PM -0400, Jessica Yu wrote:
> Hi,
>
> A few months ago, Chris Arges reported a bug involving alternatives/paravirt
> patching that was discussed here [1] and here [2]. To briefly summarize the
> bug, patch modules that contained .altinstructions or .parainstructions
> sections would break because these alternative/paravirt patches would be
> applied first by the module loader (see x86 module_finalize()), then
> livepatch would later clobber these patches when applying per-object
> relocations. This lead to crashes and unpredictable behavior.
>
> One conclusion we reached from our last discussion was that we will
> need to introduce some arch-specific code to address this problem.
> This patchset presents a possible fix for the bug by adding a new
> arch-specific arch_klp_init_object_loaded() function that by default
> does nothing but can be overridden by different arches.
>
> To fix this issue for x86, since we can access a patch module's Elf
> sections through mod->klp_info, we can simply delay the calls to
> apply_paravirt() and apply_alternatives() to arch_klp_init_object_loaded(),
> which is called after relocations have been written for an object.
> In addition, for patch modules, .parainstructions and .altinstructions are
> prefixed by ".klp.arch.${objname}" so that the module loader ignores them
> and livepatch can apply them manually.
>
> Currently for kpatch, we don't support including jump table sections in
> the patch module, and supporting .smp_locks is currently broken, so we
> don't consider those sections (for now).
>
> I did some light testing with some patches to kvm and verified that the
> original issue reported in [2] was fixed.
>
> Based on linux-next.
>
Jessica,
I was able to test these patches on top of linux-next. I took your kpatch
branch and hacked it a bit to get it working and was able to
apply a patch to 'kvm_arch_vm_ioctl' while running a VM workload.
Great job!
Tested-by: Chris J Arges
--chris
> [1] http://thread.gmane.org/gmane.linux.kernel/2185604/
> [2] https://github.com/dynup/kpatch/issues/580
>
> Jessica Yu (2):
> livepatch: use arch_klp_init_object_loaded() to finish arch-specific tasks
> livepatch/x86: apply alternatives and paravirt patches after relocations
>
> arch/x86/kernel/Makefile| 1 +
> arch/x86/kernel/livepatch.c | 66
> +
> include/linux/livepatch.h | 3 +++
> kernel/livepatch/core.c | 12 +++--
> 4 files changed, 80 insertions(+), 2 deletions(-)
> create mode 100644 arch/x86/kernel/livepatch.c
>
> --
> 2.4.3
>
Re: [PATCH 0/2] Fix issue with alternatives/paravirt patches
On Thu, Jul 07, 2016 at 05:56:33PM +0200, Petr Mladek wrote:
> On Tue 2016-07-05 22:34:58, Jessica Yu wrote:
> > Hi,
> >
> > A few months ago, Chris Arges reported a bug involving alternatives/paravirt
> > patching that was discussed here [1] and here [2]. To briefly summarize the
> > bug, patch modules that contained .altinstructions or .parainstructions
> > sections would break because these alternative/paravirt patches would be
> > applied first by the module loader (see x86 module_finalize()), then
> > livepatch would later clobber these patches when applying per-object
> > relocations. This lead to crashes and unpredictable behavior.
> >
> > One conclusion we reached from our last discussion was that we will
> > need to introduce some arch-specific code to address this problem.
> > This patchset presents a possible fix for the bug by adding a new
> > arch-specific arch_klp_init_object_loaded() function that by default
> > does nothing but can be overridden by different arches.
> >
> > To fix this issue for x86, since we can access a patch module's Elf
> > sections through mod->klp_info, we can simply delay the calls to
> > apply_paravirt() and apply_alternatives() to arch_klp_init_object_loaded(),
> > which is called after relocations have been written for an object.
> > In addition, for patch modules, .parainstructions and .altinstructions are
> > prefixed by ".klp.arch.${objname}" so that the module loader ignores them
> > and livepatch can apply them manually.
>
> The solution looks correct to me. The fun will be how to generate
> the sections. If I get this correctly, it is not enough to rename
> the existing ones. Instead, we need to split .parainstructions
> and .altinstructions sections into per-object ones.
>
> I wonder if there is a plan for this. Especially I am interested
> into the patches created from sources ;-) I wonder if we could add
> a tag somewhere and improve the build infrastructure.
Yeah. I'd like to reiterate[1] that this would all be a lot easier if
we weren't circumventing module dependencies.
[1] https://lkml.kernel.org/r/20160404161428.3qap2i4vpgda6...@treble.redhat.com
--
Josh
Re: [PATCH 0/2] Fix issue with alternatives/paravirt patches
On Thu, Jul 07, 2016 at 05:56:33PM +0200, Petr Mladek wrote:
> On Tue 2016-07-05 22:34:58, Jessica Yu wrote:
> > Hi,
> >
> > A few months ago, Chris Arges reported a bug involving alternatives/paravirt
> > patching that was discussed here [1] and here [2]. To briefly summarize the
> > bug, patch modules that contained .altinstructions or .parainstructions
> > sections would break because these alternative/paravirt patches would be
> > applied first by the module loader (see x86 module_finalize()), then
> > livepatch would later clobber these patches when applying per-object
> > relocations. This lead to crashes and unpredictable behavior.
> >
> > One conclusion we reached from our last discussion was that we will
> > need to introduce some arch-specific code to address this problem.
> > This patchset presents a possible fix for the bug by adding a new
> > arch-specific arch_klp_init_object_loaded() function that by default
> > does nothing but can be overridden by different arches.
> >
> > To fix this issue for x86, since we can access a patch module's Elf
> > sections through mod->klp_info, we can simply delay the calls to
> > apply_paravirt() and apply_alternatives() to arch_klp_init_object_loaded(),
> > which is called after relocations have been written for an object.
> > In addition, for patch modules, .parainstructions and .altinstructions are
> > prefixed by ".klp.arch.${objname}" so that the module loader ignores them
> > and livepatch can apply them manually.
>
> The solution looks correct to me. The fun will be how to generate
> the sections. If I get this correctly, it is not enough to rename
> the existing ones. Instead, we need to split .parainstructions
> and .altinstructions sections into per-object ones.
>
> I wonder if there is a plan for this. Especially I am interested
> into the patches created from sources ;-) I wonder if we could add
> a tag somewhere and improve the build infrastructure.
Yeah. I'd like to reiterate[1] that this would all be a lot easier if
we weren't circumventing module dependencies.
[1] https://lkml.kernel.org/r/20160404161428.3qap2i4vpgda6...@treble.redhat.com
--
Josh
Re: [PATCH 0/2] Fix issue with alternatives/paravirt patches
On Tue 2016-07-05 22:34:58, Jessica Yu wrote:
> Hi,
>
> A few months ago, Chris Arges reported a bug involving alternatives/paravirt
> patching that was discussed here [1] and here [2]. To briefly summarize the
> bug, patch modules that contained .altinstructions or .parainstructions
> sections would break because these alternative/paravirt patches would be
> applied first by the module loader (see x86 module_finalize()), then
> livepatch would later clobber these patches when applying per-object
> relocations. This lead to crashes and unpredictable behavior.
>
> One conclusion we reached from our last discussion was that we will
> need to introduce some arch-specific code to address this problem.
> This patchset presents a possible fix for the bug by adding a new
> arch-specific arch_klp_init_object_loaded() function that by default
> does nothing but can be overridden by different arches.
>
> To fix this issue for x86, since we can access a patch module's Elf
> sections through mod->klp_info, we can simply delay the calls to
> apply_paravirt() and apply_alternatives() to arch_klp_init_object_loaded(),
> which is called after relocations have been written for an object.
> In addition, for patch modules, .parainstructions and .altinstructions are
> prefixed by ".klp.arch.${objname}" so that the module loader ignores them
> and livepatch can apply them manually.
The solution looks correct to me. The fun will be how to generate
the sections. If I get this correctly, it is not enough to rename
the existing ones. Instead, we need to split .parainstructions
and .altinstructions sections into per-object ones.
I wonder if there is a plan for this. Especially I am interested
into the patches created from sources ;-) I wonder if we could add
a tag somewhere and improve the build infrastructure.
Best Regards,
Petr
Re: [PATCH 0/2] Fix issue with alternatives/paravirt patches
On Tue 2016-07-05 22:34:58, Jessica Yu wrote:
> Hi,
>
> A few months ago, Chris Arges reported a bug involving alternatives/paravirt
> patching that was discussed here [1] and here [2]. To briefly summarize the
> bug, patch modules that contained .altinstructions or .parainstructions
> sections would break because these alternative/paravirt patches would be
> applied first by the module loader (see x86 module_finalize()), then
> livepatch would later clobber these patches when applying per-object
> relocations. This lead to crashes and unpredictable behavior.
>
> One conclusion we reached from our last discussion was that we will
> need to introduce some arch-specific code to address this problem.
> This patchset presents a possible fix for the bug by adding a new
> arch-specific arch_klp_init_object_loaded() function that by default
> does nothing but can be overridden by different arches.
>
> To fix this issue for x86, since we can access a patch module's Elf
> sections through mod->klp_info, we can simply delay the calls to
> apply_paravirt() and apply_alternatives() to arch_klp_init_object_loaded(),
> which is called after relocations have been written for an object.
> In addition, for patch modules, .parainstructions and .altinstructions are
> prefixed by ".klp.arch.${objname}" so that the module loader ignores them
> and livepatch can apply them manually.
The solution looks correct to me. The fun will be how to generate
the sections. If I get this correctly, it is not enough to rename
the existing ones. Instead, we need to split .parainstructions
and .altinstructions sections into per-object ones.
I wonder if there is a plan for this. Especially I am interested
into the patches created from sources ;-) I wonder if we could add
a tag somewhere and improve the build infrastructure.
Best Regards,
Petr
[PATCH 0/2] Fix issue with alternatives/paravirt patches
Hi,
A few months ago, Chris Arges reported a bug involving alternatives/paravirt
patching that was discussed here [1] and here [2]. To briefly summarize the
bug, patch modules that contained .altinstructions or .parainstructions
sections would break because these alternative/paravirt patches would be
applied first by the module loader (see x86 module_finalize()), then
livepatch would later clobber these patches when applying per-object
relocations. This lead to crashes and unpredictable behavior.
One conclusion we reached from our last discussion was that we will
need to introduce some arch-specific code to address this problem.
This patchset presents a possible fix for the bug by adding a new
arch-specific arch_klp_init_object_loaded() function that by default
does nothing but can be overridden by different arches.
To fix this issue for x86, since we can access a patch module's Elf
sections through mod->klp_info, we can simply delay the calls to
apply_paravirt() and apply_alternatives() to arch_klp_init_object_loaded(),
which is called after relocations have been written for an object.
In addition, for patch modules, .parainstructions and .altinstructions are
prefixed by ".klp.arch.${objname}" so that the module loader ignores them
and livepatch can apply them manually.
Currently for kpatch, we don't support including jump table sections in
the patch module, and supporting .smp_locks is currently broken, so we
don't consider those sections (for now).
I did some light testing with some patches to kvm and verified that the
original issue reported in [2] was fixed.
Based on linux-next.
[1] http://thread.gmane.org/gmane.linux.kernel/2185604/
[2] https://github.com/dynup/kpatch/issues/580
Jessica Yu (2):
livepatch: use arch_klp_init_object_loaded() to finish arch-specific tasks
livepatch/x86: apply alternatives and paravirt patches after relocations
arch/x86/kernel/Makefile| 1 +
arch/x86/kernel/livepatch.c | 66 +
include/linux/livepatch.h | 3 +++
kernel/livepatch/core.c | 12 +++--
4 files changed, 80 insertions(+), 2 deletions(-)
create mode 100644 arch/x86/kernel/livepatch.c
--
2.4.3
[PATCH 0/2] Fix issue with alternatives/paravirt patches
Hi,
A few months ago, Chris Arges reported a bug involving alternatives/paravirt
patching that was discussed here [1] and here [2]. To briefly summarize the
bug, patch modules that contained .altinstructions or .parainstructions
sections would break because these alternative/paravirt patches would be
applied first by the module loader (see x86 module_finalize()), then
livepatch would later clobber these patches when applying per-object
relocations. This lead to crashes and unpredictable behavior.
One conclusion we reached from our last discussion was that we will
need to introduce some arch-specific code to address this problem.
This patchset presents a possible fix for the bug by adding a new
arch-specific arch_klp_init_object_loaded() function that by default
does nothing but can be overridden by different arches.
To fix this issue for x86, since we can access a patch module's Elf
sections through mod->klp_info, we can simply delay the calls to
apply_paravirt() and apply_alternatives() to arch_klp_init_object_loaded(),
which is called after relocations have been written for an object.
In addition, for patch modules, .parainstructions and .altinstructions are
prefixed by ".klp.arch.${objname}" so that the module loader ignores them
and livepatch can apply them manually.
Currently for kpatch, we don't support including jump table sections in
the patch module, and supporting .smp_locks is currently broken, so we
don't consider those sections (for now).
I did some light testing with some patches to kvm and verified that the
original issue reported in [2] was fixed.
Based on linux-next.
[1] http://thread.gmane.org/gmane.linux.kernel/2185604/
[2] https://github.com/dynup/kpatch/issues/580
Jessica Yu (2):
livepatch: use arch_klp_init_object_loaded() to finish arch-specific tasks
livepatch/x86: apply alternatives and paravirt patches after relocations
arch/x86/kernel/Makefile| 1 +
arch/x86/kernel/livepatch.c | 66 +
include/linux/livepatch.h | 3 +++
kernel/livepatch/core.c | 12 +++--
4 files changed, 80 insertions(+), 2 deletions(-)
create mode 100644 arch/x86/kernel/livepatch.c
--
2.4.3
