Re: [PATCH net v2 02/15] ice: use reference counting and SRCU for PTP port access

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 1 (2) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] In the non-READY branch of ice_ptp_release() (state
  ICE_PTP_RESETTING or ICE_PTP_ERROR), the PF cancels its work, calls…
- [Medium] Lost-wakeup hang in ice_ptp_cleanup_pf()
  (drivers/net/ethernet/intel/ice/ice_ptp.c).

Pre-existing issues:
- [Medium] ice_ptp_cleanup_pf() cannot safely run twice, and the non-
  READY branch of ice_ptp_release() can run it twice.
- [Medium] ice_ptp_prepare_rebuild_sec() (E825 clock owner, non-PFR
  reset) calls ice_ptp_prepare_for_reset(peer_pf) while holding only…

--

ice: use reference counting and SRCU for PTP port access

This patch replaces the mutex that protects the ice adapter PTP port list
with an SRCU-protected list and adds a kref to each ice_ptp_port. Readers
skip ports whose kref has already dropped to zero.

ice_ptp_cleanup_pf() unlinks the port, drops the initial reference, waits
for the count to reach zero with wait_var_event(), and then calls
synchronize_srcu().

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index fe21cee4f9dee..94a66e9d8c05d 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c

[ ... ]

> @@ -673,20 +676,33 @@ static void ice_ptp_process_tx_tstamp(struct ice_ptp_tx *tx)
>  	pf->ptp.tx_hwtstamp_good += tstamp_good;
>  }
>  
> +static void ice_ptp_release_port_srcu(struct kref *ref)
> +{
> +	wake_up_var(ref);
> +}

[Severity: Medium]
Is a full memory barrier needed before this wake_up_var()?

The kernel-doc for wake_up_var() in kernel/sched/wait_bit.c says:

  In order for this to function properly there must be a full memory
  barrier after the variable is updated ... If the variable was updated
  by a fully ordered operation such as atomic_dec_and_test() then no
  extra barrier is required. Otherwise smb_mb() is needed.

kref_put() goes through refcount_dec_and_test(). That is
atomic_fetch_sub_release() followed by smp_acquire__after_ctrl_dep() on
success, so it gives release ordering plus acquire on success. It is not
fully ordered. __wake_up_bit() then checks waitqueue_active() without
taking the waitqueue lock.

Can this interleaving happen on a weakly ordered architecture such as
arm64?

CPU1 (last reader, e.g. ice_ptp_tx_tstamp_owner)
kref_put()
  refcount_dec_and_test()      /* refs 1 -> 0, release only */
  ice_ptp_release_port_srcu()
    wake_up_var(ref)
      waitqueue_active() sees an empty queue

CPU2 (ice_ptp_cleanup_pf)
wait_var_event(ref, !kref_read(ref))
  adds itself to the waitqueue, set_current_state()
  kref_read(ref) still returns 1
  schedule()

If so, the remover sleeps in TASK_UNINTERRUPTIBLE with no timeout.
var_wake_function() filters on the key, so an unrelated wakeup on the
same hashed queue would not wake it.

Would adding smp_mb__after_atomic() before wake_up_var(), or using a
fully ordered decrement, avoid this?

