Skip to content

Refactor verifier object relationship tracking#11905

Closed
kernel-patches-daemon-bpf[bot] wants to merge 10 commits into
bpf-next_basefrom
series/1084054=>bpf-next
Closed

Refactor verifier object relationship tracking#11905
kernel-patches-daemon-bpf[bot] wants to merge 10 commits into
bpf-next_basefrom
series/1084054=>bpf-next

Conversation

@kernel-patches-daemon-bpf

Copy link
Copy Markdown

Pull request for series with
subject: Refactor verifier object relationship tracking
version: 3
url: https://patchwork.kernel.org/project/netdevbpf/list/?series=1084054

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: c8f0ee9
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1084054
version: 3

@kernel-patches-review-bot

Copy link
Copy Markdown
commit fca4fba40f8c03391f715975d13ebeddd6610f41
Author: Amery Hung <ameryhung@gmail.com>

bpf: Preserve reg->id of pointer objects after null-check

This patch modifies mark_ptr_or_null_reg() to preserve reg->id for all
pointer objects after null-checking, not just spin_lock pointers. This
supports object relationship tracking introduced in a later patch.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 93003a2a96b0..0313b7d5f6c9 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -15886,15 +15886,10 @@ static void mark_ptr_or_null_reg(struct bpf_func_state *state,
>
>  		mark_ptr_not_null_reg(reg);
>
> -		if (!reg_may_point_to_spin_lock(reg)) {
> -			/* For not-NULL ptr, reg->ref_obj_id will be reset
> -			 * in release_reference().
> -			 *
> -			 * reg->id is still used by spin_lock ptr. Other
> -			 * than spin_lock ptr type, reg->id can be reset.
> -			 */
> -			reg->id = 0;
> -		}
> +		/*
> +		 * reg->id is preserved for object relationship tracking
> +		 * and spin_lock lock state tracking
> +		 */

How does preserving reg->id interact with reg_is_init_pkt_pointer()?

In v2 review, Alexei Starovoitov raised this concern: "That pointer has
to have id == 0". Eduard Zingerman responded that dropping the id == 0
check should be fine, but this change was not made in v3.

The function reg_is_init_pkt_pointer() still checks reg->id == 0:

    return reg->type == which &&
           reg->id == 0 &&
           reg->off == 0 &&

With this patch, if a PTR_TO_PACKET register goes through null-checking
in mark_ptr_or_null_reg(), it will now preserve its reg->id instead of
resetting it to 0. Would this cause reg_is_init_pkt_pointer() to
incorrectly return false for init packet pointers?

Reference: https://lore.kernel.org/bpf/CAADnVQLCKi_QL1FeybW+6MH6FAOAoRJyW06UvtX3my3yNSH5eQ@mail.gmail.com/

>  	}
>  }



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

In-Reply-To-Subject: bpf: Preserve reg->id of pointer objects after null-check
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/24749741787

@kernel-patches-review-bot

Copy link
Copy Markdown
commit 3a7b7021717b1d5c1192fde7a3dd630dfbcea625
Author: Amery Hung <ameryhung@gmail.com>

bpf: Unify dynptr handling in the verifier

This commit simplifies dynptr checking for helper and kfunc by unifying it.
It saves initialized dynptr information in meta->dynptr, eliminating the need
to call dynptr helpers such as dynptr_id(), dynptr_ref_obj_id() and
dynptr_type() in check_func_arg().

> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index b148f816f25b..dc0cff59246d 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -1319,6 +1319,12 @@ struct bpf_map_desc {
>  	int uid;
>  };
>
> +struct bpf_dynptr_desc {
> +	enum bpf_dynptr_type type;
> +	u32 id;
> +	u32 ref_obj_id;
> +};
> +

This isn't a bug, but as noted by Mykyta Yatsenko in v2 review, the new struct
bpf_dynptr_desc could benefit from a comment explaining what the ref_obj_id
field is for.

