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

[Jacob Keller <jacob.e.keller@intel.com>]

On 10/19/2026 1:14 PM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
> 
> Low priority / nit picks for your consideration:
> 
> - [Low] drivers/net/ethernet/intel/ice/ice_adapter.h now embeds `struct
>   srcu_struct srcu` by value in `struct ice_port_list` (line 32).
> 
> Pre-existing issues:
> - [Medium] ice_ptp_prepare_rebuild_sec() (E825C owner reset path) holds
>   only an SRCU read section and a kref on the peer port when it calls…
> 
> --
> 
> ice: use reference counting and SRCU for PTP port access
> 
> This commit replaces the mutex-protected adapter port list with an SRCU
> protected list plus a spinlock for writers. It also adds a kref to each
> ice_ptp_port. Readers take the kref with kref_get_unless_zero(), and
> ice_ptp_cleanup_pf() waits for the last reference to drop before it calls
> synchronize_srcu().
> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.h b/drivers/net/ethernet/intel/ice/ice_adapter.h
>> index 4f695f32da3d8..0b01c7f5cf0da 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_adapter.h
>> +++ b/drivers/net/ethernet/intel/ice/ice_adapter.h
>> @@ -17,15 +17,19 @@ struct ice_pf;
> 
> [ ... ]
> 
>>  struct ice_port_list {
>> -	struct list_head ports;
>> -	/* To synchronize the ports list operations */
>> -	struct mutex lock;
>> +	struct list_head list;
>> +	/* To synchronize write operations on the port list */
>> +	spinlock_t lock;
>> +	struct srcu_struct srcu;
> 
> [Severity: Low]
> Should ice_adapter.h include <linux/srcu.h> itself? struct ice_port_list
> now embeds struct srcu_struct by value, so it needs the complete type. The
> header only pulls in:
> 
> #include <linux/types.h>
> #include <linux/mutex.h>
> #include <linux/spinlock_types.h>
> #include <linux/refcount_types.h>
> 
> #include "ice_type.h"
> 
> The patch adds <linux/srcu.h> to ice_adapter.c and ice_ptp.c instead. The
> header seems to build today only through a transitive chain:
> 
> ice_type.h -> ice_osdep.h -> <net/udp_tunnel.h> -> ... -> <linux/notifier.h>
> 
> and notifier.h is what includes <linux/srcu.h>.
> 
> In the same patch, ice_ptp.h does get <linux/kref.h> for its new struct
> kref member.
> 

If we need a respin I can fix this, otherwise I would prefer to follow
up with a cleanup, as this series has already taken a long time :/

>>  };
>>  
>>  /**
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
>> index e1c66cd0f94ff..55241802d17ae 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> 
> [ ... ]
> 
>> @@ -2890,14 +2934,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)
>>  {
> 
> [ ... ]
> 
>> +	srcu_idx = srcu_read_lock(&ports->srcu);
>> +	list_for_each_entry_srcu(port, &ports->list, list_node,
>> +				 srcu_read_lock_held(&ports->srcu)) {
>>  		struct ice_pf *peer_pf = ptp_port_to_pf(port);
>>  
>> +		if (!kref_get_unless_zero(&port->ref))
>> +			continue;
>> +
>>  		if (!ice_is_primary(&peer_pf->hw)) {
>>  			if (rebuild) {
>>  				/* TODO: When implementing rebuild=true:
>> @@ -2909,7 +2957,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, not one introduced by this patch. Can this
> call to ice_ptp_prepare_for_reset(peer_pf) race with the peer PF's own
> ice_ptp_release()?
> 
> Here the owner holds only an SRCU read section and a kref on the peer
> port. ice_ptp_prepare_for_reset() then reads and writes peer_pf->ptp.state
> with no lock held:
> 
> 	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, the peer can be in the READY path of ice_ptp_release().
> That path does the following before it reaches ice_ptp_cleanup_pf():
> 
> 	pf->ptp.state = ICE_PTP_UNINIT;
> 
> 	/* Disable timestamping for both Tx and Rx */
> 	ice_ptp_disable_timestamp_mode(pf);
> 
> 	ice_ptp_cleanup_pf(pf);
> 
> One possible interleaving:
> 
> owner (E825C reset)                    peer PF (unbind/remove)
> ice_ptp_prepare_rebuild_sec()
>   kref_get_unless_zero() succeeds
>   ice_ptp_prepare_for_reset(peer_pf)
>     sees state == ICE_PTP_READY
>                                        ice_ptp_release()
>                                          state = ICE_PTP_UNINIT
>                                          ice_ptp_disable_timestamp_mode()
>     state = ICE_PTP_RESETTING
>     ice_ptp_disable_timestamp_mode()
> 
> Could the peer then be torn down with ptp.state left at ICE_PTP_RESETTING
> instead of ICE_PTP_UNINIT? Both sides would also run
> ice_ptp_disable_timestamp_mode() with nothing ordering them.
> 
> The new kref wait in ice_ptp_cleanup_pf() keeps the peer's kworker and Tx
> tracker alive until the owner drops its reference, so the earlier
> use-after-free window looks closed.
> 
> Before this patch, the same call ran with no locking at all. The commit
> message also limits its scope to the port list.
> 

This whole peer_pf stuff needs some investigation. I think its partly
being addressed by the work on ctrl_ptp/ctrl_pf being done, but I'm not
sure. Either way I don't believe it makes sense to address it in this
series.

> ^ permalink raw reply
>