* Re: [PATCH v14 11/19] unwind_user/sframe: Show file name in debug output
[not found] ` <20260505184638.5648DC2BCB4@smtp.kernel.org>
@ 2026-05-12 14:52 ` Jens Remus
2026-05-13 9:20 ` Jens Remus
0 siblings, 1 reply; 2+ messages in thread
From: Jens Remus @ 2026-05-12 14:52 UTC (permalink / raw)
To: Steven Rostedt, Josh Poimboeuf; +Cc: bpf, sashiko, Indu Bhagat, Heiko Carstens
On 5/5/2026 8:46 PM, sashiko-bot@kernel.org wrote:
> [ ... ]
>> @@ -496,7 +501,7 @@ static int __sframe_remove_section(struct mm_struct *mm,
>> struct sframe_section *sec)
>> {
>> if (!mtree_erase(&mm->sframe_mt, sec->text_start)) {
>> - dbg("mtree_erase failed: text=%lx\n", sec->text_start);
>> + dbg_sec("mtree_erase failed: text=%lx\n", sec->text_start);
>
> Does accessing sec->filename in dbg_sec() risk a use-after-free here? If
> mtree_erase() fails, it indicates the section might have been concurrently
> removed. The SRCU grace period could expire, causing sec to be freed before
> dbg_sec() is called.
IIUC this is a valid concern. Should this change be simply be reverted
or should __sframe_remove_section() create a temporary copy of the
filename and use dbg() for this rather unlikely case?
> [ ... ]
>> diff --git a/kernel/unwind/sframe_debug.h b/kernel/unwind/sframe_debug.h
>> index 36352124cde81..e568be4172b1b 100644
>> --- a/kernel/unwind/sframe_debug.h
>> +++ b/kernel/unwind/sframe_debug.h
> [ ... ]
>> +static inline void dbg_init(struct sframe_section *sec)
>> +{
>> + struct mm_struct *mm = current->mm;
>> + struct vm_area_struct *vma;
>> +
>> + guard(mmap_read_lock)(mm);
>> + vma = vma_lookup(mm, sec->sframe_start);
Should below be changed as follows based on Sashikos AI review feedback?
>> + if (!vma)
>> + sec->filename = kstrdup("(vma gone???)", GFP_KERNEL);
else if (vma_is_anonymous(vma))
sec->filename = kstrdup("(anonymous)", GFP_KERNEL);
>> + else if (vma->vm_file)
>> + sec->filename = kstrdup_quotable_file(vma->vm_file, GFP_KERNEL);
>
> Does checking vma->vm_file directly skip proper classification for some
> anonymous VMAs? According to the VMA guidelines, certain anonymous VMAs like
> private mappings of /dev/zero have vma->vm_file set. Should this use
> !vma_is_anonymous(vma) instead?
>
> Do these string allocations need __GFP_ACCOUNT? Since they are triggered by
> user space and persist until the section is removed, could a user allocate
> large amounts of uncharged memory and bypass memory cgroup limits?
>
>> + else if (vma->vm_ops && vma->vm_ops->name)
else if (vma->vm_ops && vma->vm_ops->name && vma->vm_ops->name(vma))
>> + sec->filename = kstrdup(vma->vm_ops->name(vma), GFP_KERNEL);
>
> If vma->vm_ops->name(vma) returns NULL, will kstrdup() return NULL and skip
> the subsequent fallback naming methods? Does this leave sec->filename as
> NULL, causing debug logs to print (null)?
>
>> + else if (arch_vma_name(vma))
>> + sec->filename = kstrdup(arch_vma_name(vma), GFP_KERNEL);
>> + else if (!vma->vm_mm)
>> + sec->filename = kstrdup("(vdso)", GFP_KERNEL);
>> + else
>> + sec->filename = kstrdup("(anonymous)", GFP_KERNEL);
sec->filename = kstrdup("(unknown vma???)", GFP_KERNEL);
>> +}
>
Thanks and regards,
Jens
--
Jens Remus
Linux on Z Development (D3303)
jremus@de.ibm.com / jremus@linux.ibm.com
IBM Deutschland Research & Development GmbH; Vorsitzender des Aufsichtsrats: Wolfgang Wendt; Geschäftsführung: David Faller; Sitz der Gesellschaft: Ehningen; Registergericht: Amtsgericht Stuttgart, HRB 243294
IBM Data Privacy Statement: https://www.ibm.com/privacy/
^ permalink raw reply [flat|nested] 2+ messages in thread