While you mentioned in your response that comments for id, ref_obj_id, and
parent_id will be added in a later refactor patch (#6) since these fields
apply to all objects tracked by the verifier, the struct currently lacks this
inline documentation. Could this documentation be added now to help readers
understand the field's purpose?

Reference: https://lore.kernel.org/bpf/87ikb2tkta.fsf@gmail.com/

[ ... ]



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

In-Reply-To-Subject: bpf: Unify dynptr handling in the verifier
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/24749741787

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: 9012cf2
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1084054
version: 3

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: e85115f
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1084054
version: 3

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: 1980023
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1084054
version: 3

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: 7f843c0
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1084054
version: 3

Simplify dynptr checking for helper and kfunc by unifying it. Remember
the initialized dynptr (i.e.,g !(arg_type |= MEM_UNINIT)) pass to a
dynptr kfunc during process_dynptr_func() so that we can easily
retrieve the information for verification later. By saving it in
meta->dynptr, there is no need to call dynptr helpers such as
dynptr_id(), dynptr_ref_obj_id() and dynptr_type() in check_func_arg().

Remove and open code the helpers in process_dynptr_func() when
saving id, ref_obj_id, and type. It is okay to drop spi < 0 check as
is_dynptr_reg_valid_init() has made sure the dynptr is valid.

Besides, since dynptr ref_obj_id information is now pass around in
meta->bpf_dynptr_desc, drop the check in helper_multiple_ref_obj_use.

Acked-by: Eduard Zingerman <eddyz87@gmail.com>
Signed-off-by: Amery Hung <ameryhung@gmail.com>
Acked-by: Mykyta Yatsenko <yatsenko@meta.com>
Assign reg->id when getting referenced kptr from read program context
to be consistent with R0 of KF_ACQUIRE kfunc. skb dynptr will track the
referenced skb in qdisc programs using a new field reg->parent_id in
a later patch.

Signed-off-by: Amery Hung <ameryhung@gmail.com>
Preserve reg->id of pointer objects after null-checking the register so
that children objects derived from it can still refer to it in the new
object relationship tracking mechanism introduced in a later patch. This
change incurs a slight increase in the number of states in one selftest
bpf object, rbtree_search.bpf.o. For Meta bpf objects, the increase of
states is also negligible.

Selftest BPF objects with insns_diff > 0

Program                   Insns (A)  Insns (B)  Insns   (DIFF)  States (A)  States (B)  States (DIFF)
------------------------  ---------  ---------  --------------  ----------  ----------  -------------
rbtree_search                  6820       7326   +506 (+7.42%)         379         398   +19 (+5.01%)

Meta BPF objects with insns_diff > 0

Program                   Insns (A)  Insns (B)  Insns   (DIFF)  States (A)  States (B)  States (DIFF)
------------------------  ---------  ---------  --------------  ----------  ----------  -------------
ned_imex_be_tclass               52         57     +5 (+9.62%)           5           6   +1 (+20.00%)
ned_imex_be_tclass               52         57     +5 (+9.62%)           5           6   +1 (+20.00%)
ned_skop_auto_flowlabel         523        526     +3 (+0.57%)          39          40    +1 (+2.56%)
ned_skop_mss                    289        292     +3 (+1.04%)          20          20    +0 (+0.00%)
ned_skopt_bet_classifier         78         82     +4 (+5.13%)           8           8    +0 (+0.00%)
dctcp_update_alpha              252        320   +68 (+26.98%)          21          27   +6 (+28.57%)
dctcp_update_alpha              252        320   +68 (+26.98%)          21          27   +6 (+28.57%)
ned_ts_func                     119        126     +7 (+5.88%)           6           7   +1 (+16.67%)
tw_egress                      1119       1128     +9 (+0.80%)          95          96    +1 (+1.05%)
tw_ingress                     1128       1137     +9 (+0.80%)          95          96    +1 (+1.05%)
tw_tproxy_router               4380       4465    +85 (+1.94%)         114         118    +4 (+3.51%)
tw_tproxy_router4              3093       3170    +77 (+2.49%)          83          88    +5 (+6.02%)
ttls_tc_ingress               34656      35717  +1061 (+3.06%)         936         970   +34 (+3.63%)
tw_twfw_egress               222327     222338    +11 (+0.00%)       10563       10564    +1 (+0.01%)
tw_twfw_ingress               78295      78299     +4 (+0.01%)        3825        3826    +1 (+0.03%)
tw_twfw_tc_eg                222839     222859    +20 (+0.01%)       10584       10585    +1 (+0.01%)
tw_twfw_tc_in                 78295      78299     +4 (+0.01%)        3825        3826    +1 (+0.03%)
tw_twfw_egress                 8080       8085     +5 (+0.06%)         456         456    +0 (+0.00%)
tw_twfw_ingress                8053       8056     +3 (+0.04%)         454         454    +0 (+0.00%)
tw_twfw_tc_eg                  8154       8174    +20 (+0.25%)         456         457    +1 (+0.22%)
tw_twfw_tc_in                  8060       8063     +3 (+0.04%)         455         455    +0 (+0.00%)
tw_twfw_egress               222327     222338    +11 (+0.00%)       10563       10564    +1 (+0.01%)
tw_twfw_ingress               78295      78299     +4 (+0.01%)        3825        3826    +1 (+0.03%)
tw_twfw_tc_eg                222839     222859    +20 (+0.01%)       10584       10585    +1 (+0.01%)
tw_twfw_tc_in                 78295      78299     +4 (+0.01%)        3825        3826    +1 (+0.03%)
tw_twfw_egress                 8080       8085     +5 (+0.06%)         456         456    +0 (+0.00%)
tw_twfw_ingress                8053       8056     +3 (+0.04%)         454         454    +0 (+0.00%)
tw_twfw_tc_eg                  8154       8174    +20 (+0.25%)         456         457    +1 (+0.22%)
tw_twfw_tc_in                  8060       8063     +3 (+0.04%)         455         455    +0 (+0.00%)

Looking into rbtree_search, the reason for such increase is that the
verifier has to explore the main loop shown below for one more iteration
until state pruning decides the current state is safe.

long rbtree_search(void *ctx)
{
	...
	bpf_spin_lock(&glock0);
	rb_n = bpf_rbtree_root(&groot0);
	while (can_loop) {
		if (!rb_n) {
			bpf_spin_unlock(&glock0);
			return __LINE__;
		}

		n = rb_entry(rb_n, struct node_data, r0);
		if (lookup_key == n->key0)
			break;
		if (nr_gc < NR_NODES)
			gc_ns[nr_gc++] = rb_n;
		if (lookup_key < n->key0)
			rb_n = bpf_rbtree_left(&groot0, rb_n);
		else
			rb_n = bpf_rbtree_right(&groot0, rb_n);
	}
	...
}

Below is what the verifier sees at the start of each iteration
(65: may_goto) after preserving id of rb_n. Without id of rb_n, the
verifier stops exploring the loop at iter 16.

           rb_n  gc_ns[15]
iter 15    257   257

iter 16    290   257    rb_n: idmap add 257->290
                        gc_ns[15]: check 257 != 290 --> state not equal

iter 17    325   257    rb_n: idmap add 290->325
                        gc_ns[15]: idmap add 257->257 --> state safe

Signed-off-by: Amery Hung <ameryhung@gmail.com>
Refactor object relationship tracking in the verifier and fix a dynptr
use-after-free bug where file/skb dynptrs are not invalidated when the
parent referenced object is freed.

Add parent_id to bpf_reg_state to precisely track child-parent
relationships. A child object's parent_id points to the parent object's
id. This replaces the PTR_TO_MEM-specific dynptr_id and does not
increase the size of bpf_reg_state on 64-bit machines as there is
existing padding.

When calling dynptr constructors (i.e., process_dynptr_func() with
MEM_UNINIT argument), track the parent's id if the parent is a
referenced object. This only applies to file dynptr and skb dynptr,
so only pass parent reg->id to kfunc constructors.

For release_reference(), invalidating an object now also invalidates
all descendants by traversing the object tree. This is done using
stack-based DFS to avoid recursive call chains of release_reference() ->
unmark_stack_slots_dynptr() -> release_reference(). Referenced objects
encountered during tree traversal cannot be indirectly released. They
require an explicit helper/kfunc call to release the acquired resources.

While the new design changes how object relationships are tracked in
the verifier, it does not change the verifier's behavior. Here is the
implication for dynptr, pointer casting, and owning/non-owning
references:

Dynptr:

When initializing a dynptr, referenced dynptrs acquire a reference for
ref_obj_id. If the dynptr has a referenced parent, parent_id tracks the
parent's id. When cloning, ref_obj_id and parent_id are copied from the
original. Releasing a referenced dynptr via release_reference(ref_obj_id)
invalidates all clones and derived slices. For non-referenced dynptrs,
only the specific dynptr and its children are invalidated.

Pointer casting:

Referenced socket pointers and their casted counterparts share the same
lifetime but have different nullness — they have different id but the
same ref_obj_id.

Owning to non-owning reference conversion:

After converting owning to non-owning by clearing ref_obj_id (e.g.,
object(id=1, ref_obj_id=1) -> object(id=1, ref_obj_id=0)), the
verifier only needs to release the reference state, so it calls
release_reference_nomark() instead of release_reference().

Note that the error message "reference has not been acquired before" in
the helper and kfunc release paths is removed. This message was already
unreachable. The verifier only calls release_reference() after
confirming meta.ref_obj_id is valid, so the condition could never
trigger in practice (no selftest exercises it either). With the
refactor, release_reference() can now be called with non-acquired ids
and have different error conditions. Report directly in
release_reference() instead.

Fixes: 870c285 ("bpf: net_sched: Add basic bpf qdisc kfuncs")
Signed-off-by: Amery Hung <ameryhung@gmail.com>
unmark_stack_slots_dynptr() already makes sure that CONST_PTR_TO_DYNPTR
cannot be released. process_dynptr_func() also prevents passing
uninitialized dynptr to helpers expecting initialized dynptr. Now that
unmark_stack_slots_dynptr() also error returned from
release_reference(), there should be no reason to keep these redundant
checks.

Signed-off-by: Amery Hung <ameryhung@gmail.com>
The verifier currently does not allow creating dynptr from dynptr data
or slice. Add a selftest to test this explicitly.

Signed-off-by: Amery Hung <ameryhung@gmail.com>
Make sure the verifier invalidates the dynptr and dynptr slice derived
from an skb after the skb is freed.

Signed-off-by: Amery Hung <ameryhung@gmail.com>
The parent object of a cloned dynptr is skb not the original dynptr.
Invalidate the original dynptr should not prevent the program from
using the slice derived from the cloned dynptr.

Signed-off-by: Amery Hung <ameryhung@gmail.com>
…dropped

File dynptr and slice should be invalidated when the parent file's
reference is dropped in the program. Without the verifier tracking
dyntpr's parent referenced object, the dynptr would continute to be
incorrectly used even if the underlying file is being tear down or gone.

Signed-off-by: Amery Hung <ameryhung@gmail.com>
@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: 7f843c0
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1084054
version: 3

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: fd30cf8
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1084054
version: 3

Pull request is NOT updated. Failed to apply https://patchwork.kernel.org/project/netdevbpf/list/?series=1084054
error message:

Cmd('git') failed due to: exit code(128)
  cmdline: git am --3way
  stdout: 'Applying: bpf: Unify dynptr handling in the verifier
Using index info to reconstruct a base tree...
M	include/linux/bpf_verifier.h
M	kernel/bpf/verifier.c
Falling back to patching base and 3-way merge...
Auto-merging include/linux/bpf_verifier.h
Auto-merging kernel/bpf/verifier.c
CONFLICT (content): Merge conflict in kernel/bpf/verifier.c
Patch failed at 0001 bpf: Unify dynptr handling in the verifier'
  stderr: 'error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"'

conflict:

diff --cc kernel/bpf/verifier.c
index ff6ff1c27517,41e4ea41c72e..000000000000
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@@ -7457,9 -7425,11 +7450,15 @@@ static int process_kptr_func(struct bpf
   * use case. The second level is tracked using the upper bit of bpf_dynptr->size
   * and checked dynamically during runtime.
   */
++<<<<<<< HEAD
 +static int process_dynptr_func(struct bpf_verifier_env *env, struct bpf_reg_state *reg, argno_t argno, int insn_idx,
 +			       enum bpf_arg_type arg_type, int clone_ref_obj_id)
++=======
+ static int process_dynptr_func(struct bpf_verifier_env *env, int regno, int insn_idx,
+ 			       enum bpf_arg_type arg_type, int clone_ref_obj_id,
+ 			       struct bpf_dynptr_desc *dynptr)
++>>>>>>> bpf: Unify dynptr handling in the verifier
  {
 -	struct bpf_reg_state *reg = reg_state(env, regno);
  	int err;
  
  	if (reg->type != PTR_TO_STACK && reg->type != CONST_PTR_TO_DYNPTR) {
@@@ -8293,74 -8271,8 +8306,79 @@@ static int check_func_arg_reg_off(struc
  	}
  }
  
++<<<<<<< HEAD
 +static struct bpf_reg_state *get_dynptr_arg_reg(struct bpf_verifier_env *env,
 +						const struct bpf_func_proto *fn,
 +						struct bpf_reg_state *regs)
 +{
 +	struct bpf_reg_state *state = NULL;
 +	int i;
 +
 +	for (i = 0; i < MAX_BPF_FUNC_REG_ARGS; i++)
 +		if (arg_type_is_dynptr(fn->arg_type[i])) {
 +			if (state) {
 +				verbose(env, "verifier internal error: multiple dynptr args\n");
 +				return NULL;
 +			}
 +			state = &regs[BPF_REG_1 + i];
 +		}
 +
 +	if (!state)
 +		verbose(env, "verifier internal error: no dynptr arg found\n");
 +
 +	return state;
 +}
 +
 +static int dynptr_id(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
 +{
 +	struct bpf_func_state *state = bpf_func(env, reg);
 +	int spi;
 +
 +	if (reg->type == CONST_PTR_TO_DYNPTR)
 +		return reg->id;
 +	spi = dynptr_get_spi(env, reg);
 +	if (spi < 0)
 +		return spi;
 +	return state->stack[spi].spilled_ptr.id;
 +}
 +
 +static int dynptr_ref_obj_id(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
 +{
 +	struct bpf_func_state *state = bpf_func(env, reg);
 +	int spi;
 +
 +	if (reg->type == CONST_PTR_TO_DYNPTR)
 +		return reg->ref_obj_id;
 +	spi = dynptr_get_spi(env, reg);
 +	if (spi < 0)
 +		return spi;
 +	return state->stack[spi].spilled_ptr.ref_obj_id;
 +}
 +
 +static enum bpf_dynptr_type dynptr_get_type(struct bpf_verifier_env *env,
 +					    struct bpf_reg_state *reg)
 +{
 +	struct bpf_func_state *state = bpf_func(env, reg);
 +	int spi;
 +
 +	if (reg->type == CONST_PTR_TO_DYNPTR)
 +		return reg->dynptr.type;
 +
 +	spi = bpf_get_spi(reg->var_off.value);
 +	if (spi < 0) {
 +		verbose(env, "verifier internal error: invalid spi when querying dynptr type\n");
 +		return BPF_DYNPTR_TYPE_INVALID;
 +	}
 +
 +	return state->stack[spi].spilled_ptr.dynptr.type;
 +}
 +
 +static int check_arg_const_str(struct bpf_verifier_env *env,
 +			       struct bpf_reg_state *reg, argno_t argno)
++=======
+ static int check_reg_const_str(struct bpf_verifier_env *env,
+ 			       struct bpf_reg_state *reg, u32 regno)
++>>>>>>> bpf: Unify dynptr handling in the verifier
  {
  	struct bpf_map *map = reg->map_ptr;
  	int err;
@@@ -8716,7 -8625,7 +8734,11 @@@ skip_type_check
  					 true, meta);
  		break;
  	case ARG_PTR_TO_DYNPTR:
++<<<<<<< HEAD
 +		err = process_dynptr_func(env, reg, argno_from_reg(regno), insn_idx, arg_type, 0);
++=======
+ 		err = process_dynptr_func(env, regno, insn_idx, arg_type, 0, &meta->dynptr);
++>>>>>>> bpf: Unify dynptr handling in the verifier
  		if (err)
  			return err;
  		break;
@@@ -9379,7 -9284,7 +9401,11 @@@ static int btf_check_func_arg_match(str
  			if (ret)
  				return ret;
  
++<<<<<<< HEAD
 +			ret = process_dynptr_func(env, reg, argno, -1, arg->arg_type, 0);
++=======
+ 			ret = process_dynptr_func(env, regno, -1, arg->arg_type, 0, NULL);
++>>>>>>> bpf: Unify dynptr handling in the verifier
  			if (ret)
  				return ret;
  		} else if (base_type(arg->arg_type) == ARG_PTR_TO_BTF_ID) {
@@@ -12328,23 -12177,10 +12314,15 @@@ static int check_kfunc_args(struct bpf_
  				}
  			}
  
++<<<<<<< HEAD
 +			ret = process_dynptr_func(env, reg, argno, insn_idx,
 +						  dynptr_arg_type, clone_ref_obj_id);
++=======
+ 			ret = process_dynptr_func(env, regno, insn_idx, dynptr_arg_type, clone_ref_obj_id,
+ 						  &meta->dynptr);
++>>>>>>> bpf: Unify dynptr handling in the verifier
  			if (ret < 0)
  				return ret;
- 
- 			if (!(dynptr_arg_type & MEM_UNINIT)) {
- 				int id = dynptr_id(env, reg);
- 
- 				if (id < 0) {
- 					verifier_bug(env, "failed to obtain dynptr id");
- 					return id;
- 				}
- 				meta->initialized_dynptr.id = id;
- 				meta->initialized_dynptr.type = dynptr_get_type(env, reg);
- 				meta->initialized_dynptr.ref_obj_id = dynptr_ref_obj_id(env, reg);
- 			}
- 
  			break;
  		}
  		case KF_ARG_PTR_TO_ITER:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant