Re: [PATCH net] bpf: devmap: provide rxq after redirect

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 9/5/24 17:15, Toke Høiland-Jørgensen wrote:
> Florian Kauer <florian.kauer@xxxxxxxxxxxxx> writes:
> 
>> rxq contains a pointer to the device from where
>> the redirect happened. Currently, the BPF program
>> that was executed after a redirect via BPF_MAP_TYPE_DEVMAP*
>> does not have it set.
>>
>> This is particularly bad since accessing ingress_ifindex, e.g.
>>
>> SEC("xdp")
>> int prog(struct xdp_md *pkt)
>> {
>>         return bpf_redirect_map(&dev_redirect_map, 0, 0);
>> }
>>
>> SEC("xdp/devmap")
>> int prog_after_redirect(struct xdp_md *pkt)
>> {
>>         bpf_printk("ifindex %i", pkt->ingress_ifindex);
>>         return XDP_PASS;
>> }
>>
>> depends on access to rxq, so a NULL pointer gets dereferenced:
>>
>> <1>[  574.475170] BUG: kernel NULL pointer dereference, address: 0000000000000000
>> <1>[  574.475188] #PF: supervisor read access in kernel mode
>> <1>[  574.475194] #PF: error_code(0x0000) - not-present page
>> <6>[  574.475199] PGD 0 P4D 0
>> <4>[  574.475207] Oops: Oops: 0000 [#1] PREEMPT SMP NOPTI
>> <4>[  574.475217] CPU: 4 UID: 0 PID: 217 Comm: kworker/4:1 Not tainted 6.11.0-rc5-reduced-00859-g780801200300 #23
>> <4>[  574.475226] Hardware name: Intel(R) Client Systems NUC13ANHi7/NUC13ANBi7, BIOS ANRPL357.0026.2023.0314.1458 03/14/2023
>> <4>[  574.475231] Workqueue: mld mld_ifc_work
>> <4>[  574.475247] RIP: 0010:bpf_prog_5e13354d9cf5018a_prog_after_redirect+0x17/0x3c
>> <4>[  574.475257] Code: cc cc cc cc cc cc cc 80 00 00 00 cc cc cc cc cc cc cc cc f3 0f 1e fa 0f 1f 44 00 00 66 90 55 48 89 e5 f3 0f 1e fa 48 8b 57 20 <48> 8b 52 00 8b 92 e0 00 00 00 48 bf f8 a6 d5 c4 5d a0 ff ff be 0b
>> <4>[  574.475263] RSP: 0018:ffffa62440280c98 EFLAGS: 00010206
>> <4>[  574.475269] RAX: ffffa62440280cd8 RBX: 0000000000000001 RCX: 0000000000000000
>> <4>[  574.475274] RDX: 0000000000000000 RSI: ffffa62440549048 RDI: ffffa62440280ce0
>> <4>[  574.475278] RBP: ffffa62440280c98 R08: 0000000000000002 R09: 0000000000000001
>> <4>[  574.475281] R10: ffffa05dc8b98000 R11: ffffa05f577fca40 R12: ffffa05dcab24000
>> <4>[  574.475285] R13: ffffa62440280ce0 R14: ffffa62440549048 R15: ffffa62440549000
>> <4>[  574.475289] FS:  0000000000000000(0000) GS:ffffa05f4f700000(0000) knlGS:0000000000000000
>> <4>[  574.475294] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> <4>[  574.475298] CR2: 0000000000000000 CR3: 000000025522e000 CR4: 0000000000f50ef0
>> <4>[  574.475303] PKRU: 55555554
>> <4>[  574.475306] Call Trace:
>> <4>[  574.475313]  <IRQ>
>> <4>[  574.475318]  ? __die+0x23/0x70
>> <4>[  574.475329]  ? page_fault_oops+0x180/0x4c0
>> <4>[  574.475339]  ? skb_pp_cow_data+0x34c/0x490
>> <4>[  574.475346]  ? kmem_cache_free+0x257/0x280
>> <4>[  574.475357]  ? exc_page_fault+0x67/0x150
>> <4>[  574.475368]  ? asm_exc_page_fault+0x26/0x30
>> <4>[  574.475381]  ? bpf_prog_5e13354d9cf5018a_prog_after_redirect+0x17/0x3c
>> <4>[  574.475386]  bq_xmit_all+0x158/0x420
>> <4>[  574.475397]  __dev_flush+0x30/0x90
>> <4>[  574.475407]  veth_poll+0x216/0x250 [veth]
>> <4>[  574.475421]  __napi_poll+0x28/0x1c0
>> <4>[  574.475430]  net_rx_action+0x32d/0x3a0
>> <4>[  574.475441]  handle_softirqs+0xcb/0x2c0
>> <4>[  574.475451]  do_softirq+0x40/0x60
>> <4>[  574.475458]  </IRQ>
>> <4>[  574.475461]  <TASK>
>> <4>[  574.475464]  __local_bh_enable_ip+0x66/0x70
>> <4>[  574.475471]  __dev_queue_xmit+0x268/0xe40
>> <4>[  574.475480]  ? selinux_ip_postroute+0x213/0x420
>> <4>[  574.475491]  ? alloc_skb_with_frags+0x4a/0x1d0
>> <4>[  574.475502]  ip6_finish_output2+0x2be/0x640
>> <4>[  574.475512]  ? nf_hook_slow+0x42/0xf0
>> <4>[  574.475521]  ip6_finish_output+0x194/0x300
>> <4>[  574.475529]  ? __pfx_ip6_finish_output+0x10/0x10
>> <4>[  574.475538]  mld_sendpack+0x17c/0x240
>> <4>[  574.475548]  mld_ifc_work+0x192/0x410
>> <4>[  574.475557]  process_one_work+0x15d/0x380
>> <4>[  574.475566]  worker_thread+0x29d/0x3a0
>> <4>[  574.475573]  ? __pfx_worker_thread+0x10/0x10
>> <4>[  574.475580]  ? __pfx_worker_thread+0x10/0x10
>> <4>[  574.475587]  kthread+0xcd/0x100
>> <4>[  574.475597]  ? __pfx_kthread+0x10/0x10
>> <4>[  574.475606]  ret_from_fork+0x31/0x50
>> <4>[  574.475615]  ? __pfx_kthread+0x10/0x10
>> <4>[  574.475623]  ret_from_fork_asm+0x1a/0x30
>> <4>[  574.475635]  </TASK>
>> <4>[  574.475637] Modules linked in: veth br_netfilter bridge stp llc iwlmvm x86_pkg_temp_thermal iwlwifi efivarfs nvme nvme_core
>> <4>[  574.475662] CR2: 0000000000000000
>> <4>[  574.475668] ---[ end trace 0000000000000000 ]---
> 
> Yikes! I wonder how that has gone unnoticed this long. Could you please
> add a selftest for this so it doesn't happen again?

Yes, I will try to construct one.

> 
>> Therefore, provide it to the program by setting rxq properly.
>>
>> Fixes: fbee97feed9b ("bpf: Add support to attach bpf program to a devmap entry")
> 
> I think the fixes tag is wrong; the original commit was fine because the
> xdp_buff was still the one initialised by the driver. So this should be:
> 
> Fixes: cb261b594b41 ("bpf: Run devmap xdp_prog on flush instead of bulk enqueue")

Oh, I agree. On first glance it looked like the old dev_map_run_prog
was also only setting txq, but didn't notice that it was just overwriting
in the original xdp_buff.

> 
> Other than that, though, the patch LGTM:
> 
> Reviewed-by: Toke Høiland-Jørgensen <toke@xxxxxxxxxx>
> 

Thanks,
Florian




[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux