diff mbox series

kexec: Support purgatories with .text.hot sections

Message ID 20230321-kexec_clang16-v1-0-a768fc2c7c4d@chromium.org
State New
Headers
Series kexec: Support purgatories with .text.hot sections |

Commit Message

Ricardo Ribalda March 21, 2023, 11:49 a.m. UTC 
  Clang16 links the purgatory text in two sections:

  [ 1] .text             PROGBITS         0000000000000000  00000040
       00000000000011a1  0000000000000000  AX       0     0     16
  [ 2] .rela.text        RELA             0000000000000000  00003498
       0000000000000648  0000000000000018   I      24     1     8
  ...
  [17] .text.hot.        PROGBITS         0000000000000000  00003220
       000000000000020b  0000000000000000  AX       0     0     1
  [18] .rela.text.hot.   RELA             0000000000000000  00004428
       0000000000000078  0000000000000018   I      24    17     8

And both of them have their range [sh_addr ... sh_addr+sh_size] on the
area pointed by `e_entry`.

This causes that image->start is calculated twice, once for .text and
another time for .text.hot. The second calculation leaves image->start
in a random location.

Because of this, the system crashes inmediatly after:

kexec_core: Starting new kernel

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
kexec: Support purgatories with .text.hot sections

Clang16 links the purgatory text in two sections:

  [ 1] .text             PROGBITS         0000000000000000  00000040
       00000000000011a1  0000000000000000  AX       0     0     16
  [ 2] .rela.text        RELA             0000000000000000  00003498
       0000000000000648  0000000000000018   I      24     1     8
  ...
  [17] .text.hot.        PROGBITS         0000000000000000  00003220
       000000000000020b  0000000000000000  AX       0     0     1
  [18] .rela.text.hot.   RELA             0000000000000000  00004428
       0000000000000078  0000000000000018   I      24    17     8

And both of them have their range [sh_addr ... sh_addr+sh_size] on the
area pointed by `e_entry`.

This causes that image->start is calculated twice, once for .text and
another time for .text.hot. The second calculation leaves image->start
in a random location.

Because of this, the system crashes inmediatly after:

kexec_core: Starting new kernel

To: Eric Biederman <ebiederm@xmission.com>
Cc: Philipp Rudo <prudo@linux.vnet.ibm.com>
Cc: kexec@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 kernel/kexec_file.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)


---
base-commit: 17214b70a159c6547df9ae204a6275d983146f6b
change-id: 20230321-kexec_clang16-4510c23d129c

Best regards,

Comments

Steven Rostedt March 22, 2023, 2:34 p.m. UTC | #1 
On Tue, Mar 21, 2023 at 12:49:08PM +0100, Ricardo Ribalda wrote:
> Clang16 links the purgatory text in two sections:
> 
>   [ 1] .text             PROGBITS         0000000000000000  00000040
>        00000000000011a1  0000000000000000  AX       0     0     16
>   [ 2] .rela.text        RELA             0000000000000000  00003498
>        0000000000000648  0000000000000018   I      24     1     8
>   ...
>   [17] .text.hot.        PROGBITS         0000000000000000  00003220
>        000000000000020b  0000000000000000  AX       0     0     1
>   [18] .rela.text.hot.   RELA             0000000000000000  00004428
>        0000000000000078  0000000000000018   I      24    17     8
> 
> And both of them have their range [sh_addr ... sh_addr+sh_size] on the
> area pointed by `e_entry`.
> 
> This causes that image->start is calculated twice, once for .text and
> another time for .text.hot. The second calculation leaves image->start
> in a random location.
> 
> Because of this, the system crashes inmediatly after:
> 
> kexec_core: Starting new kernel
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> To: Eric Biederman <ebiederm@xmission.com>
> Cc: Philipp Rudo <prudo@linux.vnet.ibm.com>
> Cc: kexec@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  kernel/kexec_file.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index f1a0e4e3fb5c..b1a25d97d5e2 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -904,7 +904,8 @@ static int kexec_purgatory_setup_sechdrs(struct purgatory_info *pi,
>  		if (sechdrs[i].sh_flags & SHF_EXECINSTR &&
>  		    pi->ehdr->e_entry >= sechdrs[i].sh_addr &&
>  		    pi->ehdr->e_entry < (sechdrs[i].sh_addr
> -					 + sechdrs[i].sh_size)) {
> +					 + sechdrs[i].sh_size) &&
> +		    kbuf->image->start != pi->ehdr->e_shnum) {

Shouldn't this be: kbuf->image->start == pi->ehdr->e_shnum) {

?

As you want to only do this update when it's not equal to the initial value.
If this did work, then you may want to make sure that was the initial value.

Also, please add a comment about why you are doing this check.

Thanks!

-- Steve

>  			kbuf->image->start -= sechdrs[i].sh_addr;
>  			kbuf->image->start += kbuf->mem + offset;
>  		}
> 
> ---
> base-commit: 17214b70a159c6547df9ae204a6275d983146f6b
> change-id: 20230321-kexec_clang16-4510c23d129c
> 
> Best regards,
> -- 
> Ricardo Ribalda <ribalda@chromium.org>
Ricardo Ribalda March 22, 2023, 2:42 p.m. UTC | #2 
Hi Steven

