Re: [PATCH]kdump: pass noefi and acpi_rsdp= to 2nd kernel

[Dave Young <dyoung@redhat.com>]

On 09/05/2012 03:45 PM, Simon Horman wrote:

> On Wed, Sep 05, 2012 at 01:44:45PM +0800, Dave Young wrote:
>>
>> In case efi booting, kdump need kernel parameter noefi and acpi_rsdp=
>> to disable efi reinit and retrieve the acpi root table physical address.
>>
>> Add a function cmdline_add_efi to get the address from /sys/firmware/efi/systab
>> If there's no such file or read fail the function will just do nothing.
>>
>> Tested efi boot Fedora 17 on thinkpad T420.
> 
> Hi Dave,
> 
> conceptually I am fine with this patch.
> However, I do have a few nits in relation to the string handling
> as explained below.


Thanks for review.

> 
>>
>> Signed-off-by: Dave Young <dyoung@redhat.com>
>> ---
>>  kexec/arch/i386/crashdump-x86.c |   47 ++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 47 insertions(+)
>>
>> --- kexec-tools.orig/kexec/arch/i386/crashdump-x86.c
>> +++ kexec-tools/kexec/arch/i386/crashdump-x86.c
>> @@ -27,6 +27,7 @@
>>  #include <sys/types.h>
>>  #include <sys/stat.h>
>>  #include <unistd.h>
>> +#include <fcntl.h>
>>  #include "../../kexec.h"
>>  #include "../../kexec-elf.h"
>>  #include "../../kexec-syscall.h"
>> @@ -658,6 +659,51 @@ static int cmdline_add_memmap_acpi(char 
>>  	return 0;
>>  }
>>  
>> +/* Appends 'noefi acpi_rsdp=' commandline for efi boot crash dump */
>> +static void cmdline_add_efi(char *cmdline)
>> +{
>> +	int cmdlen, len, fd, ret, i = 0;
>> +	char str_efi[64], str_tmp[32];
>> +
>> +	memset(str_efi, 0x0, sizeof(str_efi));
>> +	memset(str_tmp, 0x0, sizeof(str_tmp));
>> +	snprintf(str_efi, 18, "%s", " noefi acpi_rsdp=");
> 
> strncpy() might be be easier on the eyes.


Will switch to use strncpy

> 
>> +
>> +	fd = open("/sys/firmware/efi/systab", O_RDONLY);
>> +	if (fd < 0)
>> +		return;
>> +
>> +	while(1) {
>> +		ret = read(fd, &str_tmp[i], 1);
>> +		if (ret <= 0)
>> +			goto out_close;
>> +		if (str_tmp[i] == '\n') {
>> +			str_tmp[i] = '\0';
>> +			break;
>> +		}
>> +		i++;
>> +	}
> 
> It seems to me that the above code may overflow str_tmp.


Oops, will fix

> 
> Also, I wonder if read(fd, str_tmp, sizeof str_tmp) might make
> things a bit easier over all.


This is fine to me as well. Just also need to end with '\n'

As Khalid said in another reply, the ACPI20= might not be the first
line. will resolve that issue as well.

> 
> Finally, I have a personal preference for "a + i" over "&a[i]".
> Though I'm not going to get excited about it.


Both are fine to me, will convert.

> 
>> +
>> +	if (!(strncmp(str_tmp, "ACPI20=", 7)))
>> +		strcat(str_efi, &str_tmp[7]);
>> +	else if (!(strncmp(str_tmp, "ACPI=", 5)))
>> +		strcat(str_efi, &str_tmp[5]);
>> +	else
>> +		goto out_close;
> 
> I wonder if strncat() would be better above, though I don't think
> an overflow can actually occur if str_tmp is null-terminated.


Will do

> 
>> +
>> +	len = strlen(str_efi);
>> +	cmdlen = strlen(cmdline) + len;
>> +	if (cmdlen > (COMMAND_LINE_SIZE - 1))
>> +		die("Command line overflow\n");
>> +	strcat(cmdline, str_efi);
>> +
>> +	dbgprintf("Command line after adding efi\n");
>> +	dbgprintf("%s\n", cmdline);
>> +
>> +out_close:
>> +	close(fd);
>> +}
>> +
>>  static void get_backup_area(unsigned long *start, unsigned long *end)
>>  {
>>  	const char *iomem = proc_iomem();
>> @@ -830,6 +876,7 @@ int load_crashdump_segments(struct kexec
>>  	if (delete_memmap(memmap_p, elfcorehdr, memsz) < 0)
>>  		return -1;
>>  	cmdline_add_memmap(mod_cmdline, memmap_p);
>> +	cmdline_add_efi(mod_cmdline);
>>  	cmdline_add_elfcorehdr(mod_cmdline, elfcorehdr);
>>  
>>  	/* Inform second kernel about the presence of ACPI tables. */
>>
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec



-- 
Thanks
Dave

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