Re: [PATCH net v2 11/12] idpf: fix xdp crash in soft reset error path

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

Jakub: I'll drop this patch and apply the rest.
---
idpf: fix xdp crash in soft reset error path

This commit fixes a NULL pointer dereference in idpf_vport_open() when
soft reset fails by restoring vport->xdp_prog to the old value. It also
updates the restart check to use IDPF_VPORT_UP instead of netif_running().

Cc: stable@vger.kernel.org

> diff --git a/drivers/net/ethernet/intel/idpf/xdp.c b/drivers/net/ethernet/intel/idpf/xdp.c
> --- a/drivers/net/ethernet/intel/idpf/xdp.c
> +++ b/drivers/net/ethernet/intel/idpf/xdp.c
> @@ -488,6 +488,7 @@ static int idpf_xdp_setup_prog(struct idpf_vport *vport,
>  				   "Could not reopen the vport after XDP setup");
>  
>  		cfg->user_config.xdp_prog = old;
> +		vport->xdp_prog = old;

Does this also need to roll back vport->num_xdp_txq?

Because idpf_initiate_soft_reset() overwrites the vport struct with the new
queue configuration before failing, vport->num_xdp_txq might remain > 0
while vport->xdp_prog is restored to NULL.

When the interface is brought up, idpf_xdpsqs_get() evaluates
idpf_xdp_enabled(vport) as false and returns early without allocating the
NAPI timers (xdpsq->timer).

Could another interface redirect XDP frames via ndo_xdp_xmit, bypassing the
check because vport->num_xdp_txq > 0, and cause a NULL pointer dereference
in libeth_xdpsq_queue_timer() during idpf_xdp_tx_finalize()?

>  		old = prog;
>  	}

Is this old = prog assignment leading to a double-put of the BPF program?

If idpf_initiate_soft_reset() fails, this error path drops a reference to
the newly provided prog when it calls bpf_prog_put(old) right after this
block.

Since the function returns an error code to the core network stack, won't
dev_xdp_install() in net/core/dev.c also explicitly call bpf_prog_put(prog)
assuming ownership of the error cleanup?

Could this underflow the reference count and cause a use-after-free?

[ ... ]

> diff --git a/drivers/net/ethernet/intel/idpf/xsk.c b/drivers/net/ethernet/intel/idpf/xsk.c
> --- a/drivers/net/ethernet/intel/idpf/xsk.c
> +++ b/drivers/net/ethernet/intel/idpf/xsk.c
> @@ -568,7 +569,8 @@ int idpf_xsk_pool_setup(struct idpf_vport *vport, struct netdev_bpf *bpf)
>  		return -EINVAL;
>  	}
>  
> -	restart = idpf_xdp_enabled(vport) && netif_running(vport->netdev);
> +	restart = idpf_xdp_enabled(vport) &&
> +		  test_bit(IDPF_VPORT_UP, np->state);
>  	if (!restart)
>  		goto pool;

Does this function leave the queue pair permanently disabled if the pool
setup fails?

If restart is true, the queue pair is disabled via:
    idpf_qp_switch(vport, qid, false);

If the subsequent call to libeth_xsk_setup_pool() fails, the function
returns early:

    ret = libeth_xsk_setup_pool(vport->netdev, qid, pool);
    if (ret) {
        ...
        return ret;
    }

Does this early return bypass the idpf_qp_switch(vport, qid, true) call
expected later, causing a permanent partial loss of connectivity until the
interface is manually restarted?