Messages in this thread | | | Date | Tue, 23 May 2023 17:31:50 +0300 | Subject | Re: [PATCH v1 2/2] perf evsel: for_each_group fixes | From | Adrian Hunter <> |
| |
On 23/05/23 07:44, Ian Rogers wrote: > Address/memory sanitizer was reporting issues in evsel__group_pmu_name > because the for_each_group_evsel loop didn't terminate when the head > was reached, the head would then be cast and accessed as an evsel > leading to invalid memory accesses. Fix for_each_group_member and > for_each_group_evsel to terminate at the list head. Note, > evsel__group_pmu_name no longer iterates the group, but the problem is > present regardless. > > Fixes: 717e263fc354 ("perf report: Show group description when event group is enabled") > Signed-off-by: Ian Rogers <irogers@google.com> > --- > tools/perf/util/evsel.h | 24 ++++++++++++++++-------- > tools/perf/util/evsel_fprintf.c | 1 + > 2 files changed, 17 insertions(+), 8 deletions(-) > > diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h > index 820771a649b2..6a64543c7612 100644 > --- a/tools/perf/util/evsel.h > +++ b/tools/perf/util/evsel.h > @@ -462,16 +462,24 @@ static inline int evsel__group_idx(struct evsel *evsel) > } > > /* Iterates group WITHOUT the leader. */ > -#define for_each_group_member(_evsel, _leader) \ > -for ((_evsel) = list_entry((_leader)->core.node.next, struct evsel, core.node); \ > - (_evsel) && (_evsel)->core.leader == (&_leader->core); \ > - (_evsel) = list_entry((_evsel)->core.node.next, struct evsel, core.node)) > +#define for_each_group_member_head(_evsel, _leader, _head) \ > +for ((_evsel) = list_entry((_leader)->core.node.next, struct evsel, core.node); \ > + (_evsel) && (&(_evsel)->core.node != (_head)) && \
Extra parentheses perhaps not needed e.g. just
&(_evsel)->core.node != (_head)
> + (_evsel)->core.leader == (&_leader->core); \
Parentheses look odd, maybe should be:
&(_leader)->core
> + (_evsel) = list_entry((_evsel)->core.node.next, struct evsel, core.node)) > + > +#define for_each_group_member(_evsel, _leader) \ > + for_each_group_member_head(_evsel, _leader, &(_leader)->evlist->core.entries)
Did you consider using (_leader)->core.nr_members so that it is not necessary to assume the evlist back pointer is populated.
> > /* Iterates group WITH the leader. */ > -#define for_each_group_evsel(_evsel, _leader) \ > -for ((_evsel) = _leader; \ > - (_evsel) && (_evsel)->core.leader == (&_leader->core); \ > - (_evsel) = list_entry((_evsel)->core.node.next, struct evsel, core.node)) > +#define for_each_group_evsel_head(_evsel, _leader, _head) \ > +for ((_evsel) = _leader; \ > + (_evsel) && (&(_evsel)->core.node != (_head)) && \ > + (_evsel)->core.leader == (&_leader->core); \ > + (_evsel) = list_entry((_evsel)->core.node.next, struct evsel, core.node)) > + > +#define for_each_group_evsel(_evsel, _leader) \ > + for_each_group_evsel_head(_evsel, _leader, &(_leader)->evlist->core.entries) > > static inline bool evsel__has_branch_callstack(const struct evsel *evsel) > { > diff --git a/tools/perf/util/evsel_fprintf.c b/tools/perf/util/evsel_fprintf.c > index cc80ec554c0a..036a2171dc1c 100644 > --- a/tools/perf/util/evsel_fprintf.c > +++ b/tools/perf/util/evsel_fprintf.c > @@ -2,6 +2,7 @@ > #include <inttypes.h> > #include <stdio.h> > #include <stdbool.h> > +#include "util/evlist.h" > #include "evsel.h" > #include "util/evsel_fprintf.h" > #include "util/event.h"
| |