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 <[email protected]>
---
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 <[email protected]>
Cc: Philipp Rudo <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
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) {
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 <[email protected]>
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 <[email protected]>
> To: Eric Biederman <[email protected]>
> Cc: Philipp Rudo <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> 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 <[email protected]>
Hi Steven
On Wed, 22 Mar 2023 at 15:34, Steven Rostedt <[email protected]> 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 <[email protected]>
> > To: Eric Biederman <[email protected]>
> > Cc: Philipp Rudo <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > ---
> > 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 <[email protected]>
--
Ricardo Ribalda
On 03/22/23 at 03:42pm, Ricardo Ribalda wrote:
> Hi Steven
>
> On Wed, 22 Mar 2023 at 15:34, Steven Rostedt <[email protected]> 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 <[email protected]>
> > > To: Eric Biederman <[email protected]>
> > > Cc: Philipp Rudo <[email protected]>
> > > Cc: [email protected]
> > > Cc: [email protected]
> > > ---
> > > 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 <[email protected]>' if he should know this. He has joined
Redhat.
Thanks
Baoquan
On Wed, 22 Mar 2023 22:52:04 +0800
Baoquan He <[email protected]> wrote:
> When you resne patch, please fix Philipp's mail adress as
> 'Philipp Rudo <[email protected]>' if he should know this. He has joined
> Redhat.
But I thought redhat *was* IBM? ;-)
-- Steve
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 <[email protected]>
> > To: Eric Biederman <[email protected]>
> > Cc: Philipp Rudo <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > ---
> > 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 <[email protected]>
> 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 <[email protected]>
On 03/22/23 at 11:00am, Steven Rostedt wrote:
> On Wed, 22 Mar 2023 22:52:04 +0800
> Baoquan He <[email protected]> wrote:
>
> > When you resne patch, please fix Philipp's mail adress as
> > 'Philipp Rudo <[email protected]>' 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.