[PATCH][RFC] vmcore: Remove saved_max_pfn check

[PATCH][RFC] vmcore: Remove saved_max_pfn check

[Magnus Damm]

From: Magnus Damm <damm@igel.co.jp>

This patch removes the saved_max_pfn check from the /proc/vmcore
function read_from_oldmem(). No need to verify, we should be able
to just trust that "elfcorehdr=" is correctly passed to the crash
kernel on the kernel command line like we do with other parameters.

The read_from_oldmem() function in fs/proc/vmcore.c is quite similar
to read_from_oldmem() in drivers/char/mem.c, but only in the latter
it makes sense to use saved_max_pfn. For oldmem it is used to determine
when to stop reading. For vmcore we already have the elf header info
pointing out the physical memory regions, no need to pass the end-of-
old-memory twice.

Removing the saved_max_pfn check from vmcore makes it possible for
architectures to skip oldmem but still support crash dump through
vmcore - without the need for the old saved_max_pfn cruft.

Architectures that want to play safe can do the saved_max_pfn check
in copy_oldmem_page(). Not sure why anyone would want to do that,
but that's even safer than today - the saved_max_pfn check in vmcore
removed by this patch only checks the first page.

Signed-off-by: Magnus Damm <damm@igel.co.jp>
---

 fs/proc/vmcore.c |    2 --
 1 file changed, 2 deletions(-)

