Re: [PATCH 4/4] perf annotate-data: Adjust type offset for flex-array

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Division by zero when calculating the adjusted offset for a flexible array member if the array element size is zero.
- [Medium] `find_flex_array()` fails to find the flex array member if the outer struct has trailing padding.
- [Medium] Fallback struct matching fails for typedefs and unions.
- [Medium] Struct padding holes are incorrectly annotated as the last member.
--

commit 29427abf6848041f9bb5e65c33e21982de9656cd
Author: Namhyung Kim <namhyung@kernel.org>

perf annotate-data: Adjust type offset for flex-array

The flex array members are located beyond the original type size.  Also
it needs to adjust the offset in an array to find a corresponding
element using module operation.  Note that we focus on access to type and
field, so array index is not important.

Make sure to find a field name for flex arrays.

> diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
> index ad043403ced58..0789a8cbea7c0 100644
> --- a/tools/perf/util/annotate-data.c
> +++ b/tools/perf/util/annotate-data.c
[ ... ]
> @@ -332,19 +332,40 @@ static void delete_members(struct annotated_member *member)
>  }
>  
>  static int fill_member_name(char *buf, size_t sz, struct annotated_member *m,
> -			    int offset, bool first)
> +			    int offset, bool first, bool has_flex_array)
>  {
[ ... ]
> +	if (!found && has_flex_array) {
> +		/*
> +		 * It may have an intermediate struct that has another struct that
> +		 * contains a flex array.  In that case, the outer struct itself is
> +		 * has no array and the size is less than the offset so the above
> +		 * logic won't find the outer struct at the offset.  Let's use the
> +		 * last struct if it couldn't find a member for the flex array.
> +		 */
> +		child = list_last_entry(&m->children, struct annotated_member, node);

[Severity: Medium]
In fill_member_name() in tools/perf/util/annotate-data.c, if the requested
offset doesn't fall into any member's bounds (which occurs for accesses to
padding bytes between fields), found remains false. If has_flex_array is true,
this block unconditionally assigns child to the last member and sets found to
true.

Will this cause random memory accesses to padding bytes to be erroneously
annotated as valid accesses to the flexible array's parent struct?

> +
> +		if (!strstr(child->type_name, "struct "))
> +			return 0;

[Severity: Medium]
In fill_member_name() in tools/perf/util/annotate-data.c, can this fallback
struct matching fail for valid nested flexible arrays declared with a typedef
or a union?

If the nested intermediate member was declared using a typedef (e.g.,
typedef struct { ... } foo_t;), the DWARF type name will be foo_t, lacking
the substring "struct ". If it is a union, its type name begins with "union ".
In these cases, this check fails, causing the function to incorrectly return 0
and drop the annotation.

> +
> +		found = true;
> +	}
[ ... ]
> +static struct annotated_member *find_flex_array(struct annotated_member *m, int offset)
> +{
> +	struct annotated_member *child;
> +
> +	if (list_empty(&m->children))
> +		return NULL;
> +
> +	child = list_last_entry(&m->children, struct annotated_member, node);
> +	if (offset == child->offset && strstr(child->type_name, "[]"))
> +		return child;

[Severity: Medium]
In find_flex_array() in tools/perf/util/annotate-data.c, will this strict
offset equality check fail if the outer struct has trailing padding?

When get_flex_array_member() calls find_flex_array(&adt->self, adt->self.size),
the offset argument is the struct's total padded size. If the struct contains
trailing alignment padding (e.g., a previous 8-byte aligned member followed by
the flex array), the struct's padded total size will be strictly greater than
the actual starting offset of the flex array.

Will this cause find_flex_array() to fail the equality check and incorrectly
recurse into the flex array itself, returning NULL and bypassing the offset
adjustment logic?

> +
> +	return find_flex_array(child, offset);
> +}
[ ... ]
> @@ -1743,6 +1786,7 @@ struct annotated_data_type *find_data_type(struct data_loc_info *dloc)
[ ... ]
> +	if (result->flex_array && dloc->type_offset > result->self.size) {
> +		struct annotated_member *flex_array = get_flex_array_member(result);
> +		if (flex_array) {
> +			int offset = dloc->type_offset;
> +
> +			/* adjust offset in the flex array */
> +			offset -= result->self.size;
> +			offset %= flex_array->size;

[Severity: High]
In find_data_type() in tools/perf/util/annotate-data.c, is it possible for
this modulo operation to divide by the flexible array's element size without
validating that the size is non-zero?

The flex_array->size is determined by getting the size of the array's element
type. If the element type is an empty struct (a GCC extension with size 0) or
an incomplete type where DWARF size cannot be determined, flex_array->size
becomes 0.

Could evaluating offset %= 0 here directly expose the perf tool to a
divide-by-zero exception (SIGFPE) and crash?

> +			offset += result->self.size;
> +
> +			dloc->type_offset = offset;
> +		}
> +	}
> +	return result;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260912054706.1475583-1-namhyung@kernel.org?part=4