Re: [PATCH bpf-next 10/15] xdp: warn on XDP_PASS with stored values when XDP runs on an skb

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Critical] veth: race on rq->xdp_rxq in veth_disable_xdp() [1]
--

commit 7b602275436247ecf1d76650c01ef17cdab5904b
Author: Tianyi Gao <tianyi@cloudflare.com>

xdp: warn on XDP_PASS with stored values when XDP runs on an skb

This commit adds xdp_skb_ext_warn_skb() to warn when an XDP program running
on an existing skb returns XDP_PASS after storing skb extension metadata,
and skips generic XDP in xdp_skb_ext_warn_pass() to avoid warning twice.

> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> index b402f8ed50966..f0d279e2da6a4 100644
> --- a/drivers/net/veth.c
> +++ b/drivers/net/veth.c
> @@ -826,6 +826,8 @@ static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq,
>  
>  	switch (act) {
>  	case XDP_PASS:
> +		if (unlikely(xdp_buff_has_skb_ext(xdp)))
> +			xdp_skb_ext_warn_skb(xdp_prog, xdp);
>  		break;
>  	case XDP_TX:
>  		veth_xdp_get(xdp);

[Severity: Critical]
This problem wasn't introduced by this patch, but can veth_disable_xdp()
unregister rq->xdp_rxq [1] while veth_xdp_rcv_skb() or
veth_xdp_rcv_one() is still using it?

drivers/net/veth.c:veth_disable_xdp() {
	for (i = 0; i < dev->real_num_rx_queues; i++)
		rcu_assign_pointer(priv->rq[i].xdp_prog, NULL);

	if (!netif_running(dev) || !veth_gro_requested(dev))
		veth_napi_del(dev);

	veth_disable_xdp_range(dev, 0, dev->real_num_rx_queues, false);
}

When detaching an XDP program on a running veth device with GRO enabled,
veth_disable_xdp() skips veth_napi_del(), so neither napi_disable() nor
synchronize_net() runs after clearing rq->xdp_prog:

CPU0                                CPU1 (NAPI)
veth_disable_xdp()                  veth_xdp_rcv_skb()
                                      rcu_read_lock()
                                      xdp_prog = rcu_dereference(rq->xdp_prog)
  rcu_assign_pointer(rq->xdp_prog, NULL)
  veth_disable_xdp_range()
    rq->xdp_rxq.mem = rq->xdp_mem
    xdp_rxq_info_unreg(&rq->xdp_rxq)
                                      bpf_prog_run_xdp(xdp_prog, xdp)
                                      xdp_skb_ext_warn_skb(xdp_prog, xdp)
                                        xdp->rxq->dev->name <-- NULL deref

xdp_rxq_info_unreg() clears rq->xdp_rxq.dev to NULL and resets
rq->xdp_rxq.mem.type while the NAPI poll is still running with xdp->rxq
pointing to rq->xdp_rxq.  Beyond the NULL pointer dereference on
xdp->rxq->dev (here or when the BPF program reads ingress_ifindex), this
also causes page_pool leaks or use-after-free when frames are redirected,
transmitted, or freed.

[1] https://sashiko.dev/#/bug/linux-717ec225-ebb9-408c-a2e0-a2698846ef4a

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1794281983.git.tianyi@cloudflare.com?part=10