| 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: <aAkCtq4pEi2cTKV3@mini-arch>
Date: Wed, 23 Apr 2025 08:09:42 -0700
From: Stanislav Fomichev <stfomichev@...il.com>
To: Lorenzo Bianconi <lorenzo@...nel.org>
Cc: Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
John Fastabend <john.fastabend@...il.com>,
Andrii Nakryiko <andrii@...nel.org>,
Martin KaFai Lau <martin.lau@...ux.dev>,
Eduard Zingerman <eddyz87@...il.com>, Song Liu <song@...nel.org>,
Yonghong Song <yonghong.song@...ux.dev>,
KP Singh <kpsingh@...nel.org>, Stanislav Fomichev <sdf@...ichev.me>,
Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>,
"David S. Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
Jesper Dangaard Brouer <hawk@...nel.org>, bpf@...r.kernel.org,
netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next] bpf: Allow XDP dev bounded program to perform
XDP_REDIRECT into maps
On 04/23, Lorenzo Bianconi wrote:
> > On 04/23, Lorenzo Bianconi wrote:
> > > On Apr 22, Stanislav Fomichev wrote:
> > > > On 04/22, Lorenzo Bianconi wrote:
> > > > > In the current implementation if the program is bounded to a specific
> > > > > device, it will not be possible to perform XDP_REDIRECT into a DEVMAP
> > > > > or CPUMAP even if the program is not attached to the map entry. This
> > > > > seems in contrast with the explanation available in
> > > > > bpf_prog_map_compatible routine. Fix the issue taking into account
> > > > > even the attach program type and allow XDP dev bounded program to
> > > > > perform XDP_REDIRECT into maps if the attach type is not BPF_XDP_DEVMAP
> > > > > or BPF_XDP_CPUMAP.
> > > > >
> > > > > Fixes: 3d76a4d3d4e59 ("bpf: XDP metadata RX kfuncs")
> > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@...nel.org>
> > > > > ---
> > > > > kernel/bpf/core.c | 22 +++++++++++++++++++++-
> > > > > 1 file changed, 21 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> > > > > index ba6b6118cf504041278d05417c4212d57be6fca0..a33175efffc377edbfe281397017eb467bfbcce9 100644
> > > > > --- a/kernel/bpf/core.c
> > > > > +++ b/kernel/bpf/core.c
> > > > > @@ -2358,6 +2358,26 @@ static unsigned int __bpf_prog_ret0_warn(const void *ctx,
> > > > > return 0;
> > > > > }
> > > > >
> > > > > +static bool bpf_prog_dev_bound_map_compatible(struct bpf_map *map,
> > > > > + const struct bpf_prog *prog)
> > > > > +{
> > > > > + if (!bpf_prog_is_dev_bound(prog->aux))
> > > > > + return true;
> > > > > +
> > > > > + if (map->map_type == BPF_MAP_TYPE_PROG_ARRAY)
> > > > > + return false;
> > > >
> > > > [..]
> > > >
> > > > > + if (map->map_type == BPF_MAP_TYPE_DEVMAP &&
> > > > > + prog->expected_attach_type != BPF_XDP_DEVMAP)
> > > > > + return true;
> > > > > +
> > > > > + if (map->map_type == BPF_MAP_TYPE_CPUMAP &&
> > > > > + prog->expected_attach_type != BPF_XDP_CPUMAP)
> > > > > + return true;
> > > >
> > > > Not sure I understand, what does it mean exactly? That it's ok to add
> > > > a dev-bound program to the dev/cpumap if the program itself is gonna
> > > > be attached only to the real device? Can you expand more on the specific
> > > > use-case?
> > > >
> > > > The existing check makes sure that the dev-bound programs run only in the
> > > > contexts that have hw descriptors. devmap and cpumap don't satisfy
> > > > this constraint afaiu.
> > >
> > > My use-case is to use a hw-metadata kfunc like bpf_xdp_metadata_rx_timestamp()
> > > to read hw timestamp from the NIC and then redirect the xdp_buff into a DEVMP
> > > (please note there are no programs attached to any DEVMAP entries):
> > >
> > > extern int bpf_xdp_metadata_rx_timestamp(const struct xdp_md *ctx,
> > > __u64 *timestamp) __ksym;
> > >
> > > struct {
> > > __uint(type, BPF_MAP_TYPE_DEVMAP);
> > > __uint(key_size, sizeof(__u32));
> > > __uint(value_size, sizeof(struct bpf_devmap_val));
> > > __uint(max_entries, 1);
> > > } dev_map SEC(".maps");
> > >
> > > SEC("xdp")
> > > int xdp_meta_redirect(struct xdp_md *ctx)
> > > {
> > > __u64 timestamp;
> > >
> > > ...
> > > bpf_xdp_metadata_rx_timestamp(ctx, ×tamp);
> > > ...
> > >
> > > return bpf_redirect_map(&dev_map, ctx->rx_queue_index, XDP_PASS);
> > > }
> > >
> > > According to my understanding this is feasible just if the "xdp_meta_redirect"
> > > program is bounded to a device otherwise the program is reject with the following
> > > error at load time:
> > >
> > > libbpf: prog 'xdp_meta_redirect': BPF program load failed: -EINVAL
> > > libbpf: prog 'xdp_meta_redirect': -- BEGIN PROG LOAD LOG --
> > > metadata kfuncs require device-bound program
> > > processed 0 insns (limit 1000000) max_states_per_insn 0 total_states 0
> > > peak_states 0 mark_read 0
> > > -- END PROG LOAD LOG --
> > >
> > > in order to fix it:
> > >
> > > ...
> > > index = if_nametoindex(DEV);
> > > bpf_program__set_ifindex(prog, index);
> > > bpf_program__set_flags(prog, BPF_F_XDP_DEV_BOUND_ONLY);
> > > ...
> > >
> > > Doing so the program load still fails for the check in bpf_prog_map_compatible():
> > >
> > > bool bpf_prog_map_compatible()
> > > {
> > > ...
> > > if (bpf_prog_is_dev_bound(aux))
> > > return false;
> > > ...
> >
> > [..]
> >
> > > In other words, a dev-bound XDP program can't interact with a DEVMAP (or
> > > CPUMAP) even if it is not attached to a map entry.
> > > I think if the XDP program is just running in the driver NAPI context
> > > it should be doable to use a hw-metada kfunc and perform a redirect into
> > > a DEVMAP or CPUMAP, right? Am I missing something?
> >
> > Thanks for the info! Yes, that should work. I wonder if you hit
> > bpf_prog_select_runtime->bpf_check_tail_call->bpf_prog_map_compatible
> > path? Looks like we should not do bpf_prog_is_dev_bound in that case (the rest
> > of the bpf_prog_map_compatible callers should).
>
> yes, the issue occurs at the program load time when we run
> bpf_prog_map_compatible() following the call path you pointed out:
>
> bpf_prog_select_runtime() -> bpf_check_tail_call() -> bpf_prog_map_compatible()
>
> Do you mean we should get rid of the bpf_prog_is_dev_bound() check in
> bpf_prog_map_compatible() and move it in the bpf_prog_map_compatible() callers
> instead? In particular:
>
> - __cpu_map_load_bpf_program()
> - __dev_map_alloc_node()
> - prog_fd_array_get_ptr()
Maybe move existing bpf_prog_map_compatible parts (except is_dev_bound)
into some new bpf_prog_map_compatible_type helper?
bpf_prog_map_compatible() {
if (bpf_prog_is_dev_bound)
return false;
return bpf_prog_map_compatible_type(...);
}
And make bpf_check_tail_call call new bpf_prog_map_compatible_type. Not
sure about the naming though (as usual)..
Powered by blists - more mailing lists