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

Simon Horman
2008-10-22T23:25:21Z

On Tue, Oct 21, 2008 at 09:52:27AM -0700, Jay Lan wrote:
> 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.

Point taken, just to clarify, this is the patch you would like merged?

From: Jay Lan <jlan@sgi.com>

IA64: better calculate PT_LOAD segment size

This patch combines consecutive PL_LOAD segments into one.
The end address of the last PL_LOAD segment, calculated by
adding p_memsz to p_paddr & rounded up to ELF_PAGE_SIZE,
will be the end address of this loaded_segments[] entry.

This patch fixes the kdump kernel MCA problem caused by under-
allocation of memory and a "kdump broken on ALtix 350" problem
reported by Bernhard Walle.

Simon, this patch replaces my previous patch I submitted on the
underallocation issue.

Signed-off-by: Jay Lan <jlan@sgi.com>

---
kexec/arch/ia64/crashdump-ia64.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)

Index: kexec-tools/kexec/arch/ia64/crashdump-ia64.c
=================================--- kexec-tools.orig/kexec/arch/ia64/crashdump-ia64.c 2008-09-19 14:33:07.593344017 -0700
+++ kexec-tools/kexec/arch/ia64/crashdump-ia64.c 2008-09-19 17:39:03.732928237 -0700
@@ -86,19 +86,20 @@ static void add_loaded_segments_info(str
loaded_segments[loaded_segments_num].end loaded_segments[loaded_segments_num].start;

+ /* Consolidate consecutive PL_LOAD segments into one.
+ * The end addr of the last PL_LOAD segment, calculated by
+ * adding p_memsz to p_paddr & rounded up to ELF_PAGE_SIZE,
+ * will be the end address of this loaded_segments entry.
+ */
while (i < ehdr->e_phnum) {
phdr = &ehdr->e_phdr[i];
if (phdr->p_type != PT_LOAD)
break;
- if (loaded_segments[loaded_segments_num].end !- phdr->p_paddr & ~(ELF_PAGE_SIZE-1))
- break;
- loaded_segments[loaded_segments_num].end +- (phdr->p_memsz + ELF_PAGE_SIZE - 1) &
- ~(ELF_PAGE_SIZE - 1);
+ loaded_segments[loaded_segments_num].end + (phdr->p_paddr + phdr->p_memsz +
+ ELF_PAGE_SIZE - 1) & ~(ELF_PAGE_SIZE - 1);
i++;
}
-
loaded_segments_num++;
}
}