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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] idpf: leaked latches and race on disable in idpf_ptp_set_timestamp_mode()
- [High] idpf: NULL cmd_sync dereference after overwrite in idpf_ptp_get_caps()
- [Medium] idpf: tx_tstamp_timer not stopped when vport stops in idpf_vport_stop()
--

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

idpf: add direct Tx timestamp readout

This commit adds direct BAR register access support for reading hardware Tx
timestamps in the idpf driver. In direct mode, a periodic 100 us hrtimer
polls the readiness bitmap and latch registers, reports completed timestamps
to the network stack, and expires unlatched requests after 500 ms.

> diff --git a/drivers/net/ethernet/intel/idpf/idpf_ptp.c b/drivers/net/ethernet/intel/idpf/idpf_ptp.c
> index a781b3dbe2d62..adb3f31daa5d9 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_ptp.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_ptp.c

[ ... ]

> @@ -829,35 +888,258 @@ u64 idpf_ptp_extend_ts(struct idpf_vport *vport, u64 in_tstamp)

[ ... ]

> +static enum hrtimer_restart idpf_ptp_tx_tstamp_timer_cb(struct hrtimer *timer)
> +{
> +	struct idpf_vport *vport;
> +
> +	vport = container_of(timer, struct idpf_vport, tx_tstamp_timer);
> +
> +	idpf_ptp_get_tx_tstamp_direct(vport);
> +	hrtimer_forward_now(timer, IDPF_PTP_TX_TSTAMP_PERIOD_NS);
> +
> +	return HRTIMER_RESTART;
> +}
> +
> +/**
> + * idpf_ptp_tx_tstamp_timer_init - Set up the Tx timestamp collection timer
> + * @vport: Virtual port structure
> + */
> +void idpf_ptp_tx_tstamp_timer_init(struct idpf_vport *vport)
> +{
> +	hrtimer_setup(&vport->tx_tstamp_timer, idpf_ptp_tx_tstamp_timer_cb,
> +		      CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
> +}
> +
> +/**
> + * idpf_ptp_tx_tstamp_timer_set - Arm or disarm the Tx timestamp timer
> + * @vport: Virtual port structure
> + * @enable: whether the Tx timestamps are to be collected
> + *
> + * In the direct mode nothing in the Tx path reads the latches, so the
> + * collection is driven by a timer that runs for as long as the Tx
> + * timestamping is enabled.
> + */
> +static void idpf_ptp_tx_tstamp_timer_set(struct idpf_vport *vport, bool enable)
> +{
> +	if (!idpf_ptp_is_vport_tx_tstamp_ena(vport) ||
> +	    vport->tx_tstamp_caps->access != IDPF_PTP_DIRECT)
> +		return;
> +
> +	if (enable)
> +		hrtimer_start(&vport->tx_tstamp_timer,
> +			      IDPF_PTP_TX_TSTAMP_PERIOD_NS,
> +			      HRTIMER_MODE_REL_SOFT);
> +	else
> +		hrtimer_cancel(&vport->tx_tstamp_timer);
> +}

[Severity: Medium]
Does vport->tx_tstamp_timer keep running at 10 kHz if the interface is brought
down while hardware Tx timestamping is enabled?

When HWTSTAMP_TX_ON is active in IDPF_PTP_DIRECT mode and the interface is
stopped via idpf_stop() -> idpf_vport_stop(), the queues are torn down and
vport->link_up is set to false:

drivers/net/ethernet/intel/idpf/idpf_lib.c:idpf_vport_stop() {
    ...
	vport->link_up = false;
	idpf_vport_intr_deinit(vport, rsrc);
	idpf_xdp_rxq_info_deinit_all(rsrc);
	idpf_vport_queues_rel(vport, rsrc);
	idpf_vport_intr_rel(rsrc, &adapter->irq);
	clear_bit(IDPF_VPORT_UP, np->state);
    ...
}

However, idpf_vport_stop() does not cancel vport->tx_tstamp_timer or flush
vport->tx_tstamp_caps->latches_in_use, and idpf_ptp_tx_tstamp_timer_cb()
unconditionally rearms the timer with HRTIMER_RESTART. Because
idpf_hwtstamp_set() rejects changes when !vport->link_up with -EPERM,
userspace cannot disable Tx timestamping while the interface is down either.

In addition, any skbs with SKBTX_IN_PROGRESS that were cleaned from the Tx
ring during idpf_vport_stop() remain on latches_in_use for 500 ms until
idpf_ptp_get_tx_tstamp_direct() expires them, logging false-positive warnings
and counting them as expired rather than flushed.

Should idpf_vport_stop() cancel vport->tx_tstamp_timer and flush
latches_in_use (and idpf_vport_open() restart the timer if
vport->tstamp_config.tx_type == HWTSTAMP_TX_ON)?

> @@ -917,12 +1199,18 @@ 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 before the collection is disarmed, so that the
> +	 * Tx path stops handing out latches that nothing would collect.
> +	 */
> +	WRITE_ONCE(vport->tstamp_config.tx_type, config->tx_type);
> +	idpf_ptp_tx_tstamp_timer_set(vport, config->tx_type == HWTSTAMP_TX_ON);

[Severity: High]
Can disabling Tx timestamping in idpf_ptp_set_timestamp_mode() strand in-use
latches and leak their skb references?

In IDPF_PTP_DIRECT mode, idpf_tx_read_tstamp() is a no-op, so
vport->tx_tstamp_timer running idpf_ptp_get_tx_tstamp_direct() is the only
place that collects or expires entries from tstamp_caps->latches_in_use.

When userspace transitions config->tx_type to HWTSTAMP_TX_OFF via
idpf_hwtstamp_set() -> idpf_ptp_set_timestamp_mode(),
idpf_ptp_tx_tstamp_timer_set(vport, false) immediately cancels
vport->tx_tstamp_timer without draining or flushing entries already queued on
tstamp_caps->latches_in_use.

Furthermore, the Tx datapath checks tstamp_config.tx_type outside of
tstamp_caps->latches_lock:

drivers/net/ethernet/intel/idpf/idpf_txrx.c:idpf_tx_tstamp() {
    ...
	if (READ_ONCE(idpf_netdev_to_vport(tx_q->netdev)->tstamp_config.tx_type) !=
	    HWTSTAMP_TX_ON)
		return -1;
    ...
	err = idpf_ptp_request_ts(tx_q, skb, &idx);
}

A concurrent Tx thread can pass the READ_ONCE() check in idpf_tx_tstamp()
just before WRITE_ONCE() sets HWTSTAMP_TX_OFF, and then call
idpf_ptp_request_ts() after hrtimer_cancel() has finished, moving a latch
onto latches_in_use and taking an skb_get(skb) reference after the timer is
already stopped.

Those stranded entries on latches_in_use hold their skb references (with
SKBTX_IN_PROGRESS set) and latch indexes until teardown or until
HWTSTAMP_TX_ON is re-enabled. If HWTSTAMP_TX_ON is later re-enabled after the
32-bit PHC counter wraps, idpf_ptp_get_tx_tstamp_direct() only checks
ptp_tx_tstamp->deadline when !ptp_tx_tstamp->tstamp, so a latch whose
readiness and valid bits were set before the timer stopped will have its old
32-bit timestamp extended against the current PHC time and reported via
skb_tstamp_tx().

Should idpf_ptp_request_ts() verify tx_type under latches_lock, and should
disabling Tx timestamping flush latches_in_use (or let the timer drain
remaining entries before stopping)?

> diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl_ptp.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl_ptp.c
> index 427b15281ad0d..6443ce34a0d92 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl_ptp.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl_ptp.c
> @@ -48,6 +48,7 @@ int idpf_ptp_get_caps(struct idpf_adapter *adapter)
>  				    VIRTCHNL2_CAP_PTP_SET_DEVICE_CLK_TIME_MB |
>  				    VIRTCHNL2_CAP_PTP_ADJ_DEVICE_CLK |
>  				    VIRTCHNL2_CAP_PTP_ADJ_DEVICE_CLK_MB |
> +				    VIRTCHNL2_CAP_PTP_TX_TSTAMPS |
>  				    VIRTCHNL2_CAP_PTP_TX_TSTAMPS_MB)
>  	};

