[PATCH bpf-next v2 00/43] bpf: Memory safety, lifetime and locking fixes

[PATCH bpf-next v2 00/43] bpf: Memory safety, lifetime and locking fixes

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

This series collects fixes for bugs in the BPF core, the verifier and the
BPF side of the networking stack. Most of them are from Josef.

Every bug was reproduced in a VM on top of bpf-next, on the kernel right
before and right after the patch that fixes it. The splats are in the
commit logs. That goes for what the reviews of v1 have found as well.

Sleeping, or doing without RCU, where that is not allowed:
  02 hash map for_each callbacks of sleepable programs
  05 sleepable programs on the task_vma iterator
  06 bpf_find_vma() callbacks of sleepable programs
  24 maps that rely on RCU in bpf_wq and bpf_task_work callbacks
  32 extensions of functions that run sleepable

Buffers that iterators and writable tracepoints hand to programs:
  19 BPF_LINK_UPDATE skips the size check of the attach
  25 tail calls from an iterator program
  26 extensions of an iterator program
  27 tail calls from a writable raw tracepoint program
  28 extensions of a writable raw tracepoint program
  29 writes over bpf_timer, bpf_spin_lock, kptr in a live map value

The sk_storage map iterator:
  14 a socket that sk_clone() is still building can be found and used
  15 NULL dereference if the socket goes away
  16 an element of a storage that was never published can be found
  17 the socket is not held while the program runs

Lifetime of local storage, programs and maps:
  10 owner_refcnt saturates on the last unpin
  30 map_free() frees elements under sleepable readers
  31 values of a dead owner are freed under sleepable programs
  22 a program is freed under its bpf_wq callback
  23 an inner map is freed under a sleepable callback
  33 the exception table of a program that still runs is not found

XDP and devices:
  01 devmap egress programs free page_pool fragments as plain pages
  20 stale offload->prog after bpf_prog_realloc()
  21 a program is bound to a netdev that is being unregistered
  35 cpumap uses the device of a queued frame after it is gone

seg6local End.BPF and bpf_skb_adjust_room():
  11 the SRH of the inner packet is not validated after End.DT6
  12 the inner header is not pulled for End.DT6
  13 stale SRH pointer if End.B6 fails late
  08 gso_size can drop to 0 or wrap around

skb dynptrs, where a write can reallocate the head of the skb:
  36 kfuncs that write through a dynptr
  37 bpf_dynptr_write() in a global function
  38 a dynptr passed to a global function
  39 extensions get the unclone prologue without an skb in R1
  40 kfuncs that write through a dynptr do not unclone the skb
  41 the source of the write is a slice of the same skb
  42 overlapping source and destination
  43 slices as arguments of helpers that change the packet

The rest:
  03 inode leak if BPF_TOKEN_CREATE fails
  04 preallocated hash map hands out an element that is still linked
  07 BPF_JSET with a 64 bit mask, 32-bit kernels only
  09 probe names of a perf event are read without event_mutex
  18 unbounded recursion in the BTF show code
  34 BPF_PROG_TEST_RUN of sk_lookup uses the socket outside of RCU

Some of the fixes make the kernel refuse what it has accepted so far:

 - 02, 06: helpers, kfuncs and global functions that may sleep are rejected
   in these callbacks.
 - 05: sleepable programs cannot be attached to the task_vma iterator.
 - 24: in sleepable callbacks, and for inner maps in sleepable programs,
   maps that rely on RCU need bpf_rcu_read_lock().
 - 25, 27: a program with a tail call cannot be attached to a map element
   iterator or to a writable tracepoint.
 - 29: an iterator program that writes over a special field cannot be
   attached to that map.
 - 36-38, 43: packet pointers and slices are invalidated in more places.
   That hits some programs which are harmless at run time. A program that
   keeps a packet pointer across a global function which only writes to a
   ringbuf dynptr is one example.
 - 04: a nested update of a preallocated hash map on the same CPU can fail
   with -EBUSY.
 - 08: bpf_skb_adjust_room() returns -ENOTSUPP if gso_size would drop below
   TCP_MIN_GSO_SIZE.
 - 28: writable raw tracepoint programs cannot be extended.
 - 32: the main function of a sleepable program, and a global function that
   is called without the RCU read lock, cannot be extended. The
   summarization selftest is adjusted.
 - 39: an extension that writes to packet data has to get the context as
   its first argument, and scalars beside it.

test_progs in its three flavors, test_maps and test_verifier show no new
failures. cpumask/test_insert_leave is flaky when the tests run in
parallel, with and without the series: its program does not check
is_test_task(), so any other fork in the system makes it fail.

veristat sees no difference for the 5689 programs of the selftests, nor for
the 466 programs of the sched_ext schedulers that BPF CI builds: same
verdicts, insns, states and stack depth.

v1 -> v2, all of it after the reviews by sashiko and the BPF CI bot:
 - rebased
 - 04: cancel the special fields before the element goes into the slot
 - 08: leave at least TCP_MIN_GSO_SIZE
 - 16: new. The check in 17 did not hold for a storage that lost the race
   in bpf_local_storage_alloc().
 - 20: update offload->prog under bpf_devs_lock, in
   bpf_jit_prog_release_other() as well
 - 24: a global function that is called with preemption or interrupts
   disabled got around the check
 - 28, 32: new. Extensions got around 27 and 31.
 - 31: leave the special fields to the RCU Tasks Trace callback too
 - 39, 40: new. The kfuncs of 36 wrote to cloned skbs. 39 is needed for
   40 not to give the prologue to extensions that cannot take it.
 - 41: make the skb writable once
 - 18: no change. bpf_snprintf_btf() and bpf_seq_printf_btf() only take
   vmlinux BTF, and the map read in bpffs is BTF_SHOW_UNSAFE.

v1: https://lore.kernel.org/bpf/20261017012058.355787-1-alexei.starovoitov@gmail.com/

Alexei Starovoitov (6):
  bpf: Clear sk_refcnt of a clone before its storage is linked
  bpf: Link the first element after its storage is published
  bpf: Don't take an extension for a writable raw tracepoint program
  bpf: Don't take an extension for a function that runs sleepable
  bpf: Refuse the prologue for an extension that cannot take it
  bpf: Unclone the skb for kfuncs that write through a dynptr

Josef Bacik (37):
  bpf: use the frame mem type in devmap egress XDP
  bpf: forbid sleeping in hash map for_each callbacks
  bpf: don't leak the inode when token creation fails
  bpf: don't hand out a linked element as extra element
  bpf: reject sleepable programs for task_vma iterator
  bpf: forbid sleeping in bpf_find_vma() callbacks
  bpf: look at all 64 bits of a BPF_JSET mask on 32-bit
  bpf: don't let bpf_skb_adjust_room() use up the MSS
  bpf: Read the probe of a perf event under event_mutex
  bpf: Don't saturate owner_refcnt on the last unpin
  bpf: Validate the SRH which End.DT6 uncovers in End.BPF
  bpf: Pull the inner header for End.DT6 in End.BPF
  bpf: Update the SRH state if End.B6 fails late
  bpf: Fix NULL deref in the sk_storage map iterator
  bpf: Hold the socket in the sk_storage map iterator
  bpf: Bound the nesting that the BTF show code follows
  bpf: Check the new program of an iterator link update
  bpf: Fix stale offload->prog pointer after realloc
  bpf: Don't bind a program to a netdev that goes away
  bpf: Keep a program until its bpf_wq callback has ended
  bpf: Count sleepable callbacks as sleepable map users
  bpf: Check the maps that sleepable callbacks use
  bpf: Assume no bounds for buffers behind a tail call
  bpf: Don't take an extension for an iterator program
  bpf: Assume no bounds for a writable tp with tail calls
  bpf: Keep iterator programs off special map fields
  bpf: Free what map_free() unlinks for sleepable readers
  bpf: Keep values of a dead owner for sleepable programs
  bpf: Keep the symbols of a program until it has ended
  bpf: Use the socket of a sk_lookup test run under RCU
  bpf, cpumap: Hold the device of frames in the queue
  bpf: Drop packet pointers behind kfuncs writing an skb
  bpf: Let bpf_dynptr_write() mark a global function
  bpf: Don't take a dynptr argument for a local dynptr
  bpf: Follow the head if an skb is written from a slice
  bpf: Use memmove() to store bytes in an skb
  bpf: Refuse packet slices for packet-changing helpers

 include/linux/bpf.h                           |  43 ++-
 include/linux/bpf_local_storage.h             |  13 +
 include/linux/bpf_verifier.h                  |   9 +
 kernel/bpf/bpf_iter.c                         |  31 +-
 kernel/bpf/bpf_local_storage.c                |  50 +++-
 kernel/bpf/btf.c                              |  14 +-
 kernel/bpf/cfg.c                              |   4 +-
 kernel/bpf/core.c                             |  56 ++--
 kernel/bpf/cpumap.c                           |  35 ++-
 kernel/bpf/devmap.c                           |   1 +
 kernel/bpf/fixups.c                           |  30 ++
 kernel/bpf/hashtab.c                          |  22 +-
 kernel/bpf/helpers.c                          |   9 +-
 kernel/bpf/map_in_map.c                       |   7 +-
 kernel/bpf/map_iter.c                         |  79 +++--
 kernel/bpf/offload.c                          |  25 +-
 kernel/bpf/states.c                           |   3 +
 kernel/bpf/syscall.c                          |  28 +-
 kernel/bpf/task_iter.c                        |  12 +-
 kernel/bpf/token.c                            |   5 +-
 kernel/bpf/verifier.c                         | 269 +++++++++++++++---
 kernel/trace/bpf_trace.c                      |  13 +
 net/bpf/test_run.c                            |  19 +-
 net/core/bpf_sk_storage.c                     |  96 ++++++-
 net/core/filter.c                             |  58 +++-
 net/core/sock_map.c                           |  17 +-
 .../selftests/bpf/prog_tests/summarization.c  |  21 +-
 27 files changed, 781 insertions(+), 188 deletions(-)


base-commit: 458575e1f2ddea944d679fe778c92e370a9b61a4
-- 
2.55.0

[PATCH bpf-next v2 01/43] bpf: use the frame mem type in devmap egress XDP

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

A devmap egress program that removes a whole fragment of a redirected
xdp_frame with bpf_xdp_adjust_tail() or bpf_xdp_pull_data() frees it
with page_frag_free() whatever the frame's memory type is. For a
MEM_TYPE_PAGE_POOL frame from a page_pool based driver this drops the
page reference owned by the pool while the pool still accounts the
fragment ("page_pool leak" bad page, leaked pp reference).

kernel/bpf/devmap.c:dev_map_bpf_prog_run() points xdp.rxq at an
on-stack

    struct xdp_rxq_info rxq = { .dev = rx_dev };

whose mem.type is 0, i.e. MEM_TYPE_PAGE_SHARED, and
net/core/filter.c:bpf_xdp_shrink_data() releases frags according to
rxq->mem.type. cpumap, which runs programs on xdp_frames the same way,
already copies xdpf->mem_type into its private rxq for each frame.

With 9000 byte frames from virtio_net and an egress program that cuts the
fragments off:

  BUG: Bad page state in process swapper/3  pfn:10660b
  page dumped because: page_pool leak
   bad_page+0xbd/0x190
   __free_frozen_pages+0x5c7/0x900
   bpf_xdp_adjust_tail+0x3be/0x480
   bpf_prog_dad43f894a3faff7_egress+0x15c/0x1cd
   dev_map_bpf_prog_run+0x288/0x610

The pool waits for these pages for ever when its device is gone. That is
the harmless case. A page of the pool can hold more than one fragment
while its refcount is 1: the first page_frag_free() frees it under the
fragment which is still in use, and the second one runs into

  page dumped because: VM_BUG_ON_PAGE(page_ref_count(page) == 0)
  kernel BUG at include/linux/mm.h:1846!

veth is not affected. It takes references of its own on the pages and
redirects the frame as MEM_TYPE_PAGE_SHARED.

Fix by setting rxq.mem.type from each frame before running the devmap
program, as cpu_map_bpf_prog_run_xdp() does.

Fixes: ca9984c5f0ab ("bpf: devmap: provide rxq after redirect")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/devmap.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
index dc7b859e8bbf..1b8f0d7f8455 100644
--- a/kernel/bpf/devmap.c
+++ b/kernel/bpf/devmap.c
@@ -350,6 +350,7 @@ static int dev_map_bpf_prog_run(struct bpf_prog *xdp_prog,
 
 		xdp_convert_frame_to_buff(xdpf, &xdp);
 		xdp.txq = &txq;
+		rxq.mem.type = xdpf->mem_type;
 		xdp.rxq = &rxq;
 
 		act = bpf_prog_run_xdp(xdp_prog, &xdp);
-- 
2.55.0

[PATCH bpf-next v2 02/43] bpf: forbid sleeping in hash map for_each callbacks

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

A sleepable program (e.g. SEC("syscall"), lsm.s, fentry.s) can pass a
callback to bpf_for_each_map_elem() on a hash map and call a sleepable
helper or kfunc such as bpf_copy_from_user() from it. The verifier
accepts this, but at run time the callback is invoked from inside an RCU
read-side critical section:

    bpf_for_each_hash_elem()
      rcu_read_lock();
      callback_fn(map, key, val, ctx, 0);
        bpf_copy_from_user()       <- may fault and sleep
      rcu_read_unlock();

bpf_each_rhash_elem() does the same for BPF_MAP_TYPE_RHASH. Sleeping
there triggers the "Voluntary context switch within RCU read-side
critical section!" warning, delays grace periods, and with
CONFIG_PREEMPT_RCU=n silently ends the read-side critical section that
the bucket walk relies on.

Synchronous callbacks are verified with the caller's state, so
kernel/bpf/verifier.c:in_sleepable_context() only looks at the program's
sleepability and the locks the program took itself, and does not know
that the helper holds rcu_read_lock() around the callback.

With a SEC("syscall") program whose callback reads a page that has to be
read from disk first:

  BUG: sleeping function called from invalid context at include/linux/uaccess.h:169
  RCU nest depth: 1, expected: 0
   __might_fault+0x29/0x80
   _copy_from_user+0x26/0x90
   bpf_copy_from_user+0x23/0x60
   bpf_prog_17d44fa39eebd6cc_cb+0x35/0x46
   bpf_for_each_hash_elem+0xdb/0x280

Fix by marking the callback frame of hash map iteration as running under
RCU and making in_sleepable_context() false while such a frame is on the
call stack, so sleepable helpers, kfuncs and global functions are
rejected there as inside bpf_rcu_read_lock(). The rules for trusting RCU
protected pointers are left alone, which is conservative and keeps
callbacks that verify today and do not sleep working. The flag is
compared in func_states_equal() so that pruning never mixes the two
contexts. Array map callbacks run without any lock and keep the caller's
context.

Fixes: 314ee05e2fc6 ("bpf: Add hashtab support for bpf_for_each_map_elem() helper")
Fixes: 818e00848227 ("bpf: Implement iteration ops for resizable hashtab")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 include/linux/bpf_verifier.h |  5 +++++
 kernel/bpf/states.c          |  3 +++
 kernel/bpf/verifier.c        | 28 +++++++++++++++++++++++++++-
 3 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index 112ca1511da4..816948df8fff 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -379,6 +379,11 @@ struct bpf_func_state {
 	bool in_async_callback_fn;
 	bool in_exception_callback_fn;
 	bool no_stack_arg_load;
+	/*
+	 * The helper invokes this callback under rcu_read_lock(), so the
+	 * callback and its callees run in an RCU read-side critical section.
+	 */
+	bool in_rcu_callback_fn;
 	/* For callback calling functions that limit number of possible
 	 * callback executions (e.g. bpf_loop) keeps track of current
 	 * simulated iteration number.
diff --git a/kernel/bpf/states.c b/kernel/bpf/states.c
index 953700822fa7..b3d250caaaa6 100644
--- a/kernel/bpf/states.c
+++ b/kernel/bpf/states.c
@@ -995,6 +995,9 @@ static bool func_states_equal(struct bpf_verifier_env *env, struct bpf_func_stat
 	if (!old->no_stack_arg_load && cur->no_stack_arg_load)
 		return false;
 
+	if (old->in_rcu_callback_fn != cur->in_rcu_callback_fn)
+		return false;
+
 	for (i = 0; i < MAX_BPF_REG; i++)
 		if (((1 << i) & live_regs) &&
 		    !regsafe(env, &old->regs[i], &cur->regs[i],
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index ddfa0c6d2755..883cf148235c 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -11530,6 +11530,19 @@ static int set_map_elem_callback_state(struct bpf_verifier_env *env,
 	if (err)
 		return err;
 
+	switch (map->map_type) {
+	case BPF_MAP_TYPE_HASH:
+	case BPF_MAP_TYPE_PERCPU_HASH:
+	case BPF_MAP_TYPE_LRU_HASH:
+	case BPF_MAP_TYPE_LRU_PERCPU_HASH:
+	case BPF_MAP_TYPE_RHASH:
+		/* Hash tables run the callback under rcu_read_lock(). */
+		callee->in_rcu_callback_fn = true;
+		break;
+	default:
+		break;
+	}
+
 	callee->in_callback_fn = true;
 	callee->callback_ret_range = retval_range(0, 1);
 	return 0;
@@ -12273,10 +12286,22 @@ int bpf_get_helper_proto(struct bpf_verifier_env *env, int func_id,
 	return *ptr && (*ptr)->func ? 0 : -EINVAL;
 }
 
+/* Is any frame on the current path a callback that runs under RCU? */
+static bool in_rcu_callback(struct bpf_verifier_env *env)
+{
+	struct bpf_verifier_state *state = env->cur_state;
+	int i;
+
+	for (i = 0; i <= state->curframe; i++)
+		if (state->frame[i]->in_rcu_callback_fn)
+			return true;
+	return false;
+}
+
 /* Check if we're in a sleepable context. */
 static inline bool in_sleepable_context(struct bpf_verifier_env *env)
 {
-	return !in_rcu_cs(env);
+	return !in_rcu_cs(env) && !in_rcu_callback(env);
 }
 
 static const char *non_sleepable_context_description(struct bpf_verifier_env *env)
@@ -12290,6 +12315,7 @@ static const char *non_sleepable_context_description(struct bpf_verifier_env *en
 			return "IRQ-disabled region";
 		if (env->cur_state->active_locks)
 			return "lock region";
+		return "callback running under rcu_read_lock";
 	}
 	return "non-sleepable prog";
 }
-- 
2.55.0

[PATCH bpf-next v2 03/43] bpf: don't leak the inode when token creation fails

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

bpf_token_create() allocates a bpffs inode and hands it to
alloc_file_pseudo().  The inode reference only moves to the new file when
that call succeeds: alloc_file_pseudo() leaves the caller's reference alone
on failure, and FD_PREPARE() does not even evaluate the file expression
when it cannot get a descriptor.

Before the conversion to FD_PREPARE() the error path did iput(inode).  The
conversion dropped it, so each BPF_TOKEN_CREATE that fails with -EMFILE,
-ENFILE or -ENOMEM at this point leaks an inode.  A task that is allowed to
create tokens can hit this at will by lowering RLIMIT_NOFILE, and the
leaked inodes make umount of that bpffs instance complain about busy
inodes.

Put the inode again if no file was created.

Fixes: 981bec8f698b ("bpf: convert bpf_token_create() to FD_PREPARE()")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/token.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/token.c b/kernel/bpf/token.c
index e85a179523f0..19660ef95ddb 100644
--- a/kernel/bpf/token.c
+++ b/kernel/bpf/token.c
@@ -169,8 +169,11 @@ int bpf_token_create(union bpf_attr *attr)
 	FD_PREPARE(fdf, O_CLOEXEC,
 		   alloc_file_pseudo(inode, path.mnt, BPF_TOKEN_INODE_NAME,
 				     O_RDWR, &bpf_token_fops));
-	if (fdf.err)
+	if (fdf.err) {
+		/* no file took over the inode reference */
+		iput(inode);
 		return fdf.err;
+	}
 
 	token = kzalloc_obj(*token, GFP_USER);
 	if (!token)
-- 
2.55.0

[PATCH bpf-next v2 04/43] bpf: don't hand out a linked element as extra element

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

