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

[Jay Lan <jlan@sgi.com>]

Simon Horman wrote:
> 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?

Hi Simon,

Yes. This is the patch that i would like merged.

Thanks,
jay

> 
> 
> 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++;
>  	}
>  }
> 


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec