From: "Emil Tsalapatis" <emil@etsalapatis.com>
To: "Amery Hung" <ameryhung@gmail.com>,
"Emil Tsalapatis" <emil@etsalapatis.com>
Cc: <bpf@vger.kernel.org>, <ast@kernel.org>, <andrii@kernel.org>,
<eddyz87@gmail.com>, <memxor@gmail.com>, <daniel@iogearbox.net>,
<netdev@vger.kernel.org>,
"Nicholas Carlini" <nicholas@carlini.com>
Subject: Re: [PATCH bpf v2 10/11] bpf: Track skb memory invalidation by packet-backed dynptrs
Date: Tue, 22 Sep 2026 20:29:27 +0000 [thread overview]
Message-ID: <DLM4O2QP87FM.1F2UYDTP6VY7V@etsalapatis.com> (raw)
In-Reply-To: <CAMB2axMWU469XKFGEkvp46R7y+Uw0p4gAK8nT9-o2zdFGu0Vng@mail.gmail.com>
On Tue Sep 22, 2026 at 8:06 PM UTC, Amery Hung wrote:
> On Tue, Sep 22, 2026 at 10:24 AM Emil Tsalapatis <emil@etsalapatis.com> wrote:
>>
>> A dynptr can be backed by skb memory, and kfuncs that
>> write but also read the underlying area may reallocate
>> the backing memory in the process of pulling the skb.
>> However, the verifier does not track these calls as
>> possibly invalidating packet pointers, and does not
>> do so after their call site.
>>
>> Expand the verifier to track dynptr kfuncs for packet
>> invalidation.
>>
>> Fixes: 5fc5d8fded57 ("bpf: Add bpf_dynptr_memset() kfunc")
>> Fixes: a498ee7576de ("bpf: Implement dynptr copy kfuncs")
>> Fixes: daec295a7094 ("bpf/helpers: Introduce bpf_dynptr_copy kfunc")
>> Reported-by: Nicholas Carlini <nicholas@carlini.com>
>> Suggested-by: Nicholas Carlini <nicholas@carlini.com>
>> Signed-off-by: Emil Tsalapatis <emil@etsalapatis.com>
>> ---
>> include/linux/bpf_verifier.h | 2 ++
>> kernel/bpf/cfg.c | 10 +++++--
>> kernel/bpf/verifier.c | 51 ++++++++++++++++++++++++++++++++++--
>> 3 files changed, 59 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
>> index f57730d1d..8a9b7a2f2 100644
>> --- a/include/linux/bpf_verifier.h
>> +++ b/include/linux/bpf_verifier.h
>> @@ -1589,6 +1589,7 @@ struct bpf_call_arg_meta {
>>
>> /* Only set by kfunc */
>> bool r0_rdonly;
>> + bool dynptr_may_clobber_pkt_ptr;
>
> I would suggest making it common to helper and kfunc and not specific to dynptr:
>
> bool pkt_changed;
>
> Then, helper and kfunc and all share this following block:
>
> if (meta->pkt_changed)
> clear_all_pkt_pointers(env);
>
> This also matches the existing changes_pkt_data terminology.
This and the point below both make sense to me, thank you. I will adjust accordingly.
>
>> u32 kfunc_flags;
>> const struct btf_type *func_proto;
>> const char *func_name;
>> @@ -1642,6 +1643,7 @@ static inline bool bpf_is_kfunc_sleepable(struct bpf_call_arg_meta *meta)
>> return meta->kfunc_flags & KF_SLEEPABLE;
>> }
>> bool bpf_is_kfunc_pkt_changing(struct bpf_call_arg_meta *meta);
>> +bool bpf_is_kfunc_maybe_pkt_changing(struct bpf_call_arg_meta *meta);
>> struct bpf_iarray *bpf_iarray_realloc(struct bpf_iarray *old, size_t n_elem);
>> int bpf_copy_insn_array_uniq(struct bpf_map *map, u32 start, u32 end, u32 *off);
>> bool bpf_insn_is_cond_jump(u8 code);
>> diff --git a/kernel/bpf/cfg.c b/kernel/bpf/cfg.c
>> index 842c7d1ea..cb499d19d 100644
>> --- a/kernel/bpf/cfg.c
>> +++ b/kernel/bpf/cfg.c
>> @@ -73,6 +73,12 @@ static void mark_subprog_might_throw(struct bpf_verifier_env *env, int off)
>> subprog->might_throw = true;
>> }
>>
>> +static bool bpf_helper_maybe_changes_pkt_data(enum bpf_func_id func_id)
>> +{
>> + return bpf_helper_changes_pkt_data(func_id) ||
>> + func_id == BPF_FUNC_dynptr_write;
>> +}
>> +
>> /* 't' is an index of a call-site.
>> * 'w' is a callee entry point.
>> * Eventually this function would be called when env->cfg.insn_state[w] == EXPLORED.
>> @@ -510,7 +516,7 @@ 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))
>> + if (bpf_helper_maybe_changes_pkt_data(insn->imm))
>> mark_subprog_changes_pkt_data(env, t);
>> if (insn->imm == BPF_FUNC_tail_call) {
>> ret = visit_abnormal_return_insn(env, t);
>> @@ -543,7 +549,7 @@ static int visit_insn(int t, struct bpf_verifier_env *env)
>> */
>> if (ret == 0 && bpf_is_kfunc_sleepable(&meta))
>> mark_subprog_might_sleep(env, t);
>> - if (ret == 0 && bpf_is_kfunc_pkt_changing(&meta))
>> + if (ret == 0 && bpf_is_kfunc_maybe_pkt_changing(&meta))
>> mark_subprog_changes_pkt_data(env, t);
>> if (ret == 0 && bpf_is_throw_kfunc(insn))
>> mark_subprog_might_throw(env, t);
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index da110cdbc..e916ce89c 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -8278,6 +8278,7 @@ static bool is_kfunc_arg_scalar_with_name(const struct btf *btf,
>> const char *name);
>> 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_kfunc_dynptr_may_clobber_pkt_ptr(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_bpf_obj_drop_kfunc(u32 func_id);
>> static bool is_bpf_percpu_obj_drop_kfunc(u32 func_id);
>> @@ -9320,6 +9321,15 @@ 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;
>> + /*
>> + * These kfuncs only clobber packet pointers when their
>> + * destination dynptr, argument 0, is backed by skb packet data.
>> + */
>> + if (arg == 0 && is_kfunc_dynptr_may_clobber_pkt_ptr(meta) &&
>> + (meta->dynptr.type_unknown ||
>> + meta->dynptr.type == BPF_DYNPTR_TYPE_SKB ||
>> + meta->dynptr.type == BPF_DYNPTR_TYPE_SKB_META))
>> + meta->dynptr_may_clobber_pkt_ptr = true;
>> break;
>> }
>> case ARG_PTR_TO_ITER:
>> @@ -11759,7 +11769,8 @@ 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 ||
>> + if (meta.dynptr.type_unknown ||
>> + dynptr_type == BPF_DYNPTR_TYPE_SKB ||
>> dynptr_type == BPF_DYNPTR_TYPE_SKB_META)
>> /* this will trigger clear_all_pkt_pointers(), which will
>> * invalidate all dynptr slices associated with the skb
>> @@ -12787,9 +12798,45 @@ 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]);
>> }
>>
>> +/*
>> + * Dynptr kfuncs that may clobber packet pointers when called with an skb or
>> + * skb_meta backed destination dynptr by pulling the packet.
>> + */
>> +BTF_SET_START(dynptr_may_clobber_pkt_ptr_kfuncs)
>
> This set identifies kfuncs that write to dynptr-backed memory. Whether
> such a write can invalidate packet pointers is determined separately
> from the destination dynptr type. How about:
>
> BTF_SET_START(dynptr_memory_write_kfuncs)
>
>> +BTF_ID(func, bpf_dynptr_memset)
>> +BTF_ID(func, bpf_dynptr_copy)
>> +#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_may_clobber_pkt_ptr_kfuncs)
>> +
>> +static bool is_kfunc_dynptr_may_clobber_pkt_ptr(struct bpf_call_arg_meta *meta)
>
> Likewise, perhaps:
>
> static bool is_kfunc_dynptr_memory_write(...)
>
> This keeps the two concepts separate: the set classifies the
> operation, while meta.changes_pkt_data records the effect for this
> particular call.
>
>> +{
>> + return meta->btf && btf_id_set_contains(&dynptr_may_clobber_pkt_ptr_kfuncs,
>> + meta->func_id);
>> +}
>> +
>> 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]) ||
>> + meta->dynptr_may_clobber_pkt_ptr;
>> +}
>> +
>> +/*
>> + * More conservative version of the above used in check_cfg(),
>> + * where no register state exists and the dynptr type is unknown.
>> + */
>> +bool bpf_is_kfunc_maybe_pkt_changing(struct bpf_call_arg_meta *meta)
>> +{
>> + return bpf_is_kfunc_pkt_changing(meta) ||
>> + is_kfunc_dynptr_may_clobber_pkt_ptr(meta);
>> }
>>
>> static u32 kfunc_abi_slots(const struct btf_func_model *fm)
>> --
>> 2.54.0
>>
>>
next prev parent reply other threads:[~2026-09-22 20:29 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-09-22 17:20 [PATCH bpf v2 00/11] skb/arena bugfixes Emil Tsalapatis
2026-09-22 17:20 ` [PATCH bpf v2 01/11] bpf: Fix bounds check for skb-backed dynptrs Emil Tsalapatis
2026-09-22 20:15 ` Amery Hung
2026-09-22 17:20 ` [PATCH bpf v2 02/11] selftests/bpf: Test dynptr slices past end of skb Emil Tsalapatis
2026-09-22 20:16 ` Amery Hung
2026-09-22 17:20 ` [PATCH bpf v2 03/11] bpf: Fix bpf_sock context code generation Emil Tsalapatis
2026-09-22 17:20 ` [PATCH bpf v2 04/11] selftests/bpf: Add selftests for rx_queue_mapping context access Emil Tsalapatis
2026-09-22 17:20 ` [PATCH bpf v2 05/11] bpf: Reject pkt arguments in mutating subprogs Emil Tsalapatis
2026-09-22 20:32 ` Amery Hung
2026-09-22 17:20 ` [PATCH bpf v2 06/11] selftests/bpf: Test rejection of pkt args to " Emil Tsalapatis
2026-09-22 17:20 ` [PATCH bpf v2 07/11] bpf: Prevent variable arena/non-arena register contents Emil Tsalapatis
2026-09-22 17:20 ` [PATCH bpf v2 08/11] selftests/bpf: Test for mixed arena/nonarena code paths Emil Tsalapatis
2026-09-22 17:20 ` [PATCH bpf v2 09/11] bpf: Track whether dynptr type is known Emil Tsalapatis
2026-09-22 18:46 ` Alexei Starovoitov
2026-09-22 20:23 ` Amery Hung
2026-09-22 20:28 ` Emil Tsalapatis
2026-09-22 17:20 ` [PATCH bpf v2 10/11] bpf: Track skb memory invalidation by packet-backed dynptrs Emil Tsalapatis
2026-09-22 20:06 ` Amery Hung
2026-09-22 20:29 ` Emil Tsalapatis [this message]
2026-09-22 17:20 ` [PATCH bpf v2 11/11] selftests/bpf: Test dynptr slice invalidation on skb clobber Emil Tsalapatis
2026-09-22 19:40 ` [PATCH bpf v2 00/11] skb/arena bugfixes patchwork-bot+netdevbpf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=DLM4O2QP87FM.1F2UYDTP6VY7V@etsalapatis.com \
--to=emil@etsalapatis.com \
--cc=ameryhung@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=eddyz87@gmail.com \
--cc=memxor@gmail.com \
--cc=netdev@vger.kernel.org \
--cc=nicholas@carlini.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox