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.
>
> Signed-off-by: Dave Young <dyoung at 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.
> +
> + 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.
Also, I wonder if read(fd, str_tmp, sizeof str_tmp) might make
things a bit easier over all.
Finally, I have a personal preference for "a + i" over "&a[i]".
Though I'm not going to get excited about it.
> +
> + 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.
> +
> + 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. */
>
