Hi Vinayak,
On Fri, 26 Mar 2021 at 11:56, vinayak menon <vinayakm.list@xxxxxxxxx> wrote:
>
> Hi Bhupesh,
>
> On Thu, Mar 25, 2021 at 11:15 PM Bhupesh Sharma
> <bhupesh.sharma@xxxxxxxxxx> wrote:
> >
> > Hi Vinayak,
> >
> > Thanks for the patch.
> > Some queries below:
> >
> > On Thu, 25 Mar 2021 at 20:08, Vinayak Menon <vinayakm.list@xxxxxxxxx> wrote:
> > >
> > > Raw ramdumps without vmcoreinfo does not work currently
> > > with pointer authentication or memory tagging enabled.
> > > The arm capability array can be queried but that creates
> > > a dependency on the bits identifying the capabilities.
> > > Add a machdep option instead to ignore the tags when any
> > > feature using the tag bits is enabled.
> >
> > Hmm.. I am still trying to understand why we need to ignore the tags.
> > Can you please explain the issue at hand and also share the steps you
> > used to reproduce it.
> >
> > I have an arm64 hardware with memory tagging support available, I can
> > try to reproduce it at my end.
> >
> > I suspect we need to enable reading capabilities instead. Do you
> > foresee any issues with the same?
>
>
> Actually it is just an extension of this patch below. Without the patch below
> some commands are broken. For e.g. "bt" with PAC enabled, due to
> the top bits being used for metadata. The patch below fixes it but depends on
> getting the mask from vmcoreinfo which is not available for raw
> ramdumps (ramdump.c).
Right, I am aware of Dave A's patch, but
> So the mask has to be passed in some manner. One way is to pass mask itself
> as a machdep option, but if we see the mask that the kernel generates in
> ptrauth_user_pac_mask, it is a generic 63-to-vabits_actual mask.
Hmm.. but I remember reading (and confirmed via
'Documentation/arm64/pointer-authentication.rst'), that it's 55 -
va_bits-actual instead, see section 'Basic support
':
"The number of bits that the PAC occupies in a pointer is 55 minus the
virtual address size configured by the kernel. For example, with a
virtual address size of 48, the PAC is 7 bits wide."
> So thought there
> isn't a point in passing a mask value. Another way is dynamic
> detection of feature
> enablement, but it has a problem that I described in commit msg.
> There is a potential problem with MTE enabled kasan builds too, though
> I have not
> tested one. "bt" should be fine with MTE but maybe some commands where the
> slub object addresses are involved for e.g. That was actually the
> reason I named the
> machdep option "tag_ignore" incase if we end up using it for cases
> other than PAC.
Correct, I am wondering if we need to separate out the PAC and MTE
related handling paths here. While from my p-o-v, 'tag_ignore' makes
sense for PAC, I am not sure if it affects MTE handling inside crash.
So, if we don't have actual use cases/failures for the same, maybe we
can avoid the MTE related handling for now. Just my 2 cents.
> Let me know.
>
> commit 41d61189d60e0fdd6509b96dc8160795263f3229
> Author: Dave Anderson <anderson@xxxxxxxxxx>
> Date: Fri Apr 24 13:16:47 2020 -0400
>
> Prepare for the introduction of ARM64 8.3 Pointer Authentication
> as in-kernel feature. The value of CONFIG_ARM64_KERNELPACMASK
> will be exported as a vmcoreinfo entry, and will be used with text
> return addresses on the kernel stack.
> (amit.kachhap@xxxxxxx)
>
>
> >
> > > Signed-off-by: Vinayak Menon <vinayakm.list@xxxxxxxxx>
> > > ---
> > > arm64.c | 26 +++++++++++++++++++-------
> > > crash.8 | 1 +
> > > defs.h | 1 +
> > > help.c | 1 +
> > > 4 files changed, 22 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/arm64.c b/arm64.c
> > > index 37aed07..3ed6795 100644
> > > --- a/arm64.c
> > > +++ b/arm64.c
> > > @@ -745,7 +745,8 @@ arm64_parse_machdep_arg_l(char *argstring, char *param, ulong *value)
> > > char *p;
> > >
> > > len = strlen(param);
> > > - if (!STRNEQ(argstring, param) || (argstring[len] != '='))
> > > + if (!STRNEQ(argstring, param) || (!STRNEQ(argstring, "tag_ignore") &&
> > > + (argstring[len] != '=')))
> > > return FALSE;
> > >
> > > if ((LASTCHAR(argstring) == 'm') ||
> > > @@ -763,6 +764,8 @@ arm64_parse_machdep_arg_l(char *argstring, char *param, ulong *value)
> > > *value = dtol(p, flags, &err);
> > > } else if (STRNEQ(argstring, "vabits_actual")) {
> > > *value = dtol(p, flags, &err);
> > > + } else if (STRNEQ(argstring, "tag_ignore")) {
> > > + *value = 1;
> > > } else if (megabytes) {
> > > *value = dtol(p, flags, &err);
> > > if (!err)
> > > @@ -797,12 +800,6 @@ arm64_parse_cmdline_args(void)
> > > if (!machdep->cmdline_args[index])
> > > break;
> > >
> > > - if (!strstr(machdep->cmdline_args[index], "=")) {
> > > - error(WARNING, "ignoring --machdep option: %x\n",
> > > - machdep->cmdline_args[index]);
> > > - continue;
> > > - }
> > > -
> > > strcpy(buf, machdep->cmdline_args[index]);
> > >
> > > for (p = buf; *p; p++) {
> > > @@ -838,6 +835,11 @@ arm64_parse_cmdline_args(void)
> > > "setting vabits_actual to: %ld\n\n",
> > > machdep->machspec->VA_BITS_ACTUAL);
> > > continue;
> > > + } else if (arm64_parse_machdep_arg_l(arglist[i], "tag_ignore",
> > > + &machdep->machspec->tag_ignore)) {
> > > + error(NOTE,
> > > + "setting tag_ignore\n\n");
> > > + continue;
> > > }
> > >
> > > error(WARNING, "ignoring --machdep option: %s\n",
> > > @@ -4122,6 +4124,9 @@ arm64_swp_offset(ulong pte)
> > > return pte;
> > > }
> > >
> > > +#define __GENMASK_ULL(h, l) \
> > > + (((~0ULL) - (1ULL << (l)) + 1) & \
> > > + (~0ULL >> (64 - 1 - (h))))
> >
> > Please add it to defs.h instead of a .c file. Also since this is
> > picked up from Linux kernel, please add a comment about this being
> > borrowed from the Linux kernel (something like: Borrowed from /*
> > include/linux/bits.h */). Also add the relevant Linux comment which is
> > very useful:
>
>
> Done. Will fix it in the next version.
>
>
> >
> > /*
> > * Create a contiguous bitmask starting at bit position @l and ending at
> > * position @h. For example
> > * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
> > */
> >
> > > static void arm64_calc_KERNELPACMASK(void)
> > > {
> > > ulong value;
> > > @@ -4133,6 +4138,13 @@ static void arm64_calc_KERNELPACMASK(void)
> > > machdep->machspec->CONFIG_ARM64_KERNELPACMASK = value;
> > > if (CRASHDEBUG(1))
> > > fprintf(fp, "CONFIG_ARM64_KERNELPACMASK: %lx\n", value);
> > > + } else if (machdep->machspec->VA_BITS_ACTUAL &&
> > > + machdep->machspec->tag_ignore) {
> > > + machdep->machspec->CONFIG_ARM64_KERNELPACMASK =
> > > + __GENMASK_ULL(63, machdep->machspec->VA_BITS_ACTUAL);
> > > + if (CRASHDEBUG(1))
> > > + fprintf(fp, "CONFIG_ARM64_KERNELPACMASK: %lx\n",
> > > + machdep->machspec->CONFIG_ARM64_KERNELPACMASK);
> >
> > We can make this fprintf common for the if and else-if legs, right?
> > Just take it out and calculate value in both the cases and print it.
>
>
> Fine.
>
>
> >
> > Also why do we set machdep->machspec->CONFIG_ARM64_KERNELPACMASK =
> > __GENMASK_ULL(63, machdep->machspec->VA_BITS_ACTUAL)
> > here, can you add some comments to this le, since the kernel passed a
> > 0 value for KERNELPACMASK in vmcoreinfo.
>
>
> This is actually for ramdumps that do not have vmcoreinfo. Let me know
> if the comments
> above explain.
No, I meant it should be `55 - va_bits_actual` (bit 63 is actually
TTBR_select in a virtual addr, so using it for PAC seems confusing).
See comment above for more details.
Also adding a note from the kernel documentation here as a comment
would help someone reading this code later-on as well.
Thanks,
Bhupesh
> >
> > Thanks,
> > Bhupesh
> >
> > > }
> > > }
> > >
> > > diff --git a/crash.8 b/crash.8
> > > index 5020ce1..91f01fc 100644
> > > --- a/crash.8
> > > +++ b/crash.8
> > > @@ -289,6 +289,7 @@ ARM64:
> > > kimage_voffset=<kimage_voffset-value>
> > > max_physmem_bits=<value>
> > > vabits_actual=<value>
> > > + tag_ignore
> > > X86:
> > > page_offset=<CONFIG_PAGE_OFFSET-value>
> > > .fi
> > > diff --git a/defs.h b/defs.h
> > > index 35b983a..cd3bcf9 100644
> > > --- a/defs.h
> > > +++ b/defs.h
> > > @@ -3331,6 +3331,7 @@ struct machine_specific {
> > > ulong VA_START;
> > > ulong CONFIG_ARM64_KERNELPACMASK;
> > > ulong physvirt_offset;
> > > + ulong tag_ignore;
> > > };
> > >
> > > struct arm64_stackframe {
> > > diff --git a/help.c b/help.c
> > > index 531f50a..d66125b 100644
> > > --- a/help.c
> > > +++ b/help.c
> > > @@ -182,6 +182,7 @@ char *program_usage_info[] = {
> > > " kimage_voffset=<kimage_voffset-value>",
> > > " max_physmem_bits=<value>",
> > > " vabits_actual=<value>",
> > > + " tag_ignore",
> > > " X86:",
> > > " page_offset=<CONFIG_PAGE_OFFSET-value>",
> > > "",
> > > --
> > >
> > > --
> > > Crash-utility mailing list
> > > Crash-utility@xxxxxxxxxx
> > > https://listman.redhat.com/mailman/listinfo/crash-utility
> > >
> >
> > --
> > Crash-utility mailing list
> > Crash-utility@xxxxxxxxxx
> > https://listman.redhat.com/mailman/listinfo/crash-utility
> >
>
> --
> Crash-utility mailing list
> Crash-utility@xxxxxxxxxx
> https://listman.redhat.com/mailman/listinfo/crash-utility
>
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility
