Re: [patch] ia64: Order of operations bug in PT_LOAD segment reader

Jay Lan
2008-10-21T16:52:27Z

Simon Horman wrote:

Hi Simon,

Just got back from vacation. Sorry for late response.

> This bug was discovered by Jay Lan and he also proposed this fix, however
> thee is some discussion about what if any related changes should be made at
> the same time.
>
> The bug comes about because the break statment was never executed because
> the if clause would bever be true because the if clause will never be true
> because & has higher precedence than !=.
>
> My position on this is that with the if logic fixed, as per this patch, the
> break statment and the rest of the while() loop makes sense and should work
> as intended.
>
> As I understand it, Jay's position is that the code should be simplified,
> after all it never worked as intended.
>
> There is a related kernel bug that lead Jay to discover this problem.
> The kernel bug has been resolved by Tony Luck and was
> included in Linus's tree between 2.6.27-rc8 and 2.6.27-rc9 as
> "[IA64] Put the space for cpu0 per-cpu area into .data section".
>
> Now that the kernel bug is out of the way, I am providing this patch to
> continue discussion on what to do on the kexec-tools side of things. I do
> not intend to apply this patch until there is some conclusion in the
> discussion between Jay and myself.

I think this patch is not right for two reasons:
1) The if-statement below has never proved the correctness of
its intent because the 'break' statement never got executed
due to a logic error.
if (loaded_segments[loaded_segments_num].end ! (phdr->p_paddr & ~(ELF_PAGE_SIZE-1)))
break;
2) With your patch in my testing, the kdump kernel boot hung
even earlier in a PAL_CALL that was not returned to the kernel.
I understand that my test case was based on a kernel without
Tony's latest fix, but that was the only situation we can
see the if-statement becomes true. I do not know any other
way to make a memory gap happen. However, when it happens,
your patch only makes kdump kenrel boot hang earlier.

I still root for my patch because the kdump kernel would boot
correctly even if a memory gap indeed happened. ;) However,
if you do not feel comfortable with my patch, i think the best
alternative is to take out the if-statement above completely.

Regards,
jay

>
> Cc: Jay Lan <jlan@sgi.com>
> Signed-off-by: Simon Horman <horms@verge.net.au>
>
> Index: kexec-tools/kexec/arch/ia64/crashdump-ia64.c
> =================================> --- kexec-tools.orig/kexec/arch/ia64/crashdump-ia64.c 2008-10-08 17:31:42.000000000 +1100
> +++ kexec-tools/kexec/arch/ia64/crashdump-ia64.c 2008-10-08 17:32:08.000000000 +1100
> @@ -91,7 +91,7 @@ static void add_loaded_segments_info(str
> if (phdr->p_type != PT_LOAD)
> break;
> if (loaded_segments[loaded_segments_num].end !> - phdr->p_paddr & ~(ELF_PAGE_SIZE-1))
> + (phdr->p_paddr & ~(ELF_PAGE_SIZE-1)))
> break;
> loaded_segments[loaded_segments_num].end +> (phdr->p_memsz + ELF_PAGE_SIZE - 1) &
>