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

[Jacob Keller <jacob.e.keller@intel.com>]

Here's the full review on the website if it helps:

https://sashiko.dev/#/patchset/20260319224159.23885-1-emil.s.tantilov%40intel.com

Thanks,
Jake

On 4/18/2026 12:00 PM, Jakub Kicinski wrote:
> 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?