[PATCH v2 0/2] s390/kexec_file: improve error handling and messages

[PATCH v2 0/2] s390/kexec_file: improve error handling and messages

[Philipp Rudo]

Hi everybody,

here is v2 of the patch I sent yesterday.

Thanks
Philipp

v2:
	- EINVAL -> ENOEXEC
	- print error message when encountering an unknown relocation
	- new patch to print error messages for all error cases in the function

Philipp Rudo (2):
  s390/kexec_file: print some more error messages
  s390/kexec_file: fix error handling when applying relocations

 arch/s390/kernel/machine_kexec_file.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

-- 
2.31.1

[PATCH v2 1/2] s390/kexec_file: print some more error messages

[Philipp Rudo]

Be kind and give some more information on what went wrong.

Signed-off-by: Philipp Rudo <prudo@redhat.com>
---
 arch/s390/kernel/machine_kexec_file.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/arch/s390/kernel/machine_kexec_file.c b/arch/s390/kernel/machine_kexec_file.c
index 9975ad200d74..b53fa670e289 100644
--- a/arch/s390/kernel/machine_kexec_file.c
+++ b/arch/s390/kernel/machine_kexec_file.c
@@ -7,6 +7,8 @@
  * Author(s): Philipp Rudo <prudo@linux.vnet.ibm.com>
  */
 
+#define pr_fmt(fmt)	"kexec: " fmt
+
 #include <linux/elf.h>
 #include <linux/errno.h>
 #include <linux/kexec.h>
@@ -304,15 +306,22 @@ int arch_kexec_apply_relocations_add(struct purgatory_info *pi,
 		sym = (void *)pi->ehdr + symtab->sh_offset;
 		sym += ELF64_R_SYM(relas[i].r_info);
 
-		if (sym->st_shndx == SHN_UNDEF)
+		if (sym->st_shndx == SHN_UNDEF) {
+			pr_err("Undefined symbol\n");
 			return -ENOEXEC;
+		}
 
-		if (sym->st_shndx == SHN_COMMON)
+		if (sym->st_shndx == SHN_COMMON) {
+			pr_err("symbol in common section\n");
 			return -ENOEXEC;
+		}
 
 		if (sym->st_shndx >= pi->ehdr->e_shnum &&
-		    sym->st_shndx != SHN_ABS)
+		    sym->st_shndx != SHN_ABS) {
+			pr_err("Invalid section %d for symbol\n",
+			       sym->st_shndx);
 			return -ENOEXEC;
+		}
 
 		loc = pi->purgatory_buf;
 		loc += section->sh_offset;
-- 
2.31.1

Re: [PATCH v2 1/2] s390/kexec_file: print some more error messages

[Heiko Carstens]

Hi Philipp,

On Tue, Dec 07, 2021 at 01:57:48PM +0100, Philipp Rudo wrote:
> Be kind and give some more information on what went wrong.
> 
> Signed-off-by: Philipp Rudo <prudo@redhat.com>
...
> @@ -304,15 +306,22 @@ int arch_kexec_apply_relocations_add(struct purgatory_info *pi,
>  		sym = (void *)pi->ehdr + symtab->sh_offset;
>  		sym += ELF64_R_SYM(relas[i].r_info);
>  
> -		if (sym->st_shndx == SHN_UNDEF)
> +		if (sym->st_shndx == SHN_UNDEF) {
> +			pr_err("Undefined symbol\n");
>  			return -ENOEXEC;
> +		}
>  
> -		if (sym->st_shndx == SHN_COMMON)
> +		if (sym->st_shndx == SHN_COMMON) {
> +			pr_err("symbol in common section\n");
>  			return -ENOEXEC;
> +		}
>  
>  		if (sym->st_shndx >= pi->ehdr->e_shnum &&
> -		    sym->st_shndx != SHN_ABS)
> +		    sym->st_shndx != SHN_ABS) {
> +			pr_err("Invalid section %d for symbol\n",
> +			       sym->st_shndx);
>  			return -ENOEXEC;

So, if you add the additional error messages here, then I'd really
like to see also the name of the symbol which is causing
problems. Just like it is done on x86.
Sorry for nitpicking :)

Re: [PATCH v2 1/2] s390/kexec_file: print some more error messages

[Philipp Rudo]

Hi Heiko,

On Tue, 7 Dec 2021 16:29:55 +0100
Heiko Carstens <hca@linux.ibm.com> wrote:

