Hi Pingfan,
On Fri, May 7, 2021 at 2:26 PM piliu <piliu@xxxxxxxxxx> wrote:
>
>
>
> On 5/3/21 2:16 PM, vinayak menon wrote:
> > Hi Pingfan
> >
> > On Thu, Apr 29, 2021 at 3:35 PM piliu <piliu@xxxxxxxxxx> wrote:
> [...]
> >
> > Okay I think I got your concern now. Actually when I started
> > implementing the patchset PAC mask was
> > already present in crash utility. But the issue with that was it
> > worked only with vmcoreinfo and not raw ramdumps.
> > So the first patch created a machdep option tasg_mask. When doing that
> > I made it a generic tag_mask and not
> > pac_mask to avoid the need for 2 different machdep options, one later
> > for MTE, essentially doing similar things.
> > So that's the reason why LR/PC modifications are seen, which is for
> > PAC. Do you really think I need to split into
> > 2 different machdep options or we do it later when we find an absolute
> > need for it ? AFAICT it does not break anything
> > and takes care of MTE, PAC, MTE+PAC scenarios.
> >
> Yes, please split the patch as each patch has a dedicated purpose.
>
> Also please limit terms to PAC instead of MTE, since this patch tries to
> resolves PAC issue.
> >>>> Thanks,
> >>>> Pingfan
> >>>>>>
> >>>>>> "
> >>>>>> please wait... (gathering kmem slab cache data)
> >>>>>> crash: invalid kernel virtual address: f0ffff878000201c type:
> >>
> >> This bug should be caused by MTE, i.e. a tagged pointer.
> >>>>>> "kmem_cache objsize/object_size"
> >>>>>> crash: get_active_set: no tasks found?
> >>>>>> please wait... (gathering task table data)
> >>>>>> crash: invalid kernel virtual address: f1ffff87f51e3530 type:
> >>>>>> "xa_node shift"
> >>>>>> "
> >>>>>>
> >>>>>> Make the mask introduced for pointer authentication generic
> >>>>>> and use it in vtop and kvaddr validation.
> >>>>>>
> >>>>>> Signed-off-by: Vinayak Menon <vinayakm.list@xxxxxxxxx>
> >>>>>> ---
> >>>>>> arm64.c | 50 +++++++++++++++++++++++++++++++-------------------
> >>>>>> defs.h | 2 +-
> >>>>>> 2 files changed, 32 insertions(+), 20 deletions(-)
> >>>>>>
> >>>>>> diff --git a/arm64.c b/arm64.c
> >>>>>> index 5b59972..bb41cbb 100644
> >>>>>> --- a/arm64.c
> >>>>>> +++ b/arm64.c
> >>>>>> @@ -85,7 +85,8 @@ static int arm64_get_kvaddr_ranges(struct
> >>>>>> vaddr_range *);
> >>>>>> static void arm64_get_crash_notes(void);
> >>>>>> static void arm64_calc_VA_BITS(void);
> >>>>>> static int arm64_is_uvaddr(ulong, struct task_context *);
> >>>>>> -static void arm64_calc_KERNELPACMASK(void);
> >>>>>> +static int arm64_is_kvaddr(ulong);
> >>>>>> +static void arm64_calc_KERNELTAGMASK(void);
> >>>>>> /*
> >>>>>> @@ -215,7 +216,7 @@ arm64_init(int when)
> >>>>>> machdep->pagemask = ~((ulonglong)machdep->pageoffset);
> >>>>>> arm64_calc_VA_BITS();
> >>>>>> - arm64_calc_KERNELPACMASK();
> >>>>>> + arm64_calc_KERNELTAGMASK();
> >>>>>> ms = machdep->machspec;
> >>>>>> if (ms->VA_BITS_ACTUAL) {
> >>>>>> ms->page_offset = ARM64_PAGE_OFFSET_ACTUAL;
> >>>>>> @@ -228,7 +229,7 @@ arm64_init(int when)
> >>>>>> machdep->kvbase = ARM64_VA_START;
> >>>>>> ms->userspace_top = ARM64_USERSPACE_TOP;
> >>>>>> }
> >>>>>> - machdep->is_kvaddr = generic_is_kvaddr;
> >>>>>> + machdep->is_kvaddr = arm64_is_kvaddr;
> >>>>>> machdep->kvtop = arm64_kvtop;
> >>>>>> if (machdep->flags & NEW_VMEMMAP) {
> >>>>>> struct syment *sp;
> >>>>>> @@ -477,7 +478,7 @@ arm64_init(int when)
> >>>>>> case LOG_ONLY:
> >>>>>> machdep->machspec = &arm64_machine_specific;
> >>>>>> arm64_calc_VA_BITS();
> >>>>>> - arm64_calc_KERNELPACMASK();
> >>>>>> + arm64_calc_KERNELTAGMASK();
> >>>>>> arm64_calc_phys_offset();
> >>>>>> machdep->machspec->page_offset = ARM64_PAGE_OFFSET;
> >>>>>> arm64_calc_physvirt_offset();
> >>>>>> @@ -608,7 +609,7 @@ arm64_dump_machdep_table(ulong arg)
> >>>>>> fprintf(fp, " dis_filter: arm64_dis_filter()\n");
> >>>>>> fprintf(fp, " cmd_mach: arm64_cmd_mach()\n");
> >>>>>> fprintf(fp, " get_smp_cpus: arm64_get_smp_cpus()\n");
> >>>>>> - fprintf(fp, " is_kvaddr: generic_is_kvaddr()\n");
> >>>>>> + fprintf(fp, " is_kvaddr: arm64_is_kvaddr()\n");
> >>>>>> fprintf(fp, " is_uvaddr: arm64_is_uvaddr()\n");
> >>>>>> fprintf(fp, " value_to_symbol:
> >>>>>> generic_machdep_value_to_symbol()\n");
> >>>>>> fprintf(fp, " init_kernel_pgd: arm64_init_kernel_pgd\n");
> >>>>>> @@ -668,9 +669,9 @@ arm64_dump_machdep_table(ulong arg)
> >>>>>> fprintf(fp, "%ld\n", ms->VA_BITS_ACTUAL);
> >>>>>> else
> >>>>>> fprintf(fp, "(unused)\n");
> >>>>>> - fprintf(fp, "CONFIG_ARM64_KERNELPACMASK: ");
> >>>>>> - if (ms->CONFIG_ARM64_KERNELPACMASK)
> >>>>>> - fprintf(fp, "%lx\n", ms->CONFIG_ARM64_KERNELPACMASK);
> >>>>>> + fprintf(fp, "CONFIG_ARM64_KERNELTAGMASK: ");
> >>>>>> + if (ms->CONFIG_ARM64_KERNELTAGMASK)
> >>>>>> + fprintf(fp, "%lx\n", ms->CONFIG_ARM64_KERNELTAGMASK);
> >>>>>> else
> >>>>>> fprintf(fp, "(unused)\n");
> >>>>>> fprintf(fp, " userspace_top: %016lx\n", ms->userspace_top);
> >>>>>> @@ -1208,6 +1209,9 @@ arm64_kvtop(struct task_context *tc, ulong
> >>>>>> kvaddr, physaddr_t *paddr, int verbos
> >>>>>> if (!IS_KVADDR(kvaddr))
> >>>>>> return FALSE;
> >>>>>> + if (kvaddr & (1UL << 63))
> >>>>>> + kvaddr |= machdep->machspec->CONFIG_ARM64_KERNELTAGMASK;
> >>>>>> +
> >>>>>> if (!vt->vmalloc_start) {
> >>>>>> *paddr = VTOP(kvaddr);
> >>>>>> return TRUE;
> >>>>>> @@ -1828,7 +1832,7 @@ arm64_is_kernel_exception_frame(struct bt_info
> >>>>>> *bt, ulong stkptr)
> >>>>>> if (INSTACK(regs->sp, bt) && INSTACK(regs->regs[29], bt) &&
> >>>>>> !(regs->pstate & (0xffffffff00000000ULL | PSR_MODE32_BIT)) &&
> >>>>>> is_kernel_text(regs->pc) &&
> >>>>>> - is_kernel_text(regs->regs[30] |
> >>>>>> ms->CONFIG_ARM64_KERNELPACMASK)) {
> >>>>>> + is_kernel_text(regs->regs[30] |
> >>>>>> ms->CONFIG_ARM64_KERNELTAGMASK)) {
> >>>>>> switch (regs->pstate & PSR_MODE_MASK)
> >>>>>> {
> >>>>>> case PSR_MODE_EL1t:
> >>>>>> @@ -2198,8 +2202,8 @@ arm64_unwind_frame(struct bt_info *bt, struct
> >>>>>> arm64_stackframe *frame)
> >>>>>> frame->sp = fp + 0x10;
> >>>>>> frame->fp = GET_STACK_ULONG(fp);
> >>>>>> frame->pc = GET_STACK_ULONG(fp + 8);
> >>>>>> - if (is_kernel_text(frame->pc | ms->CONFIG_ARM64_KERNELPACMASK))
> >>>>>> - frame->pc |= ms->CONFIG_ARM64_KERNELPACMASK;
> >>>>>> + if (is_kernel_text(frame->pc | ms->CONFIG_ARM64_KERNELTAGMASK))
> >>>>>> + frame->pc |= ms->CONFIG_ARM64_KERNELTAGMASK;
> >>>>>> if ((frame->fp == 0) && (frame->pc == 0))
> >>>>>> return FALSE;
> >>>>>> @@ -2869,8 +2873,8 @@ arm64_print_text_symbols(struct bt_info *bt,
> >>>>>> struct arm64_stackframe *frame, FIL
> >>>>>> for (i = (start - bt->stackbase)/sizeof(ulong); i <
> >>>>>> LONGS_PER_STACK; i++) {
> >>>>>> up = (ulong *)(&bt->stackbuf[i*sizeof(ulong)]);
> >>>>>> val = *up;
> >>>>>> - if (is_kernel_text(val | ms->CONFIG_ARM64_KERNELPACMASK)) {
> >>>>>> - val |= ms->CONFIG_ARM64_KERNELPACMASK;
> >>>>>> + if (is_kernel_text(val | ms->CONFIG_ARM64_KERNELTAGMASK)) {
> >>>>>> + val |= ms->CONFIG_ARM64_KERNELTAGMASK;
> >>>>>> name = closest_symbol(val);
> >>>>>> fprintf(ofp, " %s[%s] %s at %lx",
> >>>>>> bt->flags & BT_ERROR_MASK ?
> >>>>>> @@ -3205,8 +3209,8 @@ arm64_print_exception_frame(struct bt_info *bt,
> >>>>>> ulong pt_regs, int mode, FILE *o
> >>>>>> rows = 4;
> >>>>>> } else {
> >>>>>> LR = regs->regs[30];
> >>>>>> - if (is_kernel_text (LR | ms->CONFIG_ARM64_KERNELPACMASK))
> >>>>>> - LR |= ms->CONFIG_ARM64_KERNELPACMASK;
> >>>>>> + if (is_kernel_text (LR | ms->CONFIG_ARM64_KERNELTAGMASK))
> >>>>>> + LR |= ms->CONFIG_ARM64_KERNELTAGMASK;
> >>
> >> This looks alike to the scenario on page 17 of
> >> https://events.static.linuxfound.org/sites/events/files/slides/slides_23.pdf
> >>
> >> When LR is saved on frame, it should not be MTE tagged, instead, it is
> >> PAed. Because it is not an object pointer, and a tagged pointer is
> >> generated by explicit code, e.g.
> >>
> >> __kasan_slab_alloc()
> >> {
> >> tag = assign_tag(cache, object, false);
> >> tagged_object = set_tag(object, tag);
> >> }
> >>
> >> I do not see any opportunity for PC/LR to handle by similar code.
> >>
> >> So here, I think it should be something like LR |=
> >> ms->CONFIG_ARM64_KERNELTAGMASK.PAC_SUBMASK.
> >>>>>> SP = regs->sp;
> >>>>>> top_reg = 29;
> >>>>>> is_64_bit = TRUE;
> >>>>>> @@ -4102,6 +4106,14 @@ arm64_calc_virtual_memory_ranges(void)
> >>>>>> }
> >>>>>> static int
> >>>>>> +arm64_is_kvaddr(ulong addr)
> >>>>>> +{
> >>>>>> + if (addr & (1UL << 63))
> >>>>>> + addr |= machdep->machspec->CONFIG_ARM64_KERNELTAGMASK;
> >>>>>> + return generic_is_kvaddr(addr);
> >>>>>> +}
> >>>>>> +
> >>>>>> +static int
> >>>>>> arm64_is_uvaddr(ulong addr, struct task_context *tc)
> >>>>>> {
> >>>>>> return (addr < machdep->machspec->userspace_top);
> >>>>>> @@ -4129,21 +4141,21 @@ arm64_swp_offset(ulong pte)
> >>>>>> return pte;
> >>>>>> }
> >>>>>> -static void arm64_calc_KERNELPACMASK(void)
> >>>>>> +static void arm64_calc_KERNELTAGMASK(void)
> >>>>>> {
> >>>>>> ulong value = 0;
> >>>>>> char *string;
> >>>>>> - if ((string = pc->read_vmcoreinfo("NUMBER(KERNELPACMASK)"))) {
> >>>>>> + if ((string = pc->read_vmcoreinfo("NUMBER(KERNELTAGMASK)"))) {
> >>>>>
> >>>>> I git grep 5.12 kernel, but did not find this. Do you plan to do it in
> >>>>> kernel firstly?
> >>>>>
> >> Again, this does occur in v5.12 kernel. But I guess you plan to export
> >> both MTE and PAC mask through one interface.
> >>
> >> But I think the usage should be separated. As above mentioned.
> >>
> >> Sincerely hope you can address my concerns, due to the unavailable MTE
> >> machine to test and verify my thoughts.
> >
> > Trying to understand this better. Without MTE or PAC or both, it is
> > just a matter of
> > using the proper tasg_mask value ? Ideally it should work for any of
> > these combinations.
> >
> Sorry that I can not totally agree with you.
>
> The tag_mask subfileds should be applied to different things, E.g. here
> PC/LR can not accept MTE subfield.
>
Sure I can split the patch then.
> Thanks,
> Pingfan
>
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility
