[PATCH bpf v1 0/3] Fixes for BPF timer lockup and UAF

[PATCH bpf v1 0/3] Fixes for BPF timer lockup and UAF

[Kumar Kartikeya Dwivedi]

The following patches contain fixes for timer lockups and a
use-after-free scenario.

This set proposes to fix the following lockup situation for BPF timers.

CPU 1					CPU 2

bpf_timer_cb				bpf_timer_cb
  timer_cb1				  timer_cb2
    bpf_timer_cancel(timer_cb2)		    bpf_timer_cancel(timer_cb1)
      hrtimer_cancel			      hrtimer_cancel

In this case, both callbacks will continue waiting for each other to
finish synchronously, causing a lockup.

The proposed fix adds support for tracking in-flight cancellations
*begun by other timer callbacks* for a particular BPF timer.  Whenever
preparing to call hrtimer_cancel, a callback will increment the target
timer's counter, then inspect its in-flight cancellations, and if
non-zero, return -EDEADLK to avoid situations where the target timer's
callback is waiting for its completion.

This does mean that in cases where a callback is fired and cancelled, it
will be unable to cancel any timers in that execution. This can be
alleviated by maintaining the list of waiting callbacks in bpf_hrtimer
and searching through it to avoid interdependencies, but this may
introduce additional delays in bpf_timer_cancel, in addition to
requiring extra state at runtime which may need to be allocated or
reused from bpf_hrtimer storage. Moreover, extra synchronization is
needed to delete these elements from the list of waiting callbacks once
hrtimer_cancel has finished.

The second patch is for a deadlock situation similar to above in
bpf_timer_cancel_and_free, but also a UAF scenario that can occur if
timer is armed before entering it, if hrtimer_running check causes the
hrtimer_cancel call to be skipped.

As seen above, synchronous hrtimer_cancel would lead to deadlock (if
same callback tries to free its timer, or two timers free each other),
therefore we queue work onto the global workqueue to ensure outstanding
timers are cancelled before bpf_hrtimer state is freed.

Further details are in the patches.

Kumar Kartikeya Dwivedi (3):
  bpf: Fail bpf_timer_cancel when callback is being cancelled
  bpf: Defer work in bpf_timer_cancel_and_free
  selftests/bpf: Add timer lockup selftest

 kernel/bpf/helpers.c                          | 99 +++++++++++++++----
 .../selftests/bpf/prog_tests/timer_lockup.c   | 65 ++++++++++++
 .../selftests/bpf/progs/timer_lockup.c        | 85 ++++++++++++++++
 3 files changed, 232 insertions(+), 17 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/timer_lockup.c
 create mode 100644 tools/testing/selftests/bpf/progs/timer_lockup.c


base-commit: 528269fe117f3b19461733a0fa408c55a5270aff
-- 
2.43.0

[PATCH bpf v1 1/3] bpf: Fail bpf_timer_cancel when callback is being cancelled

[Kumar Kartikeya Dwivedi]

Given a schedule:

timer1 cb			timer2 cb

bpf_timer_cancel(timer2);	bpf_timer_cancel(timer1);

Both bpf_timer_cancel calls would wait for the other callback to finish
executing, introducing a lockup.

Add an atomic_t count named 'cancelling' in bpf_hrtimer. This keeps
track of all in-flight cancellation requests for a given BPF timer.
Whenever cancelling a BPF timer, we must check if we have outstanding
cancellation requests, and if so, we must fail the operation with an
error (-EDEADLK) since cancellation is synchronous and waits for the
callback to finish executing. This implies that we can enter a deadlock
situation involving two or more timer callbacks executing in parallel
and attempting to cancel one another.

Note that we avoid incrementing the cancelling counter for the target
timer (the one being cancelled) if bpf_timer_cancel is not invoked from
a callback, to avoid spurious errors. The whole point of detecting
cur->cancelling and returning -EDEADLK is to not enter a busy wait loop
(which may or may not lead to a lockup). This does not apply in case the
caller is in a non-callback context, the other side can continue to
cancel as it sees fit without running into errors.

Background on prior attempts:

Earlier versions of this patch used a bool 'cancelling' bit and used the
following pattern under timer->lock to publish cancellation status.

lock(t->lock);
t->cancelling = true;
mb();
if (cur->cancelling)
	return -EDEADLK;
unlock(t->lock);
hrtimer_cancel(t->timer);
t->cancelling = false;

The store outside the critical section could overwrite a parallel
requests t->cancelling assignment to true, to ensure the parallely
executing callback observes its cancellation status.

It would be necessary to clear this cancelling bit once hrtimer_cancel
is done, but lack of serialization introduced races. Another option was
explored where bpf_timer_start would clear the bit when (re)starting the
timer under timer->lock. This would ensure serialized access to the
cancelling bit, but may allow it to be cleared before in-flight
hrtimer_cancel has finished executing, such that lockups can occur
again.

Thus, we choose an atomic counter to keep track of all outstanding
cancellation requests and use it to prevent lockups in case callbacks
attempt to cancel each other while executing in parallel.

Reported-by: Dohyun Kim <dohyunkim@google.com>
Reported-by: Neel Natu <neelnatu@google.com>
Fixes: b00628b1c7d5 ("bpf: Introduce bpf timers.")
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 kernel/bpf/helpers.c | 38 +++++++++++++++++++++++++++++++++++---
 1 file changed, 35 insertions(+), 3 deletions(-)

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 2a69a9a36c0f..22e779ca50d5 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -1107,6 +1107,7 @@ struct bpf_async_cb {
 struct bpf_hrtimer {
 	struct bpf_async_cb cb;
 	struct hrtimer timer;
+	atomic_t cancelling;
 };
 
 struct bpf_work {
@@ -1262,6 +1263,7 @@ static int __bpf_async_init(struct bpf_async_kern *async, struct bpf_map *map, u
 		clockid = flags & (MAX_CLOCKS - 1);
 		t = (struct bpf_hrtimer *)cb;
 
+		atomic_set(&t->cancelling, 0);
 		hrtimer_init(&t->timer, clockid, HRTIMER_MODE_REL_SOFT);
 		t->timer.function = bpf_timer_cb;
 		cb->value = (void *)async - map->record->timer_off;
@@ -1440,7 +1442,8 @@ static void drop_prog_refcnt(struct bpf_async_cb *async)
 
 BPF_CALL_1(bpf_timer_cancel, struct bpf_async_kern *, timer)
 {
-	struct bpf_hrtimer *t;
+	struct bpf_hrtimer *t, *cur_t;
+	bool inc = false;
 	int ret = 0;
 
 	if (in_nmi())
@@ -1452,14 +1455,41 @@ BPF_CALL_1(bpf_timer_cancel, struct bpf_async_kern *, timer)
 		ret = -EINVAL;
 		goto out;
 	}
-	if (this_cpu_read(hrtimer_running) == t) {
+
+	cur_t = this_cpu_read(hrtimer_running);
+	if (cur_t == t) {
 		/* If bpf callback_fn is trying to bpf_timer_cancel()
 		 * its own timer the hrtimer_cancel() will deadlock
-		 * since it waits for callback_fn to finish
+		 * since it waits for callback_fn to finish.
+		 */
+		ret = -EDEADLK;
+		goto out;
+	}
+
+	/* Only account in-flight cancellations when invoked from a timer
+	 * callback, since we want to avoid waiting only if other _callbacks_
+	 * are waiting on us, to avoid introducing lockups. Non-callback paths
+	 * are ok, since nobody would synchronously wait for their completion.
+	 */
+	if (!cur_t)
+		goto drop;
+	atomic_inc(&t->cancelling);
+	/* Need full barrier after relaxed atomic_inc */
+	smp_mb__after_atomic();
+	inc = true;
+	if (atomic_read(&cur_t->cancelling)) {
+		/* We're cancelling timer t, while some other timer callback is
+		 * attempting to cancel us. In such a case, it might be possible
+		 * that timer t belongs to the other callback, or some other
+		 * callback waiting upon it (creating transitive dependencies
+		 * upon us), and we will enter a deadlock if we continue
+		 * cancelling and waiting for it synchronously, since it might
+		 * do the same. Bail!
 		 */
 		ret = -EDEADLK;
 		goto out;
 	}
+drop:
 	drop_prog_refcnt(&t->cb);
 out:
 	__bpf_spin_unlock_irqrestore(&timer->lock);
@@ -1467,6 +1497,8 @@ BPF_CALL_1(bpf_timer_cancel, struct bpf_async_kern *, timer)
 	 * if it was running.
 	 */
 	ret = ret ?: hrtimer_cancel(&t->timer);
+	if (inc)
+		atomic_dec(&t->cancelling);
 	rcu_read_unlock();
 	return ret;
 }
-- 
2.43.0

[PATCH bpf v1 2/3] bpf: Defer work in bpf_timer_cancel_and_free

[Kumar Kartikeya Dwivedi]

Currently, the same case as previous patch (two timer callbacks trying
to cancel each other) can be invoked through bpf_map_update_elem as
well, or more precisely, freeing map elements containing timers. Since
this relies on hrtimer_cancel as well, it is prone to the same deadlock
situation as the previous patch.

It would be sufficient to use hrtimer_try_to_cancel to fix this problem,
as the timer cannot be enqueued after async_cancel_and_free. Once
async_cancel_and_free has been done, the timer must be reinitialized
before it can be armed again. The callback running in parallel trying to
arm the timer will fail, and freeing bpf_hrtimer without waiting is
sufficient (given kfree_rcu), and bpf_timer_cb will return
HRTIMER_NORESTART, preventing the timer from being rearmed again.

However, there exists a UAF scenario where the callback arms the timer
before entering this function, such that if cancellation fails (due to
timer callback invoking this routine, or the target timer callback
running concurrently). In such a case, if the timer expiration is
significantly far in the future, the RCU grace period expiration
happening before it will free the bpf_hrtimer state and along with it
the struct hrtimer, that is enqueued.

Hence, it is clear cancellation needs to occur after
async_cancel_and_free, and yet it cannot be done inline due to deadlock
issues. We thus modify bpf_timer_cancel_and_free to defer work to the
global workqueue, adding a work_struct alongside rcu_head (both used at
_different_ points of time, so can share space).

Update existing code comments to reflect the new state of affairs.

Fixes: b00628b1c7d5 ("bpf: Introduce bpf timers.")
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 kernel/bpf/helpers.c | 61 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 47 insertions(+), 14 deletions(-)

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 22e779ca50d5..3243c83ef3e3 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -1084,7 +1084,10 @@ struct bpf_async_cb {
 	struct bpf_prog *prog;
 	void __rcu *callback_fn;
 	void *value;
-	struct rcu_head rcu;
+	union {
+		struct rcu_head rcu;
+		struct work_struct delete_work;
+	};
 	u64 flags;
 };
 
@@ -1220,6 +1223,21 @@ static void bpf_wq_delete_work(struct work_struct *work)
 	kfree_rcu(w, cb.rcu);
 }
 
+static void bpf_timer_delete_work(struct work_struct *work)
+{
+	struct bpf_hrtimer *t = container_of(work, struct bpf_hrtimer, cb.delete_work);
+
+	/* Cancel the timer and wait for callback to complete if it was running.
+	 * If hrtimer_cancel() can be safely called it's safe to call
+	 * kfree_rcu(t) right after for both preallocated and non-preallocated
+	 * maps.  The async->cb = NULL was already done and no code path can see
+	 * address 't' anymore. Timer if armed for existing bpf_hrtimer before
+	 * bpf_timer_cancel_and_free will have been cancelled.
+	 */
+	hrtimer_cancel(&t->timer);
+	kfree_rcu(t, cb.rcu);
+}
+
 static int __bpf_async_init(struct bpf_async_kern *async, struct bpf_map *map, u64 flags,
 			    enum bpf_async_type type)
 {
@@ -1264,6 +1282,7 @@ static int __bpf_async_init(struct bpf_async_kern *async, struct bpf_map *map, u
 		t = (struct bpf_hrtimer *)cb;
 
 		atomic_set(&t->cancelling, 0);
+		INIT_WORK(&t->cb.delete_work, bpf_timer_delete_work);
 		hrtimer_init(&t->timer, clockid, HRTIMER_MODE_REL_SOFT);
 		t->timer.function = bpf_timer_cb;
 		cb->value = (void *)async - map->record->timer_off;
@@ -1544,25 +1563,39 @@ void bpf_timer_cancel_and_free(void *val)
 
 	if (!t)
 		return;
-	/* Cancel the timer and wait for callback to complete if it was running.
-	 * If hrtimer_cancel() can be safely called it's safe to call kfree(t)
-	 * right after for both preallocated and non-preallocated maps.
-	 * The async->cb = NULL was already done and no code path can
-	 * see address 't' anymore.
-	 *
-	 * Check that bpf_map_delete/update_elem() wasn't called from timer
-	 * callback_fn. In such case don't call hrtimer_cancel() (since it will
-	 * deadlock) and don't call hrtimer_try_to_cancel() (since it will just
-	 * return -1). Though callback_fn is still running on this cpu it's
+	/* We check that bpf_map_delete/update_elem() was called from timer
+	 * callback_fn. In such case we don't call hrtimer_cancel() (since it
+	 * will deadlock) and don't call hrtimer_try_to_cancel() (since it will
+	 * just return -1). Though callback_fn is still running on this cpu it's
 	 * safe to do kfree(t) because bpf_timer_cb() read everything it needed
 	 * from 't'. The bpf subprog callback_fn won't be able to access 't',
 	 * since async->cb = NULL was already done. The timer will be
 	 * effectively cancelled because bpf_timer_cb() will return
 	 * HRTIMER_NORESTART.
+	 *
+	 * However, it is possible the timer callback_fn calling us armed the
+	 * timer _before_ calling us, such that failing to cancel it here will
+	 * cause it to possibly use struct hrtimer after freeing bpf_hrtimer.
+	 * Therefore, we _need_ to cancel any outstanding timers before we do
+	 * kfree_rcu, even though no more timers can be armed.
+	 *
+	 * Moreover, we need to schedule work even if timer does not belong to
+	 * the calling callback_fn, as on two different CPUs, we can end up in a
+	 * situation where both sides run in parallel, try to cancel one
+	 * another, and we end up waiting on both sides in hrtimer_cancel
+	 * without making forward progress, since timer1 depends on time2
+	 * callback to finish, and vice versa.
+	 *
+	 *  CPU 1 (timer1_cb)			CPU 2 (timer2_cb)
+	 *  bpf_timer_cancel_and_free(timer2)	bpf_timer_cancel_and_free(timer1)
+	 *
+	 * To avoid these issues, punt to workqueue context when we are in a
+	 * timer callback.
 	 */
-	if (this_cpu_read(hrtimer_running) != t)
-		hrtimer_cancel(&t->timer);
-	kfree_rcu(t, cb.rcu);
+	if (this_cpu_read(hrtimer_running))
+		queue_work(system_unbound_wq, &t->cb.delete_work);
+	else
+		bpf_timer_delete_work(&t->cb.delete_work);
 }
 
 /* This function is called by map_delete/update_elem for individual element and
-- 
2.43.0

[PATCH bpf v1 3/3] selftests/bpf: Add timer lockup selftest

[Kumar Kartikeya Dwivedi]

Add a selftest that tries to trigger a situation where two timer
callbacks are attempting to cancel each other's timer. By running them
continuously, we hit a condition where both run in parallel and cancel
each other. Without the fix in the previous patch, this would cause a
lockup as hrtimer_cancel on either side will wait for forward progress
from the callback.

Ensure that this situation leads to a EDEADLK error.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 .../selftests/bpf/prog_tests/timer_lockup.c   | 65 ++++++++++++++
 .../selftests/bpf/progs/timer_lockup.c        | 85 +++++++++++++++++++
 2 files changed, 150 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/timer_lockup.c
 create mode 100644 tools/testing/selftests/bpf/progs/timer_lockup.c

diff --git a/tools/testing/selftests/bpf/prog_tests/timer_lockup.c b/tools/testing/selftests/bpf/prog_tests/timer_lockup.c
new file mode 100644
index 000000000000..73e376fc5bbd
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/timer_lockup.c
@@ -0,0 +1,65 @@
+// SPDX-License-Identifier: GPL-2.0
+#define _GNU_SOURCE
+#include <sched.h>
+#include <test_progs.h>
+#include <pthread.h>
+#include <network_helpers.h>
+#include "timer_lockup.skel.h"
+
+long cpu;
+int *timer1_err;
+int *timer2_err;
+
+static void *timer_lockup_thread(void *arg)
+{
+	LIBBPF_OPTS(bpf_test_run_opts, opts,
+		.data_in = &pkt_v4,
+		.data_size_in = sizeof(pkt_v4),
+		.repeat = 10000,
+	);
+	int prog_fd = *(int *)arg;
+	cpu_set_t cpuset;
+
+	CPU_ZERO(&cpuset);
+	CPU_SET(__sync_fetch_and_add(&cpu, 1), &cpuset);
+	ASSERT_OK(pthread_setaffinity_np(pthread_self(), sizeof(cpuset), &cpuset), "cpu affinity");
+
+	while (!*timer1_err && !*timer2_err)
+		bpf_prog_test_run_opts(prog_fd, &opts);
+
+	return NULL;
+}
+
+void test_timer_lockup(void)
+{
+	struct timer_lockup *skel;
+	pthread_t thrds[2];
+	void *ret;
+
+	skel = timer_lockup__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "timer_lockup__open_and_load"))
+		return;
+
+	int timer1_prog = bpf_program__fd(skel->progs.timer1_prog);
+	int timer2_prog = bpf_program__fd(skel->progs.timer2_prog);
+
+	timer1_err = &skel->bss->timer1_err;
+	timer2_err = &skel->bss->timer2_err;
+
+	if (!ASSERT_OK(pthread_create(&thrds[0], NULL, timer_lockup_thread, &timer1_prog), "pthread_create thread1"))
+		return;
+	if (!ASSERT_OK(pthread_create(&thrds[1], NULL, timer_lockup_thread, &timer2_prog), "pthread_create thread2")) {
+		pthread_exit(&thrds[0]);
+		return;
+	}
+
+	pthread_join(thrds[1], &ret);
+	pthread_join(thrds[0], &ret);
+
+	if (*timer1_err != -EDEADLK && *timer1_err != 0)
+		ASSERT_FAIL("timer1_err bad value");
+	if (*timer2_err != -EDEADLK && *timer2_err != 0)
+		ASSERT_FAIL("timer2_err bad value");
+
+	return;
+}
diff --git a/tools/testing/selftests/bpf/progs/timer_lockup.c b/tools/testing/selftests/bpf/progs/timer_lockup.c
new file mode 100644
index 000000000000..ca29da9ff25c
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/timer_lockup.c
@@ -0,0 +1,85 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/bpf.h>
+#include <time.h>
+#include <errno.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include "bpf_misc.h"
+
+char _license[] SEC("license") = "GPL";
+
+struct elem {
+	struct bpf_timer t;
+};
+
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__uint(max_entries, 1);
+	__type(key, int);
+	__type(value, struct elem);
+} timer1_map SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__uint(max_entries, 1);
+	__type(key, int);
+	__type(value, struct elem);
+} timer2_map SEC(".maps");
+
+int timer1_err;
+int timer2_err;
+
+static int timer_cb1(void *map, int *k, struct bpf_timer *timer)
+{
+	int key = 0;
+
+	timer = bpf_map_lookup_elem(&timer2_map, &key);
+	if (timer) {
+		timer2_err = bpf_timer_cancel(timer);
+	}
+	return 0;
+}
+
+static int timer_cb2(void *map, int *k, struct bpf_timer *timer)
+{
+	int key = 0;
+
+	timer = bpf_map_lookup_elem(&timer1_map, &key);
+	if (timer) {
+		timer1_err = bpf_timer_cancel(timer);
+	}
+	return 0;
+}
+
+SEC("tc")
+int timer1_prog(void *ctx)
+{
+	int key = 0;
+	struct bpf_timer *timer;
+
+	timer = bpf_map_lookup_elem(&timer1_map, &key);
+	if (timer) {
+		bpf_timer_init(timer, &timer1_map, CLOCK_BOOTTIME);
+		bpf_timer_set_callback(timer, timer_cb1);
+		bpf_timer_start(timer, 1, BPF_F_TIMER_CPU_PIN);
+	}
+
+	return 0;
+}
+
+SEC("tc")
+int timer2_prog(void *ctx)
+{
+	int key = 0;
+	struct bpf_timer *timer;
+
+	timer = bpf_map_lookup_elem(&timer2_map, &key);
+	if (timer) {
+		bpf_timer_init(timer, &timer2_map, CLOCK_BOOTTIME);
+		bpf_timer_set_callback(timer, timer_cb2);
+		bpf_timer_start(timer, 1, BPF_F_TIMER_CPU_PIN);
+	}
+
+	return 0;
+}
-- 
2.43.0

Re: [PATCH bpf v1 3/3] selftests/bpf: Add timer lockup selftest

[Kumar Kartikeya Dwivedi]

On Tue, 9 Jul 2024 at 20:54, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
>
> Add a selftest that tries to trigger a situation where two timer
> callbacks are attempting to cancel each other's timer. By running them
> continuously, we hit a condition where both run in parallel and cancel
> each other. Without the fix in the previous patch, this would cause a
> lockup as hrtimer_cancel on either side will wait for forward progress
> from the callback.
>
> Ensure that this situation leads to a EDEADLK error.
>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
>  .../selftests/bpf/prog_tests/timer_lockup.c   | 65 ++++++++++++++
>  .../selftests/bpf/progs/timer_lockup.c        | 85 +++++++++++++++++++
>  2 files changed, 150 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/timer_lockup.c
>  create mode 100644 tools/testing/selftests/bpf/progs/timer_lockup.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/timer_lockup.c b/tools/testing/selftests/bpf/prog_tests/timer_lockup.c
> new file mode 100644
> index 000000000000..73e376fc5bbd
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/timer_lockup.c
> @@ -0,0 +1,65 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#define _GNU_SOURCE
> +#include <sched.h>
> +#include <test_progs.h>
> +#include <pthread.h>
> +#include <network_helpers.h>
> +#include "timer_lockup.skel.h"
> +
> +long cpu;
> +int *timer1_err;
> +int *timer2_err;
> +
> +static void *timer_lockup_thread(void *arg)
> +{
> +       LIBBPF_OPTS(bpf_test_run_opts, opts,
> +               .data_in = &pkt_v4,
> +               .data_size_in = sizeof(pkt_v4),
> +               .repeat = 10000,
> +       );
> +       int prog_fd = *(int *)arg;
> +       cpu_set_t cpuset;
> +
> +       CPU_ZERO(&cpuset);
> +       CPU_SET(__sync_fetch_and_add(&cpu, 1), &cpuset);
> +       ASSERT_OK(pthread_setaffinity_np(pthread_self(), sizeof(cpuset), &cpuset), "cpu affinity");
> +
> +       while (!*timer1_err && !*timer2_err)
> +               bpf_prog_test_run_opts(prog_fd, &opts);
> +
> +       return NULL;
> +}
> +
> +void test_timer_lockup(void)
> +{
> +       struct timer_lockup *skel;
> +       pthread_t thrds[2];
> +       void *ret;
> +
> +       skel = timer_lockup__open_and_load();
> +       if (!ASSERT_OK_PTR(skel, "timer_lockup__open_and_load"))
> +               return;
> +
> +       int timer1_prog = bpf_program__fd(skel->progs.timer1_prog);
> +       int timer2_prog = bpf_program__fd(skel->progs.timer2_prog);
> +
> +       timer1_err = &skel->bss->timer1_err;
> +       timer2_err = &skel->bss->timer2_err;
> +
> +       if (!ASSERT_OK(pthread_create(&thrds[0], NULL, timer_lockup_thread, &timer1_prog), "pthread_create thread1"))
> +               return;
> +       if (!ASSERT_OK(pthread_create(&thrds[1], NULL, timer_lockup_thread, &timer2_prog), "pthread_create thread2")) {
> +               pthread_exit(&thrds[0]);
> +               return;

A goto out: timer_lockup___destroy(skel) is missing here and above
this. Will wait for a day or so before respinning.

> [...]

Re: [PATCH bpf v1 3/3] selftests/bpf: Add timer lockup selftest

[Alexei Starovoitov]

On Tue, Jul 9, 2024 at 2:07 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> > +       while (!*timer1_err && !*timer2_err)
> > +               bpf_prog_test_run_opts(prog_fd, &opts);
> > +
> > +       return NULL;
> > +}
> > +
> > +void test_timer_lockup(void)
> > +{
> > +       struct timer_lockup *skel;
> > +       pthread_t thrds[2];
> > +       void *ret;
> > +
> > +       skel = timer_lockup__open_and_load();
> > +       if (!ASSERT_OK_PTR(skel, "timer_lockup__open_and_load"))
> > +               return;
> > +
> > +       int timer1_prog = bpf_program__fd(skel->progs.timer1_prog);
> > +       int timer2_prog = bpf_program__fd(skel->progs.timer2_prog);

Pls don't declare inline. Some compiler might warn.

> > +
> > +       timer1_err = &skel->bss->timer1_err;
> > +       timer2_err = &skel->bss->timer2_err;
> > +
> > +       if (!ASSERT_OK(pthread_create(&thrds[0], NULL, timer_lockup_thread, &timer1_prog), "pthread_create thread1"))

pls shorten the line.

> > +               return;
> > +       if (!ASSERT_OK(pthread_create(&thrds[1], NULL, timer_lockup_thread, &timer2_prog), "pthread_create thread2")) {
> > +               pthread_exit(&thrds[0]);
> > +               return;
>
> A goto out: timer_lockup___destroy(skel) is missing here and above
> this. Will wait for a day or so before respinning.

I was thinking to fix all these up while applying.
So I fixed it, but then noticed that the new test is quite flaky.
It seems it can get stuck in that while() loop forever.
Pls investigate.

So I applied the first two patches only.

Also pls fix:
+static int timer_cb2(void *map, int *k, struct bpf_timer *timer)
+{
+       int key = 0;
+
+       timer = bpf_map_lookup_elem(&timer1_map, &key);

1. no need to do a lookup.
2. the 3rd arg is not a bpf_timer. It's a pointer to map value.
So use
timer_cb2(void *map, int *k, struct elem *v)
then cast it to bpf_timer and use it w/o lookup.

Re: [PATCH bpf v1 3/3] selftests/bpf: Add timer lockup selftest

[Kumar Kartikeya Dwivedi]

On Thu, 11 Jul 2024 at 01:28, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Jul 9, 2024 at 2:07 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> > > +       while (!*timer1_err && !*timer2_err)
> > > +               bpf_prog_test_run_opts(prog_fd, &opts);
> > > +
> > > +       return NULL;
> > > +}
> > > +
> > > +void test_timer_lockup(void)
> > > +{
> > > +       struct timer_lockup *skel;
> > > +       pthread_t thrds[2];
> > > +       void *ret;
> > > +
> > > +       skel = timer_lockup__open_and_load();
> > > +       if (!ASSERT_OK_PTR(skel, "timer_lockup__open_and_load"))
> > > +               return;
> > > +
> > > +       int timer1_prog = bpf_program__fd(skel->progs.timer1_prog);
> > > +       int timer2_prog = bpf_program__fd(skel->progs.timer2_prog);
>
> Pls don't declare inline. Some compiler might warn.
>

ack.

> > > +
> > > +       timer1_err = &skel->bss->timer1_err;
> > > +       timer2_err = &skel->bss->timer2_err;
> > > +
> > > +       if (!ASSERT_OK(pthread_create(&thrds[0], NULL, timer_lockup_thread, &timer1_prog), "pthread_create thread1"))
>
> pls shorten the line.
>

ack.

> > > +               return;
> > > +       if (!ASSERT_OK(pthread_create(&thrds[1], NULL, timer_lockup_thread, &timer2_prog), "pthread_create thread2")) {
> > > +               pthread_exit(&thrds[0]);
> > > +               return;
> >
> > A goto out: timer_lockup___destroy(skel) is missing here and above
> > this. Will wait for a day or so before respinning.
>
> I was thinking to fix all these up while applying.
> So I fixed it, but then noticed that the new test is quite flaky.
> It seems it can get stuck in that while() loop forever.
> Pls investigate.
>

Will do.

> So I applied the first two patches only.
>
> Also pls fix:
> +static int timer_cb2(void *map, int *k, struct bpf_timer *timer)
> +{
> +       int key = 0;
> +
> +       timer = bpf_map_lookup_elem(&timer1_map, &key);
>
> 1. no need to do a lookup.
> 2. the 3rd arg is not a bpf_timer. It's a pointer to map value.
> So use
> timer_cb2(void *map, int *k, struct elem *v)
> then cast it to bpf_timer and use it w/o lookup.

2 makes sense, will fix. Lookup is still needed. We need the timer
from timer1_map, while the callback gets timer of timer2_map.

Re: [PATCH bpf v1 0/3] Fixes for BPF timer lockup and UAF

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Tue,  9 Jul 2024 18:54:37 +0000 you wrote:
> The following patches contain fixes for timer lockups and a
> use-after-free scenario.
> 
> This set proposes to fix the following lockup situation for BPF timers.
> 
> CPU 1					CPU 2
> 
> [...]

Here is the summary with links:
  - [bpf,v1,1/3] bpf: Fail bpf_timer_cancel when callback is being cancelled
    https://git.kernel.org/bpf/bpf/c/d4523831f07a
  - [bpf,v1,2/3] bpf: Defer work in bpf_timer_cancel_and_free
    https://git.kernel.org/bpf/bpf/c/a6fcd19d7eac
  - [bpf,v1,3/3] selftests/bpf: Add timer lockup selftest
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html