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: 24+ 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 17:27 ` sashiko-bot
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 17:28 ` sashiko-bot
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:36 ` sashiko-bot
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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.