| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAMB2axNnCWp0-ow7Xbg2Go7G61N=Ls_e+DVNq5wBWFbqbFZn-A@mail.gmail.com>
Date: Tue, 6 Jan 2026 09:46:49 -0800
From: Amery Hung <ameryhung@...il.com>
To: Jakub Sitnicki <jakub@...udflare.com>
Cc: Alexei Starovoitov <alexei.starovoitov@...il.com>, Martin KaFai Lau <martin.lau@...ux.dev>,
Martin KaFai Lau <martin.lau@...nel.org>, bpf <bpf@...r.kernel.org>,
Network Development <netdev@...r.kernel.org>, "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>,
Jesper Dangaard Brouer <hawk@...nel.org>, John Fastabend <john.fastabend@...il.com>,
Stanislav Fomichev <sdf@...ichev.me>, Simon Horman <horms@...nel.org>, Andrii Nakryiko <andrii@...nel.org>,
Eduard Zingerman <eddyz87@...il.com>, Song Liu <song@...nel.org>,
Yonghong Song <yonghong.song@...ux.dev>, KP Singh <kpsingh@...nel.org>,
Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>,
kernel-team <kernel-team@...udflare.com>
Subject: Re: [PATCH bpf-next v2 15/16] bpf: Realign skb metadata for TC progs
using data_meta
On Tue, Jan 6, 2026 at 9:36 AM Jakub Sitnicki <jakub@...udflare.com> wrote:
>
> On Mon, Jan 05, 2026 at 06:04 PM -08, Alexei Starovoitov wrote:
> > On Mon, Jan 5, 2026 at 3:19 PM Amery Hung <ameryhung@...il.com> wrote:
> >>
> >> >
> >> > >
> >> > > I guess we can mark such emitted call in insn_aux_data as finalized
> >> > > and get_func_proto() isn't needed.
> >> >
> >> > It is a good idea.
> >> >
> >>
> >> Hmm, insn_aux_data has to be marked in gen_{pro,epi}logue since this
> >> is the only place we know whether the call needs fixup or not. However
> >> insn_aux_data is not available yet in gen_{pro,epi}logue because we
> >> haven't resized insn_aux_data.
> >>
> >> Can we do some hack based on the fact that calls emitted by
> >> BPF_EMIT_CALL() are finalized while calls emitted by BPF_RAW_INSN()
> >> most likely are not?
> >> Let BPF_EMIT_CALL() mark the call insn as finalized temporarily (e.g.,
> >> .off = 1). Then, when do_misc_fixups() encounters it just reset off to
> >> 0 and don't call get_func_proto().
> >
> > marking inside insn via off=1 or whatever is an option,
> > but once we remove BPF_CALL_KFUNC from gen_prologue we can
> > delete add_kfunc_in_insns() altogether and replace it with
> > a similar loop that does
> > if (bpf_helper_call()) mark insn_aux_data.
> >
> > That would be a nice benefit, since add_kfunc_call() from there
> > was always a bit odd, since we're adding kfuncs early before the main
> > verifier pass and after, because of gen_prologue.
>
> Thanks for all the pointers.
>
> I understood we're looking for something like this:
>
> ---8<---
> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index b32ddf0f0ab3..9ccd56c04a45 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -561,6 +561,7 @@ struct bpf_insn_aux_data {
> bool non_sleepable; /* helper/kfunc may be called from non-sleepable context */
> bool is_iter_next; /* bpf_iter_<type>_next() kfunc call */
> bool call_with_percpu_alloc_ptr; /* {this,per}_cpu_ptr() with prog percpu alloc */
> + bool finalized_call; /* call holds function offset relative to __bpf_base_call */
> u8 alu_state; /* used in combination with alu_limit */
> /* true if STX or LDX instruction is a part of a spill/fill
> * pattern for a bpf_fastcall call.
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 1ca5c5e895ee..cc737d103cdd 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -21806,6 +21806,14 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
> env->prog = new_prog;
> delta += cnt - 1;
>
> + /* gen_prologue emits function calls with target address
> + * relative to __bpf_call_base. Skip patch_call_imm fixup.
> + */
> + for (i = 0; i < cnt - 1; i++) {
> + if (bpf_helper_call(&env->prog->insnsi[i]))
> + env->insn_aux_data[i].finalized_call = true;
> + }
> +
> ret = add_kfunc_in_insns(env, insn_buf, cnt - 1);
And then we can get rid of this function as there is no use case for
having a new kfunc in gen_{pro,epi}logue.
> if (ret < 0)
> return ret;
> @@ -23412,6 +23420,9 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
> goto next_insn;
> }
> patch_call_imm:
> + if (env->insn_aux_data[i + delta].finalized_call)
> + goto next_insn;
> +
> fn = env->ops->get_func_proto(insn->imm, env->prog);
> /* all functions that have prototype and verifier allowed
> * programs to call them, must be real in-kernel functions
> @@ -23423,6 +23434,7 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
> return -EFAULT;
> }
> insn->imm = fn->func - __bpf_call_base;
> + env->insn_aux_data[i + delta].finalized_call = true;
> next_insn:
> if (subprogs[cur_subprog + 1].start == i + delta + 1) {
> subprogs[cur_subprog].stack_depth += stack_depth_extra;
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 7f5bc6a505e1..53993c2c492d 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -9082,8 +9082,7 @@ static int bpf_unclone_prologue(struct bpf_insn *insn_buf, u32 pkt_access_flags,
> /* ret = bpf_skb_pull_data(skb, 0); */
> *insn++ = BPF_MOV64_REG(BPF_REG_6, BPF_REG_1);
> *insn++ = BPF_ALU64_REG(BPF_XOR, BPF_REG_2, BPF_REG_2);
> - *insn++ = BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
> - BPF_FUNC_skb_pull_data);
This is why I was suggesting setting off = 1 in BPF_EMIT_CALL to mark
a call as finalized. So that we can continue to support using
BPF_RAW_INSN to emit a helper call in prologue and epilogue.
> + *insn++ = BPF_EMIT_CALL(bpf_skb_pull_data);
> /* if (!ret)
> * goto restore;
> * return TC_ACT_SHOT;
> @@ -9135,11 +9134,8 @@ static int bpf_gen_ld_abs(const struct bpf_insn *orig,
> return insn - insn_buf;
> }
>
> -__bpf_kfunc_start_defs();
> -
> -__bpf_kfunc void bpf_skb_meta_realign(struct __sk_buff *skb_)
> +static void bpf_skb_meta_realign(struct sk_buff *skb)
> {
> - struct sk_buff *skb = (typeof(skb))skb_;
> u8 *meta_end = skb_metadata_end(skb);
> u8 meta_len = skb_metadata_len(skb);
> u8 *meta;
> @@ -9161,14 +9157,6 @@ __bpf_kfunc void bpf_skb_meta_realign(struct __sk_buff *skb_)
> bpf_compute_data_pointers(skb);
> }
>
> -__bpf_kfunc_end_defs();
> -
> -BTF_KFUNCS_START(tc_cls_act_hidden_ids)
> -BTF_ID_FLAGS(func, bpf_skb_meta_realign)
> -BTF_KFUNCS_END(tc_cls_act_hidden_ids)
> -
> -BTF_ID_LIST_SINGLE(bpf_skb_meta_realign_ids, func, bpf_skb_meta_realign)
> -
> static int tc_cls_act_prologue(struct bpf_insn *insn_buf, u32 pkt_access_flags,
> const struct bpf_prog *prog)
> {
> @@ -9182,8 +9170,10 @@ static int tc_cls_act_prologue(struct bpf_insn *insn_buf, u32 pkt_access_flags,
> * r0 = bpf_skb_meta_realign(r1); // r0 is undefined
> * r1 = r6;
> */
> + BUILD_BUG_ON(!__same_type(&bpf_skb_meta_realign,
> + (void (*)(struct sk_buff *skb))NULL));
> *insn++ = BPF_MOV64_REG(BPF_REG_6, BPF_REG_1);
> - *insn++ = BPF_CALL_KFUNC(0, bpf_skb_meta_realign_ids[0]);
> + *insn++ = BPF_EMIT_CALL(bpf_skb_meta_realign);
> *insn++ = BPF_MOV64_REG(BPF_REG_1, BPF_REG_6);
> }
> cnt = bpf_unclone_prologue(insn, pkt_access_flags, prog, TC_ACT_SHOT);
Powered by blists - more mailing lists