> Hi Philipp,
> 
> On Tue, Dec 07, 2021 at 01:57:48PM +0100, Philipp Rudo wrote:
> > Be kind and give some more information on what went wrong.
> > 
> > Signed-off-by: Philipp Rudo <prudo@redhat.com>  
> ...
> > @@ -304,15 +306,22 @@ int arch_kexec_apply_relocations_add(struct purgatory_info *pi,
> >  		sym = (void *)pi->ehdr + symtab->sh_offset;
> >  		sym += ELF64_R_SYM(relas[i].r_info);
> >  
> > -		if (sym->st_shndx == SHN_UNDEF)
> > +		if (sym->st_shndx == SHN_UNDEF) {
> > +			pr_err("Undefined symbol\n");
> >  			return -ENOEXEC;
> > +		}
> >  
> > -		if (sym->st_shndx == SHN_COMMON)
> > +		if (sym->st_shndx == SHN_COMMON) {
> > +			pr_err("symbol in common section\n");
> >  			return -ENOEXEC;
> > +		}
> >  
> >  		if (sym->st_shndx >= pi->ehdr->e_shnum &&
> > -		    sym->st_shndx != SHN_ABS)
> > +		    sym->st_shndx != SHN_ABS) {
> > +			pr_err("Invalid section %d for symbol\n",
> > +			       sym->st_shndx);
> >  			return -ENOEXEC;  
> 
> So, if you add the additional error messages here, then I'd really
> like to see also the name of the symbol which is causing
> problems. Just like it is done on x86.
> Sorry for nitpicking :)

I actually dropped the name on purpose. At least for my work flow
knowing which check failed is more important as that already allows
me to search for, e.g. all undefined symbols as each of them can cause
trouble. Which symbol exactly triggered the check isn't that important.
In addition, the code to get a symbol name is rather ugly. At least
when you compare it to its usefulness.

But when you insist...

Philipp

P.S. To avoid an other round for that patch. Do you also want the two
     pr_debugs?

Re: [PATCH v2 1/2] s390/kexec_file: print some more error messages

[Heiko Carstens]

> > > -		if (sym->st_shndx == SHN_COMMON)
> > > +		if (sym->st_shndx == SHN_COMMON) {
> > > +			pr_err("symbol in common section\n");
> > >  			return -ENOEXEC;
> > > +		}
> > >  
> > >  		if (sym->st_shndx >= pi->ehdr->e_shnum &&
> > > -		    sym->st_shndx != SHN_ABS)
> > > +		    sym->st_shndx != SHN_ABS) {
> > > +			pr_err("Invalid section %d for symbol\n",
> > > +			       sym->st_shndx);
> > >  			return -ENOEXEC;  
> > 
> > So, if you add the additional error messages here, then I'd really
> > like to see also the name of the symbol which is causing
> > problems. Just like it is done on x86.
> > Sorry for nitpicking :)
> 
> I actually dropped the name on purpose. At least for my work flow
> knowing which check failed is more important as that already allows
> me to search for, e.g. all undefined symbols as each of them can cause
> trouble. Which symbol exactly triggered the check isn't that important.
> In addition, the code to get a symbol name is rather ugly. At least
> when you compare it to its usefulness.
> 
> But when you insist...

Yes, please.

> P.S. To avoid an other round for that patch. Do you also want the two
>      pr_debugs?

No, I don't think they are needed.

Thank you!

[PATCH v2 2/2] s390/kexec_file: fix error handling when applying relocations

[Philipp Rudo]

arch_kexec_apply_relocations_add currently ignores all errors returned
by arch_kexec_do_relocs. This means that every unknown relocation is
silently skipped causing unpredictable behavior while the relocated code
runs. Fix this by checking for errors and fail kexec_file_load if an
unknown relocation type is encountered.

The problem was found after gcc changed its behavior and used
R_390_PLT32DBL relocations for brasl instruction and relied on ld to
resolve the relocations in the final link in case direct calls are
possible. As the purgatory code is only linked partially (option -r)
ld didn't resolve the relocations leaving them for arch_kexec_do_relocs.
But arch_kexec_do_relocs doesn't know how to handle R_390_PLT32DBL
relocations so they were silently skipped. This ultimately caused an
endless loop in the purgatory as the brasl instructions kept branching
to itself.

Fixes: 71406883fd35 ("s390/kexec_file: Add kexec_file_load system call")
Reported-by: Tao Liu <ltao@redhat.com>
Signed-off-by: Philipp Rudo <prudo@redhat.com>
---
 arch/s390/kernel/machine_kexec_file.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/s390/kernel/machine_kexec_file.c b/arch/s390/kernel/machine_kexec_file.c
index b53fa670e289..5e1da2e67cb7 100644
--- a/arch/s390/kernel/machine_kexec_file.c
+++ b/arch/s390/kernel/machine_kexec_file.c
@@ -294,6 +294,7 @@ int arch_kexec_apply_relocations_add(struct purgatory_info *pi,
 {
 	Elf_Rela *relas;
 	int i, r_type;
+	int ret;
 
 	relas = (void *)pi->ehdr + relsec->sh_offset;
 
@@ -335,7 +336,11 @@ int arch_kexec_apply_relocations_add(struct purgatory_info *pi,
 		addr = section->sh_addr + relas[i].r_offset;
 
 		r_type = ELF64_R_TYPE(relas[i].r_info);
-		arch_kexec_do_relocs(r_type, loc, val, addr);
+		ret = arch_kexec_do_relocs(r_type, loc, val, addr);
+		if (ret) {
+			pr_err("Unknown rela relocation: %d\n", r_type);
+			return -ENOEXEC;
+		}
 	}
 	return 0;
 }
-- 
2.31.1