Messages in this thread | | | Date | Tue, 7 Jul 2009 10:07:56 +0200 | From | Frederic Weisbecker <> | Subject | Re: [PATCH 2/2] trace_workqueue: add refcnt to struct cpu_workqueue_stats |
| |
On Tue, Jul 07, 2009 at 02:07:35PM +0800, Li Zefan wrote: > > The stat entries can be freed when the stat file is being read. > > The worse is, the ptr can be freed immediately after it's returned > > from workqueue_stat_start/next(). > > > > Add a refcnt to struct cpu_workqueue_stats to avoid use-after-free. > > > > Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com> > > Signed-off-by: Li Zefan <lizf@cn.fujitsu.com> > > --- > ... > > @@ -175,11 +184,14 @@ static void *workqueue_stat_next(void *prev, int idx) > > return NULL; > > } while (!(ret = workqueue_stat_start_cpu(cpu))); > > return ret; > > + } else { > > + ret = list_entry(prev_cws->list.next, > > + struct cpu_workqueue_stats, list); > > I just realized accessing prev_cws->list.next can be invalid! > > We can fix it by using list_del_init() to delete cws->list in > probe_workqueue_destruction(), but then if the race happened, > the next time stat_next() is called, NULL will be returned. > I guess this is Ok, since the race is rare.
If you ensure the kref_get/put are under the workqueue_cpu_stat(cpu)->lock, it should be fine, right?
> (I never like the design of trace_stat..Fortunately we'll > probably switch to perfcounter for this kind of statistics > reporting)
I don't like its design either. I wrote it specifically for the branch tracer and didn't think about free-able events :-/
> > > + kref_get(&ret->kref); > > } > > spin_unlock_irqrestore(&workqueue_cpu_stat(cpu)->lock, flags); > > > > - return list_entry(prev_cws->list.next, struct cpu_workqueue_stats, > > - list); > > + return ret; > > } >
| |