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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] perf: annotate-data: incorrect offset bounds check misses struct padding
- [Medium] perf: annotate-data: incorrect union flexible array resolution using list_last_entry
- [Medium] perf: annotate-data: indiscriminate has_flex_array propagation misattributes padding
--

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

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

This patch adjusts the type offset for flex-array members in perf's data
annotation. It ensures that accesses beyond the original type size are mapped
to the correct elements using modulo operations. It also ensures field names
for flex arrays are correctly found.

> diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
> index 1ee2b74f99205..d4410d720e126 100644
> --- a/tools/perf/util/annotate-data.c
> +++ b/tools/perf/util/annotate-data.c

[ ... ]

> @@ -333,19 +338,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)
>  {
>  	struct annotated_member *child;
> +	bool found = false;
> +	int len;
>  
>  	if (list_empty(&m->children))
>  		return 0;
>  
>  	list_for_each_entry(child, &m->children, node) {
> -		int len;
> -
>  		if (offset < child->offset || offset >= child->offset + child->size)
>  			continue;
>  
> +		found = true;
> +		break;
> +	}
> +
> +	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);
> +
> +		if (offset < child->offset)
> +			return 0;

[Severity: Medium]
Does blindly selecting the last child work for unions where the flexible array
is not the last member in DWARF order?

It appears list_last_entry() will pick the last DWARF entry, which could
break flexible array annotation for unions if the compiler places the flexible
array member before other elements.

> +
> +		found = true;
> +	}
> +
> +	if (found) {
>  		/* It can have anonymous struct/union members */
>  		if (child->var_name) {
>  			len = scnprintf(buf, sz, "%s%s",
> @@ -355,15 +381,37 @@ static int fill_member_name(char *buf, size_t sz, struct annotated_member *m,
>  			len = 0;
>  		}
>  
> -		return fill_member_name(buf + len, sz - len, child, offset, first) + len;
> +		return fill_member_name(buf + len, sz - len, child, offset, first,
> +					has_flex_array) + len;

[Severity: Medium]
Does propagating has_flex_array unconditionally into all recursive calls cause
out-of-bounds logic to activate for unrelated sub-structs?

If an offset lands in the trailing padding of an intermediate sub-struct, it
seems the fallback logic might incorrectly attribute it to the last member of
that sub-struct instead of resolving as no member.

>  	}
> +
>  	return 0;
>  }
>  
>  int annotated_data_type__get_member_name(struct annotated_data_type *adt,
>  					 char *buf, size_t sz, int member_offset)
>  {
> -	return fill_member_name(buf, sz, &adt->self, member_offset, /*first=*/true);
> +	return fill_member_name(buf, sz, &adt->self, member_offset, /*first=*/true,
> +				adt->flex_array);
> +}
> +
> +static struct annotated_member *find_flex_array(struct annotated_member *m)
> +{
> +	struct annotated_member *child;
> +
> +	if (list_empty(&m->children))
> +		return NULL;
> +
> +	child = list_last_entry(&m->children, struct annotated_member, node);
> +	if (strstr(child->type_name, "[]"))
> +		return child;

[Severity: Medium]
Similar to fill_member_name(), can this fail for unions where the flexible array
is not the last member in DWARF order?

> +
> +	return find_flex_array(child);
> +}
> +
> +static struct annotated_member *get_flex_array_member(struct annotated_data_type *adt)
> +{
> +	return find_flex_array(&adt->self);
>  }

[ ... ]

> @@ -1740,6 +1788,7 @@ struct annotated_data_type *find_data_type(struct data_loc_info *dloc)
>  {
>  	struct dso *dso = map__dso(dloc->ms->map);
>  	Dwarf_Die type_die;
> +	struct annotated_data_type *result;
>  
>  	/*
>  	 * The type offset is the same as instruction offset by default.
> @@ -1752,7 +1801,25 @@ struct annotated_data_type *find_data_type(struct data_loc_info *dloc)
>  	if (find_data_type_die(dloc, &type_die) < 0)
>  		return NULL;
>  
> -	return dso__findnew_data_type(dso, &type_die);
> +	result = dso__findnew_data_type(dso, &type_die);
> +	if (result == NULL)
> +		return NULL;
> +
> +	if (result->flex_array && dloc->type_offset > result->self.size) {

[Severity: Medium]
Does this condition correctly handle flexible arrays if the elements start
within the struct's trailing padding or exactly at the struct's size boundary?

If dloc->type_offset is less than or equal to result->self.size, but lands in
the padding, it seems the modulo-adjustment will be skipped. This could cause
the fallback logic to incorrectly recurse into the last field of the element
type, which might yield wrong struct field names in the output.

> +		struct annotated_member *flex_array = get_flex_array_member(result);
> +
> +		if (flex_array && flex_array->size > 0) {
> +			int offset = dloc->type_offset;
> +
> +			/* adjust offset in the flex array */
> +			offset -= flex_array->offset;
> +			offset %= flex_array->size;
> +			offset += flex_array->offset;
> +
> +			dloc->type_offset = offset;
> +		}
> +	}
> +	return result;
>  }

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