When htab_map_update_elem() replaces an element of a preallocated map,
alloc_htab_elem() takes the spare element of this cpu and puts the old
element into the per-cpu slot right away, long before the old element is
unlinked from its bucket.  The bucket lock disables interrupts, but it
does not keep out an NMI, or a tracing program on the return of
alloc_htab_elem().  bpf_prog_active only helps if both programs are
kprobe, tracepoint or perf_event programs.  A program that updates an
existing key in another bucket of the same map gets to use the slot:

  htab_map_update_elem(k1)              tc, xdp, fentry, raw_tp, ...
    alloc_htab_elem()
      l_new = *pl_new
      *pl_new = l_old                   still linked in bucket 1
      <NMI>
        htab_map_update_elem(k2)        perf_event program
          l_new = *pl_new               the l_old of the outer update
          hlist_nulls_add_head_rcu(l_new, bucket 2)
          hlist_nulls_del_rcu(k2's old element)
      </NMI>
    hlist_nulls_add_head_rcu(l_new, bucket 1)
    hlist_nulls_del_rcu(l_old)

If l_old was the first element of bucket 1, the outer add points its pprev
at l_new, and the delete takes it out of bucket 1 only.  l_new->next now
leads into bucket 2, where l_old stays with a poisoned pprev, on which a
delete of k2 crashes.  If it was not the first, the delete takes it out of
bucket 2: k2 is gone from the map, and the chain of bucket 1 still contains
l_old, whose next pointer leads into bucket 2.

Either way a walk of bucket 1 ends on the nulls value of bucket 2.
lookup_nulls_elem_raw() restarts whenever that happens, so every lookup in
bucket 1 which does not find its key spins forever:

  rcu: INFO: rcu_preempt detected stalls on CPUs/tasks:
   __htab_map_lookup_elem+0x354/0x4d0

  BUG: KASAN: wild-memory-access in htab_map_delete_elem+0x30c/0x670
  Write of size 8 at addr dead000000000122 by task extra/102

An NMI between the load and the store of the slot makes both updates use
the same element, with much the same result.

Empty the slot when the spare element is taken, in a way that is atomic
on this cpu, and put the old element in only after it has been unlinked
and its timers and the like have been cancelled.  A nested update that
finds the slot empty, which is the case that corrupted the map, fails with
-EBUSY.

Fixes: 8c290e60fa2a ("bpf: fix hashmap extra_elems logic")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/hashtab.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
index 9d4791ee1741..66ebb5a32807 100644
--- a/kernel/bpf/hashtab.c
+++ b/kernel/bpf/hashtab.c
@@ -1279,11 +1279,17 @@ static struct htab_elem *alloc_htab_elem(struct bpf_htab *htab, void *key,
 	if (prealloc) {
 		if (old_elem) {
 			/* if we're updating the existing element,
-			 * use per-cpu extra elems to avoid freelist_pop/push
+			 * use per-cpu extra elems to avoid freelist_pop/push.
+			 * The caller puts old_elem into the slot once it is
+			 * unlinked. Until then the slot is empty for an NMI
+			 * or a kprobe that updates this map on this cpu.
 			 */
 			pl_new = this_cpu_ptr(htab->extra_elems);
-			l_new = *pl_new;
-			*pl_new = old_elem;
+			l_new = READ_ONCE(*pl_new);
+			do {
+				if (!l_new)
+					return ERR_PTR(-EBUSY);
+			} while (!try_cmpxchg_local(pl_new, &l_new, NULL));
 		} else {
 			struct pcpu_freelist_node *l;
 
@@ -1459,11 +1465,15 @@ static long htab_map_update_elem(struct bpf_map *map, void *key, void *value,
 	if (l_old) {
 		hlist_nulls_del_rcu(&l_old->hash_node);
 
-		/* l_old has already been stashed in htab->extra_elems, cancel
-		 * its reusable special fields before it is available for reuse.
+		/*
+		 * l_old replaces the extra element that alloc_htab_elem() took.
+		 * Cancel its reusable special fields first: once it is in the
+		 * slot, the next update on this cpu can reuse it.
 		 */
-		if (htab_is_prealloc(htab))
+		if (htab_is_prealloc(htab)) {
 			check_and_cancel_fields(htab, l_old);
+			WRITE_ONCE(*this_cpu_ptr(htab->extra_elems), l_old);
+		}
 	}
 	htab_unlock_bucket(b, flags);
 	if (l_old && !htab_is_prealloc(htab))
-- 
2.55.0

[PATCH bpf-next v2 05/43] bpf: reject sleepable programs for task_vma iterator

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

A sleepable "iter.s/task_vma" program can be attached, but its ->show()
callback runs with the mmap_lock of the iterated mm held for read. As soon
as the program runs, bpf_iter_run_prog() calls might_fault(), and any
bpf_copy_from_user() or bpf_copy_from_user_task() it does may really fault
or take mmap_lock again. With lockdep this is reported as recursive
locking of mmap_lock. Without lockdep, a fault on the reader's own mm, or
access_process_vm() on the iterated mm, has to take mmap_lock for read
again. If a writer queued up in the meantime that blocks behind the
writer, which in turn waits for the read lock the iterator already holds,
and the read() hangs:

    reader (iter.s/task_vma)        other thread of the same mm
    task_vma_seq_get_next()
      mmap_read_lock(mm)
                                    mmap_write_lock(mm)  <- waits
    bpf prog: copy_from_user()
      page fault
        mmap_read_lock(mm)          <- queued behind the writer

The reader waits killable, so the hang lasts until it gets a fatal signal.

bpf_iter_link_attach() allows sleepable programs for the targets that set
BPF_ITER_RESCHED, and task_vma sets it because cond_resched() between VMAs
is fine with mmap_lock held. Rescheduling between objects and running a
sleepable program inside ->show() are different properties, though, and
task_vma only has the first one. The open-coded task_vma iterator does not
have this problem since it hands out a snapshot of the VMA and holds no
lock when it returns to the program.

The vma handed to the program is only stable under mmap_lock, so the lock
can't be dropped around ->show(). Add a feature flag BPF_ITER_NOSLEEP for
targets which can't run sleepable programs although they can reschedule,
and set it for task_vma.

The check at attach time is not enough. BPF_LINK_UPDATE replaces the
program of a link, and bpf_iter_link_replace() only compares the type, the
attach type and the BTF id of the two programs, which are the same for a
sleepable and a non-sleepable one. Do the check there as well. That also
closes this way around the existing rule: so far a sleepable program could
be put on a link of a target that doesn't set BPF_ITER_RESCHED, like the
hash map element iterator, which runs it under rcu_read_lock().

BPF_LINK_CREATE of a sleepable program for the task_vma target, and
BPF_LINK_UPDATE to one, now fail with -EINVAL, also for programs that
don't touch user memory and have been working since sleepable iterator
programs exist. The programs can still be loaded. They have to be
loaded as non-sleepable programs, or use the open-coded iterator.

Fixes: b77fb25dcb34 ("bpf: Add support for bpf iterator programs to use sleepable helpers")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 include/linux/bpf.h    |  3 ++-
 kernel/bpf/bpf_iter.c  | 17 ++++++++++++++---
 kernel/bpf/task_iter.c |  6 +++++-
 3 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 54144372281c..46d325aa178e 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -3137,7 +3137,8 @@ typedef const struct bpf_func_proto *
 			     const struct bpf_prog *prog);
 
 enum bpf_iter_feature {
-	BPF_ITER_RESCHED	= BIT(0),
+	BPF_ITER_RESCHED	= BIT(0),	/* may cond_resched() between objects */
+	BPF_ITER_NOSLEEP	= BIT(1),	/* ->show() can't run sleepable progs */
 };
 
 #define BPF_ITER_CTX_ARG_MAX 2
diff --git a/kernel/bpf/bpf_iter.c b/kernel/bpf/bpf_iter.c
index b40eb404adab..fc3a75684ef7 100644
--- a/kernel/bpf/bpf_iter.c
+++ b/kernel/bpf/bpf_iter.c
@@ -72,6 +72,15 @@ static inline bool bpf_iter_target_support_resched(const struct bpf_iter_target_
 	return tinfo->reg_info->feature & BPF_ITER_RESCHED;
 }
 
+/* Only allow sleepable program for resched-able iterator that can run one */
+static bool bpf_iter_target_support_prog(const struct bpf_iter_target_info *tinfo,
+					 const struct bpf_prog *prog)
+{
+	return !prog->sleepable ||
+	       (bpf_iter_target_support_resched(tinfo) &&
+		!(tinfo->reg_info->feature & BPF_ITER_NOSLEEP));
+}
+
 static bool bpf_iter_support_resched(struct seq_file *seq)
 {
 	struct bpf_iter_priv_data *iter_priv;
@@ -409,6 +418,8 @@ static int bpf_iter_link_replace(struct bpf_link *link,
 				 struct bpf_prog *new_prog,
 				 struct bpf_prog *old_prog)
 {
+	struct bpf_iter_link *iter_link =
+		container_of(link, struct bpf_iter_link, link);
 	int ret = 0;
 
 	mutex_lock(&link_mutex);
@@ -419,7 +430,8 @@ static int bpf_iter_link_replace(struct bpf_link *link,
 
 	if (link->prog->type != new_prog->type ||
 	    link->prog->expected_attach_type != new_prog->expected_attach_type ||
-	    link->prog->aux->attach_btf_id != new_prog->aux->attach_btf_id) {
+	    link->prog->aux->attach_btf_id != new_prog->aux->attach_btf_id ||
+	    !bpf_iter_target_support_prog(iter_link->tinfo, new_prog)) {
 		ret = -EINVAL;
 		goto out_unlock;
 	}
@@ -544,8 +556,7 @@ int bpf_iter_link_attach(const union bpf_attr *attr, bpfptr_t uattr,
 	if (!tinfo)
 		return -ENOENT;
 
-	/* Only allow sleepable program for resched-able iterator */
-	if (prog->sleepable && !bpf_iter_target_support_resched(tinfo))
+	if (!bpf_iter_target_support_prog(tinfo, prog))
 		return -EINVAL;
 
 	link = kzalloc_obj(*link, GFP_USER | __GFP_NOWARN);
diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
index 13e1aabe6f88..cf2bf388e181 100644
--- a/kernel/bpf/task_iter.c
+++ b/kernel/bpf/task_iter.c
@@ -737,7 +737,11 @@ static const struct bpf_iter_seq_info task_vma_seq_info = {
 static struct bpf_iter_reg task_vma_reg_info = {
 	.target			= "task_vma",
 	.attach_target		= bpf_iter_attach_task,
-	.feature		= BPF_ITER_RESCHED,
+	/*
+	 * ->show() runs with the mmap_lock of the iterated mm held, which a
+	 * sleepable program takes again when it faults on user memory.
+	 */
+	.feature		= BPF_ITER_RESCHED | BPF_ITER_NOSLEEP,
 	.ctx_arg_info_size	= 2,
 	.ctx_arg_info		= {
 		{ offsetof(struct bpf_iter__task_vma, task),
-- 
2.55.0

[PATCH bpf-next v2 06/43] bpf: forbid sleeping in bpf_find_vma() callbacks

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

A sleepable program can call bpf_find_vma() and use a sleepable helper
such as bpf_copy_from_user() or bpf_copy_from_user_task() in the
callback. The verifier accepts this, but bpf_find_vma() runs the callback
with the mmap_lock of the mm held for read. If the user copy faults on
that mm, the fault handler has to take mmap_lock again. Lockdep reports
recursive locking, and if a writer queued up in between, the fault blocks
behind the writer, which waits for the read lock the helper already
holds:

    sleepable prog                  other thread of the same mm
    bpf_find_vma(current, ...)
      mmap_read_trylock(mm)
                                    mmap_write_lock(mm)  <- waits
      callback: copy_from_user()
        page fault
          mmap_read_lock(mm)        <- queued behind the writer

This is the same problem as with sleepable task_vma iterator programs.
The verifier checks synchronous callbacks in the caller's context, so
in_sleepable_context() is true in the callback of a sleepable program.

Verify the callback like a hash map for_each callback, as running in an
RCU read-side critical section where sleeping is not allowed, and actually
hold rcu_read_lock() around it, so that global functions the callback
calls, which are then verified for a non-sleepable context, see the RCU
protection they are verified with.

Non-sleepable programs already run the callback under RCU, nothing changes
for them. A sleepable program whose bpf_find_vma() callback calls any
helper, kfunc or global function that may sleep is now rejected by the
verifier, including ones that would be harmless under mmap_lock as they
don't touch user memory.

Fixes: 7c7e3d31e785 ("bpf: Introduce helper bpf_find_vma")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/task_iter.c | 6 ++++++
 kernel/bpf/verifier.c  | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
index cf2bf388e181..2fa6b8dae8c4 100644
--- a/kernel/bpf/task_iter.c
+++ b/kernel/bpf/task_iter.c
@@ -810,8 +810,14 @@ BPF_CALL_5(bpf_find_vma, struct task_struct *, task, u64, start,
 	vma = find_vma(mm, start);
 
 	if (vma && vma->vm_start <= start && vma->vm_end > start) {
+		/*
+		 * The callback must not sleep while mmap_lock is held, and the
+		 * verifier checks it as an RCU read-side critical section.
+		 */
+		rcu_read_lock();
 		callback_fn((u64)(long)task, (u64)(long)vma,
 			    (u64)(long)callback_ctx, 0, 0);
+		rcu_read_unlock();
 		ret = 0;
 	}
 	bpf_mmap_unlock_mm(work, mm);
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 883cf148235c..430d5309eb5c 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -11629,6 +11629,8 @@ static int set_find_vma_callback_state(struct bpf_verifier_env *env,
 	bpf_mark_reg_not_init(env, &callee->regs[BPF_REG_4]);
 	bpf_mark_reg_not_init(env, &callee->regs[BPF_REG_5]);
 	callee->in_callback_fn = true;
+	/* bpf_find_vma() runs the callback under mmap_lock and RCU. */
+	callee->in_rcu_callback_fn = true;
 	callee->callback_ret_range = retval_range(0, 1);
 	return 0;
 }
-- 
2.55.0

[PATCH bpf-next v2 07/43] bpf: look at all 64 bits of a BPF_JSET mask on 32-bit

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

In the branch of a BPF_JSET which is taken, the verifier can learn
something about the register only if the mask has a single bit: then this
bit is set. regs_refine_cond_op() tests this with is_power_of_2(val) and
then ORs val into var_off.

val is a u64, but is_power_of_2() takes an unsigned long. On a 32-bit
kernel it only sees the lower half of the mask. For

	if r1 & 0xffffffff80000000 goto l

which is a BPF_JSET | BPF_K with an immediate of 0x80000000, the lower
half is a power of two. So the verifier takes all 33 bits for set at l,
while the branch is taken if any of them is. A mask of 0x100000001 from a
register does the same for bit 0.

The log of an i386 kernel, for r6 = bpf_get_prandom_u32() << 31:

	3: (45) if r6 & 0x80000000 goto pc+2
	from 3 to 6: R6=0xffffffff80000000 R10=fp0
	6: (77) r6 >>= 63                     ; R6=1

When the program runs, r6 >> 63 is 0.

From then on the verifier works with known bits which the register does
not have at run time. It accepts, for example, an offset into a map value
which it derives from these bits and believes to be constant, and the
program can access memory outside of the map value.

Count the bits of the whole value.

Only 32-bit kernels are affected. On a 64-bit kernel unsigned long holds
the whole mask, and this patch changes nothing.

Fixes: 960ea056561a ("bpf: verifier: teach the verifier to reason about the BPF_JSET instruction")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/verifier.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 430d5309eb5c..d0ba7156ba4f 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -18145,7 +18145,7 @@ static void regs_refine_cond_op(struct bpf_reg_state *reg1, struct bpf_reg_state
 		 * this restriction. I.e., !(r1 & 0x3) means neither bit 0 nor
 		 * bit 1 is set, which we can readily use in adjustments.
 		 */
-		if (!is_power_of_2(val))
+		if (hweight64(val) != 1)
 			break;
 		if (is_jmp32) {
 			t = tnum_or(tnum_subreg(reg1->var_off), tnum_const(val));
-- 
2.55.0

[PATCH bpf-next v2 08/43] bpf: don't let bpf_skb_adjust_room() use up the MSS

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

When bpf_skb_adjust_room() grows a TCP GSO packet without
BPF_F_ADJ_ROOM_FIXED_GSO, gso_size is lowered by the number of bytes
added. Nothing looks at how large it is.

gso_size is the size of the segments the peer has sent if the packet
comes out of GRO, and what the sender has written into the virtio header
if it comes from a tun device or a packet socket. If it equals len_diff
the result is 0, and a packet of up to 64 KB is no GSO packet any more. If
it is smaller, the 16 bit value wraps around.

One less than len_diff gives GSO_BY_FRAGS. skb_segment() then takes the
length of each segment from the frag_list. A packet with more than 64 KB
of payload gets that far, and one from a tun device has no frag_list:

  BUG: KASAN: null-ptr-deref in skb_segment+0xcf1/0x1a90
  BUG: kernel NULL pointer dereference, address: 0000000000000070
   tcp_gso_segment+0x1be/0x9f0
   inet_gso_segment+0x288/0x740
   inet_gso_segment+0x288/0x740
   skb_mac_gso_segment+0x179/0x2e0

What is left must not be less than TCP_MIN_GSO_SIZE either. The stack
relies on that for the number of segments to fit into gso_segs, see
commit c27c449d455a ("virtio-net: Ensure that TCP packets don't overflow
gso_segs").

Refuse to grow a packet that would end up with less, with the error that
other packets which cannot be adjusted get.

Fixes: 2be7e212d541 ("bpf: add bpf_skb_adjust_room helper")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 net/core/filter.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/core/filter.c b/net/core/filter.c
index ba536be2915f..dc630fc9f3eb 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3612,6 +3612,14 @@ static int bpf_skb_net_grow(struct sk_buff *skb, u32 off, u32 len_diff,
 			return -ENOTSUPP;
 	}
 
+	/*
+	 * The MSS is lowered by len_diff. Leave the least that the stack lets a
+	 * TCP GSO packet have, or gso_segs can overflow.
+	 */
+	if (skb_is_gso(skb) && !(flags & BPF_F_ADJ_ROOM_FIXED_GSO) &&
+	    skb_shinfo(skb)->gso_size < len_diff + TCP_MIN_GSO_SIZE)
+		return -ENOTSUPP;
+
 	ret = skb_cow_head(skb, meta_len + len_diff);
 	if (unlikely(ret < 0))
 		return ret;
-- 
2.55.0

[PATCH bpf-next v2 09/43] bpf: Read the probe of a perf event under event_mutex

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

For a perf event on a kprobe or uprobe event which was made in tracefs,
bpf_get_kprobe_info() and bpf_get_uprobe_info() look the probe up by
name in the list of dynamic events, and hand a pointer to its symbol or
file name back to the caller. None of the callers holds event_mutex.

The perf event keeps its trace event alive, nothing else. Other dynamic
events are removed from the list and freed during the walk. And a probe
which shares its event with other probes can be deleted while the event
is in use, so even the probe which is found can be freed, together with
the string, before the caller has measured and copied it. The result is
a use-after-free in the walk, in strlen() and in the copy to user space,
or a NULL pointer dereference.

The readers are BPF_OBJ_GET_INFO_BY_FD and fdinfo of a perf link, for
whoever holds a descriptor of the link, and BPF_TASK_FD_QUERY. The other
side is anybody who may write to dynamic_events, kprobe_events or
uprobe_events.

Take event_mutex in bpf_get_perf_event_info() and give the caller a copy
of the name, which it has to free.

Fixes: 41bdc4b40ed6 ("bpf: introduce bpf subcommand BPF_TASK_FD_QUERY")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/syscall.c     | 17 +++++++++++------
 kernel/trace/bpf_trace.c | 13 +++++++++++++
 2 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 0f27604a54aa..0f5eb55d49d6 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -4020,19 +4020,19 @@ static int bpf_perf_link_fill_common(const struct perf_event *event,
 		*ulenp = 1;
 	}
 	if (!uname)
-		return 0;
+		goto out;
 
 	if (buf) {
 		err = bpf_copy_to_user(uname, buf, ulen, len);
-		if (err)
-			return err;
 	} else {
 		char zero = '\0';
 
 		if (put_user(zero, uname))
-			return -EFAULT;
+			err = -EFAULT;
 	}
-	return 0;
+out:
+	kfree_const(buf);
+	return err;
 }
 
 #ifdef CONFIG_KPROBE_EVENTS
@@ -4089,6 +4089,7 @@ static void bpf_perf_link_fdinfo_kprobe(const struct perf_event *event,
 		   name, offset, missed, addr,
 		   type == BPF_FD_TYPE_KRETPROBE ?  "kretprobe" : "kprobe",
 		   event->bpf_cookie);
+	kfree_const(name);
 }
 #endif
 
@@ -4142,6 +4143,7 @@ static void bpf_perf_link_fdinfo_uprobe(const struct perf_event *event,
 		   name, offset, ref_ctr_offset,
 		   type == BPF_FD_TYPE_URETPROBE ?  "uretprobe" : "uprobe",
 		   event->bpf_cookie);
+	kfree_const(name);
 }
 #endif
 
@@ -4240,6 +4242,7 @@ static void bpf_tracepoint_link_show_fdinfo(const struct perf_event *event,
 		   "event_type:\t%s\n"
 		   "cookie:\t%llu\n",
 		   name, "tracepoint", event->bpf_cookie);
+	kfree_const(name);
 }
 
 static void bpf_probe_link_show_fdinfo(const struct perf_event *event,
@@ -5767,11 +5770,13 @@ static int bpf_task_fd_query(const union bpf_attr *attr,
 		err = bpf_get_perf_event_info(event, &prog_id, &fd_type,
 					      &buf, &probe_offset,
 					      &probe_addr, NULL);
-		if (!err)
+		if (!err) {
 			err = bpf_task_fd_query_copy(attr, uattr, prog_id,
 						     fd_type, buf,
 						     probe_offset,
 						     probe_addr);
+			kfree_const(buf);
+		}
 		goto put_file;
 	}
 
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 5465e7eba238..304b426bd44d 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2209,6 +2209,7 @@ int bpf_probe_unregister(struct bpf_raw_event_map *btp, struct bpf_raw_tp_link *
 	return tracepoint_probe_unregister(btp->tp, (void *)btp->bpf_func, link);
 }
 
+/* On success the caller releases *buf with kfree_const() */
 int bpf_get_perf_event_info(const struct perf_event *event, u32 *prog_id,
 			    u32 *fd_type, const char **buf,
 			    u64 *probe_offset, u64 *probe_addr,
@@ -2218,6 +2219,12 @@ int bpf_get_perf_event_info(const struct perf_event *event, u32 *prog_id,
 	struct bpf_prog *prog;
 	int flags, err = 0;
 
+	/*
+	 * The list of dynamic events is walked, and a probe which has siblings
+	 * can be deleted, with its strings, while its event is in use.
+	 */
+	guard(mutex)(&event_mutex);
+
 	prog = event->prog;
 	if (!prog)
 		return -ENOENT;
@@ -2258,6 +2265,12 @@ int bpf_get_perf_event_info(const struct perf_event *event, u32 *prog_id,
 #endif
 	}
 
+	if (!err && *buf) {
+		*buf = kstrdup_const(*buf, GFP_KERNEL);
+		if (!*buf)
+			err = -ENOMEM;
+	}
+
 	return err;
 }
 
-- 
2.55.0

[PATCH bpf-next v2 10/43] bpf: Don't saturate owner_refcnt on the last unpin

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

bpf_local_storage_map_free() pins the owner with refcount_inc_not_zero()
while it uncharges an element.  The pin counts when
bpf_local_storage_destroy() could not get local_storage->lock: destroy()
then goes on, drops the owner's reference and spins until the count is
zero.  So the pin is the last reference whenever it is needed, and
refcount_dec() takes that for a bug: it warns "decrement hit 0; leaking
memory" and saturates the count.  Unless destroy() happens to read the
count between the two stores it spins for ever, under rcu_read_lock().

Drop the pin with refcount_dec_and_test().

Fixes: 5d800f87d0a5 ("bpf: Support lockless unlink when freeing map or local storage")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 include/linux/bpf_local_storage.h | 13 +++++++++++++
 kernel/bpf/bpf_local_storage.c    |  6 +++---
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/include/linux/bpf_local_storage.h b/include/linux/bpf_local_storage.h
index 9e4f5c45c974..6c643ac51079 100644
--- a/include/linux/bpf_local_storage.h
+++ b/include/linux/bpf_local_storage.h
@@ -104,6 +104,19 @@ struct bpf_local_storage {
 	refcount_t owner_refcnt;/* Used to pin owner when map_free is uncharging */
 };
 
+/*
+ * Drop a reference got by refcount_inc_not_zero(&local_storage->owner_refcnt).
+ * bpf_local_storage_destroy() may have dropped the owner's reference meanwhile
+ * and wait for the count to reach zero, so this one can be the last.
+ */
+static inline void
+bpf_local_storage_unpin_owner(struct bpf_local_storage *local_storage)
+{
+	bool last __maybe_unused;
+
+	last = refcount_dec_and_test(&local_storage->owner_refcnt);
+}
+
 /* U16_MAX is much more than enough for sk local storage
  * considering a tcp_sock is ~2k.
  */
diff --git a/kernel/bpf/bpf_local_storage.c b/kernel/bpf/bpf_local_storage.c
index 4642da062f0f..7721834e4bff 100644
--- a/kernel/bpf/bpf_local_storage.c
+++ b/kernel/bpf/bpf_local_storage.c
@@ -199,7 +199,7 @@ static void bpf_selem_unlink_storage_nolock_misc(struct bpf_local_storage_elem *
 	}
 
 	if (pin_owner)
-		refcount_dec(&local_storage->owner_refcnt);
+		bpf_local_storage_unpin_owner(local_storage);
 }
 
 /* local_storage->lock must be held and selem->local_storage == local_storage.
@@ -753,8 +753,8 @@ u32 bpf_local_storage_destroy(struct bpf_local_storage *local_storage)
 		while (refcount_read(&local_storage->owner_refcnt))
 			cpu_relax();
 		/*
-		 * Paired with refcount_dec() in bpf_selem_unlink_nofail()
-		 * to make sure destroy() sees the correct local_storage->mem_charge.
+		 * Paired with bpf_local_storage_unpin_owner() to make sure
+		 * destroy() sees the correct local_storage->mem_charge.
 		 */
 		smp_mb();
 	}
-- 
2.55.0

[PATCH bpf-next v2 11/43] bpf: Validate the SRH which End.DT6 uncovers in End.BPF

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

input_action_end_bpf() validates the SRH of a packet and pulls it into the
head before the program runs. If the program decapsulates the packet with
bpf_lwt_seg6_action(SEG6_LOCAL_ACTION_END_DT6), bpf_update_srh_state()
looks for a routing header in the inner packet and takes it for the new
SRH as it is.

Nobody has looked at that header, and whoever has sent the packet has
written it. ipv6_find_hdr() finds it in a fragment of the skb as well, but
hdrlen is then read at that offset from skb->data, past the end of the
head. The header need not be an SRH, and its hdrlen need not fit into the
packet, but it is marked as valid, and bpf_lwt_seg6_adjust_srh() and
bpf_lwt_seg6_store_bytes() check their offsets against it.

A program which then adds a TLV at the end of the SRH, where hdrlen has it,
makes bpf_lwt_seg6_adjust_srh() move and write memory behind the head of
the skb, as far as the sender likes within the 2 KB that hdrlen can say:

  BUG: KASAN: slab-out-of-bounds in bpf_skb_net_hdr_push+0x185/0x240
  Read of size 2088 at addr ffff888107d6f09e by task srh/102
  BUG: KASAN: slab-out-of-bounds in bpf_skb_net_hdr_push+0x1b5/0x240
  Write of size 8 at addr ffff888107d6f8be by task srh/102

Use seg6_get_srh(), which pulls and validates the header. Without a valid
SRH the state says that there is none, and the packet goes its way as
before. The pull can move the head, so compute the data pointers behind
it.

Fixes: 486cdf21583e ("bpf: add End.DT6 action to bpf_lwt_seg6_action helper")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 net/core/filter.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index dc630fc9f3eb..a62f3decf1e2 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -7025,15 +7025,15 @@ static void bpf_update_srh_state(struct sk_buff *skb)
 {
 	struct seg6_bpf_srh_state *srh_state =
 		this_cpu_ptr(&seg6_bpf_srh_states);
-	int srhoff = 0;
 
-	if (ipv6_find_hdr(skb, &srhoff, IPPROTO_ROUTING, NULL, NULL) < 0) {
-		srh_state->srh = NULL;
-	} else {
-		srh_state->srh = (struct ipv6_sr_hdr *)(skb->data + srhoff);
+	/* Behind a decapsulation this is a header that nobody has looked at */
+	srh_state->srh = seg6_get_srh(skb, 0);
+	if (srh_state->srh) {
 		srh_state->hdrlen = srh_state->srh->hdrlen << 3;
 		srh_state->valid = true;
 	}
+	/* it may have been pulled into the head */
+	bpf_compute_data_pointers(skb);
 }
 
 BPF_CALL_4(bpf_lwt_seg6_action, struct sk_buff *, skb,
-- 
2.55.0

[PATCH bpf-next v2 12/43] bpf: Pull the inner header for End.DT6 in End.BPF

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

bpf_lwt_seg6_action(SEG6_LOCAL_ACTION_END_DT6) pulls the outer headers off
the packet and hands it to seg6_lookup_nexthop(), which reads the inner
IPv6 header at skb->data. Nothing says that this header is in the head of
the skb, or that there is one. If the head ends with the outer headers,
the route is looked up for whatever lies behind the head, and the packet
is dropped or leaves by the wrong route. If the program then pushes an SRH
with SEG6_LOCAL_ACTION_END_B6, seg6_do_srh_inline() pulls an IPv6 header
off a head which has none, and __skb_pull() hits its BUG(): the kernel
panics on a packet from the network.

GRO pulls the inner header of a plain IPv6 in IPv6 packet into the head. It
takes a packet which GRO does not look into that far, one with a fragment
header in front of the inner header for example, or a device without GRO,
and a driver which leaves the inner header in a fragment.

Make sure of the inner header together with the outer ones. The program
goes on behind an error of the helper, so do it before the packet is
changed. A pull which fails can still have moved the head, so look for the
SRH again.

Fixes: 486cdf21583e ("bpf: add End.DT6 action to bpf_lwt_seg6_action helper")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 net/core/filter.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index a62f3decf1e2..6fbe0cea2140 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -7066,9 +7066,14 @@ BPF_CALL_4(bpf_lwt_seg6_action, struct sk_buff *, skb,
 
 		if (ipv6_find_hdr(skb, &hdroff, IPPROTO_IPV6, NULL, NULL) < 0)
 			return -EBADMSG;
-		if (!pskb_pull(skb, hdroff))
+		/* As long as nothing has been changed, the program goes on */
+		if (!pskb_may_pull(skb, hdroff + sizeof(struct ipv6hdr))) {
+			/* but the head may have moved */
+			bpf_update_srh_state(skb);
 			return -EBADMSG;
+		}
 
+		__skb_pull(skb, hdroff);
 		skb_postpull_rcsum(skb, skb_network_header(skb), hdroff);
 		skb_reset_network_header(skb);
 		skb_reset_transport_header(skb);
-- 
2.55.0

[PATCH bpf-next v2 13/43] bpf: Update the SRH state if End.B6 fails late

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

For SEG6_LOCAL_ACTION_END_B6 and END_B6_ENCAP bpf_lwt_seg6_action() looks
for the SRH again only if bpf_push_seg6_encap() has succeeded. That
function pushes the new header, for which the head of the skb is
reallocated if it has not got the room, and then looks up the route to the
first segment. If there is none, it returns an error, and the pointer to
the SRH in seg6_bpf_srh_states still points into the old head, which has
been freed.

The program goes on behind the error. bpf_lwt_seg6_store_bytes() and
bpf_lwt_seg6_adjust_srh() read the SRH through that pointer.

Look for the SRH whatever bpf_push_seg6_encap() says.

Fixes: 486cdf21583e ("bpf: add End.DT6 action to bpf_lwt_seg6_action helper")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 net/core/filter.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index 6fbe0cea2140..15354134091a 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -7087,8 +7087,8 @@ BPF_CALL_4(bpf_lwt_seg6_action, struct sk_buff *, skb,
 			return -EBADMSG;
 		err = bpf_push_seg6_encap(skb, BPF_LWT_ENCAP_SEG6_INLINE,
 					  param, param_len);
-		if (!err)
-			bpf_update_srh_state(skb);
+		/* The header can be in place although err says otherwise */
+		bpf_update_srh_state(skb);
 
 		return err;
 	case SEG6_LOCAL_ACTION_END_B6_ENCAP:
@@ -7096,8 +7096,8 @@ BPF_CALL_4(bpf_lwt_seg6_action, struct sk_buff *, skb,
 			return -EBADMSG;
 		err = bpf_push_seg6_encap(skb, BPF_LWT_ENCAP_SEG6,
 					  param, param_len);
-		if (!err)
-			bpf_update_srh_state(skb);
+		/* The header can be in place although err says otherwise */
+		bpf_update_srh_state(skb);
 
 		return err;
 	default:
-- 
2.55.0

[PATCH bpf-next v2 14/43] bpf: Clear sk_refcnt of a clone before its storage is linked

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

sk_clone() sets sk_refcnt of the new socket some lines behind
bpf_sk_storage_clone(). But the elements which that function copies are
linked to their maps at once, and an element leads to its owner: the
iterator over a sk_storage map hands the owner to its program.
bpf_sk_storage_get() with BPF_SK_STORAGE_GET_F_CREATE and
bpf_sk_storage_delete() rely on refcount_inc_not_zero() to tell whether
they may touch the socket.

sk_refcnt is in the part of struct sock which sock_copy() leaves out, and
sk_prot_alloc() does not clear a clone unless init_on_alloc is on. It is
zero in an object which has been a socket before, and anything in one
which has not. If it is not zero, a helper gets a reference which
refcount_set() wipes out, and the helper's sock_put() then drops one of
the two references of the new socket, which is freed too early:

  IPv4: Attempt to release TCP socket in state 1
  BUG: KASAN: slab-use-after-free in __raw_spin_lock_init+0x47/0x90

In the meantime the helper changes a storage which bpf_sk_storage_clone()
fills without taking its lock.

Nothing else can find the clone at this point. Say that it is not alive
yet, before the first element is linked. Sockets whose parent has no
storage are left alone.

Fixes: a50a85e40c59 ("bpf: Expose bpf_sk_storage_* to iterator programs")
Suggested-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 net/core/bpf_sk_storage.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
index 1d295a8769fa..b5349bec5695 100644
--- a/net/core/bpf_sk_storage.c
+++ b/net/core/bpf_sk_storage.c
@@ -164,6 +164,13 @@ int bpf_sk_storage_clone(const struct sock *sk, struct sock *newsk)
 	if (!sk_storage || hlist_empty(&sk_storage->list))
 		goto out;
 
+	/*
+	 * The maps lead to newsk as soon as an element is linked, but
+	 * sk_clone() sets sk_refcnt only later.  Until then it holds whatever
+	 * the slab object held before: no reference can be had yet.
+	 */
+	refcount_set(&newsk->sk_refcnt, 0);
+
 	hlist_for_each_entry_rcu(selem, &sk_storage->list, snode) {
 		struct bpf_local_storage_elem *copy_selem;
 		struct bpf_local_storage_map *smap;
-- 
2.55.0

[PATCH bpf-next v2 15/43] bpf: Fix NULL deref in the sk_storage map iterator

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

bpf_sk_storage_map_seq_find_next() hands out an element only if its
local_storage pointer is set. __bpf_sk_storage_map_seq_show() then reads
the pointer a second time and dereferences it without a test.

Since bpf_local_storage_destroy() unlinks the elements with
bpf_selem_unlink_nofail(), the pointer is reset to NULL when the socket
which owns the element is destroyed, so it can change between the two
reads. Reading the iterator while sockets with an element in the map are
closed ends in a NULL pointer dereference. The sockets need not belong to
the reader: where a program gives every new socket an element, an
unprivileged user who opens and closes sockets is enough.

Skip such an element, as the TCP and UDP iterators skip a socket which
has been unhashed.

Fixes: 0be08389c7f2 ("bpf: Switch to bpf_selem_unlink_nofail in bpf_local_storage_{map_free, destroy}")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 net/core/bpf_sk_storage.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
index b5349bec5695..aea67b446c7c 100644
--- a/net/core/bpf_sk_storage.c
+++ b/net/core/bpf_sk_storage.c
@@ -813,6 +813,9 @@ static int __bpf_sk_storage_map_seq_show(struct seq_file *seq,
 		ctx.map = info->map;
 		if (selem) {
 			sk_storage = rcu_dereference(selem->local_storage);
+			/* reset since ->next() looked: the socket goes away */
+			if (!sk_storage)
+				return SEQ_SKIP;
 			ctx.sk = sk_storage->owner;
 			ctx.value = SDATA(selem)->data;
 		}
-- 
2.55.0

[PATCH bpf-next v2 16/43] bpf: Link the first element after its storage is published

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

bpf_local_storage_alloc() links the first element of a new storage to the
bucket of its map, and then publishes the storage in the owner with
cmpxchg().  If another cpu has been faster, the element is taken out of
the bucket again and is freed together with the storage.

In between the iterator over a sk_storage map can find the element.  It
stays linked to a storage that points to the socket, but the socket knows
nothing of that storage: bpf_sk_storage_free() neither unlinks the element
nor waits for anybody who uses the storage.  So nothing tells the iterator
that the socket has gone away.

Three cpus that create the first storage of a new AF_UNIX socket at the
same time, and a fourth that reads the iterator, are enough: in 20 seconds
its program was shown some 4000 elements of storages that had lost.  If the
socket is closed in time, the program is handed a freed socket.

Link the element once the storage is published.  Both happens under the
lock of the bucket, which holds off whoever finds the element through the
owner and wants to unlink it.

Fixes: 5ce6e77c7edf ("bpf: Implement bpf iterator for sock local storage map")
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/bpf_local_storage.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/kernel/bpf/bpf_local_storage.c b/kernel/bpf/bpf_local_storage.c
index 7721834e4bff..c08afb33d038 100644
--- a/kernel/bpf/bpf_local_storage.c
+++ b/kernel/bpf/bpf_local_storage.c
@@ -509,8 +509,6 @@ int bpf_local_storage_alloc(void *owner,
 	if (err)
 		goto uncharge;
 
-	bpf_selem_link_map_nolock(b, first_selem);
-
 	owner_storage_ptr =
 		(struct bpf_local_storage **)owner_storage(smap, owner);
 	/* Publish storage to the owner.
@@ -525,11 +523,17 @@ int bpf_local_storage_alloc(void *owner,
 	 */
 	prev_storage = cmpxchg(owner_storage_ptr, NULL, storage);
 	if (unlikely(prev_storage)) {
-		bpf_selem_unlink_map_nolock(first_selem);
 		raw_res_spin_unlock_irqrestore(&b->lock, flags);
 		err = -EAGAIN;
 		goto uncharge;
 	}
+	/*
+	 * Not before the storage is published: the owner does not wait for
+	 * users of a storage that it knows nothing of, so that nobody may find
+	 * such a storage through the map. b->lock holds off those who find
+	 * first_selem through the owner and want to unlink it.
+	 */
+	bpf_selem_link_map_nolock(b, first_selem);
 	raw_res_spin_unlock_irqrestore(&b->lock, flags);
 
 	return 0;
-- 
2.55.0

[PATCH bpf-next v2 17/43] bpf: Hold the socket in the sk_storage map iterator

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

The iterator over a sk_storage map passes the owner of an element to its
program as ctx->sk, without a reference. All that it holds is the RCU read
lock, which keeps the element and the storage, not the socket.

An element is unlinked by bpf_sk_storage_free(), which __sk_destruct()
calls right before it frees the socket. That is no better for a socket
with SOCK_RCU_FREE: the grace period comes in front of __sk_destruct(),
so a reader which starts behind call_rcu() still finds the element. The
program and the helpers it hands ctx->sk to, such as
bpf_sk_storage_get() or bpf_sk_storage_delete(), use a socket which is
being freed on another CPU. It takes nothing but reading the iterator
while sockets with an element in the map are closed, by whomever.

Take a reference for the time of the program, and skip the element if
there is none to be had. To look at the owner at all, do what
bpf_selem_unlink_nofail() does for bpf_local_storage_map_free(): as long
as the element is linked under the lock of the storage,
bpf_local_storage_destroy() has not got past it, unless it has given up
on the lock, and then it waits for owner_refcnt.

The last reference may now be dropped by the iterator. The end of a socket
may want process context: rfcomm_sock_destruct() takes a mutex. So only
drop the reference under RCU, and free a socket whose last one it was when
the RCU read lock has been released, which is at the end of a bucket and
of every read() at the latest.

Fixes: c69d2ddb2072 ("bpf: Using rcu_read_lock for bpf_sk_storage_map iterator")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 net/core/bpf_sk_storage.c | 73 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 68 insertions(+), 5 deletions(-)

diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
index aea67b446c7c..ea22ec5ef5b1 100644
--- a/net/core/bpf_sk_storage.c
+++ b/net/core/bpf_sk_storage.c
@@ -699,8 +699,37 @@ struct bpf_iter_seq_sk_storage_map_info {
 	struct bpf_map *map;
 	unsigned int bucket_id;
 	unsigned skip_elems;
+	struct rcu_head *dead_sks;
 };
 
+/*
+ * sock_put() in two halves. The end of a socket may want process context:
+ * rfcomm_sock_destruct() takes a mutex. If the iterator drops the last
+ * reference, it keeps the socket until it has left the RCU read side critical
+ * section. Nobody uses sk_rcu before sk_destruct().
+ */
+static void
+bpf_sk_storage_map_seq_put_sk(struct bpf_iter_seq_sk_storage_map_info *info,
+			      struct sock *sk)
+{
+	if (refcount_dec_and_test(&sk->sk_refcnt)) {
+		sk->sk_rcu.next = info->dead_sks;
+		info->dead_sks = &sk->sk_rcu;
+	}
+}
+
+static void
+bpf_sk_storage_map_seq_free_sks(struct bpf_iter_seq_sk_storage_map_info *info)
+{
+	struct sock *sk;
+
+	while (info->dead_sks) {
+		sk = container_of(info->dead_sks, struct sock, sk_rcu);
+		info->dead_sks = sk->sk_rcu.next;
+		sk_free(sk);
+	}
+}
+
 static struct bpf_local_storage_elem *
 bpf_sk_storage_map_seq_find_next(struct bpf_iter_seq_sk_storage_map_info *info,
 				 struct bpf_local_storage_elem *prev_selem)
@@ -729,6 +758,7 @@ bpf_sk_storage_map_seq_find_next(struct bpf_iter_seq_sk_storage_map_info *info,
 			/* not found, unlock and go to the next bucket */
 			b = &smap->buckets[bucket_id++];
 			rcu_read_unlock();
+			bpf_sk_storage_map_seq_free_sks(info);
 			skip_elems = 0;
 			break;
 		}
@@ -796,12 +826,44 @@ DEFINE_BPF_ITER_FUNC(bpf_sk_storage_map, struct bpf_iter_meta *meta,
 		     struct bpf_map *map, struct sock *sk,
 		     void *value)
 
+/*
+ * Take a reference on the socket which owns @selem. RCU keeps the element, not
+ * its owner. As long as the element is linked under the lock of the storage,
+ * the owner has not got past bpf_sk_storage_free(), unless that has given up
+ * on the lock. Then it still waits for owner_refcnt.
+ */
+static struct sock *
+bpf_sk_storage_selem_get_sk(struct bpf_local_storage_elem *selem)
+{
+	struct bpf_local_storage *sk_storage;
+	struct sock *sk = NULL;
+	unsigned long flags;
+
+	sk_storage = rcu_dereference(selem->local_storage);
+	/* reset since ->next() looked: the socket goes away */
+	if (!sk_storage)
+		return NULL;
+
+	if (raw_res_spin_lock_irqsave(&sk_storage->lock, flags))
+		return NULL;
+
+	if (!hlist_unhashed(&selem->snode) &&
+	    refcount_inc_not_zero(&sk_storage->owner_refcnt)) {
+		sk = sk_storage->owner;
+		if (!refcount_inc_not_zero(&sk->sk_refcnt))
+			sk = NULL;
+		bpf_local_storage_unpin_owner(sk_storage);
+	}
+	raw_res_spin_unlock_irqrestore(&sk_storage->lock, flags);
+
+	return sk;
+}
+
 static int __bpf_sk_storage_map_seq_show(struct seq_file *seq,
 					 struct bpf_local_storage_elem *selem)
 {
 	struct bpf_iter_seq_sk_storage_map_info *info = seq->private;
 	struct bpf_iter__bpf_sk_storage_map ctx = {};
-	struct bpf_local_storage *sk_storage;
 	struct bpf_iter_meta meta;
 	struct bpf_prog *prog;
 	int ret = 0;
@@ -812,14 +874,14 @@ static int __bpf_sk_storage_map_seq_show(struct seq_file *seq,
 		ctx.meta = &meta;
 		ctx.map = info->map;
 		if (selem) {
-			sk_storage = rcu_dereference(selem->local_storage);
-			/* reset since ->next() looked: the socket goes away */
-			if (!sk_storage)
+			ctx.sk = bpf_sk_storage_selem_get_sk(selem);
+			if (!ctx.sk)
 				return SEQ_SKIP;
-			ctx.sk = sk_storage->owner;
 			ctx.value = SDATA(selem)->data;
 		}
 		ret = bpf_iter_run_prog(prog, &ctx);
+		if (ctx.sk)
+			bpf_sk_storage_map_seq_put_sk(info, ctx.sk);
 	}
 
 	return ret;
@@ -837,6 +899,7 @@ static void bpf_sk_storage_map_seq_stop(struct seq_file *seq, void *v)
 		(void)__bpf_sk_storage_map_seq_show(seq, v);
 	else
 		rcu_read_unlock();
+	bpf_sk_storage_map_seq_free_sks(seq->private);
 }
 
 static int bpf_iter_init_sk_storage_map(void *priv_data,
-- 
2.55.0

[PATCH bpf-next v2 18/43] bpf: Bound the nesting that the BTF show code follows

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

btf_struct_show() and btf_array_show() recurse for every nested struct
and array.  MAX_RESOLVE_DEPTH bounds one resolve walk when BTF is loaded,
not the nesting: a member whose type is resolved already is not walked
again, so BTF that defines its types from the inside out can nest them as
deep as it has types.

Whoever may load BTF, with CAP_BPF or a BPF token, can give such a type
to the value of a map.  Reading the file of that map in bpffs then
overruns the kernel stack: with a few hundred levels the stack guard page
is hit and the kernel panics.

Stop at 32 levels.  That is MAX_RESOLVE_DEPTH, and __btf_show_indent()
has as many columns.  Nothing is shown for a member that lies deeper, and
the status becomes -E2BIG, which bpf_snprintf_btf() and
bpf_seq_printf_btf() return.  The types of an x86_64 defconfig kernel nest
14 deep.

Fixes: b00b8daec828 ("bpf: btf: Add pretty print capability for data with BTF type info")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/btf.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 64c78434fa05..a9bf4c3c0a27 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -1081,6 +1081,8 @@ static const struct btf_type *btf_type_skip_qualifiers(const struct btf *btf,
 }
 
 #define BTF_SHOW_MAX_ITER	10
+/* nested structs and arrays shown; __btf_show_indent() has as many columns */
+#define BTF_SHOW_MAX_DEPTH	32
 
 #define BTF_KIND_BIT(kind)	(1ULL << kind)
 
@@ -1474,8 +1476,18 @@ static void *btf_show_start_aggr_type(struct btf_show *show,
 				      const struct btf_type *t,
 				      u32 type_id, void *data)
 {
-	void *safe_data = btf_show_start_type(show, t, type_id, data);
+	void *safe_data;
 
+	/*
+	 * Structs and arrays are shown by recursion.  BTF loading bounds the
+	 * depth of one resolve walk, not how deep resolved types may nest.
+	 */
+	if (show->state.depth >= BTF_SHOW_MAX_DEPTH) {
+		show->state.status = -E2BIG;
+		return NULL;
+	}
+
+	safe_data = btf_show_start_type(show, t, type_id, data);
 	if (!safe_data)
 		return safe_data;
 
-- 
2.55.0

[PATCH bpf-next v2 19/43] bpf: Check the new program of an iterator link update

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

The bpf_map_elem, bpf_sk_storage_map and sockmap iterators hand the key
and the value of an element to the program as buffers.  The verifier only
records how far a program reaches into them, and ->attach_target()
compares that with the key and value size of the map.

bpf_iter_link_replace() does not.  BPF_LINK_UPDATE installs a program that
an attach refuses with -EACCES, and the program then reads, and for a
value writes, behind every element that the iterator visits.  It takes
CAP_BPF and CAP_PERFMON, or a BPF token.

Split the test out of the three ->attach_target() and ask it, as
->check_prog(), for the new program of an update as well.  Targets without
such a test take every program of their kind, as before.

Fixes: d6c4503cc296 ("bpf: Implement bpf iterator for hash maps")
Fixes: 5ce6e77c7edf ("bpf: Implement bpf iterator for sock local storage map")
Fixes: 0365351524d7 ("net: Allow iterating sockmap and sockhash")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 include/linux/bpf.h       |  4 +++
 kernel/bpf/bpf_iter.c     |  7 ++++++
 kernel/bpf/map_iter.c     | 51 ++++++++++++++++++++++-----------------
 net/core/bpf_sk_storage.c | 17 ++++++++++---
 net/core/sock_map.c       | 17 ++++++++++---
 5 files changed, 66 insertions(+), 30 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 46d325aa178e..3fb3c7d22d99 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -3128,6 +3128,8 @@ typedef int (*bpf_iter_attach_target_t)(struct bpf_prog *prog,
 					union bpf_iter_link_info *linfo,
 					struct bpf_iter_aux_info *aux);
 typedef void (*bpf_iter_detach_target_t)(struct bpf_iter_aux_info *aux);
+typedef int (*bpf_iter_check_prog_t)(const struct bpf_prog *prog,
+				     const struct bpf_iter_aux_info *aux);
 typedef void (*bpf_iter_show_fdinfo_t) (const struct bpf_iter_aux_info *aux,
 					struct seq_file *seq);
 typedef int (*bpf_iter_fill_link_info_t)(const struct bpf_iter_aux_info *aux,
@@ -3146,6 +3148,8 @@ struct bpf_iter_reg {
 	const char *target;
 	bpf_iter_attach_target_t attach_target;
 	bpf_iter_detach_target_t detach_target;
+	/* may the program run on what the link is attached to? */
+	bpf_iter_check_prog_t check_prog;
 	bpf_iter_show_fdinfo_t show_fdinfo;
 	bpf_iter_fill_link_info_t fill_link_info;
 	bpf_iter_get_func_proto_t get_func_proto;
diff --git a/kernel/bpf/bpf_iter.c b/kernel/bpf/bpf_iter.c
index fc3a75684ef7..4668c1696a08 100644
--- a/kernel/bpf/bpf_iter.c
+++ b/kernel/bpf/bpf_iter.c
@@ -436,6 +436,13 @@ static int bpf_iter_link_replace(struct bpf_link *link,
 		goto out_unlock;
 	}
 
+	if (iter_link->tinfo->reg_info->check_prog) {
+		ret = iter_link->tinfo->reg_info->check_prog(new_prog,
+							     &iter_link->aux);
+		if (ret)
+			goto out_unlock;
+	}
+
 	old_prog = xchg(&link->prog, new_prog);
 	bpf_prog_put(old_prog);
 
diff --git a/kernel/bpf/map_iter.c b/kernel/bpf/map_iter.c
index 2077d46d167c..16537356c1a2 100644
--- a/kernel/bpf/map_iter.c
+++ b/kernel/bpf/map_iter.c
@@ -97,13 +97,29 @@ static struct bpf_iter_reg bpf_map_reg_info = {
 	.seq_info		= &bpf_map_seq_info,
 };
 
+static int bpf_iter_map_check_prog(const struct bpf_prog *prog,
+				   const struct bpf_iter_aux_info *aux)
+{
+	const struct bpf_map *map = aux->map;
+	u32 value_size = map->value_size;
+
+	if (map->map_type == BPF_MAP_TYPE_PERCPU_HASH ||
+	    map->map_type == BPF_MAP_TYPE_LRU_PERCPU_HASH ||
+	    map->map_type == BPF_MAP_TYPE_PERCPU_ARRAY)
+		value_size = round_up(value_size, 8) * num_possible_cpus();
+
+	if (prog->aux->max_rdonly_access > map->key_size ||
+	    prog->aux->max_rdwr_access > value_size)
+		return -EACCES;
+
+	return 0;
+}
+
 static int bpf_iter_attach_map(struct bpf_prog *prog,
 			       union bpf_iter_link_info *linfo,
 			       struct bpf_iter_aux_info *aux)
 {
-	u32 key_acc_size, value_acc_size, key_size, value_size;
 	struct bpf_map *map;
-	bool is_percpu = false;
 	int err = -EINVAL;
 
 	if (!linfo->map.map_fd)
@@ -123,30 +139,20 @@ static int bpf_iter_attach_map(struct bpf_prog *prog,
 		goto put_map;
 	}
 
-	if (map->map_type == BPF_MAP_TYPE_PERCPU_HASH ||
-	    map->map_type == BPF_MAP_TYPE_LRU_PERCPU_HASH ||
-	    map->map_type == BPF_MAP_TYPE_PERCPU_ARRAY)
-		is_percpu = true;
-	else if (map->map_type != BPF_MAP_TYPE_HASH &&
-		 map->map_type != BPF_MAP_TYPE_LRU_HASH &&
-		 map->map_type != BPF_MAP_TYPE_ARRAY &&
-		 map->map_type != BPF_MAP_TYPE_RHASH)
+	if (map->map_type != BPF_MAP_TYPE_PERCPU_HASH &&
+	    map->map_type != BPF_MAP_TYPE_LRU_PERCPU_HASH &&
+	    map->map_type != BPF_MAP_TYPE_PERCPU_ARRAY &&
+	    map->map_type != BPF_MAP_TYPE_HASH &&
+	    map->map_type != BPF_MAP_TYPE_LRU_HASH &&
+	    map->map_type != BPF_MAP_TYPE_ARRAY &&
+	    map->map_type != BPF_MAP_TYPE_RHASH)
 		goto put_map;
 
-	key_acc_size = prog->aux->max_rdonly_access;
-	value_acc_size = prog->aux->max_rdwr_access;
-	key_size = map->key_size;
-	if (!is_percpu)
-		value_size = map->value_size;
-	else
-		value_size = round_up(map->value_size, 8) * num_possible_cpus();
-
-	if (key_acc_size > key_size || value_acc_size > value_size) {
-		err = -EACCES;
+	aux->map = map;
+	err = bpf_iter_map_check_prog(prog, aux);
+	if (err)
 		goto put_map;
-	}
 
-	aux->map = map;
 	return 0;
 
 put_map:
@@ -179,6 +185,7 @@ static const struct bpf_iter_reg bpf_map_elem_reg_info = {
 	.target			= "bpf_map_elem",
 	.attach_target		= bpf_iter_attach_map,
 	.detach_target		= bpf_iter_detach_map,
+	.check_prog		= bpf_iter_map_check_prog,
 	.show_fdinfo		= bpf_iter_map_show_fdinfo,
 	.fill_link_info		= bpf_iter_map_fill_link_info,
 	.ctx_arg_info_size	= 2,
diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
index ea22ec5ef5b1..72b9ee4c98de 100644
--- a/net/core/bpf_sk_storage.c
+++ b/net/core/bpf_sk_storage.c
@@ -919,6 +919,15 @@ static void bpf_iter_fini_sk_storage_map(void *priv_data)
 	bpf_map_put_with_uref(seq_info->map);
 }
 
+static int bpf_iter_map_check_prog(const struct bpf_prog *prog,
+				   const struct bpf_iter_aux_info *aux)
+{
+	if (prog->aux->max_rdwr_access > aux->map->value_size)
+		return -EACCES;
+
+	return 0;
+}
+
 static int bpf_iter_attach_map(struct bpf_prog *prog,
 			       union bpf_iter_link_info *linfo,
 			       struct bpf_iter_aux_info *aux)
@@ -936,12 +945,11 @@ static int bpf_iter_attach_map(struct bpf_prog *prog,
 	if (map->map_type != BPF_MAP_TYPE_SK_STORAGE)
 		goto put_map;
 
-	if (prog->aux->max_rdwr_access > map->value_size) {
-		err = -EACCES;
+	aux->map = map;
+	err = bpf_iter_map_check_prog(prog, aux);
+	if (err)
 		goto put_map;
-	}
 
-	aux->map = map;
 	return 0;
 
 put_map:
@@ -972,6 +980,7 @@ static struct bpf_iter_reg bpf_sk_storage_map_reg_info = {
 	.target			= "bpf_sk_storage_map",
 	.attach_target		= bpf_iter_attach_map,
 	.detach_target		= bpf_iter_detach_map,
+	.check_prog		= bpf_iter_map_check_prog,
 	.show_fdinfo		= bpf_iter_map_show_fdinfo,
 	.fill_link_info		= bpf_iter_map_fill_link_info,
 	.ctx_arg_info_size	= 2,
diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index 38df84284328..057e2f3c0265 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
@@ -1933,6 +1933,15 @@ int sock_map_link_create(const union bpf_attr *attr, struct bpf_prog *prog)
 	return ret;
 }
 
+static int sock_map_iter_check_prog(const struct bpf_prog *prog,
+				    const struct bpf_iter_aux_info *aux)
+{
+	if (prog->aux->max_rdonly_access > aux->map->key_size)
+		return -EACCES;
+
+	return 0;
+}
+
 static int sock_map_iter_attach_target(struct bpf_prog *prog,
 				       union bpf_iter_link_info *linfo,
 				       struct bpf_iter_aux_info *aux)
@@ -1951,12 +1960,11 @@ static int sock_map_iter_attach_target(struct bpf_prog *prog,
 	    map->map_type != BPF_MAP_TYPE_SOCKHASH)
 		goto put_map;
 
-	if (prog->aux->max_rdonly_access > map->key_size) {
-		err = -EACCES;
+	aux->map = map;
+	err = sock_map_iter_check_prog(prog, aux);
+	if (err)
 		goto put_map;
-	}
 
-	aux->map = map;
 	return 0;
 
 put_map:
@@ -1973,6 +1981,7 @@ static struct bpf_iter_reg sock_map_iter_reg = {
 	.target			= "sockmap",
 	.attach_target		= sock_map_iter_attach_target,
 	.detach_target		= sock_map_iter_detach_target,
+	.check_prog		= sock_map_iter_check_prog,
 	.show_fdinfo		= bpf_iter_map_show_fdinfo,
 	.fill_link_info		= bpf_iter_map_fill_link_info,
 	.ctx_arg_info_size	= 2,
-- 
2.55.0

[PATCH bpf-next v2 20/43] bpf: Fix stale offload->prog pointer after realloc

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

bpf_prog_realloc() moves a program that the rewrites of the verifier make
outgrow its pages.  It updates aux->prog, but not the offload->prog of a
device-bound program.  When the device is unregistered,
__bpf_offload_dev_netdev_unregister() hands the stale pointer to
__bpf_prog_offload_destroy(), which faults on the freed program:

  BUG: unable to handle page fault for address: ffffc90000071050
  RIP: 0010:__bpf_prog_offload_destroy+0x3f/0x310

It takes the right to load a device-bound XDP program, CAP_BPF and
CAP_NET_ADMIN or a BPF token, and a device that can be removed.

Commit a1aa9ef47c29 ("bpf: Fix stale offload->prog pointer after constant
blinding") repaired the other place that replaces a program.  Do the same
here.

Nothing keeps the device while a program is loaded, so it can also be
unregistered at that very moment.  That frees the offload, and it may have
read offload->prog already when the old copy of the program is freed.  In
both places update the pointer under bpf_devs_lock, before the old copy
goes.

Fixes: ab3f0063c48c ("bpf: offload: add infrastructure for loading programs for a specific netdev")
Fixes: 2b3486bc2d23 ("bpf: Introduce device-bound XDP programs")
Fixes: a1aa9ef47c29 ("bpf: Fix stale offload->prog pointer after constant blinding")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 include/linux/bpf.h  |  5 +++++
 kernel/bpf/core.c    |  4 ++--
 kernel/bpf/offload.c | 16 ++++++++++++++++
 3 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 3fb3c7d22d99..0de5947dceaa 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -3792,6 +3792,7 @@ int bpf_dev_bound_kfunc_check(struct bpf_verifier_log *log,
 void *bpf_dev_bound_resolve_kfunc(struct bpf_prog *prog, u32 func_id);
 int bpf_prog_dev_bound_init(struct bpf_prog *prog, union bpf_attr *attr);
 int bpf_prog_dev_bound_inherit(struct bpf_prog *new_prog, struct bpf_prog *old_prog);
+void bpf_prog_dev_bound_moved(struct bpf_prog *prog);
 void bpf_dev_bound_netdev_unregister(struct net_device *dev);
 
 static inline bool bpf_prog_is_dev_bound(const struct bpf_prog_aux *aux)
@@ -3853,6 +3854,10 @@ static inline int bpf_prog_dev_bound_inherit(struct bpf_prog *new_prog,
 	return -EOPNOTSUPP;
 }
 
+static inline void bpf_prog_dev_bound_moved(struct bpf_prog *prog)
+{
+}
+
 static inline void bpf_dev_bound_netdev_unregister(struct net_device *dev)
 {
 }
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 642f0ab8eb2d..69285abcf594 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -269,6 +269,7 @@ struct bpf_prog *bpf_prog_realloc(struct bpf_prog *fp_old, unsigned int size,
 		memcpy(fp, fp_old, fp_old->pages * PAGE_SIZE);
 		fp->pages = pages;
 		fp->aux->prog = fp;
+		bpf_prog_dev_bound_moved(fp);
 
 		/* We keep fp->aux from fp_old around in the new
 		 * reallocated structure.
@@ -1558,8 +1559,7 @@ void bpf_jit_prog_release_other(struct bpf_prog *fp, struct bpf_prog *fp_other)
 	 * know whether fp here is the clone or the original.
 	 */
 	fp->aux->prog = fp;
-	if (fp->aux->offload)
-		fp->aux->offload->prog = fp;
+	bpf_prog_dev_bound_moved(fp);
 	bpf_prog_clone_free(fp_other);
 }
 
diff --git a/kernel/bpf/offload.c b/kernel/bpf/offload.c
index d855399812ee..5a10f40a7831 100644
--- a/kernel/bpf/offload.c
+++ b/kernel/bpf/offload.c
@@ -96,6 +96,22 @@ static int __bpf_offload_dev_netdev_register(struct bpf_offload_dev *offdev,
 	return err;
 }
 
+/*
+ * @prog takes the place of a copy that the caller is about to free. Nothing
+ * keeps the netdev while a program is loaded: once the lock is dropped, no
+ * unregistration is left that has got the old copy from offload->prog.
+ */
+void bpf_prog_dev_bound_moved(struct bpf_prog *prog)
+{
+	if (!bpf_prog_is_dev_bound(prog->aux))
+		return;
+
+	down_read(&bpf_devs_lock);
+	if (prog->aux->offload)
+		prog->aux->offload->prog = prog;
+	up_read(&bpf_devs_lock);
+}
+
 static void __bpf_prog_offload_destroy(struct bpf_prog *prog)
 {
 	struct bpf_prog_offload *offload = prog->aux->offload;
-- 
2.55.0

[PATCH bpf-next v2 21/43] bpf: Don't bind a program to a netdev that goes away

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

bpf_prog_dev_bound_init() looks the device up, takes bpf_devs_lock and
registers the device for the program.  The reference it holds does not
keep the device registered.  If bpf_dev_bound_netdev_unregister() has run
by the time the lock is taken, nobody takes the program off the device any
more: the bpf_offload_netdev and offload->netdev point to a net_device
that is freed as soon as the reference is dropped.
bpf_dev_bound_resolve_kfunc() dereferences it while the program is
verified, and a new device at the same address finds the old entry.

unregister_netdevice_many_notify() leaves NETREG_REGISTERED before it
calls bpf_dev_bound_netdev_unregister(), which takes bpf_devs_lock.  Look
at reg_state under that lock, and treat a device that goes like one that
is not there.

Fixes: 2b3486bc2d23 ("bpf: Introduce device-bound XDP programs")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/offload.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/offload.c b/kernel/bpf/offload.c
index 5a10f40a7831..f7bfff1760cc 100644
--- a/kernel/bpf/offload.c
+++ b/kernel/bpf/offload.c
@@ -266,7 +266,14 @@ int bpf_prog_dev_bound_init(struct bpf_prog *prog, union bpf_attr *attr)
 	prog->aux->offload_requested = !(attr->prog_flags & BPF_F_XDP_DEV_BOUND_ONLY);
 
 	down_write(&bpf_devs_lock);
-	err = __bpf_prog_dev_bound_init(prog, netdev);
+	/*
+	 * Behind bpf_dev_bound_netdev_unregister() nobody would take the
+	 * program off the device, which goes once the reference is dropped.
+	 */
+	if (READ_ONCE(netdev->reg_state) == NETREG_REGISTERED)
+		err = __bpf_prog_dev_bound_init(prog, netdev);
+	else
+		err = -EINVAL;
 	up_write(&bpf_devs_lock);
 
 out:
-- 
2.55.0

[PATCH bpf-next v2 22/43] bpf: Keep a program until its bpf_wq callback has ended

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

bpf_wq_work() runs the callback under rcu_read_lock_trace() only, since
the callback may sleep.  The program it belongs to need not be sleepable,
and such a program is freed behind a plain RCU grace period.  The
reference that the bpf_wq holds is dropped when the map element goes, or
when another callback is set, no matter whether the work is running.

If that was the last reference the program is freed under the running
callback, and the worker goes on in the freed JIT image:

  Oops: int3: 0000 [#1] SMP NOPTI
  RIP: 0010:0xffffffffc0600947

All it takes is to close the map and the program while the callback runs.

Note in the program that it has a callback that runs sleepable, and wait
for an RCU Tasks Trace grace period, which implies a plain one, before
such a program is freed.  Let bpf_wq_work() enter its read side critical
section before it reads callback_fn.

bpf_task_work holds a reference to the program until the callback is done.

Fixes: 8e83da9732d9 ("bpf: add bpf_wq_start")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 include/linux/bpf.h   | 10 ++++++++++
 kernel/bpf/helpers.c  |  9 ++++++---
 kernel/bpf/syscall.c  |  4 ++--
 kernel/bpf/verifier.c |  2 ++
 4 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 0de5947dceaa..db56d5d07462 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1873,6 +1873,7 @@ struct bpf_prog_aux {
 	bool attach_btf_trace; /* true if attaching to BTF-enabled raw tp */
 	bool attach_tracing_prog; /* true if tracing another tracing program */
 	bool tramp_linked; /* true if it was ever linked to a trampoline */
+	bool sleepable_async_cb; /* has a bpf_wq or bpf_task_work callback */
 	bool func_proto_unreliable;
 	bool tail_call_reachable;
 	bool xdp_has_frags;
@@ -2006,6 +2007,15 @@ struct bpf_prog {
 	};
 };
 
+/*
+ * Does the program, or a callback of it, run under RCU Tasks Trace protection
+ * only, outside of any RCU read side critical section?
+ */
+static inline bool bpf_prog_runs_sleepable(const struct bpf_prog *prog)
+{
+	return prog->sleepable || prog->aux->sleepable_async_cb;
+}
+
 struct bpf_array_aux {
 	/* Programs with direct jumps into programs part of this array. */
 	struct list_head poke_progs;
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 439370d73cc5..648abe1ba63a 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -1262,18 +1262,21 @@ static void bpf_wq_work(struct work_struct *work)
 
 	BTF_TYPE_EMIT(struct bpf_wq);
 
+	/* keeps the program that callback_fn belongs to */
+	rcu_read_lock_trace();
+
 	callback_fn = READ_ONCE(cb->callback_fn);
 	if (!callback_fn)
-		return;
+		goto out;
 
 	key = map_key_from_value(map, value, &idx);
 
-        rcu_read_lock_trace();
-        migrate_disable();
+	migrate_disable();
 
 	callback_fn((u64)(long)map, (u64)(long)key, (u64)(long)value, 0, 0);
 
 	migrate_enable();
+out:
 	rcu_read_unlock_trace();
 }
 
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 0f5eb55d49d6..30d570c1c9a0 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2477,7 +2477,7 @@ static void __bpf_prog_put_rcu_tasks(struct rcu_head *rcu)
 {
 	struct bpf_prog *prog = container_of(rcu, struct bpf_prog_aux, rcu)->prog;
 
-	if (prog->sleepable)
+	if (bpf_prog_runs_sleepable(prog))
 		call_rcu_tasks_trace(rcu, __bpf_prog_put_rcu);
 	else
 		call_rcu(rcu, __bpf_prog_put_rcu);
@@ -2497,7 +2497,7 @@ static void __bpf_prog_put_noref(struct bpf_prog *prog, bool deferred)
 	if (deferred) {
 		if (IS_ENABLED(CONFIG_TASKS_RCU) && prog->aux->tramp_linked)
 			call_rcu_tasks(&prog->aux->rcu, __bpf_prog_put_rcu_tasks);
-		else if (prog->sleepable)
+		else if (bpf_prog_runs_sleepable(prog))
 			call_rcu_tasks_trace(&prog->aux->rcu, __bpf_prog_put_rcu);
 		else
 			call_rcu(&prog->aux->rcu, __bpf_prog_put_rcu);
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index d0ba7156ba4f..6b15862a15e8 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -11211,6 +11211,8 @@ static int push_callback_call(struct bpf_verifier_env *env, struct bpf_insn *ins
 					 is_async_cb_sleepable(env, insn));
 		if (IS_ERR(async_cb))
 			return PTR_ERR(async_cb);
+		if (is_async_cb_sleepable(env, insn))
+			env->prog->aux->sleepable_async_cb = true;
 		callee = async_cb->frame[0];
 		callee->async_entry_cnt = state->frame[0]->async_entry_cnt + 1;
 
-- 
2.55.0

[PATCH bpf-next v2 23/43] bpf: Count sleepable callbacks as sleepable map users

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

map->sleepable_refcnt tells bpf_map_fd_put_ptr() whether an inner map that
is taken out of its outer map has to wait for an RCU Tasks Trace grace
period as well.  It counts the sleepable programs that use the outer map.
The bpf_wq or bpf_task_work callback of a program that is not sleepable
runs under rcu_read_lock_trace() only, and is not counted.

Such a callback can look an inner map up and keep it while user space
deletes it from the outer map.  The inner map is freed behind a plain RCU
grace period, under the callback:

  BUG: KASAN: slab-use-after-free in array_map_update_elem+0x6bc/0x7d0

Count a program with such a callback.  The maps are bound before the
verifier meets the callback, so raise the count of those that are bound
when it does.

Fixes: 8e83da9732d9 ("bpf: add bpf_wq_start")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/core.c     |  2 +-
 kernel/bpf/syscall.c  |  2 +-
 kernel/bpf/verifier.c | 17 +++++++++++++++--
 3 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 69285abcf594..e1216927373a 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -3042,7 +3042,7 @@ void __bpf_free_used_maps(struct bpf_prog_aux *aux,
 	bool sleepable;
 	u32 i;
 
-	sleepable = aux->prog->sleepable;
+	sleepable = bpf_prog_runs_sleepable(aux->prog);
 	for (i = 0; i < len; i++) {
 		map = used_maps[i];
 		if (map->ops->map_poke_untrack)
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 30d570c1c9a0..c412bcd1903f 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -6257,7 +6257,7 @@ static int bpf_prog_bind_map(union bpf_attr *attr)
 	/* The bpf program will not access the bpf map, but for the sake of
 	 * simplicity, increase sleepable_refcnt for sleepable program as well.
 	 */
-	if (prog->sleepable)
+	if (bpf_prog_runs_sleepable(prog))
 		atomic64_inc(&map->sleepable_refcnt);
 	memcpy(used_maps_new, used_maps_old,
 	       sizeof(used_maps_old[0]) * prog->aux->used_map_cnt);
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 6b15862a15e8..1f2a846ae415 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -593,6 +593,19 @@ static bool is_async_cb_sleepable(struct bpf_verifier_env *env, struct bpf_insn
 	return false;
 }
 
+/* From here on the maps of the program count it as a sleepable user. */
+static void mark_sleepable_async_cb(struct bpf_verifier_env *env)
+{
+	int i;
+
+	if (bpf_prog_runs_sleepable(env->prog))
+		return;
+
+	env->prog->aux->sleepable_async_cb = true;
+	for (i = 0; i < env->used_map_cnt; i++)
+		atomic64_inc(&env->used_maps[i]->sleepable_refcnt);
+}
+
 bool bpf_is_may_goto_insn(struct bpf_insn *insn)
 {
 	return insn->code == (BPF_JMP | BPF_JCOND) && insn->src_reg == BPF_MAY_GOTO;
@@ -11212,7 +11225,7 @@ static int push_callback_call(struct bpf_verifier_env *env, struct bpf_insn *ins
 		if (IS_ERR(async_cb))
 			return PTR_ERR(async_cb);
 		if (is_async_cb_sleepable(env, insn))
-			env->prog->aux->sleepable_async_cb = true;
+			mark_sleepable_async_cb(env);
 		callee = async_cb->frame[0];
 		callee->async_entry_cnt = state->frame[0]->async_entry_cnt + 1;
 
@@ -20776,7 +20789,7 @@ static int __add_used_map(struct bpf_verifier_env *env, struct bpf_map *map)
 	if (err)
 		return err;
 
-	if (env->prog->sleepable)
+	if (bpf_prog_runs_sleepable(env->prog))
 		atomic64_inc(&map->sleepable_refcnt);
 
 	/* hold the map. If the program is rejected by verifier,
-- 
2.55.0

[PATCH bpf-next v2 24/43] bpf: Check the maps that sleepable callbacks use

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

Sleepable programs can only use the kinds of map that do without an RCU
read side critical section.  check_map_prog_compatibility() sees to that
when a map is bound to the program, and asks whether the program is
sleepable.  That misses the bpf_wq or bpf_task_work callback of a program
that is not, which runs sleepable all the same, and it misses an inner
map, which is not bound.

The bpf_wq callback of a tc program that looks a key up in a sockhash
gets as far as

  WARNING: net/core/sock_map.c:903 at __sock_hash_lookup_elem+0x18f/0x1e0

Ask the same question for the map that is handed to a helper in sleepable
context, unless the RCU read lock is held: by bpf_rcu_read_lock(), or by
the helper that runs the callback, as bpf_for_each_map_elem() does for a
hash map.  Programs that take the lock go on working.  Disabled preemption
or interrupts are not enough, since the maps ask for rcu_read_lock_held().
The other parts of the program may use such a map as before.

A global function that is called with preemption or interrupts disabled is
verified for the non-sleepable context, in which nothing is known of all
that.  Verify it for a context of its own: not sleepable, and without the
RCU read lock.  It differs in what maps can be used, and in nothing else.

What is refused from now on runs into the warning above, or uses what the
map frees behind a plain RCU grace period: an entry of a devmap, of a perf
event array or of a cgroup array.

A bloom filter does without RCU.  Add it to the list, so that callbacks
which use one go on loading.  As a side effect sleepable programs can use
bloom filters as well.

A pointer to the value of a devmap which a program keeps beyond
bpf_rcu_read_unlock() is not dealt with here.

Fixes: 638e4b825d52 ("bpf: Allows per-cpu maps and map-in-map in sleepable programs")
Fixes: 8e83da9732d9 ("bpf: add bpf_wq_start")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 include/linux/bpf.h          |  14 ++++-
 include/linux/bpf_verifier.h |   2 +
 kernel/bpf/verifier.c        | 109 ++++++++++++++++++++++++-----------
 3 files changed, 89 insertions(+), 36 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index db56d5d07462..1823ca3a1d5e 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1742,12 +1742,20 @@ static inline void bpf_trampoline_multi_detach(struct bpf_prog *prog,
 static inline void bpf_trampoline_set_flags(struct bpf_trampoline *tr, u32 flags) {}
 #endif
 
+/* The contexts that a global function is called in, and is verified for. */
+enum bpf_func_context {
+	BPF_FUNC_CTX_NON_SLEEPABLE,
+	BPF_FUNC_CTX_SLEEPABLE,
+	/* Sleepable code has disabled preemption or interrupts: no RCU read lock. */
+	BPF_FUNC_CTX_NO_RCU_LOCK,
+	BPF_FUNC_CTX_CNT,
+};
+
 struct bpf_func_info_aux {
 	u16 linkage;
 	bool unreliable;
-	/* Indexed by in_sleepable. */
-	bool called[2];
-	bool verified[2];
+	bool called[BPF_FUNC_CTX_CNT];
+	bool verified[BPF_FUNC_CTX_CNT];
 };
 
 enum bpf_jit_poke_reason {
diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index 816948df8fff..e420d211f423 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -1090,6 +1090,8 @@ struct bpf_verifier_env {
 	bool bypass_spec_v1;
 	bool bypass_spec_v4;
 	bool seen_direct_write;
+	/* a global function is verified for BPF_FUNC_CTX_NO_RCU_LOCK */
+	bool no_rcu_read_lock;
 	bool seen_exception;
 	bool signature;
 	u32 insn_aux_data_len;
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 1f2a846ae415..81660ffce99a 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -219,6 +219,7 @@ static int ref_set_non_owning(struct bpf_verifier_env *env,
 			      struct bpf_reg_state *reg);
 static bool is_trusted_reg(struct bpf_verifier_env *env, const struct bpf_reg_state *reg);
 static inline bool in_sleepable_context(struct bpf_verifier_env *env);
+static bool in_rcu_callback(struct bpf_verifier_env *env);
 static const char *non_sleepable_context_description(struct bpf_verifier_env *env);
 static void scalar32_min_max_add(struct bpf_reg_state *dst_reg, struct bpf_reg_state *src_reg);
 static void scalar_min_max_add(struct bpf_reg_state *dst_reg, struct bpf_reg_state *src_reg);
@@ -10379,12 +10380,71 @@ bool bpf_allow_tail_call_in_subprogs(struct bpf_verifier_env *env)
 	       bpf_jit_supports_subprog_tailcalls();
 }
 
+/*
+ * Is rcu_read_lock_held() true at run time? Unlike for in_rcu_cs(), disabled
+ * preemption or interrupts will not do.
+ */
+static bool has_rcu_read_lock(struct bpf_verifier_env *env)
+{
+	struct bpf_verifier_state *state = env->cur_state;
+
+	if (state->active_rcu_locks || in_rcu_callback(env))
+		return true;
+	if (in_sleepable(env))
+		return false;
+	return !env->no_rcu_read_lock || state->frame[0]->in_async_callback_fn;
+}
+
+/* May the map be used outside of an RCU read side critical section? */
+static bool map_needs_no_rcu(const struct bpf_map *map)
+{
+	switch (map->map_type) {
+	case BPF_MAP_TYPE_HASH:
+	case BPF_MAP_TYPE_RHASH:
+	case BPF_MAP_TYPE_LRU_HASH:
+	case BPF_MAP_TYPE_ARRAY:
+	case BPF_MAP_TYPE_PERCPU_HASH:
+	case BPF_MAP_TYPE_PERCPU_ARRAY:
+	case BPF_MAP_TYPE_LRU_PERCPU_HASH:
+	case BPF_MAP_TYPE_LPM_TRIE:
+	case BPF_MAP_TYPE_ARRAY_OF_MAPS:
+	case BPF_MAP_TYPE_HASH_OF_MAPS:
+	case BPF_MAP_TYPE_RINGBUF:
+	case BPF_MAP_TYPE_USER_RINGBUF:
+	case BPF_MAP_TYPE_INODE_STORAGE:
+	case BPF_MAP_TYPE_SK_STORAGE:
+	case BPF_MAP_TYPE_TASK_STORAGE:
+	case BPF_MAP_TYPE_CGRP_STORAGE:
+	case BPF_MAP_TYPE_QUEUE:
+	case BPF_MAP_TYPE_STACK:
+	case BPF_MAP_TYPE_ARENA:
+	case BPF_MAP_TYPE_INSN_ARRAY:
+	case BPF_MAP_TYPE_PROG_ARRAY:
+	case BPF_MAP_TYPE_BLOOM_FILTER:
+		return true;
+	default:
+		return false;
+	}
+}
+
 static int check_map_func_compatibility(struct bpf_verifier_env *env,
 					struct bpf_map *map, int func_id)
 {
 	if (!map)
 		return 0;
 
+	/*
+	 * check_map_prog_compatibility() asks the program.  That misses the
+	 * sleepable callback of another program, and an inner map.  They may
+	 * use any map under the RCU read lock.  Disabled preemption or
+	 * interrupts will not do: the maps ask for rcu_read_lock_held().
+	 */
+	if (!has_rcu_read_lock(env) && !map_needs_no_rcu(map)) {
+		verbose(env, "map '%s' needs bpf_rcu_read_lock() in sleepable context\n",
+			map->name);
+		return -EINVAL;
+	}
+
 	/* We need a two way check, first is from map perspective ... */
 	switch (map->map_type) {
 	case BPF_MAP_TYPE_PROG_ARRAY:
@@ -11373,7 +11433,12 @@ static int check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
 		if (env->log.level & BPF_LOG_LEVEL)
 			verbose(env, "Func#%d ('%s') is global and assumed valid.\n",
 				subprog, sub_name);
-		sub_aux->called[in_sleepable_context(env)] = true;
+		if (in_sleepable_context(env))
+			sub_aux->called[BPF_FUNC_CTX_SLEEPABLE] = true;
+		else if (has_rcu_read_lock(env))
+			sub_aux->called[BPF_FUNC_CTX_NON_SLEEPABLE] = true;
+		else
+			sub_aux->called[BPF_FUNC_CTX_NO_RCU_LOCK] = true;
 		returns_void = subprog_returns_void(env, subprog);
 		if (env->subprog_info[subprog].changes_pkt_data)
 			clear_all_pkt_pointers(env);
@@ -20676,35 +20741,11 @@ static int check_map_prog_compatibility(struct bpf_verifier_env *env,
 		return -EINVAL;
 	}
 
-	if (prog->sleepable)
-		switch (map->map_type) {
-		case BPF_MAP_TYPE_HASH:
-		case BPF_MAP_TYPE_RHASH:
-		case BPF_MAP_TYPE_LRU_HASH:
-		case BPF_MAP_TYPE_ARRAY:
-		case BPF_MAP_TYPE_PERCPU_HASH:
-		case BPF_MAP_TYPE_PERCPU_ARRAY:
-		case BPF_MAP_TYPE_LRU_PERCPU_HASH:
-		case BPF_MAP_TYPE_LPM_TRIE:
-		case BPF_MAP_TYPE_ARRAY_OF_MAPS:
-		case BPF_MAP_TYPE_HASH_OF_MAPS:
-		case BPF_MAP_TYPE_RINGBUF:
-		case BPF_MAP_TYPE_USER_RINGBUF:
-		case BPF_MAP_TYPE_INODE_STORAGE:
-		case BPF_MAP_TYPE_SK_STORAGE:
-		case BPF_MAP_TYPE_TASK_STORAGE:
-		case BPF_MAP_TYPE_CGRP_STORAGE:
-		case BPF_MAP_TYPE_QUEUE:
-		case BPF_MAP_TYPE_STACK:
-		case BPF_MAP_TYPE_ARENA:
-		case BPF_MAP_TYPE_INSN_ARRAY:
-		case BPF_MAP_TYPE_PROG_ARRAY:
-			break;
-		default:
-			verbose(env,
-				"Sleepable programs can only use array, hash, ringbuf and local storage maps\n");
-			return -EINVAL;
-		}
+	if (prog->sleepable && !map_needs_no_rcu(map)) {
+		verbose(env,
+			"Sleepable programs can only use array, hash, ringbuf and local storage maps\n");
+		return -EINVAL;
+	}
 
 	if (bpf_map_is_cgroup_storage(map) &&
 	    bpf_cgroup_storage_assign(env->prog->aux, map)) {
@@ -21426,7 +21467,8 @@ static void free_states(struct bpf_verifier_env *env)
 	}
 }
 
-static int do_check_common(struct bpf_verifier_env *env, int subprog, bool is_sleepable)
+static int do_check_common(struct bpf_verifier_env *env, int subprog,
+			   enum bpf_func_context context)
 {
 	bool pop_log = !(env->log.level & BPF_LOG_LEVEL2);
 	struct bpf_subprog_info *sub = subprog_info(env, subprog);
@@ -21446,7 +21488,8 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog, bool is_sl
 	state->curframe = 0;
 	state->speculative = false;
 	state->branches = 1;
-	state->in_sleepable = is_sleepable;
+	state->in_sleepable = context == BPF_FUNC_CTX_SLEEPABLE;
+	env->no_rcu_read_lock = context == BPF_FUNC_CTX_NO_RCU_LOCK;
 	state->frame[0] = kzalloc_obj(struct bpf_func_state, GFP_KERNEL_ACCOUNT);
 	if (!state->frame[0]) {
 		kfree(state);
@@ -21663,7 +21706,7 @@ static int do_check_subprogs(struct bpf_verifier_env *env)
 
 	/*
 	 * We can't loop forever as each pass verifies at least one new context,
-	 * and there are only two contexts per global subprog.
+	 * and there are only three contexts per global subprog.
 	 */
 	if (new_cnt)
 		goto again;
-- 
2.55.0

[PATCH bpf-next v2 25/43] bpf: Assume no bounds for buffers behind a tail call

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

The bpf_map_elem, bpf_sk_storage_map and sockmap iterators hand the key
and the value of an element to the program as buffers.  The verifier notes
how far a program reaches into them, and the iterator compares that with
the map when the program is attached.  A program that does not touch the
buffers passes.  It can tail-call another program of the same target,
which gets the same context, and whose reach nobody compares with the
map:

  BUG: KASAN: slab-out-of-bounds in ___bpf_prog_run+0xa46a/0xae80

The program array can be filled at any time, so there is nothing to look
at when the link is made.  For the same reason a program with a tail call
is taken to use the whole stack and the whole packet.  Take it to reach
as far as can be into the buffers as well, so that these iterators refuse
it with -EACCES.

Fixes: d6c4503cc296 ("bpf: Implement bpf iterator for hash maps")
Fixes: 5ce6e77c7edf ("bpf: Implement bpf iterator for sock local storage map")
Fixes: 0365351524d7 ("net: Allow iterating sockmap and sockhash")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/fixups.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
index 608621d7f225..002c3935a8eb 100644
--- a/kernel/bpf/fixups.c
+++ b/kernel/bpf/fixups.c
@@ -2151,6 +2151,8 @@ int bpf_do_misc_fixups(struct bpf_verifier_env *env)
 			if (!bpf_allow_tail_call_in_subprogs(env))
 				prog->aux->stack_depth = MAX_BPF_STACK;
 			prog->aux->max_pkt_offset = MAX_PACKET_OFF;
+			prog->aux->max_rdonly_access = U32_MAX;
+			prog->aux->max_rdwr_access = U32_MAX;
 
 			/* mark bpf_tail_call as different opcode to avoid
 			 * conditional branch in the interpreter for every normal
-- 
2.55.0

[PATCH bpf-next v2 26/43] bpf: Don't take an extension for an iterator program

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

An extension inherits the expected_attach_type of the program it extends.
If that is BPF_TRACE_ITER, check_attach_btf_id() hands the extension to
bpf_iter_prog_supported(), which goes by the name of the attach function.
For an extension that is the name of the subprogram it replaces, so the
load fails with -EINVAL, unless that subprogram is called like the function
of a target, bpf_iter_bpf_map_elem for example.

Then the extension gets the context of that target.  If the subprogram
has as many arguments, it can use the key and the value as buffers, but
how far it reaches is noted in the extension, where the test of the
iterator link does not look: it reads and writes behind the elements of
the map.  And the id of the subprogram in the BTF of the program is cached
as the id of the target, so that programs of that target which are loaded
already cannot be attached.

Refuse all extensions here.

Fixes: 15d83c4d7cef ("bpf: Allow loading of a bpf_iter program")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/bpf_iter.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/kernel/bpf/bpf_iter.c b/kernel/bpf/bpf_iter.c
index 4668c1696a08..5fd376bd210b 100644
--- a/kernel/bpf/bpf_iter.c
+++ b/kernel/bpf/bpf_iter.c
@@ -351,6 +351,13 @@ int bpf_iter_prog_supported(struct bpf_prog *prog)
 	const char *prefix = BPF_ITER_FUNC_PREFIX;
 	int prefix_len = strlen(prefix);
 
+	/*
+	 * The attach function of an extension is a subprogram of the iterator
+	 * program that it extends, whatever its name is.
+	 */
+	if (prog->type == BPF_PROG_TYPE_EXT)
+		return -EINVAL;
+
 	if (strncmp(attach_fname, prefix, prefix_len))
 		return -EINVAL;
 
-- 
2.55.0

[PATCH bpf-next v2 27/43] bpf: Assume no bounds for a writable tp with tail calls

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

A writable raw tracepoint hands a buffer to the program.  The verifier
notes how far the program reaches into it, and bpf_probe_register() refuses
a program that reaches further than the tracepoint allows.  A program that
does not touch the buffer passes, and can tail-call one of its type that
reads and writes behind it.

Take a BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE program with a tail call to
reach as far as can be, as for the stack, the packet and the buffers of
iterators.  It cannot be attached any more.  Plain raw tracepoint programs,
for which the limit is zero, cannot get at the buffer, and a tail call
stays within one type: they are left alone.

Fixes: 9df1c28bb752 ("bpf: add writable context for raw tracepoints")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/fixups.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
index 002c3935a8eb..fe59e3114d82 100644
--- a/kernel/bpf/fixups.c
+++ b/kernel/bpf/fixups.c
@@ -2153,6 +2153,8 @@ int bpf_do_misc_fixups(struct bpf_verifier_env *env)
 			prog->aux->max_pkt_offset = MAX_PACKET_OFF;
 			prog->aux->max_rdonly_access = U32_MAX;
 			prog->aux->max_rdwr_access = U32_MAX;
+			if (prog->type == BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE)
+				prog->aux->max_tp_access = U32_MAX;
 
 			/* mark bpf_tail_call as different opcode to avoid
 			 * conditional branch in the interpreter for every normal
-- 
2.55.0

[PATCH bpf-next v2 28/43] bpf: Don't take an extension for a writable raw tracepoint program

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

bpf_probe_register() compares max_tp_access of the program that is
attached with the size of the buffer that the tracepoint lets programs
write to.  An extension of that program, or of one of its global
functions, gets the same context and the same pointer to the buffer.  What
it accesses is noted in its own max_tp_access, which nobody looks at.  So
it reads and writes behind the buffer, of a tracepoint that may have none:

  BUG: KASAN: stack-out-of-bounds in bpf_prog_5843f2b820eb4929_tr26_ext_far+0xf4/0x10c
  Write of size 4 at addr ffffc90000d8f894 by task wext/103

Refuse such extensions.

Fixes: be8704ff07d2 ("bpf: Introduce dynamic program extensions")
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/verifier.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 81660ffce99a..0d1b729989d6 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -22232,6 +22232,15 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
 					"Extension programs should be JITed\n");
 				return -EINVAL;
 			}
+			/*
+			 * How much of the buffer of the tracepoint is used
+			 * is checked for the program that is attached to it.
+			 */
+			if (tgt_prog->type == BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE) {
+				bpf_log(log,
+					"Cannot extend a writable raw tracepoint program\n");
+				return -EINVAL;
+			}
 			tgt_changes_pkt_data = aux->func
 					       ? aux->func[subprog]->aux->changes_pkt_data
 					       : aux->changes_pkt_data;
-- 
2.55.0

[PATCH bpf-next v2 29/43] bpf: Keep iterator programs off special map fields

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

The bpf_map_elem iterator, for all but the per-cpu maps, and the
bpf_sk_storage_map iterator hand the live value of an element to the
program as a writable buffer.  The verifier does not know the map when it
checks the program, so nothing keeps the program from writing over a
bpf_spin_lock, a bpf_timer, a kptr or another special field, which neither
a map update nor a pointer to a map value can touch.  With CAP_BPF and
CAP_PERFMON, or a BPF token, a program can so make the kernel follow a
pointer of its choice: after a write over a bpf_timer the kernel crashes
on the written value when the element is removed.

Offsets into a buffer are constants.  Let the verifier record the range of
bytes that a program may write through a read-write buffer, and refuse the
program with -EACCES, at attach and at a link update, if a special field
of the map lies in that range.

A program that only reads, or that writes beside the special fields, works
as before.  One that writes on both sides of a special field is refused,
since the range covers the field.

Fixes: d6c4503cc296 ("bpf: Implement bpf iterator for hash maps")
Fixes: 5ce6e77c7edf ("bpf: Implement bpf iterator for sock local storage map")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 include/linux/bpf.h       |  5 +++++
 kernel/bpf/map_iter.c     | 36 ++++++++++++++++++++++++++++++++----
 kernel/bpf/verifier.c     | 12 +++++++++---
 net/core/bpf_sk_storage.c |  2 +-
 4 files changed, 47 insertions(+), 8 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 1823ca3a1d5e..22947fa192e4 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1866,6 +1866,9 @@ struct bpf_prog_aux {
 	u32 ctx_arg_info_size;
 	u32 max_rdonly_access;
 	u32 max_rdwr_access;
+	/* [min, max): the bytes of a rdwr buffer that may be written */
+	u32 min_rdwr_write;
+	u32 max_rdwr_write;
 	u32 subprog_start;
 	struct btf *attach_btf;
 	struct bpf_ctx_arg_aux *ctx_arg_info;
@@ -3204,6 +3207,8 @@ void bpf_iter_map_show_fdinfo(const struct bpf_iter_aux_info *aux,
 			      struct seq_file *seq);
 int bpf_iter_map_fill_link_info(const struct bpf_iter_aux_info *aux,
 				struct bpf_link_info *info);
+int bpf_iter_map_check_value_write(const struct bpf_prog *prog,
+				   const struct bpf_map *map);
 
 int map_set_for_each_callback_args(struct bpf_verifier_env *env,
 				   struct bpf_func_state *caller,
diff --git a/kernel/bpf/map_iter.c b/kernel/bpf/map_iter.c
index 16537356c1a2..a58c148692e6 100644
--- a/kernel/bpf/map_iter.c
+++ b/kernel/bpf/map_iter.c
@@ -97,22 +97,50 @@ static struct bpf_iter_reg bpf_map_reg_info = {
 	.seq_info		= &bpf_map_seq_info,
 };
 
+/*
+ * For an iterator that hands the live map value to the program as a writable
+ * buffer: the verifier knows nothing of the map, so the special fields of the
+ * value have to lie outside of what the program may write.
+ */
+int bpf_iter_map_check_value_write(const struct bpf_prog *prog,
+				   const struct bpf_map *map)
+{
+	const struct btf_record *rec = map->record;
+	const struct btf_field *field;
+	u32 i;
+
+	if (IS_ERR_OR_NULL(rec) || !prog->aux->max_rdwr_write)
+		return 0;
+
+	for (i = 0; i < rec->cnt; i++) {
+		field = &rec->fields[i];
+		if (field->offset < prog->aux->max_rdwr_write &&
+		    field->offset + field->size > prog->aux->min_rdwr_write)
+			return -EACCES;
+	}
+
+	return 0;
+}
+
 static int bpf_iter_map_check_prog(const struct bpf_prog *prog,
 				   const struct bpf_iter_aux_info *aux)
 {
 	const struct bpf_map *map = aux->map;
 	u32 value_size = map->value_size;
+	bool is_percpu;
 
-	if (map->map_type == BPF_MAP_TYPE_PERCPU_HASH ||
-	    map->map_type == BPF_MAP_TYPE_LRU_PERCPU_HASH ||
-	    map->map_type == BPF_MAP_TYPE_PERCPU_ARRAY)
+	is_percpu = map->map_type == BPF_MAP_TYPE_PERCPU_HASH ||
+		    map->map_type == BPF_MAP_TYPE_LRU_PERCPU_HASH ||
+		    map->map_type == BPF_MAP_TYPE_PERCPU_ARRAY;
+	if (is_percpu)
 		value_size = round_up(value_size, 8) * num_possible_cpus();
 
 	if (prog->aux->max_rdonly_access > map->key_size ||
 	    prog->aux->max_rdwr_access > value_size)
 		return -EACCES;
 
-	return 0;
+	/* the program gets a copy of a per-cpu value */
+	return is_percpu ? 0 : bpf_iter_map_check_value_write(prog, map);
 }
 
 static int bpf_iter_attach_map(struct bpf_prog *prog,
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 0d1b729989d6..2c812a5f1da8 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -6053,9 +6053,10 @@ static int check_buffer_access(struct bpf_verifier_env *env,
 			       const struct bpf_reg_state *reg,
 			       argno_t argno, int off, int size,
 			       bool zero_size_allowed,
-			       u32 *max_access)
+			       u32 *max_access, bool write)
 {
 	const char *buf_info = type_is_rdonly_mem(reg->type) ? "rdonly" : "rdwr";
+	struct bpf_prog_aux *aux = env->prog->aux;
 	u32 access_end;
 	int err;
 
@@ -6064,6 +6065,11 @@ static int check_buffer_access(struct bpf_verifier_env *env,
 		return err;
 
 	*max_access = max(access_end, *max_access);
+	if (write && size) {
+		if (!aux->max_rdwr_write || access_end - size < aux->min_rdwr_write)
+			aux->min_rdwr_write = access_end - size;
+		aux->max_rdwr_write = max(access_end, aux->max_rdwr_write);
+	}
 
 	return 0;
 }
@@ -7339,7 +7345,7 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, struct b
 		}
 
 		err = check_buffer_access(env, reg, argno, off, size, false,
-					  max_access);
+					  max_access, t == BPF_WRITE);
 
 		if (!err && value_regno >= 0 && (rdonly_mem || t == BPF_READ))
 			mark_reg_unknown(env, regs, value_regno);
@@ -7852,7 +7858,7 @@ static int check_helper_mem_access(struct bpf_verifier_env *env, struct bpf_reg_
 		}
 		return check_buffer_access(env, reg, argno, 0,
 					   access_size, zero_size_allowed,
-					   max_access);
+					   max_access, access_type == BPF_WRITE);
 	case PTR_TO_STACK:
 		return check_stack_range_initialized(
 				env, reg,
diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
index 72b9ee4c98de..1c59300af05c 100644
--- a/net/core/bpf_sk_storage.c
+++ b/net/core/bpf_sk_storage.c
@@ -925,7 +925,7 @@ static int bpf_iter_map_check_prog(const struct bpf_prog *prog,
 	if (prog->aux->max_rdwr_access > aux->map->value_size)
 		return -EACCES;
 
-	return 0;
+	return bpf_iter_map_check_value_write(prog, aux->map);
 }
 
 static int bpf_iter_attach_map(struct bpf_prog *prog,
-- 
2.55.0

[PATCH bpf-next v2 30/43] bpf: Free what map_free() unlinks for sleepable readers

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

bpf_local_storage_map_free() takes the elements of the map off the lists
of their owners, which are alive, and frees them, and the storage of an
owner with its last element, behind a plain RCU grace period.  Nobody uses
the map any more, but the other maps of these owners are in use.  A
sleepable program calls the get and delete helpers under
rcu_read_lock_trace() only.  While it walks the list of an owner for its
own map, or is about to lock the storage, the element it stands on, or the
storage, can be freed under it.

Whatever else takes an element off the list of a live owner frees it
behind an RCU Tasks Trace grace period, which implies a plain one.  Do so
in bpf_local_storage_map_free() as well.  It waits for these callbacks
before it frees the map.

This does not help a sleepable program that hands an owner to the helpers
which nobody holds, and which may be gone by then.

Fixes: dcf456c9a095 ("bpf: Fix usage of trace RCU in local storage.")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/bpf_local_storage.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/bpf_local_storage.c b/kernel/bpf/bpf_local_storage.c
index c08afb33d038..49ea00c99390 100644
--- a/kernel/bpf/bpf_local_storage.c
+++ b/kernel/bpf/bpf_local_storage.c
@@ -431,13 +431,17 @@ static void bpf_selem_unlink_nofail(struct bpf_local_storage_elem *selem,
 	 * then freed after an RCU grace period. However, if destroy() and map_free() are
 	 * racing or rqspinlock returns errors in unlikely situations (unlink != 2), free
 	 * the selem only after both map_free() and destroy() see the selem.
+	 *
+	 * The owner that map_free() takes an selem from is alive: sleepable code, which
+	 * is in no RCU read side critical section, may be walking its list for another
+	 * map.
 	 */
 	if (unlink == 2 ||
 	    atomic_cmpxchg(&selem->state, SELEM_UNLINKED, SELEM_TOFREE) == SELEM_UNLINKED)
-		bpf_selem_free(selem, true);
+		bpf_selem_free(selem, !in_map_free);
 
 	if (free_storage)
-		bpf_local_storage_free(local_storage, true);
+		bpf_local_storage_free(local_storage, !in_map_free);
 }
 
 void __bpf_local_storage_insert_cache(struct bpf_local_storage *local_storage,
-- 
2.55.0

[PATCH bpf-next v2 31/43] bpf: Keep values of a dead owner for sleepable programs

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

bpf_local_storage_destroy() frees the elements of an owner behind a plain
RCU grace period, on the ground that no program can refer to an owner that
goes away.  But the pointer that bpf_task_storage_get() and its like
return stays valid for the verifier after the program has released the
owner, or has left the bpf_rcu_read_lock() section it had the owner from.
A sleepable program is in no RCU read side critical section, so it can
write to the value after the owner has died and the element was freed:

  BUG: KASAN: slab-use-after-free in _copy_from_user+0x42/0xa0
  Write of size 8 at addr ffff8880074404a8 by task lsuaf/55

Free the element behind an RCU Tasks Trace grace period if sleepable code
may use the map.  A program that a map is bound to was loaded, and counted
in map->sleepable_refcnt, before it got a value from the live owner, and
stays counted until a grace period behind its last run.

A program can also get at a storage map by a lookup in an outer map.  Then
only the outer map is counted, its programs can come after the inner map
was put into it, and can hold on to the inner map after it was taken out.
So note in a map that it has been an inner map, and treat it like one with
sleepable users from then on.

The same goes for what the special fields of the value hold.  The page of
a uptr has to stay pinned, and a kptr which the program stores after the
owner has died has to be released.  Leave the fields, and the smap that it
takes to free them, to bpf_selem_free_trace_rcu(), as bpf_selem_unlink()
does.  bpf_local_storage_map_free() waits for that callback.

Elements of such maps now wait for sleepable programs that are running
when their owner dies.  Other maps are freed as before.

Fixes: dcf456c9a095 ("bpf: Fix usage of trace RCU in local storage.")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 include/linux/bpf.h            |  1 +
 kernel/bpf/bpf_local_storage.c | 30 ++++++++++++++++++++++++------
 kernel/bpf/map_in_map.c        |  7 +++++--
 3 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 22947fa192e4..a10af47978a4 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -341,6 +341,7 @@ struct bpf_map {
 	bool frozen; /* write-once; write-protected by freeze_mutex */
 	bool free_after_mult_rcu_gp;
 	bool free_after_rcu_gp;
+	bool was_inner_map;
 	atomic64_t sleepable_refcnt;
 	s64 __percpu *elem_count;
 	u64 cookie; /* write-once */
diff --git a/kernel/bpf/bpf_local_storage.c b/kernel/bpf/bpf_local_storage.c
index 49ea00c99390..699430f97178 100644
--- a/kernel/bpf/bpf_local_storage.c
+++ b/kernel/bpf/bpf_local_storage.c
@@ -352,7 +352,8 @@ int bpf_selem_unlink(struct bpf_local_storage_elem *selem)
 static void bpf_selem_unlink_nofail(struct bpf_local_storage_elem *selem,
 				    struct bpf_local_storage_map_bucket *b)
 {
-	bool in_map_free = !!b, free_storage = false;
+	bool in_map_free = !!b, free_storage = false, sleepable_users = false;
+	bool keep_fields = false;
 	struct bpf_local_storage *local_storage;
 	struct bpf_local_storage_map *smap;
 	unsigned long flags;
@@ -362,17 +363,26 @@ static void bpf_selem_unlink_nofail(struct bpf_local_storage_elem *selem,
 	smap = rcu_dereference_check(SDATA(selem)->smap, bpf_rcu_lock_held());
 
 	if (smap) {
+		/* A map that is freed, or NULL here, has no program left. */
+		sleepable_users = !in_map_free &&
+				  (atomic64_read(&smap->map.sleepable_refcnt) ||
+				   READ_ONCE(smap->map.was_inner_map));
 		b = b ? : select_bucket(smap, local_storage);
 		err = raw_res_spin_lock_irqsave(&b->lock, flags);
 		if (!err) {
 			/*
 			 * Call bpf_obj_free_fields() under b->lock to make sure it is done
 			 * exactly once for an selem. Safe to free special fields immediately
-			 * as no BPF program should be referencing the selem.
+			 * as no BPF program should be referencing the selem, unless it
+			 * is sleepable: see below.
 			 */
 			if (likely(selem_linked_to_map(selem))) {
 				hlist_del_init_rcu(&selem->map_node);
-				bpf_obj_free_fields(smap->map.record, SDATA(selem)->data);
+				if (sleepable_users)
+					keep_fields = true;
+				else
+					bpf_obj_free_fields(smap->map.record,
+							    SDATA(selem)->data);
 				unlink++;
 			}
 			raw_res_spin_unlock_irqrestore(&b->lock, flags);
@@ -387,7 +397,14 @@ static void bpf_selem_unlink_nofail(struct bpf_local_storage_elem *selem,
 		 * with selem1 as raw_res_spin_lock_irqsave() returns -ETIMEDOUT.
 		 */
 		WARN_ON_ONCE(err && in_map_free);
-		if (!err || in_map_free)
+		/*
+		 * A sleepable program can still use, and fill, the fields of a value
+		 * that it got before it let go of the owner. As for bpf_selem_unlink(),
+		 * bpf_selem_free_trace_rcu() frees them, and takes smap for that:
+		 * map_free() waits for this RCU read side critical section and then
+		 * for the callback.
+		 */
+		if ((!err && !keep_fields) || in_map_free)
 			RCU_INIT_POINTER(SDATA(selem)->smap, NULL);
 	}
 
@@ -434,11 +451,12 @@ static void bpf_selem_unlink_nofail(struct bpf_local_storage_elem *selem,
 	 *
 	 * The owner that map_free() takes an selem from is alive: sleepable code, which
 	 * is in no RCU read side critical section, may be walking its list for another
-	 * map.
+	 * map.  And it can still use a value that it got before it let go of an owner
+	 * that destroy() is called for.
 	 */
 	if (unlink == 2 ||
 	    atomic_cmpxchg(&selem->state, SELEM_UNLINKED, SELEM_TOFREE) == SELEM_UNLINKED)
-		bpf_selem_free(selem, !in_map_free);
+		bpf_selem_free(selem, !in_map_free && !sleepable_users);
 
 	if (free_storage)
 		bpf_local_storage_free(local_storage, !in_map_free);
diff --git a/kernel/bpf/map_in_map.c b/kernel/bpf/map_in_map.c
index 5ee8aae7435b..0fa3f2ac0f99 100644
--- a/kernel/bpf/map_in_map.c
+++ b/kernel/bpf/map_in_map.c
@@ -110,10 +110,13 @@ void *bpf_map_fd_get_ptr(struct bpf_map *map,
 		return ERR_PTR(-ENOTSUPP);
 
 	inner_map_meta = map->inner_map_meta;
-	if (inner_map_meta->ops->map_meta_equal(inner_map_meta, inner_map))
+	if (inner_map_meta->ops->map_meta_equal(inner_map_meta, inner_map)) {
 		bpf_map_inc(inner_map);
-	else
+		/* Its users are those of the outer maps from now on */
+		WRITE_ONCE(inner_map->was_inner_map, true);
+	} else {
 		inner_map = ERR_PTR(-EINVAL);
+	}
 
 	return inner_map;
 }
-- 
2.55.0

[PATCH bpf-next v2 32/43] bpf: Don't take an extension for a function that runs sleepable

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

An extension cannot be sleepable, and it is verified as a program that
runs under the RCU read lock.  But it runs in the context of the function
that it replaces.  For a sleepable program, or a function that is called
from a bpf_wq or bpf_task_work callback, that is rcu_read_lock_trace().

The verifier lets such an extension use maps and pointers that rely on
RCU.  Its maps are not counted as maps with sleepable users either, so
that what it got from them is freed behind a plain RCU grace period:

  WARNING: net/core/sock_map.c:903 at __sock_hash_lookup_elem+0xe8/0xf0

  BUG: KASAN: slab-use-after-free in bpf_prog_b3bad285227d855b_ext_storage+0x245/0x297
  Write of size 8 at addr ffff888108717a50 by task fr/955

The verifier knows the contexts that a global function is called in.
Refuse to replace one that is called without the RCU read lock, and the
main function of a sleepable program.  A function that sleepable code only
calls under bpf_rcu_read_lock() can still be replaced.

The summarization test has replaced functions of sleepable programs by
ones that do not sleep.  Expect that to fail.

Fixes: 1e6c62a88215 ("bpf: Introduce sleepable BPF programs")
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/verifier.c                         | 20 ++++++++++++++++++
 .../selftests/bpf/prog_tests/summarization.c  | 21 +++++--------------
 2 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 2c812a5f1da8..d738ce6e7428 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -22151,6 +22151,21 @@ static int check_unreliable_func_args(struct bpf_verifier_log *log, struct btf *
 	return -EINVAL;
 }
 
+/*
+ * An extension is verified as a program that runs under the RCU read lock. Does
+ * the function that it is to replace?
+ */
+static bool func_has_rcu_read_lock(const struct bpf_prog *prog, int subprog)
+{
+	const struct bpf_func_info_aux *sub_aux = &prog->aux->func_info_aux[subprog];
+
+	if (!subprog)
+		return !prog->sleepable;
+
+	return !sub_aux->called[BPF_FUNC_CTX_SLEEPABLE] &&
+	       !sub_aux->called[BPF_FUNC_CTX_NO_RCU_LOCK];
+}
+
 int bpf_check_attach_target(struct bpf_verifier_log *log,
 			    const struct bpf_prog *prog,
 			    const struct bpf_prog *tgt_prog,
@@ -22247,6 +22262,11 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
 					"Cannot extend a writable raw tracepoint program\n");
 				return -EINVAL;
 			}
+			if (!func_has_rcu_read_lock(tgt_prog, subprog)) {
+				bpf_log(log,
+					"Cannot extend a function that runs without RCU read lock\n");
+				return -EINVAL;
+			}
 			tgt_changes_pkt_data = aux->func
 					       ? aux->func[subprog]->aux->changes_pkt_data
 					       : aux->changes_pkt_data;
diff --git a/tools/testing/selftests/bpf/prog_tests/summarization.c b/tools/testing/selftests/bpf/prog_tests/summarization.c
index 6951786044ca..3e4bcf071735 100644
--- a/tools/testing/selftests/bpf/prog_tests/summarization.c
+++ b/tools/testing/selftests/bpf/prog_tests/summarization.c
@@ -53,17 +53,6 @@ static void test_aux(const char *main_prog_name,
 	err = summarization_freplace__load(freplace);
 	print_verifier_log(log);
 
-	/* The might_sleep extension doesn't work yet as sleepable calls are not
-	 * allowed, but preserve the check in case it's supported later and then
-	 * this particular combination can be enabled.
-	 */
-	if (!strcmp("might_sleep", replacement) && err) {
-		ASSERT_HAS_SUBSTR(log, "sleepable helper bpf_copy_from_user#", "error log");
-		ASSERT_EQ(err, -EINVAL, "err");
-		test__skip();
-		goto out;
-	}
-
 	if (expect_load) {
 		ASSERT_OK(err, "summarization_freplace__load");
 	} else {
@@ -86,9 +75,8 @@ static void test_aux(const char *main_prog_name,
  * Also test a case when main subprogram itself is replaced and is a single
  * subprogram in a program.
  *
- * This holds for might_sleep programs. It is ok to replace might_sleep with
- * might_sleep and with does_not_sleep, but does_not_sleep cannot be replaced
- * with might_sleep.
+ * An extension is verified as a non-sleepable program, hence nothing that runs
+ * sleepable can be replaced.
  */
 void test_summarization_freplace(void)
 {
@@ -111,7 +99,7 @@ void test_summarization_freplace(void)
 		},
 	};
 	const char *pkt_err = "Extension program changes packet data";
-	const char *slp_err = "Extension program may sleep";
+	const char *slp_err = "Cannot extend a function that runs without RCU read lock";
 	struct {
 		const char *func;
 		bool has_side_effect;
@@ -136,7 +124,8 @@ void test_summarization_freplace(void)
 				if (!test__start_subtest(buf))
 					continue;
 				test_aux(mains[t][i].main, mains[t][i].to_be_replaced, replacements[t][j].func,
-					 mains[t][i].has_side_effect || !replacements[t][j].has_side_effect,
+					 t == 0 &&
+					 (mains[t][i].has_side_effect || !replacements[t][j].has_side_effect),
 					 replacements[t][j].err_msg);
 			}
 		}
-- 
2.55.0

[PATCH bpf-next v2 33/43] bpf: Keep the symbols of a program until it has ended

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

When the last reference to a program goes, __bpf_prog_put_noref() takes
its symbols out of the kallsyms tree and frees its line info at once, and
waits for the grace periods only with the rest.  But the program may still
be running: the link or the array it was called from is gone, the call is
not over.

search_bpf_extables() finds the exception table of a program by its
symbol.  A load that faults, and that the table is there to fix up, is
fatal from then on:

  Oops: general protection fault, probably for non-canonical address
  0xdead0000000006f8: 0000 [#1] SMP NOPTI
  RIP: 0010:0xffffffffc06008f6

It takes a program that is detached and closed on one CPU while it runs on
another.  Unwinders and bpf_prog_find_from_stack() do not know the program
any more either, and the stack dump of a BPF stream reads the freed line
info.

Free the line info behind the grace periods the program needs, as it is
done with the BTF that its text comes from.  Remove the symbols in
bpf_prog_free_deferred(), which runs behind them as well.  The symbols are
looked up under rcu_read_lock(), so the images and the program itself are
freed one more grace period later.  All that the program refers to is
released when it was before.

Fixes: 3dec541b2e63 ("bpf: Add support for BTF pointers to x86 JIT")
Fixes: d7c431cafcb4 ("bpf: Add dump_stack() analogue to print to BPF stderr")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 include/linux/bpf.h  |  1 +
 kernel/bpf/core.c    | 50 +++++++++++++++++++++++++++++++-------------
 kernel/bpf/syscall.c |  5 ++---
 3 files changed, 38 insertions(+), 18 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index a10af47978a4..feb7de6483e3 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1971,6 +1971,7 @@ struct bpf_prog_aux {
 	union {
 		struct work_struct work;
 		struct rcu_head	rcu;
+		struct rcu_work rwork;
 	};
 	struct bpf_stream *stream[2];
 	struct mutex st_ops_assoc_mutex;
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index e1216927373a..6e32db9777c2 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -3085,12 +3085,40 @@ static void bpf_free_used_btfs(struct bpf_prog_aux *aux)
 	kfree(aux->used_btfs);
 }
 
-static void bpf_prog_free_deferred(struct work_struct *work)
+static void bpf_prog_free_images(struct work_struct *work)
 {
 	struct bpf_prog_aux *aux;
 	int i;
 
+	aux = container_of(to_rcu_work(work), struct bpf_prog_aux, rwork);
+	for (i = 0; i < aux->real_func_cnt; i++) {
+		/* We can just unlink the subprog poke descriptor table as
+		 * it was originally linked to the main program and is also
+		 * released along with it.
+		 */
+		aux->func[i]->aux->poke_tab = NULL;
+		bpf_jit_free(aux->func[i]);
+	}
+	if (aux->real_func_cnt) {
+		kfree(aux->func);
+		bpf_prog_unlock_free(aux->prog);
+	} else {
+		bpf_jit_free(aux->prog);
+	}
+}
+
+static void bpf_prog_free_deferred(struct work_struct *work)
+{
+	struct bpf_prog_aux *aux;
+	bool ksyms;
+
 	aux = container_of(work, struct bpf_prog_aux, work);
+	/*
+	 * A prog that still runs when its last reference goes needs its symbols,
+	 * by which its exception table is found.  It has ended by now.
+	 */
+	ksyms = aux->real_func_cnt || !list_empty(&aux->ksym.lnode);
+	bpf_prog_kallsyms_del_all(aux->prog);
 #ifdef CONFIG_BPF_SYSCALL
 	bpf_free_kfunc_btf_tab(aux->kfunc_btf_tab);
 #endif
@@ -3109,20 +3137,12 @@ static void bpf_prog_free_deferred(struct work_struct *work)
 #endif
 	if (aux->dst_trampoline)
 		bpf_trampoline_put(aux->dst_trampoline);
-	for (i = 0; i < aux->real_func_cnt; i++) {
-		/* We can just unlink the subprog poke descriptor table as
-		 * it was originally linked to the main program and is also
-		 * released along with it.
-		 */
-		aux->func[i]->aux->poke_tab = NULL;
-		bpf_jit_free(aux->func[i]);
-	}
-	if (aux->real_func_cnt) {
-		kfree(aux->func);
-		bpf_prog_unlock_free(aux->prog);
-	} else {
-		bpf_jit_free(aux->prog);
-	}
+	/* The symbols are looked up under rcu_read_lock() */
+	INIT_RCU_WORK(&aux->rwork, bpf_prog_free_images);
+	if (ksyms)
+		queue_rcu_work(system_percpu_wq, &aux->rwork);
+	else
+		bpf_prog_free_images(&aux->rwork.work);
 }
 
 void bpf_prog_free(struct bpf_prog *fp)
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index c412bcd1903f..ddb986324f2f 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2461,6 +2461,8 @@ static void __bpf_prog_put_rcu(struct rcu_head *rcu)
 	struct bpf_prog_aux *aux = container_of(rcu, struct bpf_prog_aux, rcu);
 
 	btf_put(aux->btf);
+	kvfree(aux->jited_linfo);
+	kvfree(aux->linfo);
 	kvfree(aux->func_info);
 	kfree(aux->func_info_aux);
 	free_uid(aux->user);
@@ -2485,10 +2487,7 @@ static void __bpf_prog_put_rcu_tasks(struct rcu_head *rcu)
 
 static void __bpf_prog_put_noref(struct bpf_prog *prog, bool deferred)
 {
-	bpf_prog_kallsyms_del_all(prog);
 	module_put(prog->aux->mod);
-	kvfree(prog->aux->jited_linfo);
-	kvfree(prog->aux->linfo);
 	kfree(prog->aux->kfunc_tab);
 	kfree(prog->aux->ctx_arg_info);
 	if (prog->aux->attach_btf)
-- 
2.55.0

[PATCH bpf-next v2 34/43] bpf: Use the socket of a sk_lookup test run under RCU

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

bpf_sk_lookup_assign() accepts only sockets which are freed by RCU, and
takes no reference. bpf_prog_test_run_sk_lookup() looks at sk_reuseport of
the selected socket and generates its cookie, which is a write, behind
bpf_test_timer_leave(), that is outside of the RCU read side critical
section in which the program ran. If the socket is closed in the meantime
it may be gone.

Do both before the section ends.

Fixes: 7c32e8f8bc33 ("bpf: Add PROG_TEST_RUN support for sk_lookup programs")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 net/bpf/test_run.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index d73cd247f8cd..3043a7393c29 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -1656,20 +1656,19 @@ int bpf_prog_test_run_sk_lookup(struct bpf_prog *prog, const union bpf_attr *kat
 		ctx.selected_sk = NULL;
 		retval = BPF_PROG_SK_LOOKUP_RUN_ARRAY(progs, ctx, bpf_prog_run);
 	} while (bpf_test_timer_continue(&t, 1, repeat, &ret, &duration));
-	bpf_test_timer_leave(&t);
-
-	if (ret < 0)
-		goto out;
 
+	/* Nothing but RCU keeps the selected socket */
 	user_ctx->cookie = 0;
-	if (ctx.selected_sk) {
-		if (ctx.selected_sk->sk_reuseport && !ctx.no_reuseport) {
+	if (!ret && ctx.selected_sk) {
+		if (ctx.selected_sk->sk_reuseport && !ctx.no_reuseport)
 			ret = -EOPNOTSUPP;
-			goto out;
-		}
-
-		user_ctx->cookie = sock_gen_cookie(ctx.selected_sk);
+		else
+			user_ctx->cookie = sock_gen_cookie(ctx.selected_sk);
 	}
+	bpf_test_timer_leave(&t);
+
+	if (ret < 0)
+		goto out;
 
 	ret = bpf_test_finish(kattr, uattr, NULL, NULL, 0, 0, retval, duration);
 	if (!ret)
-- 
2.55.0

[PATCH bpf-next v2 35/43] bpf, cpumap: Hold the device of frames in the queue

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

A frame that is redirected to another CPU waits in the queue of the cpumap
entry with a plain pointer to the device it came from, and an skb from
generic XDP with skb->dev.  Nothing keeps that device.  Closing and
unregistering it waits for its NAPI and for RCU, but not for the kthread
of the entry.  If the kthread is late by more than that, since its CPU is
busy with something of a higher priority, it builds skbs for a device that
has been freed and hands them to the stack:

  BUG: KASAN: use-after-free in dev_gro_receive+0x1f51/0x2b30
  Read of size 8 at addr ffff88800a7340b0 by task cpumap/1/map:1/54

Take a reference to the device for each entry of the queue.  The kthread
cannot drop it at the end of the batch, since its GRO node may keep the skb
for longer.  Let it note the devices and drop the references behind a full
GRO flush, which it does whenever the queue is empty, and from now on also
when it has noted about 2048 of them.

A device whose frames are stuck in a queue is now unregistered only when
the kthread has got to them.  RTNL is not held meanwhile, but if the device
goes with its network namespace, the cleanup of all namespaces waits.

Fixes: 1c601d829ab0 ("bpf: cpumap xdp_buff to skb conversion and allocation")
Fixes: 11941f8a8536 ("bpf: cpumap: Implement generic cpumap")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/cpumap.c | 35 ++++++++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 7 deletions(-)

diff --git a/kernel/bpf/cpumap.c b/kernel/bpf/cpumap.c
index 5e59ab896f05..eb87131b1bef 100644
--- a/kernel/bpf/cpumap.c
+++ b/kernel/bpf/cpumap.c
@@ -45,6 +45,7 @@
  */
 
 #define CPU_MAP_BULK_SIZE 8  /* 8 == one cacheline on 64-bit archs */
+#define CPU_MAP_HELD_DEVS 2048
 struct bpf_cpu_map_entry;
 struct bpf_cpu_map;
 
@@ -74,6 +75,13 @@ struct bpf_cpu_map_entry {
 
 	struct completion kthread_running;
 	struct rcu_work free_work;
+
+	/*
+	 * Every frame and skb in the queue holds a reference to its device.
+	 * The kthread keeps it until nothing is left in GRO.
+	 */
+	struct net_device *held[CPU_MAP_HELD_DEVS];
+	u32 nr_held;
 };
 
 struct bpf_cpu_map {
@@ -129,9 +137,11 @@ static void __cpu_map_ring_cleanup(struct ptr_ring *ring)
 		WARN_ON_ONCE(1);
 		if (unlikely(__ptr_test_bit(0, &ptr))) {
 			__ptr_clear_bit(0, &ptr);
+			dev_put(((struct sk_buff *)ptr)->dev);
 			kfree_skb(ptr);
 			continue;
 		}
+		dev_put(((struct xdp_frame *)ptr)->dev_rx);
 		xdp_return_frame(ptr);
 	}
 }
@@ -311,7 +321,7 @@ static int cpu_map_kthread_run(void *data)
 		void *frames[CPUMAP_BATCH];
 		void *skbs[CPUMAP_BATCH];
 		u32 i, n, m;
-		bool empty;
+		bool empty, full;
 
 		/* Release CPU reschedule checks */
 		if (__ptr_ring_empty(rcpu->queue)) {
@@ -344,10 +354,12 @@ static int cpu_map_kthread_run(void *data)
 				struct sk_buff *skb = f;
 
 				__ptr_clear_bit(0, &skb);
+				rcpu->held[rcpu->nr_held++] = skb->dev;
 				skbs[ret.skb_n++] = skb;
 				continue;
 			}
 
+			rcpu->held[rcpu->nr_held++] = ((struct xdp_frame *)f)->dev_rx;
 			frames[ret.xdp_n++] = f;
 			page = virt_to_page(f);
 
@@ -398,10 +410,13 @@ static int cpu_map_kthread_run(void *data)
 		/* Flush either every 64 packets or in case of empty ring */
 		packets += n;
 		empty = __ptr_ring_empty(rcpu->queue);
-		if (packets >= NAPI_POLL_WEIGHT || empty) {
-			cpu_map_gro_flush(rcpu, empty);
+		full = empty || rcpu->nr_held > CPU_MAP_HELD_DEVS - CPUMAP_BATCH;
+		if (packets >= NAPI_POLL_WEIGHT || full) {
+			cpu_map_gro_flush(rcpu, full);
 			packets = 0;
 		}
+		while (full && rcpu->nr_held)
+			dev_put(rcpu->held[--rcpu->nr_held]);
 
 		local_bh_enable(); /* resched point, may call do_softirq() */
 	}
@@ -738,12 +753,14 @@ static void bq_flush_to_queue(struct xdp_bulk_queue *bq)
 
 	for (i = 0; i < bq->count; i++) {
 		struct xdp_frame *xdpf = bq->q[i];
-		int err;
 
-		err = __ptr_ring_produce(q, xdpf);
-		if (err) {
+		if (__ptr_ring_full(q)) {
 			drops++;
 			xdp_return_frame_rx_napi(xdpf);
+		} else {
+			/* The device cannot go before the flush, from now on it can */
+			dev_hold(xdpf->dev_rx);
+			__ptr_ring_produce(q, xdpf);
 		}
 		processed++;
 	}
@@ -807,11 +824,15 @@ int cpu_map_generic_redirect(struct bpf_cpu_map_entry *rcpu,
 
 	__skb_pull(skb, skb->mac_len);
 	skb_set_redirected(skb, false);
+	dev_hold(skb->dev);
 	__ptr_set_bit(0, &skb);
 
 	ret = ptr_ring_produce(rcpu->queue, skb);
-	if (ret < 0)
+	if (ret < 0) {
+		__ptr_clear_bit(0, &skb);
+		dev_put(skb->dev);
 		goto trace;
+	}
 
 	wake_up_process(rcpu->kthread);
 trace:
-- 
2.55.0

[PATCH bpf-next v2 36/43] bpf: Drop packet pointers behind kfuncs writing an skb

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

bpf_dynptr_write() to the dynptr of an skb ends in
__bpf_skb_store_bytes(), which makes the range writable first. That pulls
data into the head or unclones it, and either can reallocate the head. So
check_helper_call() invalidates all packet pointers and all slices of the
skb behind the helper.

Several kfuncs take the same road: bpf_dynptr_copy() and
bpf_dynptr_memset(), if they get no writable slice of the destination
because it is not linear, and bpf_probe_read_{user,kernel}[_str]_dynptr()
and bpf_copy_from_user[_task][_str]_dynptr() likewise. The verifier does
not know. A program can keep a packet pointer or a slice across such a
call and read and write the old head, which has been freed.

Treat a kfunc of that kind as bpf_dynptr_write() is treated, by the type
of the dynptr it writes to, which is the first argument of all of them.
Where a global function is only scanned for what it may do, the type is
not known yet. There any call of these kfuncs counts, as a tail call does.

Fixes: daec295a7094 ("bpf/helpers: Introduce bpf_dynptr_copy kfunc")
Fixes: a498ee7576de ("bpf: Implement dynptr copy kfuncs")
Fixes: 5fc5d8fded57 ("bpf: Add bpf_dynptr_memset() kfunc")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 include/linux/bpf_verifier.h |  2 ++
 kernel/bpf/verifier.c        | 44 ++++++++++++++++++++++++++++++++----
 2 files changed, 42 insertions(+), 4 deletions(-)

diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index e420d211f423..7a72bb06fe56 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -1803,6 +1803,8 @@ struct bpf_call_arg_meta {
 	u32 kfunc_flags;
 	const char *func_name;
 	struct arg_constant_desc arg_constant;
+	/* The type of the dynptr that the kfunc writes through */
+	enum bpf_dynptr_type dynptr_write_type;
 
 	/* arg_{btf,btf_id,owning_ref} are used by kfunc-specific handling,
 	 * generally to pass info about user-defined local kptr types to later
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index d738ce6e7428..597302cf7086 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -1002,6 +1002,11 @@ static enum bpf_dynptr_type dynptr_reg_type(struct bpf_verifier_env *env, struct
 	return bpf_stack_slot(state, spi)->spilled_ptr.dynptr.type;
 }
 
+static bool dynptr_write_changes_pkt_data(enum bpf_dynptr_type type)
+{
+	return type == BPF_DYNPTR_TYPE_SKB || type == BPF_DYNPTR_TYPE_SKB_META;
+}
+
 static bool is_dynptr_type_expected(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
 				    enum bpf_arg_type arg_type)
 {
@@ -8976,6 +8981,7 @@ static bool is_kfunc_arg_scalar_with_name(const struct btf *btf,
 static bool is_bpf_cast_to_kern_ctx_kfunc(const struct bpf_call_arg_meta *meta);
 static bool is_bpf_dynptr_clone_kfunc(const struct bpf_call_arg_meta *meta);
 static bool is_bpf_iter_css_task_new_kfunc(const struct bpf_call_arg_meta *meta);
+static bool is_kfunc_dynptr_write(struct bpf_call_arg_meta *meta);
 static bool is_bpf_obj_drop_kfunc(u32 func_id);
 static bool is_bpf_percpu_obj_drop_kfunc(u32 func_id);
 static bool is_bpf_rbtree_add_kfunc(u32 func_id);
@@ -10040,6 +10046,8 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, u32 slot, u32 p
 		err = process_dynptr_func(env, reg, argno, insn_idx, arg_type, meta);
 		if (err)
 			return err;
+		if (!arg && is_kfunc_dynptr_write(meta))
+			meta->dynptr_write_type = meta->dynptr.type;
 		break;
 	}
 	case ARG_PTR_TO_ITER:
@@ -12654,8 +12662,7 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
 		if (dynptr_type == BPF_DYNPTR_TYPE_INVALID)
 			return -EFAULT;
 
-		if (dynptr_type == BPF_DYNPTR_TYPE_SKB ||
-		    dynptr_type == BPF_DYNPTR_TYPE_SKB_META)
+		if (dynptr_write_changes_pkt_data(dynptr_type))
 			/* this will trigger clear_all_pkt_pointers(), which will
 			 * invalidate all dynptr slices associated with the skb
 			 */
@@ -13704,9 +13711,36 @@ static bool is_kfunc_bpf_preempt_enable(struct bpf_call_arg_meta *meta)
 	return is_kfunc_call(meta, special_kfunc_list[KF_bpf_preempt_enable]);
 }
 
+/* These write to what their first argument, a dynptr, points to */
+BTF_SET_START(dynptr_write_kfuncs)
+BTF_ID(func, bpf_dynptr_copy)
+BTF_ID(func, bpf_dynptr_memset)
+#ifdef CONFIG_BPF_EVENTS
+BTF_ID(func, bpf_probe_read_user_dynptr)
+BTF_ID(func, bpf_probe_read_kernel_dynptr)
+BTF_ID(func, bpf_probe_read_user_str_dynptr)
+BTF_ID(func, bpf_probe_read_kernel_str_dynptr)
+BTF_ID(func, bpf_copy_from_user_dynptr)
+BTF_ID(func, bpf_copy_from_user_str_dynptr)
+BTF_ID(func, bpf_copy_from_user_task_dynptr)
+BTF_ID(func, bpf_copy_from_user_task_str_dynptr)
+#endif
+BTF_SET_END(dynptr_write_kfuncs)
+
+static bool is_kfunc_dynptr_write(struct bpf_call_arg_meta *meta)
+{
+	return meta->btf == btf_vmlinux &&
+	       btf_id_set_contains(&dynptr_write_kfuncs, meta->func_id);
+}
+
+/*
+ * A write through the dynptr of an skb may reallocate its head. Before the
+ * arguments are checked nobody knows what kind of dynptr it is.
+ */
 bool bpf_is_kfunc_pkt_changing(struct bpf_call_arg_meta *meta)
 {
-	return is_kfunc_call(meta, special_kfunc_list[KF_bpf_xdp_pull_data]);
+	return is_kfunc_call(meta, special_kfunc_list[KF_bpf_xdp_pull_data]) ||
+	       is_kfunc_dynptr_write(meta);
 }
 
 static u32 kfunc_abi_slots(const struct btf_func_model *fm)
@@ -15644,7 +15678,9 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
 		}
 	}
 
-	if (bpf_is_kfunc_pkt_changing(&meta))
+	if (is_kfunc_dynptr_write(&meta) ?
+	    dynptr_write_changes_pkt_data(meta.dynptr_write_type) :
+	    bpf_is_kfunc_pkt_changing(&meta))
 		clear_all_pkt_pointers(env);
 
 	proto_slots = kfunc_abi_slots(&desc->func_model);
-- 
2.55.0

[PATCH bpf-next v2 37/43] bpf: Let bpf_dynptr_write() mark a global function

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

The caller of a global function loses its packet pointers if the function
may change packet data. That is found out by a scan for calls of the
helpers which bpf_helper_changes_pkt_data() lists.

bpf_dynptr_write() is not on the list, because it depends on the type of
the dynptr: check_helper_call() looks at it. The scan cannot do that, and
so a global function which makes a dynptr from the skb, or gets one as an
argument, and writes through it counts as harmless. The write can
reallocate the head of the skb, and the caller goes on with pointers into
the old one, which has been freed.

Let every call of bpf_dynptr_write() mark the function, as a tail call
does. The same mark decides whether a program may replace a function.

Fixes: b5964b968ac6 ("bpf: Add skb dynptrs")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/cfg.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/cfg.c b/kernel/bpf/cfg.c
index bce7b5abcfda..2ee950e9ac82 100644
--- a/kernel/bpf/cfg.c
+++ b/kernel/bpf/cfg.c
@@ -598,7 +598,9 @@ static int visit_insn(int t, struct bpf_verifier_env *env)
 			 */
 			if (ret == 0 && fp->might_sleep)
 				mark_subprog_might_sleep(env, t);
-			if (bpf_helper_changes_pkt_data(insn->imm))
+			/* nobody knows yet whether the dynptr is that of an skb */
+			if (bpf_helper_changes_pkt_data(insn->imm) ||
+			    insn->imm == BPF_FUNC_dynptr_write)
 				mark_subprog_changes_pkt_data(env, t);
 		} else if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
 			struct bpf_call_arg_meta meta;
-- 
2.55.0

[PATCH bpf-next v2 38/43] bpf: Don't take a dynptr argument for a local dynptr

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

A global function is verified on its own. A dynptr which it gets as an
argument is assumed to be of type BPF_DYNPTR_TYPE_LOCAL, but the callers
may pass one of any type, that of an skb for example.

The type decides whether a write through the dynptr invalidates packet
pointers, and whether a slice of it is invalidated together with them.
Inside of the function neither happens. It can take a packet pointer from
its context argument, or a slice from the dynptr, write through the dynptr
and go on using the pointer, although the write may have reallocated the
head of the skb.

Report such a dynptr as that of an skb to whoever asks for the type of the
argument of a call. The type in the register stays as it is, nothing
becomes forbidden for the dynptr itself. A clone inherits the caution.

Fixes: a64bfe618665 ("bpf: add support for passing dynptr pointer to global subprog")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/verifier.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 597302cf7086..d384b9ec3d93 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -8416,6 +8416,7 @@ static int process_dynptr_func(struct bpf_verifier_env *env, struct bpf_reg_stat
 			       argno_t argno, int insn_idx, enum bpf_arg_type arg_type,
 			       struct bpf_call_arg_meta *meta)
 {
+	bool is_arg = reg->type == CONST_PTR_TO_DYNPTR;
 	int spi, err = 0;
 
 	/*  MEM_UNINIT - Points to memory that is an appropriate candidate for
@@ -8502,7 +8503,12 @@ static int process_dynptr_func(struct bpf_verifier_env *env, struct bpf_reg_stat
 			reg = &bpf_stack_slot(state, spi)->spilled_ptr;
 		}
 
-		meta->dynptr.type = reg->dynptr.type;
+		/*
+		 * The callers of a global function may hand in a dynptr of any
+		 * type. Take it for that of an skb, so that a write through it
+		 * and its slices are treated with care.
+		 */
+		meta->dynptr.type = is_arg ? BPF_DYNPTR_TYPE_SKB : reg->dynptr.type;
 		meta->dynptr.id = reg->id;
 		meta->dynptr.parent_id = reg->parent_id;
 	}
-- 
2.55.0

[PATCH bpf-next v2 39/43] bpf: Refuse the prologue for an extension that cannot take it

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

A program that writes to packet data starts with bpf_unclone_prologue().
It takes the skb from R1, and if the skb is cloned it calls
bpf_skb_pull_data() and restores R1 only.

That suits the main function of a program.  An extension gets the prologue
too, but it starts with the arguments of the function that it replaces.
If the first one is not the context, something else is taken for an skb:

  BUG: KASAN: stack-out-of-bounds in skb_ensure_writable+0x84/0x1a0
  Read of size 8 at addr ffffc90000270ad0 by task uncl/105
   skb_ensure_writable+0x84/0x1a0
   bpf_skb_pull_data+0x32/0xf0
   bpf_prog_853eb1991d35e093_ext_slice+0x4f/0x321
   bpf_prog_9eb18752b5d0959a_t_glob+0x2ac/0x2de
  BUG: unable to handle page fault for address: 0000000082298821

And whatever R2-R5 held is lost, while the verifier goes on trusting
pointers in them.

Refuse an extension that writes to packet data unless it gets the context
as its first argument and scalars beside it.  A scalar is still clobbered
if the skb is cloned, which the verifier can cope with.

Fixes: be8704ff07d2 ("bpf: Introduce dynamic program extensions")
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/fixups.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
index fe59e3114d82..32911d7f494c 100644
--- a/kernel/bpf/fixups.c
+++ b/kernel/bpf/fixups.c
@@ -858,6 +858,25 @@ int bpf_opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env,
 	return 0;
 }
 
+/*
+ * The prologue takes the context from R1, and it can clobber R2-R5. An
+ * extension gets the arguments of the function that it replaces.
+ */
+static bool ext_prog_takes_prologue(const struct bpf_verifier_env *env)
+{
+	const struct bpf_subprog_info *sub = &env->subprog_info[0];
+	int i, cnt = min_t(int, sub->arg_slot_cnt, MAX_BPF_FUNC_REG_ARGS);
+
+	if (!cnt || sub->args[0].arg_type != ARG_PTR_TO_CTX)
+		return false;
+
+	for (i = 1; i < cnt; i++)
+		if (sub->args[i].arg_type != ARG_SCALAR)
+			return false;
+
+	return true;
+}
+
 /* convert load instructions that access fields of a context type into a
  * sequence of instructions that access fields of the underlying structure:
  *     struct __sk_buff    -> struct sk_buff
@@ -909,6 +928,13 @@ int bpf_convert_ctx_accesses(struct bpf_verifier_env *env)
 			verifier_bug(env, "gen_prologue is null");
 			return -EFAULT;
 		}
+		if (env->seen_direct_write &&
+		    env->prog->type == BPF_PROG_TYPE_EXT &&
+		    !ext_prog_takes_prologue(env)) {
+			verbose(env,
+				"Extension writing packet data must take ctx and scalars only\n");
+			return -EINVAL;
+		}
 		cnt = ops->gen_prologue(insn_buf, env->seen_direct_write,
 					env->prog);
 		if (cnt >= INSN_BUF_SIZE) {
-- 
2.55.0

[PATCH bpf-next v2 40/43] bpf: Unclone the skb for kfuncs that write through a dynptr

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

bpf_dynptr_copy(), bpf_dynptr_memset() and the kfuncs that read user or
kernel memory into a dynptr ask bpf_dynptr_slice_rdwr() for a slice and
write to it.  For the linear part of an skb that is a pointer into its
head.  bpf_dynptr_slice_rdwr() counts on the verifier to have uncloned the
skb, see bpf_unclone_prologue().

The verifier does so if the program itself calls bpf_dynptr_slice_rdwr()
or writes to the packet.  A program that only uses these kfuncs gets no
prologue, and changes the data of all the clones of the skb.  A packet
socket, whose clone is taken before tc ingress runs, sees what
bpf_dynptr_memset() has written there.

Treat a call that may write to an skb like a direct write.

Fixes: daec295a7094 ("bpf/helpers: Introduce bpf_dynptr_copy kfunc")
Fixes: a498ee7576de ("bpf: Implement dynptr copy kfuncs")
Fixes: 5fc5d8fded57 ("bpf: Add bpf_dynptr_memset() kfunc")
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/verifier.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index d384b9ec3d93..47e2969928a4 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -15689,6 +15689,14 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
 	    bpf_is_kfunc_pkt_changing(&meta))
 		clear_all_pkt_pointers(env);
 
+	/*
+	 * These kfuncs write to the head of the skb in place if what they write
+	 * to is linear. Like a direct write that takes bpf_unclone_prologue().
+	 */
+	if (is_kfunc_dynptr_write(&meta) &&
+	    dynptr_write_changes_pkt_data(meta.dynptr_write_type))
+		may_access_direct_pkt_data(env, NULL, BPF_WRITE);
+
 	proto_slots = kfunc_abi_slots(&desc->func_model);
 	if (proto_slots > MAX_BPF_FUNC_REG_ARGS) {
 		struct bpf_func_state *caller = cur_func(env);
-- 
2.55.0

[PATCH bpf-next v2 41/43] bpf: Follow the head if an skb is written from a slice

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

The source of bpf_dynptr_write() may be a slice of the skb which is
written to, that is a pointer into its head. bpf_dynptr_copy() does it by
itself for a copy inside of one skb, if the source is linear and the
destination is not.

__bpf_skb_store_bytes() and __bpf_skb_meta_store_bytes() make the skb
writable first. If that reallocates the head, the source points to the old
one, and freed memory is copied into the packet.

The offsets in the head do not change when it is reallocated here. Move
the source along with the head.

Fixes: 2012c867c800 ("bpf: verifier: Accept dynptr mem as mem in helpers")
Fixes: f38499ff45f5 ("bpf: Unclone skb head on bpf_dynptr_write to skb metadata")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 net/core/filter.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index 15354134091a..12a5e530a81b 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -1715,6 +1715,24 @@ static inline void bpf_pull_mac_rcsum(struct sk_buff *skb)
 		skb_postpull_rcsum(skb, skb_mac_header(skb), skb->mac_len);
 }
 
+/*
+ * The source of a write through a dynptr may be a slice of the same skb.
+ * Making the skb writable can move its head: follow the move.
+ */
+static int bpf_try_make_writable_from(struct sk_buff *skb, u32 write_len,
+				      const void **from)
+{
+	const void *head = skb->head;
+	bool in_head = *from >= head &&
+		       *from < (const void *)skb_end_pointer(skb);
+	int err;
+
+	err = bpf_try_make_writable(skb, write_len);
+	if (!err && in_head)
+		*from = skb->head + (*from - head);
+	return err;
+}
+
 BPF_CALL_5(bpf_skb_store_bytes, struct sk_buff *, skb, u32, offset,
 	   const void *, from, u32, len, u64, flags)
 {
@@ -1724,7 +1742,7 @@ BPF_CALL_5(bpf_skb_store_bytes, struct sk_buff *, skb, u32, offset,
 		return -EINVAL;
 	if (unlikely(offset > INT_MAX))
 		return -EFAULT;
-	if (unlikely(bpf_try_make_writable(skb, offset + len)))
+	if (unlikely(bpf_try_make_writable_from(skb, offset + len, &from)))
 		return -EFAULT;
 
 	ptr = skb->data + offset;
@@ -12427,7 +12445,7 @@ int __bpf_skb_meta_store_bytes(struct sk_buff *skb, u32 offset,
 {
 	if (unlikely(flags))
 		return -EINVAL;
-	if (unlikely(bpf_try_make_writable(skb, 0)))
+	if (unlikely(bpf_try_make_writable_from(skb, 0, &from)))
 		return -EFAULT;
 
 	memmove(bpf_skb_meta_pointer(skb, offset), from, len);
-- 
2.55.0

[PATCH bpf-next v2 42/43] bpf: Use memmove() to store bytes in an skb

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

Through bpf_dynptr_write() and bpf_dynptr_copy() the source of
bpf_skb_store_bytes() can be a slice of the same skb which overlaps the
destination. memcpy() then reads bytes which it has overwritten already. On
x86 with FSRM it repeats the first bytes of the source instead of copying
it. The other kinds of dynptr are written with memmove() for that reason.

Fixes: 2012c867c800 ("bpf: verifier: Accept dynptr mem as mem in helpers")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 net/core/filter.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index 12a5e530a81b..a9b641730f13 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -1749,7 +1749,8 @@ BPF_CALL_5(bpf_skb_store_bytes, struct sk_buff *, skb, u32, offset,
 	if (flags & BPF_F_RECOMPUTE_CSUM)
 		__skb_postpull_rcsum(skb, ptr, len, offset);
 
-	memcpy(ptr, from, len);
+	/* through a dynptr the source may be an overlapping slice of the skb */
+	memmove(ptr, from, len);
 
 	if (flags & BPF_F_RECOMPUTE_CSUM)
 		__skb_postpush_rcsum(skb, ptr, len, offset);
-- 
2.55.0

[PATCH bpf-next v2 43/43] bpf: Refuse packet slices for packet-changing helpers

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

The helpers which may change packet data do not take a packet pointer as a
memory argument: they have no pkt_access. bpf_skb_store_bytes() makes the
skb writable before it copies from its source, and that can reallocate the
head which the source points into.

A slice from bpf_dynptr_slice() of the dynptr of the skb points into the
same head, but it is a PTR_TO_MEM, and the verifier lets it pass. The
helper then reads the old head, which has been freed:
bpf_skb_store_bytes(), bpf_lwt_push_encap(), bpf_lwt_seg6_store_bytes()
and bpf_lwt_seg6_action().

Refuse a slice of a packet where a packet pointer is refused for that
reason. All other helpers take slices as before. bpf_dynptr_write() and
bpf_dynptr_copy() can be used to copy inside of a packet.

Fixes: 2012c867c800 ("bpf: verifier: Accept dynptr mem as mem in helpers")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/verifier.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 47e2969928a4..8cddbcf079b5 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -9693,6 +9693,16 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, u32 slot, u32 p
 	if (err)
 		return err;
 
+	/*
+	 * A slice of a packet is packet data under another name. A helper which
+	 * may reallocate the packet must not go on reading from the old one.
+	 */
+	if (is_helper(meta) && reg_is_dynptr_slice_pkt(reg) &&
+	    bpf_helper_changes_pkt_data(meta->func_id)) {
+		verbose(env, "function access to the packet is not allowed\n");
+		return -EACCES;
+	}
+
 	if (arg_type_is_raw_mem(arg_type))
 		meta->arg_raw_mem.mask |= BIT(slot);
 
-- 
2.55.0

Re: [PATCH bpf-next v2 18/43] bpf: Bound the nesting that the BTF show code follows

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] bpf: btf_show_obj_safe() clobbers -E2BIG from btf_show_start_aggr_type()
- [Medium] bpf: btf_show_start_array_type() leaks array_encoding on max depth return
--

commit 759371ec51181daa62496793db6556044a061470
Author: Josef Bacik <josef@toxicpanda.com>

bpf: Bound the nesting that the BTF show code follows

This patch limits BTF struct and array pretty-printing recursion to 32
levels (BTF_SHOW_MAX_DEPTH) to prevent kernel stack overflow when showing
deeply nested BTF types, recording -E2BIG in show->state.status when the
limit is reached.

> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 64c78434fa050..a9bf4c3c0a273 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -1081,6 +1081,8 @@ static const struct btf_type *btf_type_skip_qualifiers(const struct btf *btf,
>  }
>  
>  #define BTF_SHOW_MAX_ITER	10
> +/* nested structs and arrays shown; __btf_show_indent() has as many columns */
> +#define BTF_SHOW_MAX_DEPTH	32
>  
>  #define BTF_KIND_BIT(kind)	(1ULL << kind)
>  
> @@ -1474,8 +1476,18 @@ static void *btf_show_start_aggr_type(struct btf_show *show,
>  				      const struct btf_type *t,
>  				      u32 type_id, void *data)
>  {
> -	void *safe_data = btf_show_start_type(show, t, type_id, data);
> +	void *safe_data;
>  
> +	/*
> +	 * Structs and arrays are shown by recursion.  BTF loading bounds the
> +	 * depth of one resolve walk, not how deep resolved types may nest.
> +	 */
> +	if (show->state.depth >= BTF_SHOW_MAX_DEPTH) {
> +		show->state.status = -E2BIG;
> +		return NULL;
> +	}

[Severity: Medium]
Can show->state.status = -E2BIG be overwritten by subsequent members before
btf_type_snprintf_show() or btf_type_seq_show_flags() returns it?

Returning NULL from btf_show_start_aggr_type() only causes the current
__btf_struct_show() or __btf_array_show() call at BTF_SHOW_MAX_DEPTH to return
early. The enclosing __btf_struct_show() and __btf_array_show() loops at
shallower depths continue iterating over remaining members (and execute a
second pass when BTF_SHOW_ZERO is not set and a shallower member is non-zero).

When BTF_SHOW_UNSAFE is not set (such as in bpf_snprintf_btf() and
bpf_seq_printf_btf()), visiting a subsequent member outside the current
32-byte show->obj.safe window calls btf_show_start_type() ->
btf_show_obj_safe():

kernel/bpf/btf.c:btf_show_obj_safe() {
	...
	if (!safe) {
		size_left = btf_show_obj_size_left(show, data);
		if (size_left > BTF_SHOW_OBJ_SAFE_SIZE)
			size_left = BTF_SHOW_OBJ_SAFE_SIZE;
		show->state.status = copy_from_kernel_nofault(show->obj.safe,
							      data, size_left);
		if (!show->state.status) {
			show->obj.data = data;
			safe = show->obj.safe;
		}
	}
	...
}

When copy_from_kernel_nofault() succeeds on that later member, doesn't it
unconditionally reset show->state.status to 0, causing bpf_snprintf_btf() and
bpf_seq_printf_btf() to return success with truncated output instead of
-E2BIG?

[Severity: Medium]
Also, what happens to show->state.array_encoding when an array of 1-byte
integers hits BTF_SHOW_MAX_DEPTH?

In __btf_array_show(), btf_show_start_array_type() sets
show->state.array_encoding before calling btf_show_start_aggr_type():

kernel/bpf/btf.c:btf_show_start_array_type() {
	show->state.array_encoding = array_encoding;
	show->state.array_terminated = 0;
	return btf_show_start_aggr_type(show, t, type_id, data);
}

When btf_show_start_aggr_type() returns NULL at BTF_SHOW_MAX_DEPTH,
__btf_array_show() returns early without calling btf_show_end_array_type() to
reset show->state.array_encoding to 0:

kernel/bpf/btf.c:__btf_array_show() {
	...
	if (!btf_show_start_array_type(show, t, type_id, encoding, data))
		return;
	...
	btf_show_end_array_type(show);
}

If the skipped array had 1-byte integer elements (encoding == BTF_INT_CHAR),
won't show->state.array_encoding remain BTF_INT_CHAR while enclosing structs
continue printing shallower members, causing btf_int_show() to format
subsequent non-array 8-bit integer fields as character literals and treat any
zero byte as a NUL terminator that sets show->state.array_terminated = 1 and
suppresses remaining 8-bit integer fields?

> +
> +	safe_data = btf_show_start_type(show, t, type_id, data);
>  	if (!safe_data)
>  		return safe_data;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017163745.3341787-1-alexei.starovoitov@gmail.com?part=18

Re: [PATCH bpf-next v2 37/43] bpf: Let bpf_dynptr_write() mark a global function

[bot+bpf-ci]

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

> bpf: Let bpf_dynptr_write() mark a global function

TL;DR: Commit message only: the Fixes: tag may point at the wrong commit. It
likely should be 51081a3f25c7 (changes_pkt_data tracking), not b5964b968ac6,
which could mislead stable backports.

>     bpf: Let bpf_dynptr_write() mark a global function
>
>     The caller of a global function loses its packet pointers if the function
>     may change packet data. That is found out by a scan for calls of the
>     helpers which bpf_helper_changes_pkt_data() lists.
>     bpf_dynptr_write() is not on the list, because it depends on the type of
>     the dynptr: check_helper_call() looks at it. The scan cannot do that, and
>     so a global function which makes a dynptr from the skb, or gets one as an
>     argument, and writes through it counts as harmless. The write can
>     reallocate the head of the skb, and the caller goes on with pointers into
>     the old one, which has been freed.
>     Let every call of bpf_dynptr_write() mark the function, as a tail call
>     does. The same mark decides whether a program may replace a function.
>     Fixes: b5964b968ac6 ("bpf: Add skb dynptrs")
>     Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>     Signed-off-by: Alexei Starovoitov <ast@kernel.org>

Is this Fixes: tag pointing at the right commit?

The code being corrected here is the scan in visit_insn() (originally in
check_cfg() in kernel/bpf/verifier.c) that sets
bpf_subprog_info->changes_pkt_data from bpf_helper_changes_pkt_data().
That scan was added by 51081a3f25c7 ("bpf: track changes_pkt_data property
for global functions"), and that is the commit that left bpf_dynptr_write()
out of it.

b5964b968ac6 already handled bpf_dynptr_write() on skb dynptrs correctly in
check_helper_call() by setting changes_data = true.  At that commit no
helper called from a global function invalidated the caller's packet
pointers, so the omission fixed here did not exist yet.

A stable backport driven by the b5964b968ac6 tag would also target kernels
that have no changes_pkt_data tracking, where this patch cannot apply.

Should the tag be:

  Fixes: 51081a3f25c7 ("bpf: track changes_pkt_data property for global functions")


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/39051545464

Re: [PATCH bpf-next v2 31/43] bpf: Keep values of a dead owner for sleepable programs

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] bpf: special BTF fields leaked in bpf_selem_unlink_nofail() race
--

commit 441f9cc60cc3e26f38a89830b7d3f94a34c7d383
Author: Josef Bacik <josef@toxicpanda.com>

bpf: Keep values of a dead owner for sleepable programs

This commit defers freeing BPF local storage elements and their special BTF
fields until after an RCU Tasks Trace grace period when an owner dies and
the map may be accessed by sleepable BPF programs, including maps that have
been used as inner maps.

> diff --git a/kernel/bpf/bpf_local_storage.c b/kernel/bpf/bpf_local_storage.c
> index 49ea00c993909..699430f971787 100644
> --- a/kernel/bpf/bpf_local_storage.c
> +++ b/kernel/bpf/bpf_local_storage.c
> @@ -352,7 +352,8 @@ int bpf_selem_unlink(struct bpf_local_storage_elem *selem)
>  static void bpf_selem_unlink_nofail(struct bpf_local_storage_elem *selem,
>  				    struct bpf_local_storage_map_bucket *b)
>  {
> -	bool in_map_free = !!b, free_storage = false;
> +	bool in_map_free = !!b, free_storage = false, sleepable_users = false;
> +	bool keep_fields = false;
>  	struct bpf_local_storage *local_storage;
>  	struct bpf_local_storage_map *smap;
>  	unsigned long flags;
> @@ -362,17 +363,26 @@ static void bpf_selem_unlink_nofail(struct bpf_local_storage_elem *selem,
>  	smap = rcu_dereference_check(SDATA(selem)->smap, bpf_rcu_lock_held());
>  
>  	if (smap) {
> +		/* A map that is freed, or NULL here, has no program left. */
> +		sleepable_users = !in_map_free &&
> +				  (atomic64_read(&smap->map.sleepable_refcnt) ||
> +				   READ_ONCE(smap->map.was_inner_map));
>  		b = b ? : select_bucket(smap, local_storage);
>  		err = raw_res_spin_lock_irqsave(&b->lock, flags);
>  		if (!err) {
>  			/*
>  			 * Call bpf_obj_free_fields() under b->lock to make sure it is done
>  			 * exactly once for an selem. Safe to free special fields immediately
> -			 * as no BPF program should be referencing the selem.
> +			 * as no BPF program should be referencing the selem, unless it
> +			 * is sleepable: see below.
>  			 */
>  			if (likely(selem_linked_to_map(selem))) {
>  				hlist_del_init_rcu(&selem->map_node);
> -				bpf_obj_free_fields(smap->map.record, SDATA(selem)->data);
> +				if (sleepable_users)
> +					keep_fields = true;
> +				else
> +					bpf_obj_free_fields(smap->map.record,
> +							    SDATA(selem)->data);
>  				unlink++;
>  			}
>  			raw_res_spin_unlock_irqrestore(&b->lock, flags);
> @@ -387,7 +397,14 @@ static void bpf_selem_unlink_nofail(struct bpf_local_storage_elem *selem,
>  		 * with selem1 as raw_res_spin_lock_irqsave() returns -ETIMEDOUT.
>  		 */
>  		WARN_ON_ONCE(err && in_map_free);
> -		if (!err || in_map_free)
> +		/*
> +		 * A sleepable program can still use, and fill, the fields of a value
> +		 * that it got before it let go of the owner. As for bpf_selem_unlink(),
> +		 * bpf_selem_free_trace_rcu() frees them, and takes smap for that:
> +		 * map_free() waits for this RCU read side critical section and then
> +		 * for the callback.
> +		 */
> +		if ((!err && !keep_fields) || in_map_free)
>  			RCU_INIT_POINTER(SDATA(selem)->smap, NULL);
>  	}

[Severity: High]
Can this race between bpf_local_storage_destroy() and
bpf_local_storage_map_free() leak the special BTF fields in
SDATA(selem)->data (such as referenced kptrs or pinned uptr pages) when
smap->map.was_inner_map is true?

Once a local storage map has been inserted into an outer map via
bpf_map_fd_get_ptr(), smap->map.was_inner_map remains permanently true even
when bpf_local_storage_map_free() later tears down the map.

If bpf_local_storage_destroy() and bpf_local_storage_map_free() concurrently
call bpf_selem_unlink_nofail() on the same selem and both read a non-NULL
smap before either clears SDATA(selem)->smap:

1. In bpf_local_storage_destroy() (in_map_free is false), sleepable_users
   evaluates to true because smap->map.was_inner_map is true. If it acquires
   b->lock first, it sees selem_linked_to_map(selem), unlinks selem->map_node,
   and sets its local keep_fields = true instead of calling
   bpf_obj_free_fields(). It then leaves SDATA(selem)->smap non-NULL so
   bpf_selem_free_trace_rcu() can free the fields later.

2. In the racing bpf_local_storage_map_free() (in_map_free is true), after it
   acquires b->lock, selem_linked_to_map(selem) is now false, so it also skips
   bpf_obj_free_fields(). However, because in_map_free is true (and its local
   keep_fields is false), it then executes:

	if ((!err && !keep_fields) || in_map_free)
		RCU_INIT_POINTER(SDATA(selem)->smap, NULL);

3. When bpf_selem_free_trace_rcu() later runs after the RCU Tasks Trace grace
   period:

	smap = rcu_dereference_check(SDATA(selem)->smap, 1);

	if (smap)
		bpf_obj_free_fields(smap->map.record, SDATA(selem)->data);

   smap is now NULL, so bpf_obj_free_fields() is skipped there as well and
   never runs for selem.

[ ... ]

>
> -- 
> 2.51.2
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017163745.3341787-1-alexei.starovoitov@gmail.com?part=31

Re: [PATCH bpf-next v2 39/43] bpf: Refuse the prologue for an extension that cannot take it

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] bpf: missing changes_pkt_data check and mark for extension prologue
- [Medium] bpf: bpf_convert_ctx_accesses() rejects extensions with no-op gen_prologue()
--

commit 2e35aa5750da9b2ad248d0380937d26b001912e7
Author: Alexei Starovoitov <ast@kernel.org>

bpf: Refuse the prologue for an extension that cannot take it

This commit adds ext_prog_takes_prologue() to reject BPF_PROG_TYPE_EXT
programs that write to packet data unless their first argument is
ARG_PTR_TO_CTX and any remaining register arguments are scalars.

> diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
> index fe59e3114d828..32911d7f494cb 100644
> --- a/kernel/bpf/fixups.c
> +++ b/kernel/bpf/fixups.c
> @@ -858,6 +858,25 @@ int bpf_opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env,
>  	return 0;
>  }
>  
> +/*
> + * The prologue takes the context from R1, and it can clobber R2-R5. An
> + * extension gets the arguments of the function that it replaces.
> + */
> +static bool ext_prog_takes_prologue(const struct bpf_verifier_env *env)
> +{
> +	const struct bpf_subprog_info *sub = &env->subprog_info[0];
> +	int i, cnt = min_t(int, sub->arg_slot_cnt, MAX_BPF_FUNC_REG_ARGS);
> +
> +	if (!cnt || sub->args[0].arg_type != ARG_PTR_TO_CTX)
> +		return false;
> +
> +	for (i = 1; i < cnt; i++)
> +		if (sub->args[i].arg_type != ARG_SCALAR)
> +			return false;
> +
> +	return true;
> +}
> +
>  /* convert load instructions that access fields of a context type into a
>   * sequence of instructions that access fields of the underlying structure:
>   *     struct __sk_buff    -> struct sk_buff
> @@ -909,6 +928,13 @@ int bpf_convert_ctx_accesses(struct bpf_verifier_env *env)
>  			verifier_bug(env, "gen_prologue is null");
>  			return -EFAULT;
>  		}
> +		if (env->seen_direct_write &&
> +		    env->prog->type == BPF_PROG_TYPE_EXT &&
> +		    !ext_prog_takes_prologue(env)) {
> +			verbose(env,
> +				"Extension writing packet data must take ctx and scalars only\n");
> +			return -EINVAL;
> +		}
>  		cnt = ops->gen_prologue(insn_buf, env->seen_direct_write,
>  					env->prog);

[Severity: Critical]
Can allowing an extension program to receive bpf_unclone_prologue() here
cause a use-after-free of skb->head in the target program when the
replaced global subprogram has changes_pkt_data == false?

When bpf_unclone_prologue() is prepended to an extension program, it emits
a call to BPF_FUNC_skb_pull_data:

net/core/filter.c:bpf_unclone_prologue() {
	...
	/* ret = bpf_skb_pull_data(skb, 0); */
	*insn++ = BPF_MOV64_REG(BPF_REG_6, BPF_REG_1);
	*insn++ = BPF_ALU64_REG(BPF_XOR, BPF_REG_2, BPF_REG_2);
	*insn++ = BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
			       BPF_FUNC_skb_pull_data);
	...
}

At runtime, if skb->cloned is set (for instance, when the target program
did not perform direct packet writes itself and thus received no
bpf_unclone_prologue() at entry, or cloned the skb via
bpf_clone_redirect() before the call), bpf_skb_pull_data() calls
bpf_try_make_writable() -> skb_ensure_writable() -> pskb_expand_head(),
which reallocates skb->head and frees the old buffer.

In the target program, check_func_call() only invalidates packet pointers
and skb dynptr slices in callee-saved registers (R6-R9) if the called
subprogram has changes_pkt_data set:

kernel/bpf/verifier.c:check_func_call() {
	...
	if (env->subprog_info[subprog].changes_pkt_data)
		clear_all_pkt_pointers(env);
	...
}

However, direct packet stores (such as *(u8 *)(long)skb->data = val) and
bpf_dynptr_slice_rdwr() set env->seen_direct_write during do_check_main(),
after bpf_check_cfg() and check_attach_btf_id() ->
bpf_check_attach_target() have already run. Neither sets
prog->aux->changes_pkt_data during bpf_check_cfg(), and
bpf_convert_ctx_accesses() does not set prog->aux->changes_pkt_data or
verify that the target subprogram has tgt_changes_pkt_data set (both for
initial attach in bpf_check_attach_target() and subsequent re-attach via
bpf_tracing_prog_attach()).

If the target program holds a packet pointer (PTR_TO_PACKET,
PTR_TO_PACKET_META, or a BPF_DYNPTR_TYPE_SKB slice) in R6-R9 across a call
to a global function sub(struct __sk_buff *skb) with changes_pkt_data ==
false, and an extension replaces sub() and writes to packet data, wouldn't
the caller dereference the stale pointer in R6-R9 into the freed skb->head
after the extension's prologue runs pskb_expand_head()?

[Severity: Medium]
Also, should the ext_prog_takes_prologue() check be performed only after
ops->gen_prologue() returns cnt > 0?

In kernel/bpf/verifier.c, may_access_direct_pkt_data() sets
env->seen_direct_write = true on BPF_WRITE for BPF_PROG_TYPE_XDP,
BPF_PROG_TYPE_SK_MSG, and BPF_PROG_TYPE_CGROUP_SOCKOPT in addition to the
skb program types:

kernel/bpf/verifier.c:may_access_direct_pkt_data() {
	switch (env->prog->type) {
	...
	case BPF_PROG_TYPE_SCHED_CLS:
	case BPF_PROG_TYPE_SCHED_ACT:
	case BPF_PROG_TYPE_XDP:
	case BPF_PROG_TYPE_LWT_XMIT:
	case BPF_PROG_TYPE_SK_SKB:
	case BPF_PROG_TYPE_SK_MSG:
		if (meta && !meta->btf && meta->func_id)
			return meta->fn->pkt_access;

		if (t == BPF_WRITE)
			env->seen_direct_write = true;
		return true;

	case BPF_PROG_TYPE_CGROUP_SOCKOPT:
		if (t == BPF_WRITE)
			env->seen_direct_write = true;

		return true;
	...
}

For XDP, SK_MSG, and CGROUP_SOCKOPT targets, ops->gen_prologue() is
bpf_noop_prologue() or cg_sockopt_get_prologue(), which return 0 when
env->seen_direct_write is set. Checking ext_prog_takes_prologue() before
checking cnt > 0 rejects extensions attached to those program types (for
example, an XDP extension taking a struct bpf_dynptr * and calling
bpf_dynptr_slice_rdwr(), or taking ctx in R2) even though no prologue is
generated for them.

>  		if (cnt >= INSN_BUF_SIZE) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017163745.3341787-1-alexei.starovoitov@gmail.com?part=39

Re: [PATCH bpf-next v2 39/43] bpf: Refuse the prologue for an extension that cannot take it

[bot+bpf-ci]

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

> bpf: Refuse the prologue for an extension that cannot take it

TL;DR: The new check may wrongly reject XDP, SK_MSG and CGROUP_SOCKOPT
extensions that write packet data, since their prologues emit nothing.
Programs that loaded before could fail with -EINVAL, including on stable.

> diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
> --- a/kernel/bpf/fixups.c
> +++ b/kernel/bpf/fixups.c

[ ... ]

> @@ -909,6 +928,13 @@ int bpf_convert_ctx_accesses(struct bpf_verifier_env *env)
>  			verifier_bug(env, "gen_prologue is null");
>  			return -EFAULT;
>  		}
> +		if (env->seen_direct_write &&
> +		    env->prog->type == BPF_PROG_TYPE_EXT &&
> +		    !ext_prog_takes_prologue(env)) {
> +			verbose(env,
> +				"Extension writing packet data must take ctx and scalars only\n");
> +			return -EINVAL;
> +		}
>  		cnt = ops->gen_prologue(insn_buf, env->seen_direct_write,
>  					env->prog);
>  		if (cnt >= INSN_BUF_SIZE) {

Does this check reject extensions that never get a register-touching
prologue?

An extension inherits the verifier ops of its target, and
may_access_direct_pkt_data() sets seen_direct_write for SCHED_CLS,
SCHED_ACT, LWT_XMIT, SK_SKB, XDP, SK_MSG and CGROUP_SOCKOPT.  Only the
first four end up in bpf_unclone_prologue(), through tc_cls_act_prologue()
or sk_skb_prologue().

XDP and SK_MSG use bpf_noop_prologue(), and CGROUP_SOCKOPT uses
cg_sockopt_get_prologue().  Both return 0 and emit no instructions:

    static int bpf_noop_prologue(struct bpf_insn *insn_buf, bool direct_write,
                                 const struct bpf_prog *prog)
    {
            /* Neither direct read nor direct write requires any preliminary
             * action.
             */
            return 0;
    }

The new check runs before gen_prologue() and never looks at what the
prologue is, so for these types nothing is patched in front of the
extension and no register is read or clobbered.

An XDP extension that writes packet data is now refused with -EINVAL if its
first argument is not the ctx, or if any other argument is not a scalar.
For example, freplace of:

    int f(__u32 idx, struct xdp_md *ctx)
    int f(struct xdp_md *ctx, struct cfg *cfg)   (ARG_PTR_TO_MEM)

or of any function taking a struct bpf_dynptr * argument.
check_special_kfunc() also sets seen_direct_write for bpf_dynptr_slice_rdwr()
on any dynptr type.

These programs loaded and ran correctly before this patch.  Since the patch
carries a Fixes: tag, the load failure would also reach stable kernels.

The commit message says "A program that writes to packet data starts with
bpf_unclone_prologue()".  Is that accurate for XDP, SK_MSG and
CGROUP_SOCKOPT?

Would it be better to run the check after ops->gen_prologue() and only
when it returned a non-zero cnt, i.e. when a prologue that assumes R1 is
the ctx is actually emitted?

No later commit in the series (222dc092c64e..64e0b112d688) touches this
check.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/39051545464

Re: [PATCH bpf-next v2 42/43] bpf: Use memmove() to store bytes in an skb

[bot+bpf-ci]

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

> bpf: Use memmove() to store bytes in an skb

TL;DR: The commit message claims all other dynptr kinds already use memmove(),
but XDP dynptr writes still use memcpy(), so overlapping slices may be
corrupted the same way; should the XDP store path change too?

>     bpf: Use memmove() to store bytes in an skb
>
>     Through bpf_dynptr_write() and bpf_dynptr_copy() the source of
>     bpf_skb_store_bytes() can be a slice of the same skb which overlaps the
>     destination. memcpy() then reads bytes which it has overwritten already. On
>     x86 with FSRM it repeats the first bytes of the source instead of copying
>     it. The other kinds of dynptr are written with memmove() for that reason.
>
>     Fixes: 2012c867c800 ("bpf: verifier: Accept dynptr mem as mem in helpers")
>     Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>     Signed-off-by: Alexei Starovoitov <ast@kernel.org>

The sentence "The other kinds of dynptr are written with memmove() for that
reason." doesn't look accurate for XDP dynptrs.  Is that statement correct?

__bpf_dynptr_write() handles the cases like this:

  - LOCAL and RINGBUF use memmove()
  - SKB_META goes through __bpf_skb_meta_store_bytes(), which uses memmove()
  - SKB is fixed by this patch
  - XDP goes through __bpf_xdp_store_bytes() -> ____bpf_xdp_store_bytes():

	ptr = bpf_xdp_pointer(xdp, offset, len);
	...
	if (!ptr)
		bpf_xdp_copy_buf(xdp, offset, buf, len, true);
	else
		memcpy(ptr, buf, len);

and bpf_xdp_copy_buf() also uses memcpy() for each chunk it writes.

bpf_dynptr_slice_rdwr() on an XDP dynptr returns a direct pointer into the
xdp_buff via bpf_xdp_pointer() whenever the range lies within a single
buffer, and that slice is accepted as the ARG_PTR_TO_MEM source of
bpf_dynptr_write().  Can a program then do this:

	p = bpf_dynptr_slice_rdwr(&xdp_dptr, 0, NULL, 64);
	bpf_dynptr_write(&xdp_dptr, 8, p, 64, 0);

and reach memcpy(ptr + 8, ptr, 64) with overlapping ranges, which is the same
failure the commit message describes for skb (the first bytes get replicated
on x86 with FSRM)?

bpf_dynptr_copy() looks like it can reach the same thing when the source
slice is direct and the destination range spans the linear area and a frag.
In that case dst_slice is NULL, so __bpf_dynptr_write() runs with src_slice
as the source and bpf_xdp_copy_buf() memcpy()s the first chunk over the
overlapping linear area.

I didn't find a later commit in this series that changes the XDP store path.
Should bpf_xdp_store_bytes() use memmove() for the direct case, and
bpf_xdp_copy_buf() use memmove() for the flush case, or should the commit
message stop claiming that all other dynptr kinds are already written with
memmove()?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/39051545464

Re: [PATCH bpf-next v2 05/43] bpf: reject sleepable programs for task_vma iterator

[bot+bpf-ci]

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

> bpf: reject sleepable programs for task_vma iterator

TL;DR: Documentation nit only: bpf_iterators.rst still says only
BPF_ITER_RESCHED is supported and doesn't mention the new BPF_ITER_NOSLEEP
flag. No code problems raised.

> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 54144372281c..46d325aa178e 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -3137,7 +3137,8 @@ typedef const struct bpf_func_proto *
>  			     const struct bpf_prog *prog);
>
>  enum bpf_iter_feature {
> -	BPF_ITER_RESCHED	= BIT(0),
> +	BPF_ITER_RESCHED	= BIT(0),	/* may cond_resched() between objects */
> +	BPF_ITER_NOSLEEP	= BIT(1),	/* ->show() can't run sleepable progs */
>  };
>
>  #define BPF_ITER_CTX_ARG_MAX 2

This isn't a bug, but should Documentation/bpf/bpf_iterators.rst be updated
for the new flag?

The description of the feature field in struct bpf_iter_reg still reads:

   * - feature
     - Specifies certain action requests in the kernel BPF iterator
       infrastructure. Currently, only BPF_ITER_RESCHED is supported. This
       means that the kernel function cond_resched() is called to avoid other
       kernel subsystem (e.g., rcu) misbehaving.

With BPF_ITER_NOSLEEP added, "only BPF_ITER_RESCHED is supported" is no
longer accurate.

The text also doesn't say that BPF_ITER_RESCHED is what allows a target to
accept sleepable (iter.s/) programs, or that a target whose ->show() runs
under a lock that a faulting program would need to take again (as task_vma
does with mmap_lock) has to set BPF_ITER_NOSLEEP as well.

Someone adding a new iterator target from that document would not learn
about the flag.  None of the later patches in this series touch
bpf_iterators.rst.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/39051545464

Re: [PATCH bpf-next v2 16/43] bpf: Link the first element after its storage is published

[bot+bpf-ci]

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

> bpf: Link the first element after its storage is published

TL;DR: Commit message only: the Fixes: tag may point at the wrong commit; the
lockless iterator exposure looks introduced by c69d2ddb2072, not 5ce6e77c7edf.
The code change itself isn't questioned.

>     bpf: Link the first element after its storage is published
>
>     bpf_local_storage_alloc() links the first element of a new storage to the
>     bucket of its map, and then publishes the storage in the owner with
>     cmpxchg().  If another cpu has been faster, the element is taken out of
>     the bucket again and is freed together with the storage.
>
>     In between the iterator over a sk_storage map can find the element.  It
>     stays linked to a storage that points to the socket, but the socket knows
>     nothing of that storage: bpf_sk_storage_free() neither unlinks the element
>     nor waits for anybody who uses the storage.  So nothing tells the iterator
>     that the socket has gone away.
>
>     Three cpus that create the first storage of a new AF_UNIX socket at the
>     same time, and a fourth that reads the iterator, are enough: in 20 seconds
>     its program was shown some 4000 elements of storages that had lost.  If the
>     socket is closed in time, the program is handed a freed socket.
>
>     Link the element once the storage is published.  Both happens under the
>     lock of the bucket, which holds off whoever finds the element through the
>     owner and wants to unlink it.
>
>     Fixes: 5ce6e77c7edf ("bpf: Implement bpf iterator for sock local storage map")
>     Signed-off-by: Alexei Starovoitov <ast@kernel.org>

Is this the right commit for the Fixes: tag?

5ce6e77c7edf ("bpf: Implement bpf iterator for sock local storage map")
held the bucket lock (raw_spin_lock_bh(&b->lock)) for the whole time it
handed an element to the program.  The losing sk_storage_alloc() could
therefore not run selem_unlink_map() and free the storage until the
iterator had moved on, and the socket stayed alive because the allocating
caller still held it.

The element was already linked before the cmpxchg() back then, but nothing
walked the bucket list locklessly.  The old comment in the alloc path even
says that nobody accesses first_selem from the bucket list under
rcu_read_lock().

The lockless exposure looks like it came from c69d2ddb2072 ("bpf: Using
rcu_read_lock for bpf_sk_storage_map iterator"), which replaced the bucket
lock in bpf_sk_storage_map_seq_find_next()/seq_stop() with rcu_read_lock().
From then on the iterator can see the transiently linked first_selem of a
losing storage without excluding its unlink and free.

Should this be:

  Fixes: c69d2ddb2072 ("bpf: Using rcu_read_lock for bpf_sk_storage_map iterator")


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/39051545464

Re: [PATCH bpf-next v2 26/43] bpf: Don't take an extension for an iterator program

[bot+bpf-ci]

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

> bpf: Don't take an extension for an iterator program

TL;DR: The new EXT check in bpf_iter_prog_supported() may be incomplete: a
TRACING/BPF_TRACE_ITER program attached to a JITed program's function can
still reach it, possibly corrupting the iterator target's btf_id cache and
ctx_arg_info.

> diff --git a/kernel/bpf/bpf_iter.c b/kernel/bpf/bpf_iter.c
> index 4668c1696a08..5fd376bd210b 100644
> --- a/kernel/bpf/bpf_iter.c
> +++ b/kernel/bpf/bpf_iter.c
> @@ -351,6 +351,13 @@ int bpf_iter_prog_supported(struct bpf_prog *prog)
>  	const char *prefix = BPF_ITER_FUNC_PREFIX;
>  	int prefix_len = strlen(prefix);
>  
> +	/*
> +	 * The attach function of an extension is a subprogram of the iterator
> +	 * program that it extends, whatever its name is.
> +	 */
> +	if (prog->type == BPF_PROG_TYPE_EXT)
> +		return -EINVAL;
> +
>  	if (strncmp(attach_fname, prefix, prefix_len))
>  		return -EINVAL;

Is this check enough, or can the same problem be reached without an
extension?

A BPF_PROG_TYPE_TRACING program loaded with expected_attach_type set to
BPF_TRACE_ITER, attach_prog_fd pointing at a JITed program P1, and
attach_btf_id set to a global function of P1 named bpf_iter_bpf_map_elem
looks like it gets here too.  bpf_prog_load_check_attach() has no
BPF_PROG_TYPE_TRACING case that rejects it, and in
bpf_check_attach_target() the BPF_TRACE_ITER case with tgt_prog set only
does:

    btf_type_is_func() / func_proto checks
    btf_distill_func_proto()

Unlike the BPF_TRACE_RAW_TP case just above it, it never rejects tgt_prog.

check_attach_btf_id() then stores the subprogram name of P1 in
prog->aux->attach_func_name and calls bpf_iter_prog_supported().  Since
prog->type is BPF_PROG_TYPE_TRACING here, the new check does not refuse it:

    bpf_check()
      check_attach_btf_id()
        bpf_check_attach_target()
        bpf_iter_prog_supported()
          cache_btf_id()
            tinfo->btf_id = prog->aux->attach_btf_id

The 'bpf_iter_' prefix matches and strcmp() against reg_info->target
matches, so cache_btf_id() caches an id from P1's BTF as the btf_id of the
target, and bpf_prog_ctx_arg_info_init() copies the target's ctx_arg_info.
This happens at check_attach_btf_id() time, so it persists even if the load
fails later.

After that, bpf_iter_link_attach() compares iter->btf_id with the poisoned
id, so already loaded programs of that target fail with -ENOENT.  A vmlinux
BTF program whose attach_btf_id equals the forged id would be matched to the
wrong target (and the wrong ctx_arg_info) by the first branch of the loop in
bpf_iter_prog_supported().

The out-of-bounds buffer access described in the commit message does not
seem to apply to this route, since the program being attached is the one
whose max_rdonly_access is recorded.  Only the btf_id cache corruption and
the ctx_arg_info mix-up remain.  It needs CAP_BPF and CAP_PERFMON, the same
as the extension case.

Would it be better to refuse any program that has a dst_prog here, for
example:

    if (prog->aux->dst_prog)
        return -EINVAL;

That also covers all extensions, since they always have one.  Alternatively,
could bpf_check_attach_target() reject tgt_prog for BPF_TRACE_ITER, as it
does for BPF_TRACE_RAW_TP?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/39051545464

Re: [PATCH bpf-next v2 28/43] bpf: Don't take an extension for a writable raw tracepoint program

[bot+bpf-ci]

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

> bpf: Don't take an extension for a writable raw tracepoint program

TL;DR: The new check only rejects extensions of writable raw tracepoint
programs; an freplace of a plain raw_tp program may still read past the
tracepoint args (stack out-of-bounds read) via unchecked max_ctx_offset.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 81660ffce99a..0d1b729989d6 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -22232,6 +22232,15 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
>  					"Extension programs should be JITed\n");
>  				return -EINVAL;
>  			}
> +			/*
> +			 * How much of the buffer of the tracepoint is used
> +			 * is checked for the program that is attached to it.
> +			 */
> +			if (tgt_prog->type == BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE) {
> +				bpf_log(log,
> +					"Cannot extend a writable raw tracepoint program\n");
> +				return -EINVAL;
> +			}

Is this check sufficient for the problem described in the commit message?

The commit message reasons that an extension's accesses are noted in its own
max_tp_access, which nobody looks at.  The same applies to max_ctx_offset,
which bpf_probe_register() checks right before the max_tp_access test:

    if (prog->aux->max_ctx_offset > btp->num_args * sizeof(u64))
        return -EINVAL;

The new check only refuses BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE targets, so
an extension of a plain BPF_PROG_TYPE_RAW_TRACEPOINT program still gets
through.

check_attach_btf_id() sets env->ops = bpf_verifier_ops[tgt_prog->type], so the
extension's ctx loads go through:

    raw_tp_prog_is_valid_access()
      bpf_tracing_ctx_access()

which allows any off < 8 * MAX_BPF_FUNC_ARGS (96 bytes).  check_ctx_access()
then records the access in env->prog->aux->max_ctx_offset, which belongs to
the extension's aux and not to the target's.  Nothing copies it to the target,
and bpf_tracing_prog_attach() only compares kprobe_write_ctx.

For example, with CAP_BPF and CAP_PERFMON, as in the case from the commit
message:

  - Load a SEC("raw_tp") program P whose main function calls a global subprog
    sub(struct bpf_raw_tracepoint_args *ctx) that does not touch ctx, so P's
    max_ctx_offset is 0.

  - Load a freplace program for sub() that reads ctx->args[11], so its own
    max_ctx_offset becomes 96, and attach it.

  - Attach P to a 1-argument tracepoint.  bpf_probe_register() sees P's
    max_ctx_offset (0 <= 8) and accepts it.  bpf_trace_run1() runs P with
    u64 args[1] on its stack, and the extension reads 88 bytes past it.  That
    is a stack out-of-bounds read, the same kind of failure as the KASAN
    report in the commit message.

  - BPF_PROG_TEST_RUN on P with ctx_size_in == 0 passes the
    "ctx_size_in < prog->aux->max_ctx_offset" check (0 < 0 is false), sets
    info.ctx = NULL, and the replaced subprog then loads from NULL + 88.
    With ctx_size_in == 8, memdup_user() allocates 8 bytes and the extension
    reads past them.

None of the later patches in this series touch max_ctx_offset or the
BPF_PROG_TYPE_RAW_TRACEPOINT handling.

Should the check also cover BPF_PROG_TYPE_RAW_TRACEPOINT targets, or should
the extension's max_ctx_offset be checked against the target at attach time?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/39051545464

Re: [PATCH bpf-next v2 43/43] bpf: Refuse packet slices for packet-changing helpers

[bot+bpf-ci]

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

> bpf: Refuse packet slices for packet-changing helpers

TL;DR: Refusing slices for bpf_skb_store_bytes() may be unneeded after the
previous patch and would newly reject previously safe programs with -EACCES;
the commit message's freed-head claim may be stale.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -9693,6 +9693,16 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, u32 slot, u32 p
>  	if (err)
>  		return err;
>
> +	/*
> +	 * A slice of a packet is packet data under another name. A helper which
> +	 * may reallocate the packet must not go on reading from the old one.
> +	 */
> +	if (is_helper(meta) && reg_is_dynptr_slice_pkt(reg) &&
> +	    bpf_helper_changes_pkt_data(meta->func_id)) {
> +		verbose(env, "function access to the packet is not allowed\n");
> +		return -EACCES;
> +	}
> +
>  	if (arg_type_is_raw_mem(arg_type))
>  		meta->arg_raw_mem.mask |= BIT(slot);
>

The commit message says of bpf_skb_store_bytes() that "the helper then reads
the old head, which has been freed".  Is that still true after the previous
patch in this series, 0bd8e7a834ac ("bpf: Follow the head if an skb is
written from a slice")?

That patch changed the helper to:

    BPF_CALL_5(bpf_skb_store_bytes, ...)
    {
        ...
        if (unlikely(bpf_try_make_writable_from(skb, offset + len, &from)))
            return -EFAULT;
        ...
        memmove(ptr, from, len);

and bpf_try_make_writable_from() moves 'from' to the new head when it points
into [skb->head, skb_end_pointer(skb)).  So bpf_skb_store_bytes() no longer
reads the old head after a reallocation.  Only bpf_lwt_push_encap(),
bpf_lwt_seg6_store_bytes() and bpf_lwt_seg6_action() still read their
hdr/from/param after skb_cow_head() or bpf_try_make_writable() without
following the head.

The result also looks inconsistent with the suggested alternative.  The
commit message recommends bpf_dynptr_write(), and the verifier still accepts
a slice as its source.  For an skb dynptr that ends up in the same code:

    case BPF_DYNPTR_TYPE_SKB:
        return __bpf_skb_store_bytes(dst->data, dst->offset + offset, src, len, flags);

A program in tc, sk_skb or lwt_xmit that does:

    p = bpf_dynptr_slice(&ptr, off, buf, n);
    bpf_skb_store_bytes(skb, off2, p, n, 0);

has been accepted since 2012c867c800 and is safe with the previous patch
applied.  With this patch it fails to load with -EACCES ("function access to
the packet is not allowed"), via check_helper_call() -> check_func_args() ->
check_func_arg() with is_helper(meta) true, reg typed
PTR_TO_MEM|DYNPTR_TYPE_SKB, and bpf_helper_changes_pkt_data(BPF_FUNC_skb_store_bytes)
returning true.

Should the refusal be limited to the helpers that still read the source
after reallocating the head (or should those helpers follow the head too),
so that bpf_skb_store_bytes() keeps accepting slices?  Either way, should the
commit message stop listing bpf_skb_store_bytes() as reading freed memory?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/39051545464

Re: [PATCH bpf-next v2 40/43] bpf: Unclone the skb for kfuncs that write through a dynptr

[bot+bpf-ci]

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

> bpf: Unclone the skb for kfuncs that write through a dynptr

TL;DR: Dynptr-writing kfuncs may set seen_direct_write for programs with no
skb (XDP, global functions, callbacks), so freplace extensions with non-scalar
args that loaded before may now fail with -EINVAL.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index d384b9ec3d93..47e2969928a4 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -15689,6 +15689,14 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>  	    bpf_is_kfunc_pkt_changing(&meta))
>  		clear_all_pkt_pointers(env);
>
> +	/*
> +	 * These kfuncs write to the head of the skb in place if what they write
> +	 * to is linear. Like a direct write that takes bpf_unclone_prologue().
> +	 */
> +	if (is_kfunc_dynptr_write(&meta) &&
> +	    dynptr_write_changes_pkt_data(meta.dynptr_write_type))
> +		may_access_direct_pkt_data(env, NULL, BPF_WRITE);

Can this set env->seen_direct_write for a program that has no skb to
write to, and make an extension fail to load?

Since commit 19b8e38bcd08 ("bpf: Don't take a dynptr argument for a local
dynptr"), process_dynptr_func() reports every CONST_PTR_TO_DYNPTR argument
as an skb dynptr:

    meta->dynptr.type = is_arg ? BPF_DYNPTR_TYPE_SKB : reg->dynptr.type;

That covers dynptr arguments of global functions and extensions, and
callback dynptrs, in any program type.

may_access_direct_pkt_data(env, NULL, BPF_WRITE) also sets seen_direct_write
for XDP, SK_MSG and CGROUP_SOCKOPT.  None of those can hold an skb or
skb_meta dynptr, since bpf_kfunc_set_skb and bpf_kfunc_set_skb_meta are not
registered for them.  Their gen_prologue is bpf_noop_prologue() or
cg_sockopt_get_prologue(), which emit nothing.

seen_direct_write is now also read by the extension check that commit
222dc092c64e ("bpf: Refuse the prologue for an extension that cannot take
it") added to bpf_convert_ctx_accesses():

    if (env->seen_direct_write &&
        env->prog->type == BPF_PROG_TYPE_EXT &&
        !ext_prog_takes_prologue(env)) {
            verbose(env, "Extension writing packet data must take ctx and scalars only\n");
            return -EINVAL;
    }

This runs before gen_prologue(), so it fires even when the prologue would be
empty.

For example, take an XDP target with a global function:

    int f(struct xdp_md *ctx, struct bpf_dynptr *d)

that calls bpf_dynptr_memset(d, ...), and an freplace program for f that
does the same.  In the extension R2 is CONST_PTR_TO_DYNPTR, so
meta.dynptr_write_type becomes BPF_DYNPTR_TYPE_SKB and the new code sets
seen_direct_write.  ext_prog_takes_prologue() then fails because args[1] is
ARG_PTR_TO_DYNPTR and not ARG_SCALAR, and the load fails with -EINVAL.
Before this patch the program loaded, and nothing in it can touch an skb.

The same applies to bpf_dynptr_copy() and the probe_read/copy_from_user
dynptr kfuncs, and to callback dynptrs (for example bpf_user_ringbuf_drain())
in an extension whose signature has a non-scalar argument.  Extensions on tc
programs with such signatures are refused as well, even when every caller
passes a local dynptr.

Commit 19b8e38bcd08 says "nothing becomes forbidden for the dynptr itself",
but this makes the load fail for those programs.

Should seen_direct_write only be set for program types whose skb dynptr is
writable?  Alternatively, could the extension refusal depend on
gen_prologue() actually emitting instructions?

I did not find a later patch in this series that changes this.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/39051545464

Re: [PATCH bpf-next v2 35/43] bpf, cpumap: Hold the device of frames in the queue

[bot+bpf-ci]

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

> bpf, cpumap: Hold the device of frames in the queue

TL;DR:
- The 2048-pointer held[] array in struct bpf_cpu_map_entry makes each entry a
  ~32 KiB order-3 allocation, which may fail under fragmentation
- Stale comments and parameter name in the GRO flush code (nit)

> diff --git a/kernel/bpf/cpumap.c b/kernel/bpf/cpumap.c
> --- a/kernel/bpf/cpumap.c
> +++ b/kernel/bpf/cpumap.c

[ ... ]

> @@ -45,6 +45,7 @@
>   */
>
>  #define CPU_MAP_BULK_SIZE 8  /* 8 == one cacheline on 64-bit archs */
> +#define CPU_MAP_HELD_DEVS 2048
>  struct bpf_cpu_map_entry;
>  struct bpf_cpu_map;
>

[ ... ]

> @@ -74,6 +75,13 @@ struct bpf_cpu_map_entry {
>
>  	struct completion kthread_running;
>  	struct rcu_work free_work;
> +
> +	/*
> +	 * Every frame and skb in the queue holds a reference to its device.
> +	 * The kthread keeps it until nothing is left in GRO.
> +	 */
> +	struct net_device *held[CPU_MAP_HELD_DEVS];
> +	u32 nr_held;
>  };

Does embedding held[CPU_MAP_HELD_DEVS] in struct bpf_cpu_map_entry make the
entry too large for a slab object?

That is 2048 pointers, 16384 bytes on 64-bit, so the entry grows from about
360 bytes to about 16.7 KiB.  __cpu_map_entry_alloc() allocates it with:

    rcpu = bpf_map_kmalloc_node(map, sizeof(*rcpu), gfp | __GFP_ZERO, numa);

At that size the request is above KMALLOC_MAX_CACHE_SIZE (2 * PAGE_SIZE with
4K pages), so __do_kmalloc_node() sends it to __kmalloc_large_node_noprof().
That is an order-3 (32 KiB) physically contiguous page allocation, and about
half of it is never used.

Every cpumap entry, including each BPF_MAP_UPDATE_ELEM that changes qsize
(which reallocates the entry), now costs 32 KiB of order-3 memory charged to
the map's memcg, where it used to be a small kmalloc object.  A cpumap that
covers all CPUs on a large machine would add megabytes.

Under fragmentation the GFP_KERNEL | __GFP_NOWARN order-3 request has to go
through compaction or reclaim, or it fails with -ENOMEM where the old
allocation would have succeeded.  The commit message does not mention this
cost.

The array only bounds how many references the kthread keeps between full GRO
flushes.  Would a smaller CPU_MAP_HELD_DEVS, or allocating held[] separately
(for example with kvmalloc), avoid the large contiguous allocation?

[ ... ]

> @@ -398,10 +410,13 @@ static int cpu_map_kthread_run(void *data)
>  		/* Flush either every 64 packets or in case of empty ring */
>  		packets += n;
>  		empty = __ptr_ring_empty(rcpu->queue);
> -		if (packets >= NAPI_POLL_WEIGHT || empty) {
> -			cpu_map_gro_flush(rcpu, empty);
> +		full = empty || rcpu->nr_held > CPU_MAP_HELD_DEVS - CPUMAP_BATCH;
> +		if (packets >= NAPI_POLL_WEIGHT || full) {
> +			cpu_map_gro_flush(rcpu, full);
>  			packets = 0;
>  		}
> +		while (full && rcpu->nr_held)
> +			dev_put(rcpu->held[--rcpu->nr_held]);
>
>  		local_bh_enable(); /* resched point, may call do_softirq() */
>  	}

This isn't a bug, but are the comments describing this flush still accurate?

The full flush is now also forced when nr_held exceeds CPU_MAP_HELD_DEVS -
CPUMAP_BATCH (2040), even while the ring is non-empty, yet the comment above
still says:

    /* Flush either every 64 packets or in case of empty ring */

Also, cpu_map_gro_flush() still names its parameter 'empty' and still says:

    /*
     * If the ring is not empty, there'll be a new iteration soon, and we
     * only need to do a full flush if a tick is long (> 1 ms).
     * If the ring is empty, to not hold GRO packets in the stack for too
     * long, do a full flush.
     * ...
     */

It is now called with 'full', which can be true for a non-empty ring, so
both comments describe the old behaviour.  The reason for the extra full
flush (bounding how long the device references are held) is not visible
where the flush decision is made.

Would renaming the parameter (for example 'flush_all') and updating both
comments keep them in line with the code?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/39051545464

Re: [PATCH bpf-next v2 38/43] bpf: Don't take a dynptr argument for a local dynptr

[bot+bpf-ci]

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

> bpf: Don't take a dynptr argument for a local dynptr

TL;DR: Treating every CONST_PTR_TO_DYNPTR as an skb dynptr also hits the
bpf_user_ringbuf_drain() callback's local dynptr, so the verifier may falsely
reject valid programs; nothing unsafe.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -8416,6 +8416,7 @@ static int process_dynptr_func(struct bpf_verifier_env *env, struct bpf_reg_stat
>  			       argno_t argno, int insn_idx, enum bpf_arg_type arg_type,
>  			       struct bpf_call_arg_meta *meta)
>  {
> +	bool is_arg = reg->type == CONST_PTR_TO_DYNPTR;
>  	int spi, err = 0;

[ ... ]

> @@ -8502,7 +8503,12 @@ static int process_dynptr_func(struct bpf_verifier_env *env, struct bpf_reg_stat
>  			reg = &bpf_stack_slot(state, spi)->spilled_ptr;
>  		}
>
> -		meta->dynptr.type = reg->dynptr.type;
> +		/*
> +		 * The callers of a global function may hand in a dynptr of any
> +		 * type. Take it for that of an skb, so that a write through it
> +		 * and its slices are treated with care.
> +		 */
> +		meta->dynptr.type = is_arg ? BPF_DYNPTR_TYPE_SKB : reg->dynptr.type;
>  		meta->dynptr.id = reg->id;
>  		meta->dynptr.parent_id = reg->parent_id;
>  	}

Is is_arg limited to global function arguments?

It is just reg->type == CONST_PTR_TO_DYNPTR, and global subprog arguments
are not the only registers of that type.  set_user_ringbuf_callback_state()
also creates one for the bpf_user_ringbuf_drain() callback:

    mark_dynptr_cb_reg(env, &callee->regs[BPF_REG_1], BPF_DYNPTR_TYPE_LOCAL);

and the type of that dynptr is known.  bpf_user_ringbuf_drain() always
passes a local dynptr over the ring sample (kernel/bpf/ringbuf.c):

    bpf_dynptr_init(&dynptr, sample, BPF_DYNPTR_TYPE_LOCAL, 0, size);

After this patch, every consumer of meta->dynptr.type seems to treat the
callback sample as an skb dynptr:

- bpf_dynptr_write(sample, ...) in the callback hits

      if (dynptr_write_changes_pkt_data(dynptr_type))
              changes_data = true;

  in check_helper_call(), which ends up in clear_all_pkt_pointers().  The
  callback is a sync callback verified inside the caller's state, and
  bpf_for_each_reg_in_vstate() walks every frame, so packet pointers held
  by a calling tc/XDP program across bpf_user_ringbuf_drain() would be
  invalidated.  The dynptr-writing kfuncs (bpf_dynptr_memset(),
  bpf_dynptr_copy() on the sample) go through dynptr_write_type and behave
  the same way.

- bpf_dynptr_slice(sample, ...) now returns PTR_TO_MEM | DYNPTR_TYPE_SKB,
  which makes reg_is_dynptr_slice_pkt() true.  In any program type, that
  slice is then invalidated by a call to a global function marked
  changes_pkt_data.  Since c84400f5fe42 earlier in this series, that mark
  covers any global function that calls bpf_dynptr_write(), even just to
  fill a ringbuf record.  Passing the slice to such a function is also
  refused ("cannot pass packet pointer ... function may change packet
  data").

- bpf_dynptr_clone(sample, &c) now makes an SKB-typed clone, which carries
  the same effects.

For example, a syscall or tracing program whose drain callback does:

    m = bpf_dynptr_slice(d, 0, &buf, sizeof(buf));
    if (m)
            handle(m);

where handle(const struct msg *) is a global function that copies *m into
a ringbuf with bpf_dynptr_write(), was accepted before this patch and
looks like it is rejected after it, although no skb is involved.  Nothing
is unsafe here, but it is a false rejection of valid programs that the
commit message does not describe.

Could the SKB assumption be applied only to dynptrs set up as global
subprog arguments, for example by marking them differently where the
subprog argument setup currently calls:

    __mark_dynptr_reg(reg, BPF_DYNPTR_TYPE_LOCAL, true, ++env->id_gen, 0)

and leaving the type of the callback dynptr as BPF_DYNPTR_TYPE_LOCAL?  That
would also make the new comment match the code.

None of the later commits in this series (222dc092c64e, e17707d54c68,
0bd8e7a834ac, c04474b8dc8d, 64e0b112d688) change this, and e17707d54c68
and 64e0b112d688 add more consumers of the SKB classification.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/39051545464

Re: [PATCH bpf-next v2 33/43] bpf: Keep the symbols of a program until it has ended

[bot+bpf-ci]

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

> bpf: Keep the symbols of a program until it has ended

TL;DR: Design question: kallsyms removal now happens after the RCU callback
frees btf and linfo, so a symbol lookup in that window might touch freed
memory. The review found no such reader.

> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index e1216927373a..6e32db9777c2 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -3085,12 +3085,40 @@ static void bpf_free_used_btfs(struct bpf_prog_aux *aux)

[ ... ]

> +static void bpf_prog_free_deferred(struct work_struct *work)
> +{
> +	struct bpf_prog_aux *aux;
> +	bool ksyms;
> +
>  	aux = container_of(work, struct bpf_prog_aux, work);
> +	/*
> +	 * A prog that still runs when its last reference goes needs its symbols,
> +	 * by which its exception table is found.  It has ended by now.
> +	 */
> +	ksyms = aux->real_func_cnt || !list_empty(&aux->ksym.lnode);
> +	bpf_prog_kallsyms_del_all(aux->prog);
>  #ifdef CONFIG_BPF_SYSCALL
>  	bpf_free_kfunc_btf_tab(aux->kfunc_btf_tab);
>  #endif

[ ... ]

> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index c412bcd1903f..ddb986324f2f 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -2461,6 +2461,8 @@ static void __bpf_prog_put_rcu(struct rcu_head *rcu)
>  	struct bpf_prog_aux *aux = container_of(rcu, struct bpf_prog_aux, rcu);
>  
>  	btf_put(aux->btf);
> +	kvfree(aux->jited_linfo);
> +	kvfree(aux->linfo);
>  	kvfree(aux->func_info);
>  	kfree(aux->func_info_aux);
>  	free_uid(aux->user);
> @@ -2485,10 +2487,7 @@ static void __bpf_prog_put_rcu_tasks(struct rcu_head *rcu)
>  
>  static void __bpf_prog_put_noref(struct bpf_prog *prog, bool deferred)
>  {
> -	bpf_prog_kallsyms_del_all(prog);
>  	module_put(prog->aux->mod);
> -	kvfree(prog->aux->jited_linfo);
> -	kvfree(prog->aux->linfo);
>  	kfree(prog->aux->kfunc_tab);
>  	kfree(prog->aux->ctx_arg_info);
>  	if (prog->aux->attach_btf)

A subsystem pattern flags the ordering here as potentially concerning, so
this is a question rather than a confirmed bug.

__bpf_prog_put_noref() still issues call_rcu() (or call_rcu_tasks() or
call_rcu_tasks_trace()) on &prog->aux->rcu:

	if (deferred) {
		if (IS_ENABLED(CONFIG_TASKS_RCU) && prog->aux->tramp_linked)
			call_rcu_tasks(&prog->aux->rcu, __bpf_prog_put_rcu_tasks);
		else if (bpf_prog_runs_sleepable(prog))
			call_rcu_tasks_trace(&prog->aux->rcu, __bpf_prog_put_rcu);
		else
			call_rcu(&prog->aux->rcu, __bpf_prog_put_rcu);
	}

but with bpf_prog_kallsyms_del_all() gone from this function, aux->ksym is
still linked in the RCU-protected bpf_tree latch tree and the bpf_kallsyms
list when the callback is queued.

The removal now happens in bpf_prog_free_deferred(), which is a work item
scheduled from inside the callback chain:

__bpf_prog_put_rcu()
  bpf_prog_free()
    schedule_work()
      bpf_prog_free_deferred()
        bpf_prog_kallsyms_del_all()

In between, __bpf_prog_put_rcu() has already done:

	btf_put(aux->btf);
	kvfree(aux->jited_linfo);
	kvfree(aux->linfo);
	kvfree(aux->func_info);
	kfree(aux->func_info_aux);

and bpf_prog_free() has dropped aux->dst_prog and aux->token.

Can a tree reader that runs after the grace period but before
bpf_prog_free_deferred() still get the prog from bpf_prog_ksym_find() and
then dereference those fields?  For example:

dump_stack_cb()
  bpf_prog_get_file_line()
    prog->aux->btf, prog->aux->linfo, prog->aux->jited_linfo

The final free of aux and of the JIT images does look to be behind the
second grace period from queue_rcu_work(), so the exposure would only cover
the fields freed in __bpf_prog_put_rcu().

Looking at the callers of bpf_prog_ksym_find() that go past the ksym
fields (search_bpf_extables(), dump_stack_cb(), find_from_stack_cb(),
bpf_stack_walker(), bpf_prog_report_arena_violation()), they resolve
either the IP of a frame on the current stack through arch_bpf_stack_walk()
or the faulting IP.  In both cases the program is still running, so the
grace period chosen in __bpf_prog_put_noref() should not have finished.

The name-only readers (bpf_address_lookup(), bpf_get_kallsym(),
is_bpf_text_address(), bpf_has_frame_pointer()) read only aux->ksym, which
stays valid until the extra grace period.  I did not find a reader that
reaches the freed linfo or btf for a program that is not running.

Is the ordering of call_rcu() before the ksym removal intended, given that
the pattern of queueing an RCU callback on an object that is still
reachable through an RCU-protected structure is normally treated as a bug?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/39051545464

Re: [PATCH bpf-next v2 00/43] bpf: Memory safety, lifetime and locking fixes

[patchwork-bot+netdevbpf]

Hello:

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

On Sat, 17 Oct 2026 16:37:02 +0000 you wrote:
> From: Alexei Starovoitov <ast@kernel.org>
> 
> This series collects fixes for bugs in the BPF core, the verifier and the
> BPF side of the networking stack. Most of them are from Josef.
> 
> Every bug was reproduced in a VM on top of bpf-next, on the kernel right
> before and right after the patch that fixes it. The splats are in the
> commit logs. That goes for what the reviews of v1 have found as well.
> 
> [...]

Here is the summary with links:
  - [bpf-next,v2,01/43] bpf: use the frame mem type in devmap egress XDP
    https://git.kernel.org/bpf/bpf-next/c/b3ca872947ef
  - [bpf-next,v2,02/43] bpf: forbid sleeping in hash map for_each callbacks
    https://git.kernel.org/bpf/bpf-next/c/9bdd5e7f5d83
  - [bpf-next,v2,03/43] bpf: don't leak the inode when token creation fails
    https://git.kernel.org/bpf/bpf-next/c/f52b699c8863
  - [bpf-next,v2,04/43] bpf: don't hand out a linked element as extra element
    https://git.kernel.org/bpf/bpf-next/c/ccb88db8dac8
  - [bpf-next,v2,05/43] bpf: reject sleepable programs for task_vma iterator
    https://git.kernel.org/bpf/bpf-next/c/dfcd17659112
  - [bpf-next,v2,06/43] bpf: forbid sleeping in bpf_find_vma() callbacks
    https://git.kernel.org/bpf/bpf-next/c/c6dc0a288d4e
  - [bpf-next,v2,07/43] bpf: look at all 64 bits of a BPF_JSET mask on 32-bit
    https://git.kernel.org/bpf/bpf-next/c/ee4f44e0e3c8
  - [bpf-next,v2,08/43] bpf: don't let bpf_skb_adjust_room() use up the MSS
    https://git.kernel.org/bpf/bpf-next/c/12d07c38e1b0
  - [bpf-next,v2,09/43] bpf: Read the probe of a perf event under event_mutex
    https://git.kernel.org/bpf/bpf-next/c/3ccd700543f1
  - [bpf-next,v2,10/43] bpf: Don't saturate owner_refcnt on the last unpin
    https://git.kernel.org/bpf/bpf-next/c/30a140c9d479
  - [bpf-next,v2,11/43] bpf: Validate the SRH which End.DT6 uncovers in End.BPF
    https://git.kernel.org/bpf/bpf-next/c/e8b084646b94
  - [bpf-next,v2,12/43] bpf: Pull the inner header for End.DT6 in End.BPF
    https://git.kernel.org/bpf/bpf-next/c/434324c611a9
  - [bpf-next,v2,13/43] bpf: Update the SRH state if End.B6 fails late
    https://git.kernel.org/bpf/bpf-next/c/427e751a231a
  - [bpf-next,v2,14/43] bpf: Clear sk_refcnt of a clone before its storage is linked
    https://git.kernel.org/bpf/bpf-next/c/22f100bd0134
  - [bpf-next,v2,15/43] bpf: Fix NULL deref in the sk_storage map iterator
    https://git.kernel.org/bpf/bpf-next/c/6186c9711ac6
  - [bpf-next,v2,16/43] bpf: Link the first element after its storage is published
    https://git.kernel.org/bpf/bpf-next/c/bd897919e4dc
  - [bpf-next,v2,17/43] bpf: Hold the socket in the sk_storage map iterator
    https://git.kernel.org/bpf/bpf-next/c/070a96b2a98f
  - [bpf-next,v2,18/43] bpf: Bound the nesting that the BTF show code follows
    https://git.kernel.org/bpf/bpf-next/c/13ac7c05bc45
  - [bpf-next,v2,19/43] bpf: Check the new program of an iterator link update
    https://git.kernel.org/bpf/bpf-next/c/45e912fb1db0
  - [bpf-next,v2,20/43] bpf: Fix stale offload->prog pointer after realloc
    https://git.kernel.org/bpf/bpf-next/c/8e83d5359644
  - [bpf-next,v2,21/43] bpf: Don't bind a program to a netdev that goes away
    https://git.kernel.org/bpf/bpf-next/c/af929bc938e0
  - [bpf-next,v2,22/43] bpf: Keep a program until its bpf_wq callback has ended
    https://git.kernel.org/bpf/bpf-next/c/1e28a5e52d32
  - [bpf-next,v2,23/43] bpf: Count sleepable callbacks as sleepable map users
    https://git.kernel.org/bpf/bpf-next/c/927c9c985f3e
  - [bpf-next,v2,24/43] bpf: Check the maps that sleepable callbacks use
    https://git.kernel.org/bpf/bpf-next/c/3e6d7522af2c
  - [bpf-next,v2,25/43] bpf: Assume no bounds for buffers behind a tail call
    https://git.kernel.org/bpf/bpf-next/c/7fa323d77688
  - [bpf-next,v2,26/43] bpf: Don't take an extension for an iterator program
    https://git.kernel.org/bpf/bpf-next/c/7cc639dc45c5
  - [bpf-next,v2,27/43] bpf: Assume no bounds for a writable tp with tail calls
    https://git.kernel.org/bpf/bpf-next/c/cd0490e671d7
  - [bpf-next,v2,28/43] bpf: Don't take an extension for a writable raw tracepoint program
    https://git.kernel.org/bpf/bpf-next/c/7fe348f18b37
  - [bpf-next,v2,29/43] bpf: Keep iterator programs off special map fields
    https://git.kernel.org/bpf/bpf-next/c/8d0358f6e8b3
  - [bpf-next,v2,30/43] bpf: Free what map_free() unlinks for sleepable readers
    https://git.kernel.org/bpf/bpf-next/c/5bdd61e98da5
  - [bpf-next,v2,31/43] bpf: Keep values of a dead owner for sleepable programs
    (no matching commit)
  - [bpf-next,v2,32/43] bpf: Don't take an extension for a function that runs sleepable
    (no matching commit)
  - [bpf-next,v2,33/43] bpf: Keep the symbols of a program until it has ended
    (no matching commit)
  - [bpf-next,v2,34/43] bpf: Use the socket of a sk_lookup test run under RCU
    (no matching commit)
  - [bpf-next,v2,35/43] bpf, cpumap: Hold the device of frames in the queue
    (no matching commit)
  - [bpf-next,v2,36/43] bpf: Drop packet pointers behind kfuncs writing an skb
    (no matching commit)
  - [bpf-next,v2,37/43] bpf: Let bpf_dynptr_write() mark a global function
    (no matching commit)
  - [bpf-next,v2,38/43] bpf: Don't take a dynptr argument for a local dynptr
    (no matching commit)
  - [bpf-next,v2,39/43] bpf: Refuse the prologue for an extension that cannot take it
    (no matching commit)
  - [bpf-next,v2,40/43] bpf: Unclone the skb for kfuncs that write through a dynptr
    (no matching commit)
  - [bpf-next,v2,41/43] bpf: Follow the head if an skb is written from a slice
    (no matching commit)
  - [bpf-next,v2,42/43] bpf: Use memmove() to store bytes in an skb
    (no matching commit)
  - [bpf-next,v2,43/43] bpf: Refuse packet slices for packet-changing helpers
    (no matching commit)

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