On 2025-03-14 17:03:35 [+0100], Toke Høiland-Jørgensen wrote:
> > While at it, is there anything that ensures that only bpf_prog_run_xdp()
> > can invoke the map_redirect callback? Mainline only assigns the task
> > pointer in NAPI callback so any usage outside of bpf_prog_run_xdp() will
> > lead to a segfault and I haven't seen a report yet so…
>
> Yes, the verifier restricts which program types can call the
> map_redirect helper.
Okay. So checks for the BPF_PROG_TYPE_XDP type for the map_redirect and
that is the only one setting it. Okay. Now I remember Alexei mentioning
something…
> > --- a/include/net/xdp.h
> > +++ b/include/net/xdp.h
> > @@ -486,7 +486,12 @@ static __always_inline u32 bpf_prog_run_xdp(const struct bpf_prog *prog,
> > * under local_bh_disable(), which provides the needed RCU protection
> > * for accessing map entries.
> > */
> > - u32 act = __bpf_prog_run(prog, xdp, BPF_DISPATCHER_FUNC(xdp));
> > + struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info);
> > + u32 act;
> > +
>
> Add an if here like
>
> if (ri->map_id | ri->map_type) { /* single | to make it a single branch */
>
> > + ri->map_id = INT_MAX;
> > + ri->map_type = BPF_MAP_TYPE_UNSPEC;
>
> }
>
> Also, ri->map_id should be set to 0, not INT_MAX.
The or variant does
| add %gs:this_cpu_off(%rip), %rax # this_cpu_off, tcp_ptr__
| movl 32(%rax), %edx # _51->map_id, _51->map_id
| orl 36(%rax), %edx # _51->map_type, tmp311
| je .L1546 #,
| movq $0, 32(%rax) #, MEM <vector(2) unsigned int> [(unsigned int *)_51 + 32B]
| .L1546:
while the || does
| add %gs:this_cpu_off(%rip), %rax # this_cpu_off, tcp_ptr__
| cmpq $0, 32(%rax) #, *_51
| je .L1546 #,
| movq $0, 32(%rax) #, MEM <vector(2) unsigned int> [(unsigned int *)_51 + 32B]
| .L1546:
gcc isn't bad at optimizing here ;)
This is the or version as asked for. I don't mind doing any of the both.
I everyone agrees then I would send it to Greg.
--- a/include/net/xdp.h
+++ b/include/net/xdp.h
@@ -486,7 +486,14 @@ static __always_inline u32 bpf_prog_run_xdp(const struct bpf_prog *prog,
* under local_bh_disable(), which provides the needed RCU protection
* for accessing map entries.
*/
- u32 act = __bpf_prog_run(prog, xdp, BPF_DISPATCHER_FUNC(xdp));
+ struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info);
+ u32 act;
+
+ if (ri->map_id | ri->map_type) {
+ ri->map_id = 0;
+ ri->map_type = BPF_MAP_TYPE_UNSPEC;
+ }
+ act = __bpf_prog_run(prog, xdp, BPF_DISPATCHER_FUNC(xdp));
if (static_branch_unlikely(&bpf_master_redirect_enabled_key)) {
if (act == XDP_TX && netif_is_bond_slave(xdp->rxq->dev))
> -Toke
Sebastian
