Re: [PATCH 1/2] ice: restore DDP state during PFR recovery

[Simon Horman <horms@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.
Full review at: https://sashiko.dev/#/patchset/20260903074706.602087-2-aaron.ma@canonical.com?part=1
---
Subject: ice: restore DDP state during PFR recovery

This patch serializes rebuilds across PFs of the same adapter while allowing
each PFR to complete independently. It reloads the DDP package from its
cached copy and restores the default DVM recipes before rebuilding the
remaining PF state.

Fixes: 462acf6aca85 ("ice: Enable DDP package download")

[ ... ]

> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index d88835482d3aa..a97a1941cec6a 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -659,7 +659,9 @@ static void ice_do_reset(struct ice_pf *pf, enum ice_reset_req reset_type)
>  	 */
>  	if (reset_type == ICE_RESET_PFR) {
>  		pf->pfr_count++;
> +		mutex_lock(&pf->adapter->rebuild_lock);
>  		ice_rebuild(pf, reset_type);
> +		mutex_unlock(&pf->adapter->rebuild_lock);

[Severity: High]
Can this cause a use-after-free of pf->adapter during concurrent device
removal?

If a device reset triggered in the service task takes longer than the
timeout, ice_remove() proceeds to teardown and calls ice_adapter_put().
For the last PF on the adapter, this drops the refcount to zero and frees
the adapter structure memory.

However, the service task is only explicitly halted by ice_deinit_dev()
after this point:

ice_remove()
    ...
    ice_adapter_put(pdev);
    ice_deinit_hw(&pf->hw);
    ice_deinit_dev(pf); /* service task halted here */

If the service task was executing ice_do_reset(), it will eventually
complete the rebuild and call mutex_unlock(&pf->adapter->rebuild_lock),
dereferencing the freed adapter structure.

>  		clear_bit(ICE_PREPARED_FOR_RESET, pf->state);
>  		clear_bit(ICE_PFR_REQ, pf->state);
>  		wake_up(&pf->reset_wait_queue);
> @@ -704,7 +706,9 @@ static void ice_reset_subtask(struct ice_pf *pf)
>  		} else {
>  			/* done with reset. start rebuild */
>  			pf->hw.reset_ongoing = false;
> +			mutex_lock(&pf->adapter->rebuild_lock);
>  			ice_rebuild(pf, reset_type);
> +			mutex_unlock(&pf->adapter->rebuild_lock);

[Severity: High]
Similarly, does this reset subtask release the rebuild_lock on a potentially
freed adapter structure if the remove timeout expired while the rebuild was
in progress?

>  			/* clear bit to resume normal operations, but
>  			 * ICE_NEEDS_RESTART bit is set in case rebuild failed
>  			 */

[ ... ]