>  static void ice_ptp_tx_tstamp_owner(struct ice_pf *pf)
>  {

[ ... ]

> @@ -1424,16 +1449,22 @@ static void ice_ptp_reset_phy_timestamping(struct ice_pf *pf)
>   */
>  static void ice_ptp_restart_all_phy(struct ice_pf *pf)
>  {
> -	struct list_head *entry;
> +	struct ice_port_list *ports = &pf->adapter->ports;
> +	struct ice_ptp_port *port;
> +	int srcu_idx;
>  
> -	list_for_each(entry, &pf->adapter->ports.ports) {
> -		struct ice_ptp_port *port = list_entry(entry,
> -						       struct ice_ptp_port,
> -						       list_node);
> +	srcu_idx = srcu_read_lock(&ports->srcu);
> +	list_for_each_entry_srcu(port, &ports->list, list_node,
> +				 srcu_read_lock_held(&ports->srcu)) {
> +		if (!kref_get_unless_zero(&port->ref))
> +			continue;
>  
>  		if (port->link_up)
>  			ice_ptp_port_phy_restart(port);

[Severity: High]
Can this loop reach a port whose PF has already destroyed its kthread
worker?

When the state is ICE_PTP_RESETTING or ICE_PTP_ERROR, ice_ptp_release()
takes its non-READY branch. That branch tears down the worker and the
clock before the port is removed from the list:

ice_ptp_release() {
    ...
	if (pf->ptp.state != ICE_PTP_READY) {
		if (pf->ptp.kworker) {
			kthread_cancel_delayed_work_sync(&pf->ptp.work);
			...
			kthread_destroy_worker(pf->ptp.kworker);
			pf->ptp.kworker = NULL;
		}
		if (pf->ptp.clock) {
			ptp_clock_unregister(pf->ptp.clock);
			pf->ptp.clock = NULL;
		}
		ice_ptp_cleanup_pf(pf);
    ...
}

ice_ptp_cleanup_pf() is the only place that unlinks the port, so
kref_get_unless_zero() here still succeeds until it runs. port->link_up
can also still be true, because only ice_ptp_link_change() rewrites it,
and only in the READY state.

For ICE_MAC_GENERIC, ice_ptp_port_phy_restart() then does:

	kthread_cancel_delayed_work_sync(&ptp_port->ov_work);
	...
	kthread_queue_delayed_work(pf->ptp.kworker, &ptp_port->ov_work,
				   0);

kthread_destroy_worker() frees the worker but does not clear
ov_work.work.worker. The cancel would therefore take the lock of a freed
worker. The queue call would then pass a NULL pf->ptp.kworker.

The clock owner can hit this through ice_ptp_rebuild_owner(). Because
ptp_clock_unregister() can sleep, the window is wider than it looks.

The same window seems to affect ice_ptp_prepare_rebuild_sec(). It calls
ice_ptp_prepare_for_reset(peer_pf), which runs
kthread_cancel_delayed_work_sync(&ptp->work) on the peer.

The READY branch already calls ice_ptp_cleanup_pf() before it cancels
any work. Should the non-READY branch also call ice_ptp_cleanup_pf()
before destroying the worker?

The commit message says the kref "ensures that a PF will have a valid
lifetime and not be removed until the reference is released". It also
lists ice_ptp_restart_all_phy() as one of the iterations being fixed.

> +
> +		kref_put(&port->ref, ice_ptp_release_port_srcu);
>  	}
> +	srcu_read_unlock(&ports->srcu, srcu_idx);
>  }

[ ... ]

> @@ -2890,14 +2930,18 @@ void ice_ptp_queue_work(struct ice_pf *pf)
>  static void ice_ptp_prepare_rebuild_sec(struct ice_pf *pf, bool rebuild,
>  					enum ice_reset_req reset_type)
>  {

[ ... ]

> +		if (!kref_get_unless_zero(&port->ref))
> +			continue;
> +
>  		if (!ice_is_primary(&peer_pf->hw)) {
>  			if (rebuild) {
>  				/* TODO: When implementing rebuild=true:
> @@ -2909,7 +2953,10 @@ static void ice_ptp_prepare_rebuild_sec(struct ice_pf *pf, bool rebuild,
>  				ice_ptp_prepare_for_reset(peer_pf, reset_type);
>  			}
>  		}

[Severity: Medium]
This is a pre-existing issue, but does anything here serialize the peer's
ptp->state against the peer's own ice_ptp_release()?

ice_ptp_prepare_for_reset() checks and sets the peer's state without a
lock:

	if (ptp->state != ICE_PTP_READY)
		return;

	ptp->state = ICE_PTP_RESETTING;

	/* Disable timestamping for both Tx and Rx */
	ice_ptp_disable_timestamp_mode(pf);

	kthread_cancel_delayed_work_sync(&ptp->work);

At the same time, ice_ptp_release() on the peer reads this field and
writes ICE_PTP_UNINIT to it.

If the owner writes RESETTING after the peer has written UNINIT, the
peer exits release in a non-READY state. A later ice_ptp_release() would
then repeat the cleanup.

In the opposite order, the peer takes the non-READY branch and destroys
its kworker while the owner is about to cancel work on it. That is the
same ordering problem described above for ice_ptp_restart_all_phy().
Calling ice_ptp_cleanup_pf() first in that branch would fix this case
too.

The kref and SRCU here protect the port memory and the list linkage. They
do not protect the PF state machine. The commit message notes that the
change "focuses solely on the port list".

> +
> +		kref_put(&port->ref, ice_ptp_release_port_srcu);
>  	}
> +	srcu_read_unlock(&ports->srcu, srcu_idx);
>  }

[ ... ]

> @@ -3110,13 +3157,23 @@ static int ice_ptp_setup_pf(struct ice_pf *pf)
>  
>  static void ice_ptp_cleanup_pf(struct ice_pf *pf)
>  {

[ ... ]

> +	spin_lock(&ports->lock);
> +	list_del_rcu(&ptp->port.list_node);
> +	spin_unlock(&ports->lock);
> +
> +	ref = &ptp->port.ref;
> +	kref_put(ref, ice_ptp_release_port_srcu);
> +
> +	wait_var_event(ref, !kref_read(ref));

[Severity: Medium]
This isn't a bug introduced by this patch, but what happens if
ice_ptp_cleanup_pf() runs twice for the same PF?

The non-READY branch of ice_ptp_release() calls ice_ptp_cleanup_pf() and
returns without setting ptp.state to ICE_PTP_UNINIT. This sequence seems
to run the cleanup twice:

ice_devlink_reinit_down()->ice_unload()->ice_deinit_features()
  ->ice_ptp_release()
    (state is ICE_PTP_ERROR or ICE_PTP_RESETTING, cleanup #1)

ice_devlink_reinit_up() fails before ice_init_features()
    (ice_ptp_init() never resets the state)

ice_remove()->ice_unload()->...->ice_ptp_release()
    (cleanup #2)

Before this patch, the second list_del() on a poisoned node either
faulted or, with LIST_HARDENED/DEBUG_LIST, warned and was skipped.

With this patch, the second kref_put() runs on a kref that is already
zero. refcount_dec_and_test() warns and saturates the counter without
calling the release function.

kref_read() then never returns zero. wait_var_event() would block
uninterruptibly forever while devl_lock is held. On hardened-list
kernels, the result changes from a warning to a hang.

Should the non-READY branch move the state to ICE_PTP_UNINIT, or should
ice_ptp_cleanup_pf() be made safe to call twice?

> +
> +	synchronize_srcu(&ports->srcu);
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008215614.1987250-1-anthony.l.nguyen%40intel.com