On Wed, 22 Mar 2023 at 15:34, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Tue, Mar 21, 2023 at 12:49:08PM +0100, Ricardo Ribalda wrote:
> > Clang16 links the purgatory text in two sections:
> >
> >   [ 1] .text             PROGBITS         0000000000000000  00000040
> >        00000000000011a1  0000000000000000  AX       0     0     16
> >   [ 2] .rela.text        RELA             0000000000000000  00003498
> >        0000000000000648  0000000000000018   I      24     1     8
> >   ...
> >   [17] .text.hot.        PROGBITS         0000000000000000  00003220
> >        000000000000020b  0000000000000000  AX       0     0     1
> >   [18] .rela.text.hot.   RELA             0000000000000000  00004428
> >        0000000000000078  0000000000000018   I      24    17     8
> >
> > And both of them have their range [sh_addr ... sh_addr+sh_size] on the
> > area pointed by `e_entry`.
> >
> > This causes that image->start is calculated twice, once for .text and
> > another time for .text.hot. The second calculation leaves image->start
> > in a random location.
> >
> > Because of this, the system crashes inmediatly after:
> >
> > kexec_core: Starting new kernel
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > To: Eric Biederman <ebiederm@xmission.com>
> > Cc: Philipp Rudo <prudo@linux.vnet.ibm.com>
> > Cc: kexec@lists.infradead.org
> > Cc: linux-kernel@vger.kernel.org
> > ---
> >  kernel/kexec_file.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > index f1a0e4e3fb5c..b1a25d97d5e2 100644
> > --- a/kernel/kexec_file.c
> > +++ b/kernel/kexec_file.c
> > @@ -904,7 +904,8 @@ static int kexec_purgatory_setup_sechdrs(struct purgatory_info *pi,
> >               if (sechdrs[i].sh_flags & SHF_EXECINSTR &&
> >                   pi->ehdr->e_entry >= sechdrs[i].sh_addr &&
> >                   pi->ehdr->e_entry < (sechdrs[i].sh_addr
> > -                                      + sechdrs[i].sh_size)) {
> > +                                      + sechdrs[i].sh_size) &&
> > +                 kbuf->image->start != pi->ehdr->e_shnum) {
>
> Shouldn't this be: kbuf->image->start == pi->ehdr->e_shnum) {

You are absolutely right, I screwed up when I ported the code from my
test tree to the tree that I use for upstreaming.
Instead of removing all the printks I wrote code.

:S

Will resend the patch

>
> ?
>
> As you want to only do this update when it's not equal to the initial value.
> If this did work, then you may want to make sure that was the initial value.
>
> Also, please add a comment about why you are doing this check.
>
> Thanks!
>
> -- Steve
>
> >                       kbuf->image->start -= sechdrs[i].sh_addr;
> >                       kbuf->image->start += kbuf->mem + offset;
> >               }
> >
> > ---
> > base-commit: 17214b70a159c6547df9ae204a6275d983146f6b
> > change-id: 20230321-kexec_clang16-4510c23d129c
> >
> > Best regards,
> > --
> > Ricardo Ribalda <ribalda@chromium.org>
Baoquan He March 22, 2023, 2:52 p.m. UTC | #3 
On 03/22/23 at 03:42pm, Ricardo Ribalda wrote:
> Hi Steven
> 
> On Wed, 22 Mar 2023 at 15:34, Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > On Tue, Mar 21, 2023 at 12:49:08PM +0100, Ricardo Ribalda wrote:
> > > Clang16 links the purgatory text in two sections:
> > >
> > >   [ 1] .text             PROGBITS         0000000000000000  00000040
> > >        00000000000011a1  0000000000000000  AX       0     0     16
> > >   [ 2] .rela.text        RELA             0000000000000000  00003498
> > >        0000000000000648  0000000000000018   I      24     1     8
> > >   ...
> > >   [17] .text.hot.        PROGBITS         0000000000000000  00003220
> > >        000000000000020b  0000000000000000  AX       0     0     1
> > >   [18] .rela.text.hot.   RELA             0000000000000000  00004428
> > >        0000000000000078  0000000000000018   I      24    17     8
> > >
> > > And both of them have their range [sh_addr ... sh_addr+sh_size] on the
> > > area pointed by `e_entry`.
> > >
> > > This causes that image->start is calculated twice, once for .text and
> > > another time for .text.hot. The second calculation leaves image->start
> > > in a random location.
> > >
> > > Because of this, the system crashes inmediatly after:
> > >
> > > kexec_core: Starting new kernel
> > >
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > To: Eric Biederman <ebiederm@xmission.com>
> > > Cc: Philipp Rudo <prudo@linux.vnet.ibm.com>
> > > Cc: kexec@lists.infradead.org
> > > Cc: linux-kernel@vger.kernel.org
> > > ---
> > >  kernel/kexec_file.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > > index f1a0e4e3fb5c..b1a25d97d5e2 100644
> > > --- a/kernel/kexec_file.c
> > > +++ b/kernel/kexec_file.c
> > > @@ -904,7 +904,8 @@ static int kexec_purgatory_setup_sechdrs(struct purgatory_info *pi,
> > >               if (sechdrs[i].sh_flags & SHF_EXECINSTR &&
> > >                   pi->ehdr->e_entry >= sechdrs[i].sh_addr &&
> > >                   pi->ehdr->e_entry < (sechdrs[i].sh_addr
> > > -                                      + sechdrs[i].sh_size)) {
> > > +                                      + sechdrs[i].sh_size) &&
> > > +                 kbuf->image->start != pi->ehdr->e_shnum) {
> >
> > Shouldn't this be: kbuf->image->start == pi->ehdr->e_shnum) {
> 
> You are absolutely right, I screwed up when I ported the code from my
> test tree to the tree that I use for upstreaming.
> Instead of removing all the printks I wrote code.
> 
> :S
> 
> Will resend the patch

When you resne patch, please fix Philipp's mail adress as
'Philipp Rudo <prudo@redhat.com>' if he should know this. He has joined
Redhat.

Thanks
Baoquan
Steven Rostedt March 22, 2023, 3 p.m. UTC | #4 
On Wed, 22 Mar 2023 22:52:04 +0800
Baoquan He <bhe@redhat.com> wrote:

> When you resne patch, please fix Philipp's mail adress as
> 'Philipp Rudo <prudo@redhat.com>' if he should know this. He has joined
> Redhat.

But I thought redhat *was* IBM? ;-)

