[PATCH bpf-next v1 0/3] Object relationship refactor followup

[PATCH bpf-next v1 0/3] Object relationship refactor followup

[Amery Hung]

Hi,

The main patchset refactoring object relationship tracking in the
verifier has landed and this is a followup that addresses the remaining
feedback in v6 [0].

[0] https://lore.kernel.org/bpf/20260529014936.2811085-1-ameryhung@gmail.com/


Amery Hung (3):
  bpf: Fix dead error check on acquire_reference() in check_kfunc_call
  bpf: Compare parent_id in refsafe() for REF_TYPE_PTR
  selftests/bpf: Use bpf_dynptr_slice() to read file dynptr in leak test

 kernel/bpf/states.c                                  | 3 +++
 kernel/bpf/verifier.c                                | 3 ++-
 tools/testing/selftests/bpf/progs/file_reader_fail.c | 8 ++++----
 3 files changed, 9 insertions(+), 5 deletions(-)

-- 
2.53.0-Meta

[PATCH bpf-next v1 1/3] bpf: Fix dead error check on acquire_reference() in check_kfunc_call

[Amery Hung]

acquire_reference() returns a signed int that may be a negative errno
but was converted to unsigned, which makes the subsequent error check
deadcode. Fix it by declaring 'id' as int so the error path is taken
correctly.

Signed-off-by: Amery Hung <ameryhung@gmail.com>
---
 kernel/bpf/verifier.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 8ed484cb1a8a..6446db9628ae 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -12817,9 +12817,10 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
 	struct bpf_kfunc_call_arg_meta meta;
 	struct bpf_insn_aux_data *insn_aux;
 	int err, insn_idx = *insn_idx_p;
-	u32 i, nargs, ptr_type_id, id;
 	const struct btf_param *args;
+	u32 i, nargs, ptr_type_id;
 	struct btf *desc_btf;
+	int id;
 
 	/* skip for now, but return error when we find this in fixup_kfunc_call */
 	if (!insn->imm)
-- 
2.53.0-Meta

[PATCH bpf-next v1 2/3] bpf: Compare parent_id in refsafe() for REF_TYPE_PTR

[Amery Hung]

refsafe() compared each reference's id and type but not its parent_id,
so two states whose PTR references differ only in the parent object they
were derived from could be wrongly treated as equivalent and pruned. Fix
it by checking parent_id too.

Signed-off-by: Amery Hung <ameryhung@gmail.com>
---
 kernel/bpf/states.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/bpf/states.c b/kernel/bpf/states.c
