Re: [PATCH net-next v5 14/15] ixd: add the core initialization

[Larysa Zaremba <larysa.zaremba@intel.com>]

Sashiko says:
> --- Patch [14]: [PATCH net-next v5 14/15] ixd: add the core initialization ---
> commit 43763d63c75ee5bcdd9189c20cf8b272997bdfec
> Author: Larysa Zaremba <larysa.zaremba@intel.com>
> 
> ixd: add the core initialization
> 
> This patch initializes the core when the mailbox is set up. The driver
> confirms the virtchnl version, requests necessary capabilities, and sets up
> the message handling framework.
> 
> > diff --git a/drivers/net/ethernet/intel/ixd/ixd_ctlq.c b/drivers/net/ethernet/intel/ixd/ixd_ctlq.c
> > index 0000000000000..6d658be0695d4
> > --- /dev/null
> > +++ b/drivers/net/ethernet/intel/ixd/ixd_ctlq.c
> 
> [ ... ]
> 
> > @@ -0,0 +1,146 @@
> 
> [ ... ]
> 
> > +void ixd_ctlq_rx_task(struct work_struct *work)
> > +{
> > +	struct ixd_adapter *adapter;
> > +
> > +	adapter = container_of(work, struct ixd_adapter, mbx_task.work);
> > +
> > +	queue_delayed_work(system_unbound_wq, &adapter->mbx_task,
> > +			   msecs_to_jiffies(300));
> > +
> > +	ixd_ctlq_recv_mb_msg(adapter);
> > +}
> 
> Does queueing the delayed work before executing the main logic create a race
> condition? If cancel_delayed_work_sync() is called (for example, during
> teardown in ixd_deinit_dflt_mbx()), try_to_grab_pending() could successfully
> delete the newly queued timer and return immediately, assuming the work is
> fully cancelled.
> 
> This would bypass flush_work() and allow the teardown path to free
> adapter->xnm and set adapter->arq to NULL while ixd_ctlq_recv_mb_msg() is
> still running, potentially causing a use-after-free or NULL pointer
> dereference. Should the re-arming be moved to the end of the task?
> 

I assume the process of cancelling delayed work is more robust.

> > diff --git a/drivers/net/ethernet/intel/ixd/ixd_lib.c b/drivers/net/ethernet/intel/ixd/ixd_lib.c
> > index afc413d3650fb..ce870aba143d4 100644
> > --- a/drivers/net/ethernet/intel/ixd/ixd_lib.c
> > +++ b/drivers/net/ethernet/intel/ixd/ixd_lib.c
> 
> [ ... ]
> 
> > @@ -136,8 +145,27 @@ void ixd_init_task(struct work_struct *work)
> 
> [ ... ]
> 
> > +	libie_ctlq_xn_shutdown(adapter->xnm);/
> > +	ixd_trigger_reset(adapter);
> > +	ixd_deinit_dflt_mbx(adapter);
> 
> Is it safe to trigger a hardware reset before cancelling the mailbox polling
> task? ixd_deinit_dflt_mbx() is responsible for cancelling the mbx_task, but
> calling ixd_trigger_reset() first means the polling task could run while the
> hardware is resetting.
> 
> During reset, MMIO reads or DMA ring memory might temporarily contain invalid
> states, and the still-running mbx_task might process corrupted descriptors.
> Should the polling task be cancelled before asserting the hardware reset here,
> as well as in ixd_remove() in ixd_main.c?
> 

Yes, this is safe, we just won't receive anything at this point.