Re: [PATCH net-next 06/15] igb: Retrieve Tx timestamp from BH workqueue

[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
---
igb: Retrieve Tx timestamp from BH workqueue

Retrieve Tx timestamp from system BH instead of regular system workqueue.

The current implementation uses schedule_work() which is executed by the
system work queue and kworkers to retrieve Tx timestamps. This increases
latency and can lead to timeouts in case of heavy system load. i210 is
often used in industrial systems, where timestamp timeouts can be fatal.

Simon says:

I think that first issue below, flagged as pre-existing, can be considered
in the context of possible follow-up.

However, the issues flagged below that, regarding scheduling work
and possible softirq storms, do seem to be something that warrants
investigation in the context of this patch-set.

> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 49d46670695f3..17eff90af9e3d 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -6571,7 +6571,7 @@ netdev_tx_t igb_xmit_frame_ring(struct sk_buff *skb,

[Severity: High]
This is a pre-existing issue, but does this function modify the shared SKB
data area without verifying exclusive ownership?

In igb_xmit_frame_ring(), just before the work is queued:

	if (adapter->tstamp_config.tx_type == HWTSTAMP_TX_ON &&
	    !test_and_set_bit_lock(__IGB_PTP_TX_IN_PROGRESS,
				   &adapter->state)) {
		skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;

If the SKB is cloned (e.g. via tc mirred or packet sniffers), won't modifying
the shared shinfo region create a data race with other network paths?

[Severity: High]
This is a pre-existing issue, but can the error path sleep in atomic context?

igb_xmit_frame_ring() runs in atomic context as the ndo_start_xmit callback,
but the cleanup path calls a blocking function:

cleanup_tx_tstamp:
	if (unlikely(tx_flags & IGB_TX_FLAGS_TSTAMP)) {
		...
		if (adapter->hw.mac.type == e1000_82576)
			cancel_work_sync(&adapter->ptp_tx_work);

Could this trigger scheduling while atomic warnings or potential deadlocks?

>  			adapter->ptp_tx_skb = skb_get(skb);
>  			adapter->ptp_tx_start = jiffies;
>  			if (adapter->hw.mac.type == e1000_82576)
> -				schedule_work(&adapter->ptp_tx_work);
> +				queue_work(system_bh_wq, &adapter->ptp_tx_work);

[Severity: High]
Does this change introduce a softirq polling storm for e1000_82576 MACs?

For this specific MAC type, the work is scheduled immediately upon packet
transmission. When queueing to system_bh_wq instead of a regular workqueue,
the polling loop in igb_ptp_tx_work() will now run in softirq context.

>  		} else {
>  			adapter->tx_hwtstamp_skipped++;
>  		}

[ ... ]

> diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
> index bd85d02ecadd8..7b44f9090631a 100644
> --- a/drivers/net/ethernet/intel/igb/igb_ptp.c
> +++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
> @@ -832,7 +832,7 @@ static void igb_ptp_tx_work(struct work_struct *work)
>  		igb_ptp_tx_hwtstamp(adapter);
>  	else
>  		/* reschedule to check later */
> -		schedule_work(&adapter->ptp_tx_work);
> +		queue_work(system_bh_wq, &adapter->ptp_tx_work);

[Severity: High]
If the hardware timestamp is not yet valid, won't this work item
unconditionally reschedule itself to system_bh_wq without delay? 

Because the work requeues itself immediately, this could create a tight
polling loop in softirq context, maximizing the CPU's softirq time limit
and degrading network processing and overall system latency.

[Severity: High]
This is a pre-existing issue, but is there a use-after-free race condition
between igb_ptp_tx_work() and the watchdog task igb_ptp_tx_hang()?

Both functions independently check if the timeout has elapsed using
adapter->ptp_tx_start. If igb_ptp_tx_work() detects a timeout, it frees the
SKB and clears the state bit, allowing a new PTP packet to be queued:

	if (time_is_before_jiffies(adapter->ptp_tx_start +
				   IGB_PTP_TX_TIMEOUT)) {
		dev_kfree_skb_any(adapter->ptp_tx_skb);
		...
		clear_bit_unlock(__IGB_PTP_TX_IN_PROGRESS, &adapter->state);

If igb_ptp_tx_hang() concurrently evaluates the timeout as true based on the
old ptp_tx_start but is preempted, could it resume and mistakenly cancel
the work for the newly queued packet?

igb_ptp_tx_hang() {
	bool timeout = time_is_before_jiffies(adapter->ptp_tx_start +
					      IGB_PTP_TX_TIMEOUT);
	...
	if (timeout) {
		cancel_work_sync(&adapter->ptp_tx_work);
		dev_kfree_skb_any(adapter->ptp_tx_skb);

Would this cause a use-after-free by freeing the new SKB and corrupting the
driver's PTP state?

>  }