index 5945956a7573..06d9ae24f006 100644
--- a/kernel/bpf/states.c
+++ b/kernel/bpf/states.c
@@ -890,6 +890,9 @@ static bool refsafe(struct bpf_verifier_state *old, struct bpf_verifier_state *c
 			return false;
 		switch (old->refs[i].type) {
 		case REF_TYPE_PTR:
+			if (!check_ids(old->refs[i].parent_id, cur->refs[i].parent_id, idmap))
+				return false;
+			break;
 		case REF_TYPE_IRQ:
 			break;
 		case REF_TYPE_LOCK:
-- 
2.53.0-Meta

[PATCH bpf-next v1 3/3] selftests/bpf: Use bpf_dynptr_slice() to read file dynptr in leak test

[Amery Hung]

use_file_dynptr_slice_after_put_file() reads the dynptr via
bpf_dynptr_data(), which always returns NULL for a read-only file
dynptr, making the example confusing. Switch to bpf_dynptr_slice(), the
correct read API for file dynptrs, and read (rather than write) the slice
since it is read-only. The test still fails as expected.

Signed-off-by: Amery Hung <ameryhung@gmail.com>
---
 tools/testing/selftests/bpf/progs/file_reader_fail.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/file_reader_fail.c b/tools/testing/selftests/bpf/progs/file_reader_fail.c
index d5fae5e4cf9a..3bb9e2612f8f 100644
--- a/tools/testing/selftests/bpf/progs/file_reader_fail.c
+++ b/tools/testing/selftests/bpf/progs/file_reader_fail.c
@@ -87,7 +87,8 @@ int use_file_dynptr_slice_after_put_file(void *ctx)
 	struct task_struct *task = bpf_get_current_task_btf();
 	struct file *file = bpf_get_task_exe_file(task);
 	struct bpf_dynptr dynptr;
-	char *data;
+	char buf[1];
+	const char *data;
 
 	if (!file)
 		return 0;
@@ -95,15 +96,14 @@ int use_file_dynptr_slice_after_put_file(void *ctx)
 	if (bpf_dynptr_from_file(file, 0, &dynptr))
 		goto out;
 
-	data = bpf_dynptr_data(&dynptr, 0, 1);
+	data = bpf_dynptr_slice(&dynptr, 0, buf, sizeof(buf));
 	if (!data)
 		goto out;
 
 	/* this should fail - file dynptr should be discarded first to prevent resource leak */
 	bpf_put_file(file);
 
-	*data = 'x';
-	return 0;
+	return data[0];
 
 out:
 	bpf_dynptr_file_discard(&dynptr);
-- 
2.53.0-Meta

Re: [PATCH bpf-next v1 0/3] Object relationship refactor followup

[Kumar Kartikeya Dwivedi]

On Fri, 5 Jun 2026 at 00:09, Amery Hung <ameryhung@gmail.com> wrote:
>
> Hi,
>
> The main patchset refactoring object relationship tracking in the
> verifier has landed and this is a followup that addresses the remaining
> feedback in v6 [0].
>
> [0] https://lore.kernel.org/bpf/20260529014936.2811085-1-ameryhung@gmail.com/
>
>

bpf-next makes sense, but Fixes: tag looks missing?

Re: [PATCH bpf-next v1 1/3] bpf: Fix dead error check on acquire_reference() in check_kfunc_call

[bot+bpf-ci]

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

>     bpf: Fix dead error check on acquire_reference() in check_kfunc_call
>
>     acquire_reference() returns a signed int that may be a negative errno
>     but was converted to unsigned, which makes the subsequent error check
>     deadcode. Fix it by declaring 'id' as int so the error path is taken
>     correctly.
>
>     Signed-off-by: Amery Hung <ameryhung@gmail.com>

This looks like a bug fix, but it does not carry a Fixes: tag.  The dead
error check appears to have been introduced when check_kfunc_call() began
using the function-scoped 'u32 i, nargs, ptr_type_id, id;' declaration in
place of the earlier block-local 'int id'.

Should this include:

  Fixes: 308c7a0ae885 ("bpf: Refactor object relationship tracking and fix dynptr UAF 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/26983135146

Re: [PATCH bpf-next v1 2/3] bpf: Compare parent_id in refsafe() for REF_TYPE_PTR

[bot+bpf-ci]

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

> commit 89f2be9817e074d02702d22068104fb9fd9aa103
> Author: Amery Hung <ameryhung@gmail.com>
> Date:   Thu Jun 4 15:09:03 2026 -0700
>
>     bpf: Compare parent_id in refsafe() for REF_TYPE_PTR
>
>     refsafe() compared each reference's id and type but not its parent_id,
>     so two states whose PTR references differ only in the parent object they
>     were derived from could be wrongly treated as equivalent and pruned. Fix
>     it by checking parent_id too.
>
>     Signed-off-by: Amery Hung <ameryhung@gmail.com>

This looks like a bug fix for parent_id tracking support. Should this
include:

  Fixes: 308c7a0ae885 ("bpf: Refactor object relationship tracking and fix dynptr UAF bug")

When commit 308c7a0ae885 added parent_id tracking, it updated several
safety-checking functions to compare parent_id but missed adding the
parent_id check in refsafe() for the REF_TYPE_PTR case. This caused states
with different parent objects to be incorrectly treated as equivalent
during state pruning.

> diff --git a/kernel/bpf/states.c b/kernel/bpf/states.c
> index 5945956a7573..06d9ae24f006 100644
> --- a/kernel/bpf/states.c
> +++ b/kernel/bpf/states.c
> @@ -890,6 +890,9 @@ static bool refsafe(struct bpf_verifier_state *old, struct bpf_verifier_state *c
>  			return false;
>  		switch (old->refs[i].type) {
>  		case REF_TYPE_PTR:
> +			if (!check_ids(old->refs[i].parent_id, cur->refs[i].parent_id, idmap))
> +				return false;
> +			break;
>  		case REF_TYPE_IRQ:
>  			break;
>  		case REF_TYPE_LOCK:

This adds an extra check_ids() call for parent_id. Could this make it
easier to trigger the WARN_ON_ONCE() in check_ids() via BPF_ID_MAP_SIZE
exhaustion?

Since acquired_refs has no explicit limit, a BPF program could acquire and
leak many references (e.g., via bpf_sk_lookup_tcp) before a branch merge
point. When states_equal() invokes refsafe(), it calls check_ids() for
each acquired reference.

The idmap has a fixed size based only on registers and stack slots:

include/linux/bpf_verifier.h:
#define BPF_ID_MAP_SIZE ((MAX_BPF_REG + MAX_BPF_STACK / BPF_REG_SIZE + \
			  MAX_STACK_ARG_SLOTS) * MAX_CALL_FRAMES)

If a program accumulates enough leaked references, checking them all will
run out of idmap slots and trigger the warning:

kernel/bpf/states.c:check_ids() {
	...
	/* Reached the end of known mappings; haven't seen this id before */
	if (idmap->cnt < BPF_ID_MAP_SIZE) {
		...
		return true;
	}

	/* We ran out of idmap slots, which should be impossible */
	WARN_ON_ONCE(1);
	return false;
}

With this patch now checking both reference id and parent_id for
REF_TYPE_PTR (doubling the idmap slot usage per reference), does this
allow exhausting BPF_ID_MAP_SIZE much faster?

Could a user intentionally leak enough references to trigger this warning
and potentially panic the kernel if panic_on_warn is set?


---
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/26983135146

Re: [PATCH bpf-next v1 1/3] bpf: Fix dead error check on acquire_reference() in check_kfunc_call

[Eduard Zingerman]

On Thu, 2026-06-04 at 15:09 -0700, Amery Hung wrote:
> acquire_reference() returns a signed int that may be a negative errno
> but was converted to unsigned, which makes the subsequent error check
> deadcode. Fix it by declaring 'id' as int so the error path is taken
> correctly.
> 
> Signed-off-by: Amery Hung <ameryhung@gmail.com>
> ---

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

[...]

Re: [PATCH bpf-next v1 0/3] Object relationship refactor followup

[Amery Hung]

On Thu, Jun 4, 2026 at 3:15 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
>
> On Fri, 5 Jun 2026 at 00:09, Amery Hung <ameryhung@gmail.com> wrote:
> >
> > Hi,
> >
> > The main patchset refactoring object relationship tracking in the
> > verifier has landed and this is a followup that addresses the remaining
> > feedback in v6 [0].
> >
> > [0] https://lore.kernel.org/bpf/20260529014936.2811085-1-ameryhung@gmail.com/
> >
> >
>
> bpf-next makes sense, but Fixes: tag looks missing?

I will add fix tags to both patch 1 and 2 and respin.

Re: [PATCH bpf-next v1 2/3] bpf: Compare parent_id in refsafe() for REF_TYPE_PTR

[Eduard Zingerman]

On Thu, 2026-06-04 at 22:59 +0000, bot+bpf-ci@kernel.org wrote:
> > commit 89f2be9817e074d02702d22068104fb9fd9aa103
> > Author: Amery Hung <ameryhung@gmail.com>
> > Date:   Thu Jun 4 15:09:03 2026 -0700
> > 
> >     bpf: Compare parent_id in refsafe() for REF_TYPE_PTR
> > 
> >     refsafe() compared each reference's id and type but not its parent_id,
> >     so two states whose PTR references differ only in the parent object they
> >     were derived from could be wrongly treated as equivalent and pruned. Fix
> >     it by checking parent_id too.
> > 
> >     Signed-off-by: Amery Hung <ameryhung@gmail.com>
> 
> This looks like a bug fix for parent_id tracking support. Should this
> include:
> 
>   Fixes: 308c7a0ae885 ("bpf: Refactor object relationship tracking and fix dynptr UAF bug")
> 
> When commit 308c7a0ae885 added parent_id tracking, it updated several
> safety-checking functions to compare parent_id but missed adding the
> parent_id check in refsafe() for the REF_TYPE_PTR case. This caused states
> with different parent objects to be incorrectly treated as equivalent
> during state pruning.
> 
> > diff --git a/kernel/bpf/states.c b/kernel/bpf/states.c
> > index 5945956a7573..06d9ae24f006 100644
> > --- a/kernel/bpf/states.c
> > +++ b/kernel/bpf/states.c
> > @@ -890,6 +890,9 @@ static bool refsafe(struct bpf_verifier_state *old, struct bpf_verifier_state *c
> >  			return false;
> >  		switch (old->refs[i].type) {
> >  		case REF_TYPE_PTR:
> > +			if (!check_ids(old->refs[i].parent_id, cur->refs[i].parent_id, idmap))
> > +				return false;
> > +			break;
> >  		case REF_TYPE_IRQ:
> >  			break;
> >  		case REF_TYPE_LOCK:
> 
> This adds an extra check_ids() call for parent_id. Could this make it
> easier to trigger the WARN_ON_ONCE() in check_ids() via BPF_ID_MAP_SIZE
> exhaustion?

I think the bot has the point here. Let's switch to reallocating idmap
or failing check_ids() if there are to many ids (probably an
unrealistic scenario).

Re: [PATCH bpf-next v1 3/3] selftests/bpf: Use bpf_dynptr_slice() to read file dynptr in leak test

[Eduard Zingerman]

On Thu, 2026-06-04 at 15:09 -0700, Amery Hung wrote:
> use_file_dynptr_slice_after_put_file() reads the dynptr via
> bpf_dynptr_data(), which always returns NULL for a read-only file
> dynptr, making the example confusing. Switch to bpf_dynptr_slice(), the
> correct read API for file dynptrs, and read (rather than write) the slice
> since it is read-only. The test still fails as expected.
> 
> Signed-off-by: Amery Hung <ameryhung@gmail.com>
> ---

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

[...]

Re: [PATCH bpf-next v1 2/3] bpf: Compare parent_id in refsafe() for REF_TYPE_PTR

[Amery Hung]

On Thu, Jun 4, 2026 at 4:21 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Thu, 2026-06-04 at 22:59 +0000, bot+bpf-ci@kernel.org wrote:
> > > commit 89f2be9817e074d02702d22068104fb9fd9aa103
> > > Author: Amery Hung <ameryhung@gmail.com>
> > > Date:   Thu Jun 4 15:09:03 2026 -0700
> > >
> > >     bpf: Compare parent_id in refsafe() for REF_TYPE_PTR
> > >
> > >     refsafe() compared each reference's id and type but not its parent_id,
> > >     so two states whose PTR references differ only in the parent object they
> > >     were derived from could be wrongly treated as equivalent and pruned. Fix
> > >     it by checking parent_id too.
> > >
> > >     Signed-off-by: Amery Hung <ameryhung@gmail.com>
> >
> > This looks like a bug fix for parent_id tracking support. Should this
> > include:
> >
> >   Fixes: 308c7a0ae885 ("bpf: Refactor object relationship tracking and fix dynptr UAF bug")
> >
> > When commit 308c7a0ae885 added parent_id tracking, it updated several
> > safety-checking functions to compare parent_id but missed adding the
> > parent_id check in refsafe() for the REF_TYPE_PTR case. This caused states
> > with different parent objects to be incorrectly treated as equivalent
> > during state pruning.
> >
> > > diff --git a/kernel/bpf/states.c b/kernel/bpf/states.c
> > > index 5945956a7573..06d9ae24f006 100644
> > > --- a/kernel/bpf/states.c
> > > +++ b/kernel/bpf/states.c
> > > @@ -890,6 +890,9 @@ static bool refsafe(struct bpf_verifier_state *old, struct bpf_verifier_state *c
> > >                     return false;
> > >             switch (old->refs[i].type) {
> > >             case REF_TYPE_PTR:
> > > +                   if (!check_ids(old->refs[i].parent_id, cur->refs[i].parent_id, idmap))
> > > +                           return false;
> > > +                   break;
> > >             case REF_TYPE_IRQ:
> > >                     break;
> > >             case REF_TYPE_LOCK:
> >
> > This adds an extra check_ids() call for parent_id. Could this make it
> > easier to trigger the WARN_ON_ONCE() in check_ids() via BPF_ID_MAP_SIZE
> > exhaustion?
>
> I think the bot has the point here. Let's switch to reallocating idmap
> or failing check_ids() if there are to many ids (probably an
> unrealistic scenario).

Make sense. I will switch to failing check_ids() first since it is
unlikely for sane programs.

I will also change bpf_insn_access_aux::ref_id to int as pointed out by sashiko.