Re: [PATCH net-next v3 07/14] idpf: refactor idpf to use libie_pci APIs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing NULL check for `libie_pci_get_mmio_addr()` can lead to kernel panic.
- [High] Teardown sequence in `idpf_remove()` causes Use-After-Free and unmapped MMIO page faults.
- [Medium] Uninitialized `__free(kfree)` variable declaration violates cleanup subsystem guidelines.
--

commit e19227dc4c557d70d94d43565d6b6daafde0cccd
Author: Pavan Kumar Linga <pavan.kumar.linga@intel.com>
Subject: idpf: refactor idpf to use libie_pci APIs

This commit refactors the idpf driver to use libie_pci initialization
and MMIO mapping APIs. It transitions the driver from open-coding BAR
resource mapping to utilizing the shared libie_mmio_info structures.

> diff --git a/drivers/net/ethernet/intel/idpf/idpf_dev.c b/drivers/net/ethernet/intel/idpf/idpf_dev.c
> index 1a0c71c95ef12..e36b0017186fd 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_dev.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_dev.c
[ ... ]
> @@ -93,14 +94,17 @@ static int idpf_intr_reg_init(struct idpf_vport *vport,
>  		goto free_reg_vals;
>  	}
>  
> +	mmio = &adapter->ctlq_ctx.mmio_info;
> +
>  	for (i = 0; i < num_vecs; i++) {
>  		struct idpf_q_vector *q_vector = &rsrc->q_vectors[i];
>  		u16 vec_id = rsrc->q_vector_idxs[i] - IDPF_MBX_Q_VEC;
>  		struct idpf_intr_reg *intr = &q_vector->intr_reg;
> +		struct idpf_vec_regs *reg = &reg_vals[vec_id];
>  		u32 spacing;
>  
> -		intr->dyn_ctl = idpf_get_reg_addr(adapter,
> -						  reg_vals[vec_id].dyn_ctl_reg);
> +		intr->dyn_ctl = libie_pci_get_mmio_addr(mmio,
> +							reg->dyn_ctl_reg);

libie_pci_get_mmio_addr() can return a NULL pointer if the provided offset
is invalid. If the firmware provides an unexpected or invalid offset, could
these pointer fields be set to NULL and cause a panic when accessed in
the interrupt handler or datapath? Is it possible to validate the return
value here before assigning it?

[ ... ]
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> index d4546d62cca9f..3e6411a07e4d0 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
[ ... ]
> @@ -1037,7 +1072,6 @@ static int idpf_send_get_lan_memory_regions(struct idpf_adapter *adapter)
> +static int idpf_cfg_lan_memory_regions(struct idpf_adapter *adapter)
>  {
>  	struct virtchnl2_get_lan_memory_regions *rcvd_regions __free(kfree);
>  	struct idpf_vc_xn_params xn_params = {

This isn't a bug, but the cleanup subsystem guidelines recommend defining and
initializing scope-based cleanup variables in a single statement. If a future
patch introduces an early return before rcvd_regions is assigned, could this
cause the cleanup function to free an uninitialized stack variable?

[ ... ]
> @@ -3647,7 +3649,6 @@ int idpf_vc_core_init(struct idpf_adapter *adapter)
>   */
>  void idpf_vc_core_deinit(struct idpf_adapter *adapter)
>  {
> -	struct idpf_hw *hw = &adapter->hw;
>  	bool remove_in_prog;
>  
>  	if (!test_bit(IDPF_VC_CORE_INIT, adapter->flags))
> @@ -3672,12 +3673,10 @@ void idpf_vc_core_deinit(struct idpf_adapter *adapter)
>  
>  	idpf_vport_params_buf_rel(adapter);
>  
> -	kfree(hw->lan_regs);
> -	hw->lan_regs = NULL;
> -
>  	kfree(adapter->vports);
>  	adapter->vports = NULL;
>  
> +	idpf_decfg_lan_memory_regions(adapter);
>  	clear_bit(IDPF_VC_CORE_INIT, adapter->flags);
>  }

idpf_vc_core_deinit() now calls idpf_decfg_lan_memory_regions(), which
unmaps the MMIO regions. When this is called during the teardown sequence
in idpf_remove(), the netdevs are still registered. 

Could the datapath still execute ndo_start_xmit and attempt to write to
the now-unmapped q->tail, triggering a page fault? Would it be safer to
unregister the netdevs before unmapping the MMIO regions?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515224443.2772147-1-anthony.l.nguyen@intel.com?part=7