Re: [Crash-utility] [PATCH] arm64: add tag_ignore machdep option

Hi Vinayak, On Fri, 26 Mar 2021 at 11:56, vinayak menon <vinayakm.list(a)gmail.com&gt; wrote: > > Hi Bhupesh, > > On Thu, Mar 25, 2021 at 11:15 PM Bhupesh Sharma > <bhupesh.sharma(a)linaro.org&gt; wrote: > > > > Hi Vinayak, > > > > Thanks for the patch. > > Some queries below: > > > > On Thu, 25 Mar 2021 at 20:08, Vinayak Menon <vinayakm.list(a)gmail.com&gt; 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(a)redhat.com&gt; > 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(a)arm.com) > > > > > > > Signed-off-by: Vinayak Menon <vinayakm.list(a)gmail.com&gt; > > > --- > > > 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(a)redhat.com > > > https://listman.redhat.com/mailman/listinfo/crash-utility > > > > > > > -- > > Crash-utility mailing list > > Crash-utility(a)redhat.com > > https://listman.redhat.com/mailman/listinfo/crash-utility > > > > -- > Crash-utility mailing list > Crash-utility(a)redhat.com > https://listman.redhat.com/mailman/listinfo/crash-utility > -- Crash-utility mailing list Crash-utility(a)redhat.com https://listman.redhat.com/mailman/listinfo/crash-utility