Re: [PATCH net-next v6 08/11] net: wangxun: delete service_timer before cancel service_work

[Jakub Kicinski <kuba@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.
---
net: wangxun: delete service_timer before cancel service_work

This commit reorders timer cancellation before work cancellation in the
device removal path to prevent new work from being queued after the
device goes down.

> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c b/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c
> index ade2bfe563aaa..5565062c74713 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c
> @@ -48,6 +48,7 @@ void wxvf_remove(struct pci_dev *pdev)
>  	struct wx *wx = pci_get_drvdata(pdev);
>  	struct net_device *netdev;
>  
> +	timer_delete_sync(&wx->service_timer);

Is timer_delete_sync() the right API to use here? Since this is a device
teardown path, using timer_shutdown_sync() would permanently prevent the
timer from being re-armed by concurrent operations.

>  	cancel_work_sync(&wx->service_task);
>  	netdev = wx->netdev;
>  	unregister_netdev(netdev);

Does cancelling the timer and work before unregister_netdev() leave a race
window?

Since the network interface is still registered and hardware interrupts
are not yet disabled, could an interrupt or a concurrent interface up
operation call wx_service_event_schedule() and place wx->service_task
back into the workqueue?

If unregister_netdev() then completes without cancelling this newly
queued work, could the work execute after the wx structure is freed,
resulting in a use-after-free? Moving the sync cancellations after
unregister_netdev() might prevent this.

> diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> index ddc7cea9d7c28..01ceaf270b706 100644
> --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> @@ -936,6 +936,7 @@ static void txgbe_remove(struct pci_dev *pdev)
>  	struct txgbe *txgbe = wx->priv;
>  	struct net_device *netdev;
>  
> +	timer_delete_sync(&wx->service_timer);
>  	cancel_work_sync(&wx->service_task);
>  
>  	netdev = wx->netdev;

The same questions regarding timer_shutdown_sync() and the potential race
window with unregister_netdev() apply to this removal function as well.