[Severity: High]
In idpf_ptp_get_caps(), can the cross_tstamp block overwrite a valid
ptp->dev_clk_regs.cmd_sync pointer with NULL while leaving
get_dev_clk_time_access and adj_dev_clk_time_access set to IDPF_PTP_DIRECT?

When ptp->get_dev_clk_time_access is IDPF_PTP_DIRECT, idpf_ptp_get_caps()
maps clock_offsets.cmd_sync_trigger into ptp->dev_clk_regs.cmd_sync and
validates it in clk_regs[]:

drivers/net/ethernet/intel/idpf/idpf_virtchnl_ptp.c:idpf_ptp_get_caps() {
    ...
	temp_offset = le32_to_cpu(clock_offsets.cmd_sync_trigger);
	ptp->dev_clk_regs.cmd_sync =
		libie_pci_get_mmio_addr(mmio, temp_offset);
    ...
	ptp->get_dev_clk_time_access =
		idpf_ptp_regs_access(ptp, clk_regs, ARRAY_SIZE(clk_regs),
				     VIRTCHNL2_CAP_PTP_GET_DEVICE_CLK_TIME_MB);

cross_tstamp:
	access_type = ptp->get_cross_tstamp_access;
	if (access_type != IDPF_PTP_DIRECT)
		goto discipline_clock;

	cross_tstamp_offsets = recv_ptp_caps_msg->cross_time_offsets;
    ...
	temp_offset = le32_to_cpu(cross_tstamp_offsets.cmd_sync_trigger);
	ptp->dev_clk_regs.cmd_sync =
		libie_pci_get_mmio_addr(mmio, temp_offset);
}

If cross_tstamp_offsets.cmd_sync_trigger cannot be mapped,
libie_pci_get_mmio_addr() returns NULL and overwrites
ptp->dev_clk_regs.cmd_sync. Checking cross_regs[] then downgrades
ptp->get_cross_tstamp_access, but ptp->get_dev_clk_time_access,
ptp->adj_dev_clk_time_access (whose adj_regs[] does not include cmd_sync),
and ptp->set_dev_clk_time_access remain IDPF_PTP_DIRECT, causing subsequent
calls to idpf_ptp_tmr_cmd() and idpf_ptp_enable_shtime() to pass a NULL
ptp->dev_clk_regs.cmd_sync pointer to writel().

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