[Crash-utility] [PATCH v2] arm64: fix kernel memory map handling for kaslr-enabled kernel

[AKASHI Takahiro]

On Fri, May 27, 2016 at 09:46:10AM +0530, Pratyush Anand wrote:
> On 27/05/2016:12:18:56 PM, AKASHI Takahiro wrote:
> > On Thu, May 26, 2016 at 01:27:08PM +0530, Pratyush Anand wrote:
> > > On 26/05/2016:04:04:08 PM, AKASHI Takahiro wrote:
> > > > Pratyush,
> > > > 
> > > > Just for debug purpose.
> > > > Please add the following line to *your* arch_crash_save_vmcoreinfo():
> > > > > vmcoreinfo_append_str("NUMBER(kimage_voffset)=%llx\n", kimage_voffset);
> > > 
> > > Thanks for the pointer.
> > > 
> > > I did had VMCOREINFO_NUMBER(kimage_voffset) in arch_crash_save_vmcoreinfo().
> > > 
> > > https://github.com/pratyushanand/linux/commit/7011e478aae3e568cc8dca15b6c128fe728416f7#diff-cdf29c3b9471b9d813afc107dd154acdR291
> > > 
> > > But, I noticed that in crash code you have "ms->kimage_voffset = htol(string,
> > > QUIET, NULL);". So, the change you have suggested will work.
> > 
> > I know that. It is intentional.
> > 
> > > However, I think its preferable to use VMCOREINFO_NUMBER() macro instead.
> > > makedumpfile is able to calculate kimage_voffset correctly with that without any
> > > issue.
> > 
> > I think that VMCOREINFO_NUMBER() is, at least originally, intended
> > to be used for a small *unsigned* integer.
> 
> Not sure..When I see all the places where it has been used, it seems it can
> accommodate *long* variable.

I don't know makedumpfile side, but as far as I look at the kernel code
in crash_save_vmcoreinfo_init(), all the values seem to be small positive
numbers.

> See, read_vmcoreinfo_long() of makedumpfile [1] , which is being used to read
> "NUMBER" string.  This function uses strtol(). So I think, we must have similar
> implementation if we intend to use VMCOREINFO_NUMBER().
> 
> [1] https://sourceforge.net/p/makedumpfile/code/ci/master/tree/makedumpfile.c#l2400
> 
> > 
> > I also know that PHYS_OFFSET can now be nagative in v4.6 on arm64.
> > Yet I'm thinking of adding "0x" as a prefix to VMCOREINOF string.
> 
> I think, we should not change existing VMCOREINFO_xxxx() macros of kernel,
> because it will break the applications which rely on it's current structure.

I'm not saying that we change VMCOREINFO_NUMBER() macro.

> ATM, I also do not see any urgency of defining a new macro which can have
> implementation like vmcoreinfo_append_str("NUMBER(X)=%#llx\n", X), because
> exiting macros is able to take care of the needs.
> 
> > 
> > -Takahiro AKASHI
> > 
> > > I will suggest to take following modification in crash code:
> > > 
> > > diff --git a/arm64.c b/arm64.c
> > > index 6b97093..9397d6d 100644
> > > --- a/arm64.c
> > > +++ b/arm64.c
> > > @@ -122,7 +122,7 @@ arm64_init(int when)
> > >                 ms = machdep->machspec;
> > >                 if (!ms->kimage_voffset &&
> > >                     (string = pc->read_vmcoreinfo("NUMBER(kimage_voffset)"))) {
> > > -                       ms->kimage_voffset = htol(string, QUIET, NULL);
> > > +                       ms->kimage_voffset = dtol(string, QUIET, NULL);
> > >                         free(string);
> > >                 }
> > > 
> > > diff --git a/tools.c b/tools.c
> > > index 384bebd..1383e43 100644
> > > --- a/tools.c
> > > +++ b/tools.c
> > > @@ -877,7 +877,7 @@ dtol(char *s, int flags, int *errptr)
> > >                  s++;
> > > 
> > >          for (j = 0; s[j] != '\0'; j++)
> > > -                if ((s[j] < '0' || s[j] > '9'))
> > > +                if ( (s[j] != '-') && ((s[j] < '0' || s[j] > '9')))
> > >                          break ;
> > > 
> > >         if (s[j] != '\0') {
> > > 
> 
> ~Pratyush

-- 
Thanks,
-Takahiro AKASHI