linux-kernel - Re: [PATCH v2 2/4] ftrace: introduce workqueue_handler_exit tracepoint and rename workqueue_execution to workqueue_handler

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20090415114549.GA17775@elte.hu>
Date:	Wed, 15 Apr 2009 13:45:49 +0200
From:	Ingo Molnar <mingo@...e.hu>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Frederic Weisbecker <fweisbec@...il.com>,
	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>,
	Zhaolei <zhaolei@...fujitsu.com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Tom Zanussi <tzanussi@...il.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/4] ftrace: introduce workqueue_handler_exit
	tracepoint and rename workqueue_execution to workqueue_handler_entry


* Oleg Nesterov <oleg@...hat.com> wrote:

> On 04/15, Ingo Molnar wrote:
> >
> > * Oleg Nesterov <oleg@...hat.com> wrote:
> >
> > > > >  		lock_map_acquire(&lockdep_map);
> > > > > +		trace_workqueue_handler_entry(cwq->thread, work);
> > > > >  		f(work);
> > > > > +		trace_workqueue_handler_exit(cwq->thread, work);
> > >
> > > This doesn't look right. We must not use "work" after f(work).
> > > work->func() can kfree its work.
> >
> > We can use it as long as we use it as a 'cookie' - i.e. an
> > identifier for visualization/statistics, but dont actually
> > dereference it, right?
> 
> Yes sure.
> 
> I do not know whether this matters or not, but just in case... Of 
> course it is possible that, when trace_workqueue_handler_exit() 
> runs, this memory was already reused for another work even without 
> kfree. For example,
> 
> 	void my_work_func(struct work_struct *work)
> 	{
> 		INIT_WORK(work, another_work_func);
> 		queue_work(another_workqueue, work);
> 	}
> 
> In this case another_workqueue can report 
> trace_workqueue_handler_entry() before my_work_func() returns.
> 
> This means that trace_workqueue_handler_exit() can't use the 
> address of work as a "key" to find some data recorded by _entry(). 
> Unless this data lives in cpu_workqueue_struct.
> 
> Not sure why I am trying to explain the things which are very 
> obvious to all ;) Just because I don't really understand what 
> these patches do.

The patches try to map and instrument the life cycle of a worklet, 
and the main actions that occur in the workqueue subsystem in 
general.

The purpose is instrumentation: for debugging purposes, for 
improving kernel code and for just understanding how the system 
functions and what its dynamic actions are.

In that sense the worklet 'key' possibly getting reallocated and 
reused before the 'completed' action was traced is probably not a 
big deal - but tools have to be aware of it possibly happening (and 
most not hard-code any assumption to the contrary).

Plus the exit handler must not dereference the worklet either. 
Safest would be to make this sure in the prototype already: pass in 
a void * key, not a work structure.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/