Re: [PATCH iwl-next v3 2/2] idpf: add direct Tx timestamp readout

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] idpf: premature idpf_ptp_release() before vport stop in
  idpf_vc_core_deinit()
- [Medium] idpf: stale W1C readiness bits left set after latch timeout
  or flush
- [Medium] idpf: missing IDPF_VPORT_UP check in
  idpf_ptp_tx_tstamp_start() and mode set
--

commit 4103e67711694e72d8e2828d99514ccf51cd2f07
Author: Przemyslaw Korba <przemyslaw.korba@intel.com>

idpf: add direct Tx timestamp readout

This commit adds direct BAR register access for reading Tx timestamps via a
periodic 100 us hrtimer, along with write-1-to-clear readiness bitmap
handling and a 500 ms expiration timeout for unlatched requests.

> diff --git a/drivers/net/ethernet/intel/idpf/idpf_ptp.c b/drivers/net/ethernet/intel/idpf/idpf_ptp.c
> index df2da430ff28f..5d745a53d6865 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_ptp.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_ptp.c

[ ... ]

> @@ -851,19 +925,296 @@ int idpf_ptp_request_ts(struct idpf_tx_queue *tx_q, struct sk_buff *skb,

[ ... ]

> +		/* A latch with no timestamp is kept in use until its deadline
> +		 * passes, as the hardware may still be about to report it.
> +		 * Once it does, drop its readiness bit as well, so that the
> +		 * next user of the index is not given a stale timestamp if
> +		 * the hardware reports one after the latch has been reclaimed.
> +		 */
> +		if (!ptp_tx_tstamp->tstamp) {
> +			if (time_is_after_jiffies(ptp_tx_tstamp->deadline))
> +				continue;
> +
> +			clear_map |= BIT_ULL(idx);
> +		}
> +
> +		list_move(&ptp_tx_tstamp->list_member, &done);
> +	}
> +
> +	/* The indexes are not recycled before the splice below, so the bits
> +	 * of every latch read above can be dropped in a single write.
> +	 */
> +	idpf_ptp_clear_tx_tstamp_ready(tstamp_caps, clear_map);

[Severity: Medium]
Can a packet that latches in hardware after its deadline expires (or after
idpf_ptp_tx_tstamp_stop() flushes latches_in_use) leave its write-1-to-clear
readiness bit set while the latch sits on latches_free?

Because readiness_addr_l and readiness_addr_h are write-1-to-clear, calling
idpf_ptp_clear_tx_tstamp_ready() when the deadline expires (or when
HWTSTAMP_TX_OFF flushes latches_in_use on a live vport) is a no-op for any
in-flight packet whose readiness bit is still 0 in hardware.  If that queued
packet finishes transmission afterwards (for example, after SO_TXTIME pacing
or flow control), the hardware sets both the readiness bit and
IDPF_PTP_TX_TSTAMP_L_VALID_M while the latch is on latches_free, so the next
packet allocated that index will report the previous packet's stale
timestamp.

Clearing stale readiness bits in idpf_ptp_get_tx_tstamp_direct() for bits
not currently in latches_in_use (and in idpf_ptp_tx_tstamp_start()) would
avoid handing out a latch with a pending hardware timestamp.

[ ... ]

> +void idpf_ptp_tx_tstamp_start(struct idpf_vport *vport)
> +{
> +	if (!idpf_ptp_is_vport_tx_tstamp_ena(vport) ||
> +	    vport->tx_tstamp_caps->access != IDPF_PTP_DIRECT)
> +		return;
> +
> +	/* The callback re-arms itself, so it must be finished before the timer
> +	 * is started again, e.g. when only the Rx filter is being changed.
> +	 */
> +	if (READ_ONCE(vport->tstamp_config.tx_type) == HWTSTAMP_TX_ON) {
> +		hrtimer_cancel(&vport->tx_tstamp_timer);
> +		hrtimer_start(&vport->tx_tstamp_timer,
> +			      IDPF_PTP_TX_TSTAMP_PERIOD_NS,
> +			      HRTIMER_MODE_REL_SOFT);
> +	}
> +}

[ ... ]

> @@ -923,12 +1274,21 @@ int idpf_ptp_set_timestamp_mode(struct idpf_vport *vport,
>  	case HWTSTAMP_TX_ON:
>  		if (!idpf_ptp_is_vport_tx_tstamp_ena(vport))
>  			return -EINVAL;
> +
>  		break;
>  	default:
>  		return -EINVAL;
>  	}
>  
> -	vport->tstamp_config.tx_type = config->tx_type;
> +	/* Publish the new mode first, so that the Tx path stops handing out
> +	 * latches, then flush the ones that are still waiting.
> +	 */
> +	WRITE_ONCE(vport->tstamp_config.tx_type, config->tx_type);
> +	if (config->tx_type == HWTSTAMP_TX_ON)
> +		idpf_ptp_tx_tstamp_start(vport);
> +	else
> +		idpf_ptp_tx_tstamp_stop(vport);
> +
>  	idpf_ptp_set_rx_tstamp(vport, config->rx_filter);
>  	*config = vport->tstamp_config;

