12:36 a.m.
Hi Pingfan,
On Fri, May 7, 2021 at 2:26 PM piliu <piliu(a)redhat.com> wrote:
On 5/3/21 2:16 PM, vinayak menon wrote:
> Hi Pingfan
>
> On Thu, Apr 29, 2021 at 3:35 PM piliu <piliu(a)redhat.com> 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(a)gmail.com>
>>>>>> ---
>>>>>> 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