[Crash-utility] [PATCH] arm64: add tag_ignore machdep option
vinayak menon
vinayakm.list at gmail.com
Tue Mar 30 13:42:55 UTC 2021
On Fri, Mar 26, 2021 at 5:45 PM Bhupesh Sharma
<bhupesh.sharma at linaro.org> wrote:
>
> Hi Vinayak,
>
> On Fri, 26 Mar 2021 at 11:56, vinayak menon <vinayakm.list at gmail.com> wrote:
> >
> > Hi Bhupesh,
> >
> > On Thu, Mar 25, 2021 at 11:15 PM Bhupesh Sharma
> > <bhupesh.sharma at linaro.org> wrote:
> > >
> > > Hi Vinayak,
> > >
> > > Thanks for the patch.
> > > Some queries below:
> > >
> > > On Thu, 25 Mar 2021 at 20:08, Vinayak Menon <vinayakm.list at gmail.com> 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."
>
Ya I see that in doc, though the kernel uses a wider mask in
ptrauth_user_pac_mask.
And with MTE enabled, the bits used for PAC changes. In the v2 of
patch I have changes
the machdep optin to "tasg_mask" through which the user can set the
relevant mask.
> > 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.
>
Actually today I got a dump with KASAN with MTE enabled and crash utility is
broken with that. I am sending out a seperate patch to fix that too. In that
I have made the mask generic, so that if PAC and MTE are both enabled, user
can pass a mask that combines both.
> > Let me know.
> >
> > commit 41d61189d60e0fdd6509b96dc8160795263f3229
> > Author: Dave Anderson <anderson at redhat.com>
> > 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 at arm.com)
> >
> >
> > >
> > > > Signed-off-by: Vinayak Menon <vinayakm.list at gmail.com>
> > > > ---
> > > > 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 at redhat.com
> > > > https://listman.redhat.com/mailman/listinfo/crash-utility
> > > >
> > >
> > > --
> > > Crash-utility mailing list
> > > Crash-utility at redhat.com
> > > https://listman.redhat.com/mailman/listinfo/crash-utility
> > >
> >
> > --
> > Crash-utility mailing list
> > Crash-utility at redhat.com
> > https://listman.redhat.com/mailman/listinfo/crash-utility
> >
>
> --
> Crash-utility mailing list
> Crash-utility at redhat.com
> https://listman.redhat.com/mailman/listinfo/crash-utility
>
More information about the Crash-utility
mailing list