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).
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. 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.
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.
>
> 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
