tjh.dev / kernel
Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

bpf: Drop special callback reference handling

Logic to prevent callbacks from acquiring new references for the program
(i.e. leaving acquired references), and releasing caller references
(i.e. those acquired in parent frames) was introduced in commit
9d9d00ac29d0 ("bpf: Fix reference state management for synchronous callbacks").

This was necessary because back then, the verifier simulated each
callback once (that could potentially be executed N times, where N can
be zero). This meant that callbacks that left lingering resources or
cleared caller resources could do it more than once, operating on
undefined state or leaking memory.

With the fixes to callback verification in commit
ab5cfac139ab ("bpf: verify callbacks as if they are called unknown number of times"),
all of this extra logic is no longer necessary. Hence, drop it as part
of this commit.

Cc: Eduard Zingerman <eddyz87@gmail.com>
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Link: https://lore.kernel.org/r/20241109231430.2475236-3-memxor@gmail.com
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>

authored by

Kumar Kartikeya Dwivedi and committed by
Andrii Nakryiko ae6e3a27 f6b9a69a

+11 -39
+4 -17
include/linux/bpf_verifier.h
··· 265 265 * is used purely to inform the user of a reference leak. 266 266 */ 267 267 int insn_idx; 268 - union { 269 - /* There can be a case like: 270 - * main (frame 0) 271 - * cb (frame 1) 272 - * func (frame 3) 273 - * cb (frame 4) 274 - * Hence for frame 4, if callback_ref just stored boolean, it would be 275 - * impossible to distinguish nested callback refs. Hence store the 276 - * frameno and compare that to callback_ref in check_reference_leak when 277 - * exiting a callback function. 278 - */ 279 - int callback_ref; 280 - /* Use to keep track of the source object of a lock, to ensure 281 - * it matches on unlock. 282 - */ 283 - void *ptr; 284 - }; 268 + /* Use to keep track of the source object of a lock, to ensure 269 + * it matches on unlock. 270 + */ 271 + void *ptr; 285 272 }; 286 273 287 274 struct bpf_retval_range {
+5 -20
kernel/bpf/verifier.c
··· 1358 1358 state->refs[new_ofs].type = REF_TYPE_PTR; 1359 1359 state->refs[new_ofs].id = id; 1360 1360 state->refs[new_ofs].insn_idx = insn_idx; 1361 - state->refs[new_ofs].callback_ref = state->in_callback_fn ? state->frameno : 0; 1362 1361 1363 1362 return id; 1364 1363 } ··· 1391 1392 if (state->refs[i].type != REF_TYPE_PTR) 1392 1393 continue; 1393 1394 if (state->refs[i].id == ptr_id) { 1394 - /* Cannot release caller references in callbacks */ 1395 - if (state->in_callback_fn && state->refs[i].callback_ref != state->frameno) 1396 - return -EINVAL; 1397 1395 if (last_idx && i != last_idx) 1398 1396 memcpy(&state->refs[i], &state->refs[last_idx], 1399 1397 sizeof(*state->refs)); ··· 10263 10267 caller->regs[BPF_REG_0] = *r0; 10264 10268 } 10265 10269 10266 - /* callback_fn frame should have released its own additions to parent's 10267 - * reference state at this point, or check_reference_leak would 10268 - * complain, hence it must be the same as the caller. There is no need 10269 - * to copy it back. 10270 - */ 10271 - if (!callee->in_callback_fn) { 10272 - /* Transfer references to the caller */ 10273 - err = copy_reference_state(caller, callee); 10274 - if (err) 10275 - return err; 10276 - } 10270 + /* Transfer references to the caller */ 10271 + err = copy_reference_state(caller, callee); 10272 + if (err) 10273 + return err; 10277 10274 10278 10275 /* for callbacks like bpf_loop or bpf_for_each_map_elem go back to callsite, 10279 10276 * there function call logic would reschedule callback visit. If iteration ··· 10436 10447 bool refs_lingering = false; 10437 10448 int i; 10438 10449 10439 - if (!exception_exit && state->frameno && !state->in_callback_fn) 10450 + if (!exception_exit && state->frameno) 10440 10451 return 0; 10441 10452 10442 10453 for (i = 0; i < state->acquired_refs; i++) { 10443 10454 if (state->refs[i].type != REF_TYPE_PTR) 10444 - continue; 10445 - if (!exception_exit && state->in_callback_fn && state->refs[i].callback_ref != state->frameno) 10446 10455 continue; 10447 10456 verbose(env, "Unreleased reference id=%d alloc_insn=%d\n", 10448 10457 state->refs[i].id, state->refs[i].insn_idx); ··· 17694 17707 return false; 17695 17708 switch (old->refs[i].type) { 17696 17709 case REF_TYPE_PTR: 17697 - if (old->refs[i].callback_ref != cur->refs[i].callback_ref) 17698 - return false; 17699 17710 break; 17700 17711 case REF_TYPE_LOCK: 17701 17712 if (old->refs[i].ptr != cur->refs[i].ptr)
+2 -2
tools/testing/selftests/bpf/prog_tests/cb_refs.c
··· 11 11 const char *prog_name; 12 12 const char *err_msg; 13 13 } cb_refs_tests[] = { 14 - { "underflow_prog", "reference has not been acquired before" }, 15 - { "leak_prog", "Unreleased reference" }, 14 + { "underflow_prog", "must point to scalar, or struct with scalar" }, 15 + { "leak_prog", "Possibly NULL pointer passed to helper arg2" }, 16 16 { "nested_cb", "Unreleased reference id=4 alloc_insn=2" }, /* alloc_insn=2{4,5} */ 17 17 { "non_cb_transfer_ref", "Unreleased reference id=4 alloc_insn=1" }, /* alloc_insn=1{1,2} */ 18 18 };