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

[netdev-bot+sashiko@kernel.org]

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.

>  };
>  
>  /**
> 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.

^ permalink raw reply

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