Re: [RFT PATCH v2 7/7] perf-iostat: Enable iostat mode for HiSilicon PCIe PMU

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Use-After-Free and memory leak due to local pointer reassignment of `evlist`.
- [High] Uninitialized variable `ret` returned on specific execution paths.
- [High] Buffer overflow risk due to incorrect size bound `ARG_MAX` in `snprintf`.
- [Medium] Global list duplication and memory leak on repeated parsing.
- [Low] Off-by-one error in `snprintf` truncation check.
--

Note: Due to automated recitation restrictions, this review does not include
inline quotes of the original patch. The findings are summarized below.

commit d8278f9b546336529f0012eecaad3e662cbde173
Author: Yicong Yang <yangyicong@hisilicon.com>

perf-iostat: Enable iostat mode for HiSilicon PCIe PMU

This patch introduces support for the iostat mode on HiSilicon PCIe PMU
devices, allowing measurement of PCIe traffic throughput and latency.
It calculates throughput in MB based on 6 metrics and adds related display
formatting.

In hisi_iostat_prepare(), if the event list already has entries, the code
calls evlist__delete() on it and then allocates a new list with evlist__new().
Because the evlist pointer is passed by value, does this reassignment only
modify the local variable? It appears the caller's pointer would not be
updated and would continue to point to the freed memory, causing a
use-after-free, while the newly allocated list is leaked.

In hisi_pcie_root_ports_list_filter(), the local variable ret is declared
without initialization. If the input string is empty or contains only
delimiters, the loop is bypassed. Could this return an uninitialized value?

Similarly, in hisi_iostat_add_events(), if the root ports list is populated
but none of the ports are marked as selected, the list iteration loop skips
execution. Does this also return an uninitialized ret?

In hisi_iostat_add_events(), the iostat_cmd buffer is dynamically allocated
with a size of PATH_MAX. However, the subsequent calls to snprintf use
ARG_MAX - pos - 1 as the maximum length bound. Since ARG_MAX is typically
much larger than PATH_MAX, does this bypass the bounds checking of snprintf
and risk a buffer overflow if the format strings grow?

Looking at hisi_iostat_parse(), it calls hisi_pcie_root_ports_init()
unconditionally, which appends new port structures to the global root ports
list. If the user provides the iostat option multiple times, will this
accumulate duplicate entries in the list, potentially causing memory leaks
and duplicate hardware events?

Finally, in hisi_pcie_root_ports_add(), the length of the formatted sysfs
path is checked to see if it is strictly greater than PATH_MAX. Because
snprintf returns the number of characters that would be written excluding the
null terminator, a return value exactly equal to PATH_MAX means the output
was silently truncated. Should this condition check if the length is greater
than or equal to PATH_MAX to correctly detect truncation?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260507063737.3542950-1-wangyushan12@huawei.com?part=7