Linux-Trace-Devel Archive on lore.kernel.org
 help / Atom feed
* multi-thread read bug
@ 2019-01-03 12:08 Yordan Karadzhov (VMware)
  2019-01-03 14:32 ` Steven Rostedt
  0 siblings, 1 reply; 2+ messages in thread
From: Yordan Karadzhov (VMware) @ 2019-01-03 12:08 UTC (permalink / raw)
  To: Tzvetomir Stoyanov, Steven Rostedt; +Cc: Linux Trace Devel

[-- Attachment #1: Type: text/plain, Size: 283 bytes --]

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 am still getting a segmentation fault.
A simple program that reproduces the problem is attached as a patch.

Thanks!
Yordan

[-- Attachment #2: mt_read_bug.patch --]
[-- Type: text/x-patch, Size: 2498 bytes --]

^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: multi-thread read bug
  2019-01-03 12:08 multi-thread read bug Yordan Karadzhov (VMware)
@ 2019-01-03 14:32 ` Steven Rostedt
  0 siblings, 0 replies; 2+ messages in thread
From: Steven Rostedt @ 2019-01-03 14:32 UTC (permalink / raw)
  To: Yordan Karadzhov (VMware); +Cc: Tzvetomir Stoyanov, Linux Trace Devel

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

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, back to index

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-03 12:08 multi-thread read bug Yordan Karadzhov (VMware)
2019-01-03 14:32 ` Steven Rostedt

Linux-Trace-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-trace-devel/0 linux-trace-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-trace-devel linux-trace-devel/ https://lore.kernel.org/linux-trace-devel \
		linux-trace-devel@vger.kernel.org linux-trace-devel@archiver.kernel.org
	public-inbox-index linux-trace-devel


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-trace-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox