Re: [PATCH net-next 08/15] idpf: refactor idpf to use libie_pci APIs

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Unhandled NULL pointer returned by `libie_pci_get_mmio_addr()` when resolving dynamic MMIO offsets from virtchnl (Control Plane). If the CP provides invalid register offsets, the driver silently assigns NULL to critical pointers (like interrupt control and queue tail pointers). Subsequent hardware interactions on the fast path (e.g., `writel(..., q->tail)` during packet transmit/receive) will dereference the NULL pointer, triggering a kernel oops.
--

--- Patch [8]: [PATCH net-next 08/15] idpf: refactor idpf to use libie_pci APIs ---
commit f697d04a8f177d8259403ad799fd72beb013621d
Author: Pavan Kumar Linga <[email protected]>

idpf: refactor idpf to use libie_pci APIs

This commit refactors the idpf driver to use the libie_pci APIs for initialization and MMIO where possible. It introduces the libie_ctlq_ctx structure and replaces custom MMIO mapping and offset calculation with standard libie equivalents.

> diff --git a/drivers/net/ethernet/intel/idpf/idpf_dev.c b/drivers/net/ethernet/intel/idpf/idpf_dev.c
> --- a/drivers/net/ethernet/intel/idpf/idpf_dev.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_dev.c
[ ... ]
> @@ -110,22 +114,21 @@ static int idpf_intr_reg_init(struct idpf_vport *vport,
>  		intr->dyn_ctl_sw_itridx_ena_m =
>  			PF_GLINT_DYN_CTL_SW_ITR_INDX_ENA_M;
>  
> -		spacing = IDPF_ITR_IDX_SPACING(reg_vals[vec_id].itrn_index_spacing,
> +		spacing = IDPF_ITR_IDX_SPACING(reg->itrn_index_spacing,
>  					       IDPF_PF_ITR_IDX_SPACING);
>  		rx_itr = PF_GLINT_ITR_ADDR(VIRTCHNL2_ITR_IDX_0,
> -					   reg_vals[vec_id].itrn_reg,
> -					   spacing);
> +					   reg->itrn_reg, spacing);
>  		tx_itr = PF_GLINT_ITR_ADDR(VIRTCHNL2_ITR_IDX_1,
> -					   reg_vals[vec_id].itrn_reg,
> -					   spacing);
> -		intr->rx_itr = idpf_get_reg_addr(adapter, rx_itr);
> -		intr->tx_itr = idpf_get_reg_addr(adapter, tx_itr);
> +		intr->rx_itr = libie_pci_get_mmio_addr(mmio, rx_itr);
> +		intr->tx_itr = libie_pci_get_mmio_addr(mmio, tx_itr);

[Severity: High]
This is a pre-existing issue, but what happens in idpf_intr_reg_init() if the
Control Plane provides invalid offsets via virtchnl messages?

The old idpf_get_reg_addr() function contained an explicit BUG() statement if
the register offset couldn't be resolved. The replacement function,
libie_pci_get_mmio_addr(), returns NULL for invalid or unmapped offsets.

Since we don't validate the return value before assigning it to intr->rx_itr
and intr->tx_itr, will this result in a silent unhandled NULL pointer
dereference later when these pointers are accessed?

>  	}
>  
[ ... ]
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
[ ... ]
> @@ -1421,7 +1425,8 @@ static int __idpf_queue_reg_init(struct idpf_vport *vport,
>  
>  			for (j = 0; j < tx_qgrp->num_txq && k < num_regs; j++, k++)
>  				tx_qgrp->txqs[j]->tail =
> -					idpf_get_reg_addr(adapter, reg_vals[k]);
> +					libie_pci_get_mmio_addr(mmio,
> +								reg_vals[k]);

[Severity: High]
This is a pre-existing issue, but could this similarly result in an oops on
the hot path?

In __idpf_queue_reg_init(), if libie_pci_get_mmio_addr() returns NULL for an
invalid queue tail offset, it gets directly stored in the queue structure.
Later, when fast-path packet processing routines attempt to update the tail
pointer (e.g., via writel(..., q->tail) during transmit), they will dereference
NULL.

Should we add a validation check for the returned address before we
commit it to the tx_qgrp->txqs[j]->tail pointer?

>  		}
>  		break;
>  	case VIRTCHNL2_QUEUE_TYPE_RX:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8