Re: [PATCH v22 net-next 10/12] net/nebula-matrix: dispatch: add mutual exclusion lock for shared hardware resource ops

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
--- Patch [10]: [PATCH v22 net-next 10/12] net/nebula-matrix: dispatch: add mutual exclusion lock for shared hardware resource ops ---
commit 629ed395b64ead7623f563c4a19ea0794f996cf1
Author: illusion wang <illusion.wang@xxxxxxxxxxxxxxxxx>

net/nebula-matrix: dispatch: add mutual exclusion lock for shared hardware resource ops
    
Add ops_mutex_lock to serialize concurrent hardware-modifying dispatch
resource operations, preventing race conditions between PF local calls
and remote mailbox message handlers that manipulate MSI-X mapping and
mailbox IRQ state.

> 4. Fix cleanup ordering: unregister all channel message handlers before
>    hardware resource teardown to avoid lockdep splats and invalid mutex
>    access after destruction.

Does this cleanup ordering actually prevent the invalid mutex access?

Looking at the driver teardown path in nbl_core_remove() and
nbl_chan_remove_common(), it appears the channel workqueue is never cancelled
via teardown_queue(). 

If a remote PF sends mailbox messages while the local PF driver is being
unloaded, won't the uncancelled work item still process it?

> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dispatch.c b/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dispatch.c
> index c82c336f50e5e..175250f952506 100644
> --- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dispatch.c
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dispatch.c

[ ... ]

> @@ -227,14 +228,15 @@ static void nbl_disp_chan_configure_msix_map_resp(void *priv, u16 src_id,
>  	if (!res_ops->configure_msix_map) {
>  		err = NBL_CHAN_RESP_UNIMPLEMENTED;
>  	} else {
> -		ret = NBL_OPS_CALL_RET(res_ops->configure_msix_map,
> -				       (p, src_id,
> -					le16_to_cpu(param.num_net_msix),
> -					le16_to_cpu(param.num_others_msix),
> -					le16_to_cpu(param.msix_mask_en)));
> +		ret = NBL_OPS_CALL_LOCK_RET(disp_mgt,
> +					    res_ops->configure_msix_map, p,
> +					    src_id,
> +					    le16_to_cpu(param.num_net_msix),
> +					    le16_to_cpu(param.num_others_msix),
> +					    le16_to_cpu(param.msix_mask_en));

[Severity: High]
If the inflight work item runs after handler unregistration, will it attempt
to acquire the newly added ops_mutex_lock here via NBL_OPS_CALL_LOCK_RET?

Since this mutex is initialized via devm_mutex_init and destroyed
automatically on device detach, could this result in a use-after-free crash
on the destroyed lock?

[ ... ]

This is a pre-existing issue, but while looking at the MSI-X resource
operations I noticed a potential problem in nbl_res_intr_configure_msix_map()
in drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_interrupt.c:

release_net_bits:
        while (i--) {
                intr_index = tmp_interrupts[i];
                ...
        }
        i = num_net_msix;
release_all_bits:
        while (i--) {

[Severity: High]
Does this error path intentionally fall through to release_all_bits?

When devm_kcalloc or find_first_zero_bit fails during net interrupt
allocation, the code jumps to release_net_bits. Without a goto free_prealloc
before the i = num_net_msix assignment, it falls through into release_all_bits.

Because tmp_interrupts is unpopulated and contains zeros, will this loop
unconditionally clear bit 0 in interrupt_others_bitmap, corrupting the
MSI-X allocator state?