--- 0001/fs/proc/vmcore.c
+++ work/fs/proc/vmcore.c	2008-09-08 11:33:10.000000000 +0900
@@ -50,8 +50,6 @@ static ssize_t read_from_oldmem(char *bu
 
 	offset = (unsigned long)(*ppos % PAGE_SIZE);
 	pfn = (unsigned long)(*ppos / PAGE_SIZE);
-	if (pfn > saved_max_pfn)
-		return -EINVAL;
 
 	do {
 		if (count > (PAGE_SIZE - offset))

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

Re: [PATCH][RFC] vmcore: Remove saved_max_pfn check

[Vivek Goyal]

On Mon, Sep 08, 2008 at 12:21:32PM +0900, Magnus Damm wrote:
> From: Magnus Damm <damm@igel.co.jp>
> 
> This patch removes the saved_max_pfn check from the /proc/vmcore
> function read_from_oldmem(). No need to verify, we should be able
> to just trust that "elfcorehdr=" is correctly passed to the crash
> kernel on the kernel command line like we do with other parameters.
> 
> The read_from_oldmem() function in fs/proc/vmcore.c is quite similar
> to read_from_oldmem() in drivers/char/mem.c, but only in the latter
> it makes sense to use saved_max_pfn. For oldmem it is used to determine
> when to stop reading. For vmcore we already have the elf header info
> pointing out the physical memory regions, no need to pass the end-of-
> old-memory twice.
> 
> Removing the saved_max_pfn check from vmcore makes it possible for
> architectures to skip oldmem but still support crash dump through
> vmcore - without the need for the old saved_max_pfn cruft.
> 
> Architectures that want to play safe can do the saved_max_pfn check
> in copy_oldmem_page(). Not sure why anyone would want to do that,
> but that's even safer than today - the saved_max_pfn check in vmcore
> removed by this patch only checks the first page.
> 

Hi Magnus,

Though I don't feel very strongly for saved_max_pfn check in vmcore.c,
but at the same time I don't understand what are you gaining by removing
this check. Any way we are not getting rid of this symbol altogether
because /dev/oldmem needs it.

Is it sh arch for which you want to disable /dev/oldmem and only enable
/proc/vmcore and hence want to get rid of saved_max_pfn?

Though I agree that we should elfcorehdrs but at the same time it does not
hard doing additional check (We are anyway carrying saved_max_pfn for
/dev/oldmem). We can always extend current code to check for end page also
to make sure we are not reading beyond saved_max_pfn.

How much code is it to set value of saved_max_pfn in sh that you want to
completely get rid of it. My feeling is that it should be just few lines.

So though I don't feel strongly for saved_max_pfn check  in vmcore.c, at
the same time I don't see you are gaining anything significant by removing
it. We can just introduce saved_max_pfn in sh also.

Thanks
Vivek
 


> Signed-off-by: Magnus Damm <damm@igel.co.jp>
> ---
> 
>  fs/proc/vmcore.c |    2 --
>  1 file changed, 2 deletions(-)
> 
> --- 0001/fs/proc/vmcore.c
> +++ work/fs/proc/vmcore.c	2008-09-08 11:33:10.000000000 +0900
> @@ -50,8 +50,6 @@ static ssize_t read_from_oldmem(char *bu
>  
>  	offset = (unsigned long)(*ppos % PAGE_SIZE);
>  	pfn = (unsigned long)(*ppos / PAGE_SIZE);
> -	if (pfn > saved_max_pfn)
> -		return -EINVAL;
>  
>  	do {
>  		if (count > (PAGE_SIZE - offset))

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

Re: [PATCH][RFC] vmcore: Remove saved_max_pfn check

[Magnus Damm]

On Mon, Sep 8, 2008 at 10:04 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Mon, Sep 08, 2008 at 12:21:32PM +0900, Magnus Damm wrote:
>> From: Magnus Damm <damm@igel.co.jp>
>>
>> This patch removes the saved_max_pfn check from the /proc/vmcore
>> function read_from_oldmem(). No need to verify, we should be able
>> to just trust that "elfcorehdr=" is correctly passed to the crash
>> kernel on the kernel command line like we do with other parameters.
>>
>> The read_from_oldmem() function in fs/proc/vmcore.c is quite similar
>> to read_from_oldmem() in drivers/char/mem.c, but only in the latter
>> it makes sense to use saved_max_pfn. For oldmem it is used to determine
>> when to stop reading. For vmcore we already have the elf header info
>> pointing out the physical memory regions, no need to pass the end-of-
>> old-memory twice.
>>
>> Removing the saved_max_pfn check from vmcore makes it possible for
>> architectures to skip oldmem but still support crash dump through
>> vmcore - without the need for the old saved_max_pfn cruft.
>>
>> Architectures that want to play safe can do the saved_max_pfn check
>> in copy_oldmem_page(). Not sure why anyone would want to do that,
>> but that's even safer than today - the saved_max_pfn check in vmcore
>> removed by this patch only checks the first page.

Hi Vivek,

> Though I don't feel very strongly for saved_max_pfn check in vmcore.c,
> but at the same time I don't understand what are you gaining by removing
> this check. Any way we are not getting rid of this symbol altogether
> because /dev/oldmem needs it.

Let me try to be a bit more clear. =)

> Is it sh arch for which you want to disable /dev/oldmem and only enable
> /proc/vmcore and hence want to get rid of saved_max_pfn?

Yes, exactly. I could do as powerpc and just pass the saved_max_pfn on
the command line together with the elfcorehdr pointer, but why? For
vmcore we only need the elfcorehdr pointer.

> Though I agree that we should elfcorehdrs but at the same time it does not
> hard doing additional check (We are anyway carrying saved_max_pfn for
> /dev/oldmem). We can always extend current code to check for end page also
> to make sure we are not reading beyond saved_max_pfn.

Maybe it doesn't harm, but it doesn't do any good either. Relying on
saved_max_pfn just requires more architecture specific code. The
existing vmcore.c code can of course be fixed up to handle the end
page, but what is the exact point with using saved_max_pfn in
vmcore.c?

I understand it is needed for oldmem, so I'm not saying that we should
remove the symbol all together.

> How much code is it to set value of saved_max_pfn in sh that you want to
> completely get rid of it. My feeling is that it should be just few lines.

You are correct. We just need to add a kernel command line parameter
and change kexec-tools to pass along that value. Like powerpc does
today. I did of course do just that in my first iteration of crash
support for SuperH and it works just fine. But why should we pass more
information than we actually need?

So it's not a matter of coding effort. It's more about passing just
the information that is needed to the secondary kernel. And if we want
to use vmcore but not oldmem, saved_max_pfn isn't needed. Unless I'm
mistaken that is. =)

> So though I don't feel strongly for saved_max_pfn check  in vmcore.c, at
> the same time I don't see you are gaining anything significant by removing
> it. We can just introduce saved_max_pfn in sh also.

Yep, we could introduce that on SuperH, but pruning code that is not
really needed is a step in the right direction IMO.

Thanks for your comments!

/ magnus

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

Re: [PATCH][RFC] vmcore: Remove saved_max_pfn check

[Vivek Goyal]

On Wed, Sep 10, 2008 at 03:40:45PM +0900, Magnus Damm wrote:
> On Mon, Sep 8, 2008 at 10:04 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Mon, Sep 08, 2008 at 12:21:32PM +0900, Magnus Damm wrote:
> >> From: Magnus Damm <damm@igel.co.jp>
> >>
> >> This patch removes the saved_max_pfn check from the /proc/vmcore
> >> function read_from_oldmem(). No need to verify, we should be able
> >> to just trust that "elfcorehdr=" is correctly passed to the crash
> >> kernel on the kernel command line like we do with other parameters.
> >>
> >> The read_from_oldmem() function in fs/proc/vmcore.c is quite similar
> >> to read_from_oldmem() in drivers/char/mem.c, but only in the latter
> >> it makes sense to use saved_max_pfn. For oldmem it is used to determine
> >> when to stop reading. For vmcore we already have the elf header info
> >> pointing out the physical memory regions, no need to pass the end-of-
> >> old-memory twice.
> >>
> >> Removing the saved_max_pfn check from vmcore makes it possible for
> >> architectures to skip oldmem but still support crash dump through
> >> vmcore - without the need for the old saved_max_pfn cruft.
> >>
> >> Architectures that want to play safe can do the saved_max_pfn check
> >> in copy_oldmem_page(). Not sure why anyone would want to do that,
> >> but that's even safer than today - the saved_max_pfn check in vmcore
> >> removed by this patch only checks the first page.
> 
> Hi Vivek,
> 
> > Though I don't feel very strongly for saved_max_pfn check in vmcore.c,
> > but at the same time I don't understand what are you gaining by removing
> > this check. Any way we are not getting rid of this symbol altogether
> > because /dev/oldmem needs it.
> 
> Let me try to be a bit more clear. =)
> 
> > Is it sh arch for which you want to disable /dev/oldmem and only enable
> > /proc/vmcore and hence want to get rid of saved_max_pfn?
> 
> Yes, exactly. I could do as powerpc and just pass the saved_max_pfn on
> the command line together with the elfcorehdr pointer, but why? For
> vmcore we only need the elfcorehdr pointer.
> 
> > Though I agree that we should elfcorehdrs but at the same time it does not
> > hard doing additional check (We are anyway carrying saved_max_pfn for
> > /dev/oldmem). We can always extend current code to check for end page also
> > to make sure we are not reading beyond saved_max_pfn.
> 
> Maybe it doesn't harm, but it doesn't do any good either. Relying on
> saved_max_pfn just requires more architecture specific code. The
> existing vmcore.c code can of course be fixed up to handle the end
> page, but what is the exact point with using saved_max_pfn in
> vmcore.c?
> 
> I understand it is needed for oldmem, so I'm not saying that we should
> remove the symbol all together.
> 
> > How much code is it to set value of saved_max_pfn in sh that you want to
> > completely get rid of it. My feeling is that it should be just few lines.
> 
> You are correct. We just need to add a kernel command line parameter
> and change kexec-tools to pass along that value. Like powerpc does
> today. I did of course do just that in my first iteration of crash
> support for SuperH and it works just fine. But why should we pass more
> information than we actually need?
> 
> So it's not a matter of coding effort. It's more about passing just
> the information that is needed to the secondary kernel. And if we want
> to use vmcore but not oldmem, saved_max_pfn isn't needed. Unless I'm
> mistaken that is. =)
> 
> > So though I don't feel strongly for saved_max_pfn check  in vmcore.c, at
> > the same time I don't see you are gaining anything significant by removing
> > it. We can just introduce saved_max_pfn in sh also.
> 
> Yep, we could introduce that on SuperH, but pruning code that is not
> really needed is a step in the right direction IMO.
> 

Hi Magnus,

Ok, I can't think of a strong reason why should we retain saved_max_pfn
check in /proc/vmcore and it looks like elfcorehdr= should be equally 
reliable.

I have no objections to the patch.

Acked-by: Vivek Goyal <vgoyal@redhat.com>

Thanks
Vivek

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

Re: [PATCH][RFC] vmcore: Remove saved_max_pfn check

[Simon Horman]

On Wed, Sep 10, 2008 at 09:17:10AM -0400, Vivek Goyal wrote:
> On Wed, Sep 10, 2008 at 03:40:45PM +0900, Magnus Damm wrote:
> > On Mon, Sep 8, 2008 at 10:04 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > > On Mon, Sep 08, 2008 at 12:21:32PM +0900, Magnus Damm wrote:
> > >> From: Magnus Damm <damm@igel.co.jp>
> > >>
> > >> This patch removes the saved_max_pfn check from the /proc/vmcore
> > >> function read_from_oldmem(). No need to verify, we should be able
> > >> to just trust that "elfcorehdr=" is correctly passed to the crash
> > >> kernel on the kernel command line like we do with other parameters.
> > >>
> > >> The read_from_oldmem() function in fs/proc/vmcore.c is quite similar
> > >> to read_from_oldmem() in drivers/char/mem.c, but only in the latter
> > >> it makes sense to use saved_max_pfn. For oldmem it is used to determine
> > >> when to stop reading. For vmcore we already have the elf header info
> > >> pointing out the physical memory regions, no need to pass the end-of-
> > >> old-memory twice.
> > >>
> > >> Removing the saved_max_pfn check from vmcore makes it possible for
> > >> architectures to skip oldmem but still support crash dump through
> > >> vmcore - without the need for the old saved_max_pfn cruft.
> > >>
> > >> Architectures that want to play safe can do the saved_max_pfn check
> > >> in copy_oldmem_page(). Not sure why anyone would want to do that,
> > >> but that's even safer than today - the saved_max_pfn check in vmcore
> > >> removed by this patch only checks the first page.
> > 
> > Hi Vivek,
> > 
> > > Though I don't feel very strongly for saved_max_pfn check in vmcore.c,
> > > but at the same time I don't understand what are you gaining by removing
> > > this check. Any way we are not getting rid of this symbol altogether
> > > because /dev/oldmem needs it.
> > 
> > Let me try to be a bit more clear. =)
> > 
> > > Is it sh arch for which you want to disable /dev/oldmem and only enable
> > > /proc/vmcore and hence want to get rid of saved_max_pfn?
> > 
> > Yes, exactly. I could do as powerpc and just pass the saved_max_pfn on
> > the command line together with the elfcorehdr pointer, but why? For
> > vmcore we only need the elfcorehdr pointer.
> > 
> > > Though I agree that we should elfcorehdrs but at the same time it does not
> > > hard doing additional check (We are anyway carrying saved_max_pfn for
> > > /dev/oldmem). We can always extend current code to check for end page also
> > > to make sure we are not reading beyond saved_max_pfn.
> > 
> > Maybe it doesn't harm, but it doesn't do any good either. Relying on
> > saved_max_pfn just requires more architecture specific code. The
> > existing vmcore.c code can of course be fixed up to handle the end
> > page, but what is the exact point with using saved_max_pfn in
> > vmcore.c?
> > 
> > I understand it is needed for oldmem, so I'm not saying that we should
> > remove the symbol all together.
> > 
> > > How much code is it to set value of saved_max_pfn in sh that you want to
> > > completely get rid of it. My feeling is that it should be just few lines.
> > 
> > You are correct. We just need to add a kernel command line parameter
> > and change kexec-tools to pass along that value. Like powerpc does
> > today. I did of course do just that in my first iteration of crash
> > support for SuperH and it works just fine. But why should we pass more
> > information than we actually need?
> > 
> > So it's not a matter of coding effort. It's more about passing just
> > the information that is needed to the secondary kernel. And if we want
> > to use vmcore but not oldmem, saved_max_pfn isn't needed. Unless I'm
> > mistaken that is. =)
> > 
> > > So though I don't feel strongly for saved_max_pfn check  in vmcore.c, at
> > > the same time I don't see you are gaining anything significant by removing
> > > it. We can just introduce saved_max_pfn in sh also.
> > 
> > Yep, we could introduce that on SuperH, but pruning code that is not
> > really needed is a step in the right direction IMO.
> > 
> 
> Hi Magnus,
> 
> Ok, I can't think of a strong reason why should we retain saved_max_pfn
> check in /proc/vmcore and it looks like elfcorehdr= should be equally 
> reliable.
> 
> I have no objections to the patch.
> 
> Acked-by: Vivek Goyal <vgoyal@redhat.com>

Acked-by: Simon Horman <horms@verge.net.au>

BTW, does anyone actually use /proc/oldmem ?


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