Re: [for-next][PATCH 14/15] tracepoint: Do not fail unregistering a probe due to memory failure

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

The list of tracepoint callbacks is managed by an array that is protected
by RCU. To update this array, a new array is allocated, the updates are
copied over to the new array, and then the list of functions for the
tracepoint is switched over to the new array. After a completion of an RCU
grace period, the old array is freed.

This process happens for both adding a callback as well as removing one.
But on removing a callback, if the new array fails to be allocated, the
callback is not removed, and may be used after it is freed by the clients
of the tracepoint.

There's really no reason to fail if the allocation for a new array fails
when removing a function. Instead, the function can simply be replaced by a
stub function that could be cleaned up on the next modification of the
array. That is, instead of calling the function registered to the
tracepoint, it would call a stub function in its place.

Link: https://lore.kernel.org/r/20201115055256.65625-1-mmullins@mmlx.us (local)
Link: https://lore.kernel.org/r/20201116175107.02db396d@gandalf.local.home (local)
Link: https://lore.kernel.org/r/20201117211836.54acaef2@oasis.local.home (local)
Link: https://lkml.kernel.org/r/20201118093405.7a6d2290@gandalf.local.home

[ Note, this version does use undefined compiler behavior (assuming that
 a stub function with no parameters or return, can be called by a location
 that thinks it has parameters but still no return value. Static calls
 do the same thing, so this trick is not without precedent.

 There's another solution that uses RCU tricks and is more complex, but
 can be an alternative if this solution becomes an issue.

 Link: https://lore.kernel.org/lkml/20210127170721.58bce7cc@gandalf.local.home/ (local)
]

Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Josh Poimboeuf <redacted>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Martin KaFai Lau <redacted>
Cc: Song Liu <redacted>
Cc: Yonghong Song <redacted>
Cc: Andrii Nakryiko <redacted>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: KP Singh <redacted>
Cc: netdev <redacted>
Cc: bpf <redacted>
Cc: Kees Cook <redacted>
Cc: Florian Weimer <redacted>
Fixes: 97e1c18e8d17b ("tracing: Kernel Tracepoints")
Reported-by: syzbot+83aa762ef23b6f0d1991@syzkaller.appspotmail.com
Reported-by: syzbot+d29e58bb557324e55e5e@syzkaller.appspotmail.com
Reported-by: Matt Mullins <redacted>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Tested-by: Matt Mullins <redacted>
---
kernel/tracepoint.c | 80 ++++++++++++++++++++++++++++++++++++---------
1 file changed, 64 insertions(+), 16 deletions(-)

diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
index 7261fa0f5e3c..e8f20ae29c18 100644
--- a/kernel/tracepoint.c
+++ b/kernel/tracepoint.c

@@ -53,6 +53,12 @@ struct tp_probes {

	struct tracepoint_func probes[];
};

+/* Called in removal of a func but failed to allocate a new tp_funcs */
+static void tp_stub_func(void)
+{
+	return;
+}
+
static inline void *allocate_probes(int count)
{
	struct tp_probes *p  = kmalloc(struct_size(p, probes, count),

@@ -131,6 +137,7 @@ func_add(struct tracepoint_func **funcs, struct

tracepoint_func *tp_func,
{
	struct tracepoint_func *old, *new;
	int nr_probes = 0;
+	int stub_funcs = 0;
	int pos = -1;

	if (WARN_ON(!tp_func->func))

@@ -147,14 +154,34 @@ func_add(struct tracepoint_func **funcs, struct

tracepoint_func *tp_func,
			if (old[nr_probes].func == tp_func->func &&
			    old[nr_probes].data == tp_func->data)
				return ERR_PTR(-EEXIST);
+			if (old[nr_probes].func == tp_stub_func)
+				stub_funcs++;
		}
	}
-	/* + 2 : one for new probe, one for NULL func */
-	new = allocate_probes(nr_probes + 2);
+	/* + 2 : one for new probe, one for NULL func - stub functions */
+	new = allocate_probes(nr_probes + 2 - stub_funcs);
	if (new == NULL)
		return ERR_PTR(-ENOMEM);
	if (old) {
-		if (pos < 0) {
+		if (stub_funcs) {

+		} else if (pos < 0) {
			pos = nr_probes;
			memcpy(new, old, nr_probes * sizeof(struct tracepoint_func));
		} else {

@@ -188,8 +215,9 @@ static void *func_remove(struct tracepoint_func **funcs,

	/* (N -> M), (N > 1, M >= 0) probes */
	if (tp_func->func) {
		for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
-			if (old[nr_probes].func == tp_func->func &&
-			     old[nr_probes].data == tp_func->data)
+			if ((old[nr_probes].func == tp_func->func &&
+			     old[nr_probes].data == tp_func->data) ||
+			    old[nr_probes].func == tp_stub_func)
				nr_del++;
		}
	}

@@ -208,14 +236,32 @@ static void *func_remove(struct tracepoint_func **funcs,

		/* N -> M, (N > 1, M > 0) */
		/* + 1 for NULL */
		new = allocate_probes(nr_probes - nr_del + 1);
-		if (new == NULL)
-			return ERR_PTR(-ENOMEM);
-		for (i = 0; old[i].func; i++)
-			if (old[i].func != tp_func->func
-					|| old[i].data != tp_func->data)
-				new[j++] = old[i];
-		new[nr_probes - nr_del].func = NULL;
-		*funcs = new;
+		if (new) {
+			for (i = 0; old[i].func; i++)
+				if ((old[i].func != tp_func->func
+				     || old[i].data != tp_func->data)
+				    && old[i].func != tp_stub_func)
+					new[j++] = old[i];
+			new[nr_probes - nr_del].func = NULL;
+			*funcs = new;
+		} else {
+			/*
+			 * Failed to allocate, replace the old function
+			 * with calls to tp_stub_func.
+			 */
+			for (i = 0; old[i].func; i++)
+				if (old[i].func == tp_func->func &&
+				    old[i].data == tp_func->data) {
+					old[i].func = tp_stub_func;