Re: [PATCH v14 11/19] unwind_user/sframe: Show file name in debug output

On 5/5/2026 8:46 PM, [email protected] 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)
[email protected] / [email protected]

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/