From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Date: Sun, 30 Jun 2013 19:37:46 +0000 (-0400)
Subject: Fix: ring buffer: RING_BUFFER_FLUSH ioctl buffer corruption
X-Git-Tag: v2.1.3~5
X-Git-Url: https://git.lttng.org./?a=commitdiff_plain;h=8dfa3bb8897b8d9ac9b7ea33bebb7dad61b32d44;p=lttng-modules.git

Fix: ring buffer: RING_BUFFER_FLUSH ioctl buffer corruption

lib_ring_buffer_switch_slow() clearly states:

 * Note, however, that as a v_cmpxchg is used for some atomic
 * operations, this function must be called from the CPU which owns the
 * buffer for a ACTIVE flush.

But unfortunately, the RING_BUFFER_FLUSH ioctl does not follow these
important directives. Therefore, whenever the consumer daemon or session
daemon explicitly triggers a "flush" on a buffer, it can race with data
being written to the buffer, leading to corruption of the reserve/commit
counters, and therefore corruption of data in the buffer. It triggers
these warnings for overwrite mode buffers:

[65356.890016] WARNING: at
/home/compudj/git/lttng-modules/wrapper/ringbuffer/../../lib/ringbuffer/../../wrapper/ringbuffer/../../lib/ringbuffer/backend.h:110 lttng_event_write+0x118/0x140 [lttng_ring_buffer_client_mmap_overwrite]()

Which indicates that we are trying to write into a sub-buffer for which
we don't have exclusive access. It also causes the following warnings to
show up:

[65335.896208] ring buffer relay-overwrite-mmap, cpu 5: records were lost. Caused by:
[65335.896208]   [ 0 buffer full, 80910 nest buffer wrap-around, 0 event too big ]

Which is caused by corrupted commit counter.

Fix this by sending an IPI to the CPU owning the flushed buffer for
per-cpu synchronization. For global synchronization, no IPI is needed,
since we allow writes from remote CPUs.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
---

diff --git a/lib/ringbuffer/frontend_internal.h b/lib/ringbuffer/frontend_internal.h
index a86abb14..bc284333 100644
--- a/lib/ringbuffer/frontend_internal.h
+++ b/lib/ringbuffer/frontend_internal.h
@@ -156,6 +156,9 @@ extern
 void lib_ring_buffer_switch_slow(struct lib_ring_buffer *buf,
 				 enum switch_mode mode);
 
+extern
+void lib_ring_buffer_switch_remote(struct lib_ring_buffer *buf);
+
 /* Buffer write helpers */
 
 static inline
diff --git a/lib/ringbuffer/ring_buffer_frontend.c b/lib/ringbuffer/ring_buffer_frontend.c
index 2626afa3..942e14d3 100644
--- a/lib/ringbuffer/ring_buffer_frontend.c
+++ b/lib/ringbuffer/ring_buffer_frontend.c
@@ -1516,6 +1516,48 @@ void lib_ring_buffer_switch_slow(struct lib_ring_buffer *buf, enum switch_mode m
 }
 EXPORT_SYMBOL_GPL(lib_ring_buffer_switch_slow);
 
+static void remote_switch(void *info)
+{
+	struct lib_ring_buffer *buf = info;
+
+	lib_ring_buffer_switch_slow(buf, SWITCH_ACTIVE);
+}
+
+void lib_ring_buffer_switch_remote(struct lib_ring_buffer *buf)
+{
+	struct channel *chan = buf->backend.chan;
+	const struct lib_ring_buffer_config *config = &chan->backend.config;
+	int ret;
+
+	/*
+	 * With global synchronization we don't need to use the IPI scheme.
+	 */
+	if (config->sync == RING_BUFFER_SYNC_GLOBAL) {
+		lib_ring_buffer_switch_slow(buf, SWITCH_ACTIVE);
+		return;
+	}
+
+	/*
+	 * Taking lock on CPU hotplug to ensure two things: first, that the
+	 * target cpu is not taken concurrently offline while we are within
+	 * smp_call_function_single() (I don't trust that get_cpu() on the
+	 * _local_ CPU actually inhibit CPU hotplug for the _remote_ CPU (to be
+	 * confirmed)). Secondly, if it happens that the CPU is not online, our
+	 * own call to lib_ring_buffer_switch_slow() needs to be protected from
+	 * CPU hotplug handlers, which can also perform a remote subbuffer
+	 * switch.
+	 */
+	get_online_cpus();
+	ret = smp_call_function_single(buf->backend.cpu,
+				 remote_switch, buf, 1);
+	if (ret) {
+		/* Remote CPU is offline, do it ourself. */
+		lib_ring_buffer_switch_slow(buf, SWITCH_ACTIVE);
+	}
+	put_online_cpus();
+}
+EXPORT_SYMBOL_GPL(lib_ring_buffer_switch_remote);
+
 /*
  * Returns :
  * 0 if ok
diff --git a/lib/ringbuffer/ring_buffer_vfs.c b/lib/ringbuffer/ring_buffer_vfs.c
index 899af812..a7c0e695 100644
--- a/lib/ringbuffer/ring_buffer_vfs.c
+++ b/lib/ringbuffer/ring_buffer_vfs.c
@@ -261,7 +261,7 @@ long lib_ring_buffer_ioctl(struct file *filp, unsigned int cmd, unsigned long ar
 				 arg);
 	}
 	case RING_BUFFER_FLUSH:
-		lib_ring_buffer_switch_slow(buf, SWITCH_ACTIVE);
+		lib_ring_buffer_switch_remote(buf);
 		return 0;
 	default:
 		return -ENOIOCTLCMD;
@@ -374,7 +374,7 @@ long lib_ring_buffer_compat_ioctl(struct file *filp, unsigned int cmd,
 		return compat_put_ulong(read_offset, arg);
 	}
 	case RING_BUFFER_COMPAT_FLUSH:
-		lib_ring_buffer_switch_slow(buf, SWITCH_ACTIVE);
+		lib_ring_buffer_switch_remote(buf);
 		return 0;
 	default:
 		return -ENOIOCTLCMD;