Re: Kexec regression in next-20160906

Re: Kexec regression in next-20160906

[Thiago Jung Bauermann]

Hello Tony,

Am Dienstag, 06 September 2016, 15:09:37 schrieb Tony Lindgren:
> Looks like commit 5c01cdd2d4bc ("kexec_file: allow skipping checksum
> calculation for some segments") makes next-20160916 stop working for
> me at least on ARM.
> 
> I now get "kexec_load failed: Invalid argument error" on loading the
> new kernel to memory with kexec -l.
> 
> Reverting the following two commits makes things work for me again:
> 
> d2bf993afdf1 ("kexec_file: add mechanism to update kexec segments")
> 5c01cdd2d4bc ("kexec_file: allow skipping checksum calculation for
> some segments")

Thanks for reporting the problem and finding the commit that caused it.
The only thing in commit 5c01cdd2d4bc which can affect kexec_load is the 
fact that struct kexec_segment has a new member.

This is probably breaking the ABI on ARM, then. I verified that kexec_load 
kept working on ppc64le with a kexec binary compiled with the original 
struct kexec_segment definition, but apparently I got lucky.

I'll prepare a new version of the kexec buffer hand-over series which 
doesn't touch struct kexec_segment.

-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

Re: Kexec regression in next-20160906

[Russell King - ARM Linux]

On Tue, Sep 06, 2016 at 08:33:20PM -0300, Thiago Jung Bauermann wrote:
> Thanks for reporting the problem and finding the commit that caused it.
> The only thing in commit 5c01cdd2d4bc which can affect kexec_load is the 
> fact that struct kexec_segment has a new member.
> 
> This is probably breaking the ABI on ARM, then. I verified that kexec_load 
> kept working on ppc64le with a kexec binary compiled with the original 
> struct kexec_segment definition, but apparently I got lucky.

That _will_ definitely break the ABI on ARM, and pretty much all
32-bit architectures.  It's a silly thing to do, and I'm really
surprised that it passed through review without being spotted.

The reason you "got lucky" with ppc64le is that there was probably
padding in the structure, and you happened to place your new member
in that padding, so the structure size didn't change.

For 32-bit architectures, there will be no padding - both the pointers
and size_t members will be 32-bit, and will be naturally aligned, and
hence there will be no padding.  Any addition to the structure will
change the size of the structure.

Any change to a UAPI header needs to be carefully considered and
questioned as it is always a potential userspace breakage - and in
the kernel, we're supposed to be doing our up-most to avoid
breaking userspace.

It's not like it was in the old days when we didn't have the UAPI
seperate - today, we can find these things by looking at the patch
diffstat and seeing whether any file in "uapi" is touched.  That
should be the trigger for a really in-depth review of the change.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: Kexec regression in next-20160906

[Thiago Jung Bauermann]

Am Mittwoch, 07 September 2016, 09:08:07 schrieb Russell King - ARM Linux:
> Any change to a UAPI header needs to be carefully considered and
> questioned as it is always a potential userspace breakage - and in
> the kernel, we're supposed to be doing our up-most to avoid
> breaking userspace.
> 
> It's not like it was in the old days when we didn't have the UAPI
> seperate - today, we can find these things by looking at the patch
> diffstat and seeing whether any file in "uapi" is touched.  That
> should be the trigger for a really in-depth review of the change.

No UAPI header is touched by this patch series. That is because there are 
two definitions of struct kexec_segment, one in include/linux/kexec.h and 
the other one in include/uapi/linux/kexec.h. My patch changed the former.
I was unaware of the second definition in the latter.

-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center