Hi Dave,
On Fri, Oct 7, 2016 at 9:48 AM, Dave Young <dyoung at redhat.com> wrote:
>> > /* Traverse through the Elf headers and find the region where
>> > * _stext symbol is located in. That's where kernel is mapped */
>> > - stext_sym = get_kernel_stext_sym();
>> > + stext_sym = get_kernel_sym("stext");
>>
>>
>> I think this should be get_kernel_sym("_stext");
>>
>> Apart from that as Simon already mentioned, due to commit
>> 9f62cbd ("kexec/arch/i386: Add support for KASLR memory randomization")
>> this patch does not apply cleanly.
>
> Pratyush, I had a cleanup patch below, but I did not get time to test it
> so it is not ready to send out.
>
> The basic thought is to move the page_offset_base code to
> get_kernel_page_offset(), there is also another issue is for old kernel
> or kernel without kaslr built-in there will be an unnecessary error
> message but I have not get any idea how to fix it.
>
Are you talking about following error message:
"Cannot get kernel %s symbol address\n"
May be it can be changes as dbgprintf() rather than fprintf(stderr)
> Would you like to take below cleanup patch along with your next version?
I think, this patch series is for ARM64 support along with any generic
modifications needed by arm64 patches. Below modification is specific
to x86. So, better to send them separately.
> It is also fine to leave it as a future improvement to me.
OK.
>
> Thanks
> Dave
>
> ---
> kexec/arch/i386/crashdump-x86.c | 158 ++++++++++++++++++++--------------------
> 1 file changed, 82 insertions(+), 76 deletions(-)
>
> --- kexec-tools.orig/kexec/arch/i386/crashdump-x86.c
> +++ kexec-tools/kexec/arch/i386/crashdump-x86.c
> @@ -54,10 +54,59 @@
>
> extern struct arch_options_t arch_options;
>
> +/* Retrieve kernel symbol virtual address from /proc/kallsyms */
> +static unsigned long long get_kernel_sym(const char *symbol)
> +{
> + const char *kallsyms = "/proc/kallsyms";
> + char sym[128];
> + char line[128];
> + FILE *fp;
> + unsigned long long vaddr;
> + char type;
> +
> + fp = fopen(kallsyms, "r");
> + if (!fp) {
> + fprintf(stderr, "Cannot open %s\n", kallsyms);
> + return 0;
> + }
> +
> + while(fgets(line, sizeof(line), fp) != NULL) {
> + if (sscanf(line, "%Lx %c %s", &vaddr, &type, sym) != 3)
> + continue;
> + if (strcmp(sym, symbol) == 0) {
> + dbgprintf("kernel symbol %s vaddr = %16llx\n", symbol, vaddr);
> + return vaddr;
> + }
> + }
> +
> + fprintf(stderr, "Cannot get kernel %s symbol address\n", symbol);
> +
> + return 0;
> +}
> +
> static int get_kernel_page_offset(struct kexec_info *UNUSED(info),
> - struct crash_elf_info *elf_info)
> + struct crash_elf_info *elf_info,
> + struct mem_ehdr *ehdr)
> {
> int kv;
> + struct mem_phdr *phdr, *end_phdr;
> + const unsigned long long pud_mask = ~((1 << 30) - 1);
> + unsigned long long vaddr, lowest_vaddr = 0;
> +
> + end_phdr = &ehdr->e_phdr[ehdr->e_phnum];
> + /* Search for the real PAGE_OFFSET when KASLR memory randomization
> + * is enabled */
> + if (get_kernel_sym("page_offset_base") != 0) {
> + for(phdr = ehdr->e_phdr; phdr != end_phdr; phdr++) {
> + if (phdr->p_type == PT_LOAD) {
> + vaddr = phdr->p_vaddr & pud_mask;
> + if (lowest_vaddr == 0 || lowest_vaddr > vaddr)
Although, it is just the copy of original code from one function to
another, but I think logic can be improved a bit here. Initialize
lowest_vaddr with ULONG_MAX and we can get rid of one check of
(lowest_vaddr == 0).
> + lowest_vaddr = vaddr;
> + }
> + }
> + if (lowest_vaddr != 0)
And then here it should be (lowest_vaddr != ULONG_MAX)
> + elf_info->page_offset = lowest_vaddr;
> + }
>
> if (elf_info->machine == EM_X86_64) {
> kv = kernel_version();
> @@ -102,35 +151,6 @@ static int get_kernel_paddr(struct kexec
> return -1;
> }
>
> -/* Retrieve kernel symbol virtual address from /proc/kallsyms */
> -static unsigned long long get_kernel_sym(const char *symbol)
> -{
> - const char *kallsyms = "/proc/kallsyms";
> - char sym[128];
> - char line[128];
> - FILE *fp;
> - unsigned long long vaddr;
> - char type;
> -
> - fp = fopen(kallsyms, "r");
> - if (!fp) {
> - fprintf(stderr, "Cannot open %s\n", kallsyms);
> - return 0;
> - }
> -
> - while(fgets(line, sizeof(line), fp) != NULL) {
> - if (sscanf(line, "%Lx %c %s", &vaddr, &type, sym) != 3)
> - continue;
> - if (strcmp(sym, symbol) == 0) {
> - dbgprintf("kernel symbol %s vaddr = %16llx\n", symbol, vaddr);
> - return vaddr;
> - }
> - }
> -
> - fprintf(stderr, "Cannot get kernel %s symbol address\n", symbol);
> - return 0;
> -}
> -
> /* Retrieve info regarding virtual address kernel has been compiled for and
> * size of the kernel from /proc/kcore. Current /proc/kcore parsing from
> * from kexec-tools fails because of malformed elf notes. A kernel patch has
> @@ -139,19 +159,12 @@ static unsigned long long get_kernel_sym
> * we should get rid of backward compatible code. */
>
> static int get_kernel_vaddr_and_size(struct kexec_info *UNUSED(info),
> - struct crash_elf_info *elf_info)
> + struct crash_elf_info *elf_info,
> + struct mem_ehdr *ehdr)
> {
> - int result;
> - const char kcore[] = "/proc/kcore";
> - char *buf;
> - struct mem_ehdr ehdr;
> struct mem_phdr *phdr, *end_phdr;
> int align;
> - off_t size;
> - uint32_t elf_flags = 0;
> uint64_t stext_sym;
> - const unsigned long long pud_mask = ~((1 << 30) - 1);
> - unsigned long long vaddr, lowest_vaddr = 0;
>
> if (elf_info->machine != EM_X86_64)
> return 0;
> @@ -160,45 +173,13 @@ static int get_kernel_vaddr_and_size(str
> return 0;
>
> align = getpagesize();
> - buf = slurp_file_len(kcore, KCORE_ELF_HEADERS_SIZE, &size);
> - if (!buf) {
> - fprintf(stderr, "Cannot read %s: %s\n", kcore, strerror(errno));
> - return -1;
> - }
>
> - /* Don't perform checks to make sure stated phdrs and shdrs are
> - * actually present in the core file. It is not practical
> - * to read the GB size file into a user space buffer, Given the
> - * fact that we don't use any info from that.
> - */
> - elf_flags |= ELF_SKIP_FILESZ_CHECK;
> - result = build_elf_core_info(buf, size, &ehdr, elf_flags);
> - if (result < 0) {
> - /* Perhaps KCORE_ELF_HEADERS_SIZE is too small? */
> - fprintf(stderr, "ELF core (kcore) parse failed\n");
> - return -1;
> - }
> -
> - end_phdr = &ehdr.e_phdr[ehdr.e_phnum];
> -
> - /* Search for the real PAGE_OFFSET when KASLR memory randomization
> - * is enabled */
> - if (get_kernel_sym("page_offset_base") != 0) {
> - for(phdr = ehdr.e_phdr; phdr != end_phdr; phdr++) {
> - if (phdr->p_type == PT_LOAD) {
> - vaddr = phdr->p_vaddr & pud_mask;
> - if (lowest_vaddr == 0 || lowest_vaddr > vaddr)
> - lowest_vaddr = vaddr;
> - }
> - }
> - if (lowest_vaddr != 0)
> - elf_info->page_offset = lowest_vaddr;
> - }
> + end_phdr = &ehdr->e_phdr[ehdr->e_phnum];
>
> /* Traverse through the Elf headers and find the region where
> * _stext symbol is located in. That's where kernel is mapped */
> stext_sym = get_kernel_sym("_stext");
> - for(phdr = ehdr.e_phdr; stext_sym && phdr != end_phdr; phdr++) {
> + for(phdr = ehdr->e_phdr; stext_sym && phdr != end_phdr; phdr++) {
> if (phdr->p_type == PT_LOAD) {
> unsigned long long saddr = phdr->p_vaddr;
> unsigned long long eaddr = phdr->p_vaddr + phdr->p_memsz;
> @@ -223,7 +204,7 @@ static int get_kernel_vaddr_and_size(str
> * /proc/kallsyms, Traverse through the Elf headers again and
> * find the region where kernel is mapped using hard-coded
> * kernel mapping boundries */
> - for(phdr = ehdr.e_phdr; phdr != end_phdr; phdr++) {
> + for(phdr = ehdr->e_phdr; phdr != end_phdr; phdr++) {
> if (phdr->p_type == PT_LOAD) {
> unsigned long long saddr = phdr->p_vaddr;
> unsigned long long eaddr = phdr->p_vaddr + phdr->p_memsz;
> @@ -887,6 +868,12 @@ int load_crashdump_segments(struct kexec
> struct memory_range *mem_range, *memmap_p;
> struct crash_elf_info elf_info;
> unsigned kexec_arch;
> + char *buf;
> + off_t size;
> + struct mem_ehdr ehdr;
> + uint32_t elf_flags = 0;
> + const char kcore[] = "/proc/kcore";
> + int result;
>
> memset(&elf_info, 0x0, sizeof(elf_info));
>
> @@ -943,13 +930,32 @@ int load_crashdump_segments(struct kexec
> elf_info.class = ELFCLASS64;
> }
>
> - if (get_kernel_page_offset(info, &elf_info))
> + buf = slurp_file_len(kcore, KCORE_ELF_HEADERS_SIZE, &size);
> + if (!buf) {
> + fprintf(stderr, "Cannot read %s: %s\n", kcore, strerror(errno));
> + return -1;
> + }
> +
> + /* Don't perform checks to make sure stated phdrs and shdrs are
> + * actually present in the core file. It is not practical
> + * to read the GB size file into a user space buffer, Given the
> + * fact that we don't use any info from that.
> + */
> + elf_flags |= ELF_SKIP_FILESZ_CHECK;
> + result = build_elf_core_info(buf, size, &ehdr, elf_flags);
> + if (result < 0) {
> + /* Perhaps KCORE_ELF_HEADERS_SIZE is too small? */
> + fprintf(stderr, "ELF core (kcore) parse failed\n");
> + return -1;
> + }
> +
> + if (get_kernel_page_offset(info, &elf_info, &ehdr))
> return -1;
>
> if (get_kernel_paddr(info, &elf_info))
> return -1;
>
> - if (get_kernel_vaddr_and_size(info, &elf_info))
> + if (get_kernel_vaddr_and_size(info, &elf_info, &ehdr))
> return -1;
>
> /* Memory regions which panic kernel can safely use to boot into */
~Pratyush
