Messages in this thread Patch in this message | | | From | Namhyung Kim <> | Date | Fri, 31 Mar 2023 17:11:01 -0700 | Subject | Re: [PATCH] perf top: expand the range of multithreaded phase |
| |
Hello,
Sorry for the late reply.
On Fri, Mar 17, 2023 at 3:05 AM Hangliang Lai <laihangliang1@huawei.com> wrote: > > In __cmd_top, perf_set_multithreaded is used to enable pthread_rwlock, thus > donw_read and down_write can work to handle concurrency problems. Then top > use perf_set_singlethreaded and switch to single threaded phase, assuming > that no thread concurrency will happen later. > > However, a UAF problem could occur in perf top in single threaded phase, > The concurrent procedure is like this: > > display_thread process_thread > -------------- -------------- > > thread__comm_len > -> thread__comm_str > -> __thread__comm_str(thread) > thread__delete > -> comm__free > -> comm_str__put > -> zfree(&cs->str) > -> thread->comm_len = strlen(comm); > > Since in single thread phase, perf_singlethreaded is true, down_read and > down_write can not work to avoid concurrency problems. > > This patch put perf_set_singlethreaded to the function tail to expand the > multithreaded phase range, make display_thread and process_thread run > safe.
I think it should be unconditional as perf top is always multi-threaded.
> > Signed-off-by: Hangliang Lai <laihangliang1@huawei.com> > Reported-by: Wenyu Liu <liuwenyu7@huawei.com> > Reviewed-by: Yunfeng Ye <yeyunfeng@huawei.com> > --- > tools/perf/builtin-top.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c > index 7c6413447..74239940b 100644 > --- a/tools/perf/builtin-top.c > +++ b/tools/perf/builtin-top.c > @@ -1280,9 +1280,6 @@ static int __cmd_top(struct perf_top *top) > top->evlist->core.threads, false, > top->nr_threads_synthesize); > > - if (top->nr_threads_synthesize > 1) > - perf_set_singlethreaded();
Instead, we can simply do
perf_set_multithreaded();
If top->nr_threads_synthesize > 1, no effect. If not, it turns the switch on here.
> - > if (perf_hpp_list.socket) { > ret = perf_env__read_cpu_topology_map(&perf_env); > if (ret < 0) { > @@ -1359,6 +1356,10 @@ out_join: > out_join_thread: > pthread_cond_signal(&top->qe.cond); > pthread_join(thread_process, NULL); > + > + if (top->nr_threads_synthesize > 1) > + perf_set_singlethreaded();
And remove the condition here.
Thanks, Namhyung
> + > return ret; > } > > -- > 2.33.0 >
| |