| Message ID | 20240703122758.i6lt_jii@linutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-net] tun: Assign missing bpf_net_context. | expand |
On Wed, 3 Jul 2024 14:27:58 +0200 Sebastian Andrzej Siewior wrote: > During the introduction of struct bpf_net_context handling for > XDP-redirect, the tun driver has been missed. > > Set the bpf_net_context before invoking BPF XDP program within the TUN > driver. Sorry if I'm missing the point but I think this is insufficient. You've covered the NAPI-like entry point to the Rx stack in your initial work, but there's also netif_receive_skb() which drivers may call outside of NAPI, simply disabling BH before the call. The only concern in that case is that we end up in do_xdp_generic(), and there's no bpf_net_set_ctx() anywhere on the way. So my intuition would be to add the bpf_net_set_ctx() inside the if(xdp_prog) in do_xdp_generic(). "XDP generic" has less stringent (more TC-like) perf requirements, so setting context once per packet should be fine. And it will also cover drivers like TUN which use both netif_receive_skb() and call do_xdp_generic(), in a single place.
On 2024-07-03 12:01:43 [-0700], Jakub Kicinski wrote: > On Wed, 3 Jul 2024 14:27:58 +0200 Sebastian Andrzej Siewior wrote: > > During the introduction of struct bpf_net_context handling for > > XDP-redirect, the tun driver has been missed. > > > > Set the bpf_net_context before invoking BPF XDP program within the TUN > > driver. > > Sorry if I'm missing the point but I think this is insufficient. > You've covered the NAPI-like entry point to the Rx stack in your > initial work, but there's also netif_receive_skb() which drivers > may call outside of NAPI, simply disabling BH before the call. Ah okay. I've been looking at a few callers and they ended up in NAPI but if you say and I also noticed the one in TUN… > The only concern in that case is that we end up in do_xdp_generic(), > and there's no bpf_net_set_ctx() anywhere on the way. So my intuition > would be to add the bpf_net_set_ctx() inside the if(xdp_prog) in > do_xdp_generic(). "XDP generic" has less stringent (more TC-like) > perf requirements, so setting context once per packet should be fine. > And it will also cover drivers like TUN which use both > netif_receive_skb() and call do_xdp_generic(), in a single place. Yeah, sounds good. I would remove the wrapper in tun_get_user() and then add one to do_xdp_generic(). Sebastian
diff --git a/drivers/net/tun.c b/drivers/net/tun.c index 9254bca2813dc..df4dd6b7479e1 100644 --- a/drivers/net/tun.c +++ b/drivers/net/tun.c @@ -1661,6 +1661,7 @@ static struct sk_buff *tun_build_skb(struct tun_struct *tun, int len, int *skb_xdp) { struct page_frag *alloc_frag = ¤t->task_frag; + struct bpf_net_context __bpf_net_ctx, *bpf_net_ctx; struct bpf_prog *xdp_prog; int buflen = SKB_DATA_ALIGN(sizeof(struct skb_shared_info)); char *buf; @@ -1700,6 +1701,7 @@ static struct sk_buff *tun_build_skb(struct tun_struct *tun, local_bh_disable(); rcu_read_lock(); + bpf_net_ctx = bpf_net_ctx_set(&__bpf_net_ctx); xdp_prog = rcu_dereference(tun->xdp_prog); if (xdp_prog) { struct xdp_buff xdp; @@ -1728,12 +1730,14 @@ static struct sk_buff *tun_build_skb(struct tun_struct *tun, pad = xdp.data - xdp.data_hard_start; len = xdp.data_end - xdp.data; } + bpf_net_ctx_clear(bpf_net_ctx); rcu_read_unlock(); local_bh_enable(); return __tun_build_skb(tfile, alloc_frag, buf, buflen, len, pad); out: + bpf_net_ctx_clear(bpf_net_ctx); rcu_read_unlock(); local_bh_enable(); return NULL; @@ -1914,20 +1918,24 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, skb_record_rx_queue(skb, tfile->queue_index); if (skb_xdp) { + struct bpf_net_context __bpf_net_ctx, *bpf_net_ctx; struct bpf_prog *xdp_prog; int ret; local_bh_disable(); rcu_read_lock(); + bpf_net_ctx = bpf_net_ctx_set(&__bpf_net_ctx); xdp_prog = rcu_dereference(tun->xdp_prog); if (xdp_prog) { ret = do_xdp_generic(xdp_prog, &skb); if (ret != XDP_PASS) { + bpf_net_ctx_clear(bpf_net_ctx); rcu_read_unlock(); local_bh_enable(); goto unlock_frags; } } + bpf_net_ctx_clear(bpf_net_ctx); rcu_read_unlock(); local_bh_enable(); } @@ -2566,6 +2574,7 @@ static int tun_sendmsg(struct socket *sock, struct msghdr *m, size_t total_len) if (m->msg_controllen == sizeof(struct tun_msg_ctl) && ctl && ctl->type == TUN_MSG_PTR) { + struct bpf_net_context __bpf_net_ctx, *bpf_net_ctx; struct tun_page tpage; int n = ctl->num; int flush = 0, queued = 0; @@ -2574,6 +2583,7 @@ static int tun_sendmsg(struct socket *sock, struct msghdr *m, size_t total_len) local_bh_disable(); rcu_read_lock(); + bpf_net_ctx = bpf_net_ctx_set(&__bpf_net_ctx); for (i = 0; i < n; i++) { xdp = &((struct xdp_buff *)ctl->ptr)[i]; @@ -2588,6 +2598,7 @@ static int tun_sendmsg(struct socket *sock, struct msghdr *m, size_t total_len) if (tfile->napi_enabled && queued > 0) napi_schedule(&tfile->napi); + bpf_net_ctx_clear(bpf_net_ctx); rcu_read_unlock(); local_bh_enable();
During the introduction of struct bpf_net_context handling for XDP-redirect, the tun driver has been missed. Set the bpf_net_context before invoking BPF XDP program within the TUN driver. Reported-by: syzbot+0b5c75599f1d872bea6f@syzkaller.appspotmail.com Reported-by: syzbot+5ae46b237278e2369cac@syzkaller.appspotmail.com Reported-by: syzbot+c1e04a422bbc0f0f2921@syzkaller.appspotmail.com Fixes: 401cb7dae8130 ("net: Reference bpf_redirect_info via task_struct on PREEMPT_RT.") Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- drivers/net/tun.c | 11 +++++++++++ 1 file changed, 11 insertions(+)