[Severity: Medium]
Can idpf_hwtstamp_set() -> idpf_ptp_set_timestamp_mode() start
vport->tx_tstamp_timer and dereference a NULL rsrc->rxq_grps when the vport
is down?

idpf_hwtstamp_set() only checks vport->link_up, which
idpf_handle_event_link() sets even while IDPF_VPORT_UP is clear and queues
are not allocated.  Calling idpf_ptp_set_timestamp_mode() in that state arms
tx_tstamp_timer on a stopped vport and then crashes in
idpf_ptp_set_rx_tstamp() when indexing rsrc->rxq_grps.

Also, when a vport is reopened, idpf_vport_open() calls
idpf_ptp_tx_tstamp_start(vport) before idpf_up_complete() sets IDPF_VPORT_UP,
and never calls idpf_ptp_set_rx_tstamp() to restore the PTP flag on the
newly allocated Rx queues.

> @@ -1035,8 +1395,19 @@ static void idpf_ptp_release_vport_tstamp(struct idpf_vport *vport)
>  {
>  	struct idpf_ptp_tx_tstamp *ptp_tx_tstamp, *tmp;
>  	struct list_head *head;
> +	u64 clear_map = 0;
>  
> -	cancel_work_sync(&vport->tstamp_task);
> +	switch (vport->tx_tstamp_caps->access) {
> +	case IDPF_PTP_MAILBOX:
> +		cancel_work_sync(&vport->tstamp_task);
> +		break;
> +	case IDPF_PTP_DIRECT:
> +		hrtimer_cancel(&vport->tx_tstamp_timer);
> +		break;
> +	case IDPF_PTP_NONE:
> +	default:
> +		break;
> +	}
>  
>  	/* Remove list with free latches */
>  	spin_lock_bh(&vport->tx_tstamp_caps->latches_lock);

[ ... ]

> @@ -1061,6 +1436,10 @@ static void idpf_ptp_release_vport_tstamp(struct idpf_vport *vport)
>  	}
>  	u64_stats_update_end(&vport->tstamp_stats.stats_sync);
>  
> +	if (vport->tx_tstamp_caps->access == IDPF_PTP_DIRECT)
> +		idpf_ptp_clear_tx_tstamp_ready(vport->tx_tstamp_caps,
> +					       clear_map);
> +
>  	spin_unlock_bh(&vport->tx_tstamp_caps->latches_lock);
>  
>  	kfree(vport->tx_tstamp_caps);

[Severity: High]
Can idpf_vc_core_deinit() free vport->tx_tstamp_caps while Tx queues and
ndo_hwtstamp_set are still active?

drivers/net/ethernet/intel/idpf/idpf_virtchnl.c:idpf_vc_core_deinit() {
    ...
	idpf_ptp_release(adapter);
	idpf_deinit_task(adapter);
    ...
}

During idpf_remove() and idpf_shutdown(), idpf_ptp_release() runs before
idpf_deinit_task() stops the vports and unregisters the netdevs, and without
holding vport_ctrl_lock:

CPU0                                  CPU1
idpf_vc_core_deinit()
  idpf_ptp_release()
    idpf_ptp_release_vport_tstamp()
      hrtimer_cancel(&tx_tstamp_timer)
                                      idpf_hwtstamp_set()
                                        ...
                                          idpf_ptp_tx_tstamp_start()
                                            hrtimer_start(&tx_tstamp_timer)
      kfree(vport->tx_tstamp_caps)
      vport->tx_tstamp_caps = NULL
                                      <softirq>
                                        idpf_ptp_tx_tstamp_timer_cb()
                                          idpf_ptp_get_tx_tstamp_direct()
                                            spin_lock(&tstamp_caps->...)
                                            <-- NULL pointer dereference

In addition, because vport->tstamp_config.tx_type remains HWTSTAMP_TX_ON and
each queue's tx_q->cached_tstamp_caps still points to the freed structure
until idpf_deinit_task() -> idpf_vport_stop() disables Tx, any packet
transmitted or completed in the meantime dereferences freed
tx_q->cached_tstamp_caps in idpf_ptp_request_ts() or idpf_tx_read_tstamp().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112123503.234508-1-przemyslaw.korba@intel.com?part=2