Re: [PATCH v5 v5 8/9] perf c2c: add function view browser UI and cacheline detail

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Raw pointer assignment of reference-counted `struct thread` and `struct map_symbol` bypasses pointer handle validation and reference count lifecycle checking.
--

--- Patch [8]: [PATCH v5 v5 8/9] perf c2c: add function view browser UI and cacheline detail ---
commit 88acf11c42d184996fd7a80ec56730143d63b227
Author: Jiebin Sun <[email protected]>

perf c2c: add function view browser UI and cacheline detail
    
Add the browser front end: create/run/delete the hist_browser and add the
title. The d shortcut opens the existing per-cacheline detail view for the
selected level-3 cacheline. Level-3 entries retain the source cacheline
index, so the shortcut can locate the original entry without relying on a
potentially ambiguous virtual address.

> diff --git a/tools/perf/ui/browsers/c2c-function.c b/tools/perf/ui/browsers/c2c-function.c
> index 9a95355eedf50..44fb072fa9371 100644
> --- a/tools/perf/ui/browsers/c2c-function.c
> +++ b/tools/perf/ui/browsers/c2c-function.c

[ ... ]

> @@ -1591,8 +1650,182 @@ static int __maybe_unused build_function_view_hierarchy(void)
>  	return ret;
>  }
>  
[ ... ]
> +static struct c2c_function_browser *c2c_function_browser__new(struct hists *hists)
> +{
[ ... ]
> +	/*
> +	 * Note: symbol_conf.report_hierarchy is deliberately left unset.
> +	 * The generic browser still descends into hroot_out children via
> +	 * rb_hierarchy_next()/can_goto_child(), which key off he->unfolded,
> +	 * so 'e'/'+' expands L1 -> L2 -> L3 correctly. Setting the flag would
> +	 * additionally make hist_entry__delete() recurse hroot_out and free
> +	 * each child, but our children borrow thread/ms (see
> +	 * c2c_child_entry__alloc()), so that would underflow their refcounts.
> +	 * Teardown is handled by c2c_he__free_hierarchy() instead.
> +	 */
> +	return browser;
> +}

[Severity: High]
Does this code safely manage the reference counts for struct thread and struct
map_symbol?

The comment mentions that the child entries borrow thread and ms references
from the parent in c2c_child_entry__alloc(). This relies on raw memory copies
and pointer assignments instead of explicit reference acquisitions:

c2c_child_entry__alloc() {
    ...
    memcpy(&child_he->ms, &src_he->ms, sizeof(struct map_symbol));
    ...
    child_he->thread = src_he->thread;
    ...
}

When built with REFCNT_CHECKING, pointer handles are validated to track
lifecycle state. If we bypass the explicit thread__get() and map__get()
lifecycle helpers, could this raw aliasing corrupt the tracking and lead to
use-after-free crashes or false positive traps when the borrowed references
are later used?

> +/*
> + * perf_c2c__browse_function_view - Browse function view with TAB key support
> + *
> + * Returns: 0 on success, negative error code on failure
> + */
> +int perf_c2c__browse_function_view(void)
> +{
[ ... ]
> +	/*
> +	 * The level-2/3 children borrow thread/ms (see c2c_child_entry__alloc())
> +	 * and are torn down by c2c_he__free_hierarchy(). report_hierarchy would
> +	 * make hist_entry__delete() recurse into hroot_out and put those borrowed
> +	 * refs (and call a NULL ops->free), so refuse to run if it is ever set.
> +	 */
> +	if (WARN_ON_ONCE(symbol_conf.report_hierarchy))
> +		return -EINVAL;

[Severity: High]
Given the restrictions mentioned here regarding borrowed references, would it
be safer to update the hierarchy generation to properly acquire reference
counts for the children instead of borrowing them?

This would ensure the pointer handle validation remains correct and avoid
needing these workarounds to prevent refcount underflows.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8