-- Steve
Ross Zwisler March 22, 2023, 8:13 p.m. UTC | #5 
On Wed, Mar 22, 2023 at 10:34:46AM -0400, Steven Rostedt wrote:
> On Tue, Mar 21, 2023 at 12:49:08PM +0100, Ricardo Ribalda wrote:
> > Clang16 links the purgatory text in two sections:
> > 
> >   [ 1] .text             PROGBITS         0000000000000000  00000040
> >        00000000000011a1  0000000000000000  AX       0     0     16
> >   [ 2] .rela.text        RELA             0000000000000000  00003498
> >        0000000000000648  0000000000000018   I      24     1     8
> >   ...
> >   [17] .text.hot.        PROGBITS         0000000000000000  00003220
> >        000000000000020b  0000000000000000  AX       0     0     1
> >   [18] .rela.text.hot.   RELA             0000000000000000  00004428
> >        0000000000000078  0000000000000018   I      24    17     8
> > 
> > And both of them have their range [sh_addr ... sh_addr+sh_size] on the
> > area pointed by `e_entry`.
> > 
> > This causes that image->start is calculated twice, once for .text and
> > another time for .text.hot. The second calculation leaves image->start
> > in a random location.
> > 
> > Because of this, the system crashes inmediatly after:
> > 
> > kexec_core: Starting new kernel
> > 
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > To: Eric Biederman <ebiederm@xmission.com>
> > Cc: Philipp Rudo <prudo@linux.vnet.ibm.com>
> > Cc: kexec@lists.infradead.org
> > Cc: linux-kernel@vger.kernel.org
> > ---
> >  kernel/kexec_file.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > index f1a0e4e3fb5c..b1a25d97d5e2 100644
> > --- a/kernel/kexec_file.c
> > +++ b/kernel/kexec_file.c
> > @@ -904,7 +904,8 @@ static int kexec_purgatory_setup_sechdrs(struct purgatory_info *pi,
> >  		if (sechdrs[i].sh_flags & SHF_EXECINSTR &&
> >  		    pi->ehdr->e_entry >= sechdrs[i].sh_addr &&
> >  		    pi->ehdr->e_entry < (sechdrs[i].sh_addr
> > -					 + sechdrs[i].sh_size)) {
> > +					 + sechdrs[i].sh_size) &&
> > +		    kbuf->image->start != pi->ehdr->e_shnum) {

I think we should also be comparing against the initial value (set ~20 lines
above) of pi->ehdr->e_entry, not pi->ehdr->e_shnum.

This patch works correctly for me:

diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index f7a4fd4d243f4..967826a42cdd7 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -913,7 +913,8 @@ static int kexec_purgatory_setup_sechdrs(struct purgatory_info *pi,
                if (sechdrs[i].sh_flags & SHF_EXECINSTR &&
                    pi->ehdr->e_entry >= sechdrs[i].sh_addr &&
                    pi->ehdr->e_entry < (sechdrs[i].sh_addr
-                                        + sechdrs[i].sh_size)) {
+                                        + sechdrs[i].sh_size) &&
+                   kbuf->image->start == pi->ehdr->e_entry) {
                        kbuf->image->start -= sechdrs[i].sh_addr;
                        kbuf->image->start += kbuf->mem + offset;
                }

Great find.  With those 2 quick changes, you can add:

Reviewed-by: Ross Zwisler <zwisler@google.com>

> Shouldn't this be: kbuf->image->start == pi->ehdr->e_shnum) {
> 
> ?
> 
> As you want to only do this update when it's not equal to the initial value.
> If this did work, then you may want to make sure that was the initial value.
> 
> Also, please add a comment about why you are doing this check.
> 
> Thanks!
> 
> -- Steve
> 
> >  			kbuf->image->start -= sechdrs[i].sh_addr;
> >  			kbuf->image->start += kbuf->mem + offset;
> >  		}
> > 
> > ---
> > base-commit: 17214b70a159c6547df9ae204a6275d983146f6b
> > change-id: 20230321-kexec_clang16-4510c23d129c
> > 
> > Best regards,
> > -- 
> > Ricardo Ribalda <ribalda@chromium.org>
Baoquan He March 23, 2023, 12:17 a.m. UTC | #6 
On 03/22/23 at 11:00am, Steven Rostedt wrote:
> On Wed, 22 Mar 2023 22:52:04 +0800
> Baoquan He <bhe@redhat.com> wrote:
> 
> > When you resne patch, please fix Philipp's mail adress as
> > 'Philipp Rudo <prudo@redhat.com>' if he should know this. He has joined
> > Redhat.
> 
> But I thought redhat *was* IBM? ;-)

That's interesting questioin, maybe yes on capital operation, but no
from company operation and management, in an engineer's eyes.
diff mbox series

Patch

diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index f1a0e4e3fb5c..b1a25d97d5e2 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -904,7 +904,8 @@  static int kexec_purgatory_setup_sechdrs(struct purgatory_info *pi,
 		if (sechdrs[i].sh_flags & SHF_EXECINSTR &&
 		    pi->ehdr->e_entry >= sechdrs[i].sh_addr &&
 		    pi->ehdr->e_entry < (sechdrs[i].sh_addr
-					 + sechdrs[i].sh_size)) {
+					 + sechdrs[i].sh_size) &&
+		    kbuf->image->start != pi->ehdr->e_shnum) {
 			kbuf->image->start -= sechdrs[i].sh_addr;
 			kbuf->image->start += kbuf->mem + offset;
 		}