[PATCH v3 2/8] kexec: generalize and rename get_kernel_stext_sym()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux