Re: multi-thread read bug

[Steven Rostedt <rostedt@goodmis.org>]

On Thu, 3 Jan 2019 14:08:37 +0200
"Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:

> Hi Ceco,
> 
> I have a problem when trying to call tracecmd_read_at() from different 
> threads. Although the call of tracecmd_read_at() is protected by a 
> mutex,

I believe I stated before that adding a mutex around that wont help at
all.

> I am still getting a segmentation fault.
> A simple program that reproduces the problem is attached as a patch.

Yep, that can easily happen:

tracecmd_read_at() calls read_event() which is:

	record = peek_event(handle, offset, cpu);
	if (record)
		record = tracecmd_read_data(handle, cpu);
	return record;

Where peek_event() caches the event record to handle->cpu_data[cpu].next.

The tracecmd_read_data() will also call tracecmd_peek_data() which if
next is set, will return that, and then set next to NULL.

A easy way to crash this is to do the following:

	CPU0				CPU1
	----				----
 tracecmd_read_at() {
  read_event() {
   peek_event() {
    tracecmd_peek_data() {
      handle->cpu_data[cpu].next = record;
    }
   }
   tracecmd_read_data() {
    tracecmd_peek_data(); [ return .next ]

					tracecmd_read_at() {
					 read_event() {
					  peek_event() {
					   tracecmd_peek_data();
					    [ return .next ]
					  }
					  tracecmd_read_data() {
					   tracecmd_peek_data() {

					    if (cpu_data[cpu].next) {
    record = cpu_data[cpu].next;
    cpu_data[cpu].next = NULL;
						[ .next is now NULL ]
					      record = cpu_data[cpu].next;
					      if (!record->data)

						[ SEGFAULT! ]


There's a few ways to solve this. We could slap mutexes all over the
code, which would slow things down and be a mess.

Or, what may be a bit more complex for threads, we could make a
tracecmd_dup() function that will clone the necessary data. Just like
dup() does for file descriptors.

new_handle = tracecmd_dup(handle);

And have the threads use their own handle. And the handle should have
its own indexes (like cpu_data and such).

They could share data (with a ref count) and then when all is done just
free it normally. The ref counts will be decremented, and when they hit
zero it will be free.

Hmm, we may need a mutex to protect the counters or if there are atomic
counters in userspace, then use them. Doing a quick search, there
appears to be some atomic counters that we can use.

-- Steve