netdev - Re: [PATCH iwl-next 4/5] ixgbe: convert to ndo_hwtstamp_get() and ndo_hwtstamp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7d330d84-42ab-43aa-94f1-5240b67c49dc@intel.com>
Date: Mon, 30 Jun 2025 11:56:44 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Vladimir Oltean <vladimir.oltean@....com>,
	<intel-wired-lan@...ts.osuosl.org>, Tony Nguyen <anthony.l.nguyen@...el.com>
CC: <netdev@...r.kernel.org>, Tony Nguyen <anthony.l.nguyen@...el.com>,
	Przemek Kitszel <przemyslaw.kitszel@...el.com>, Vinicius Costa Gomes
	<vinicius.gomes@...el.com>, Vadim Fedorenko <vadim.fedorenko@...ux.dev>,
	Richard Cochran <richardcochran@...il.com>
Subject: Re: [PATCH iwl-next 4/5] ixgbe: convert to ndo_hwtstamp_get() and
 ndo_hwtstamp_set()



On 5/13/2025 3:11 AM, Vladimir Oltean wrote:
> New timestamping API was introduced in commit 66f7223039c0 ("net: add
> NDOs for configuring hardware timestamping") from kernel v6.6.
> 
> It is time to convert the Intel ixgbe driver to the new API, so that
> timestamping configuration can be removed from the ndo_eth_ioctl() path
> completely.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---

Ugh. Apologies for the late reply here, but this took for ever to track
down what was wrong in our testing.

The ixgbe patch has a somewhat subtle bug which lead to failed timestamp
configuration and likely other forms of memory corruption.

>  drivers/net/ethernet/intel/ixgbe/ixgbe.h      |  9 ++--
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |  6 +--
>  drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c  | 42 +++++++++----------
>  3 files changed, 29 insertions(+), 28 deletions(-)
> 

>  
>  /**
> - * ixgbe_ptp_get_ts_config - get current hardware timestamping configuration
> - * @adapter: pointer to adapter structure
> - * @ifr: ioctl data
> + * ixgbe_ptp_hwtstamp_get - get current hardware timestamping configuration
> + * @netdev: pointer to net device structure
> + * @config: timestamping configuration structure
>   *
>   * This function returns the current timestamping settings. Rather than
>   * attempt to deconstruct registers to fill in the values, simply keep a copy
>   * of the old settings around, and return a copy when requested.
>   */
> -int ixgbe_ptp_get_ts_config(struct ixgbe_adapter *adapter, struct ifreq *ifr)
> +int ixgbe_ptp_hwtstamp_get(struct net_device *netdev,
> +			   struct kernel_hwtstamp_config *config)
>  {
> -	struct hwtstamp_config *config = &adapter->tstamp_config;
> +	struct ixgbe_adapter *adapter = netdev_priv(netdev);
>  

ixgbe doesn't directly assign the adapter to netdev_priv and this needs
to be ixgbe_from_netdev, since there is a wrapper ixgbe_netdev_priv
structure. I didn't dig into why, but both get and set are wrong here,
and are misinterpreting the ixgbe_netdev_priv structure as
ixgbe_adapter, which is obviously wrong.

See its definition quoted here:
> static inline struct ixgbe_adapter *ixgbe_from_netdev(struct net_device *netdev)
> {
>         struct ixgbe_netdevice_priv *priv = netdev_priv(netdev);
> 
>         return priv->adapter;
> }
> 

Whats odd is that the netdev priv structure is just a wrapper around a
pointer to the adapter:

> struct ixgbe_netdevice_priv {
>         struct ixgbe_adapter *adapter;
> };


> -	return copy_to_user(ifr->ifr_data, config,
> -			    sizeof(*config)) ? -EFAULT : 0;
> +	*config = adapter->tstamp_config;
> +
> +	return 0;
>  }

Because we're completely pointing to the wrong memory, this overwrites
who knows what since the ixgbe_netdev_priv is just the pointer address.

>  
>  /**
> @@ -978,7 +980,7 @@ int ixgbe_ptp_get_ts_config(struct ixgbe_adapter *adapter, struct ifreq *ifr)
>   * mode, if required to support the specifically requested mode.
>   */
>  static int ixgbe_ptp_set_timestamp_mode(struct ixgbe_adapter *adapter,
> -				 struct hwtstamp_config *config)
> +					struct kernel_hwtstamp_config *config)
>  {
>  	struct ixgbe_hw *hw = &adapter->hw;
>  	u32 tsync_tx_ctl = IXGBE_TSYNCTXCTL_ENABLED;
> @@ -1129,31 +1131,29 @@ static int ixgbe_ptp_set_timestamp_mode(struct ixgbe_adapter *adapter,
>  }
>  
>  /**
> - * ixgbe_ptp_set_ts_config - user entry point for timestamp mode
> - * @adapter: pointer to adapter struct
> - * @ifr: ioctl data
> + * ixgbe_ptp_hwtstamp_set - user entry point for timestamp mode
> + * @netdev: pointer to net device structure
> + * @config: timestamping configuration structure
> + * @extack: netlink extended ack structure for error reporting
>   *
>   * Set hardware to requested mode. If unsupported, return an error with no
>   * changes. Otherwise, store the mode for future reference.
>   */
> -int ixgbe_ptp_set_ts_config(struct ixgbe_adapter *adapter, struct ifreq *ifr)
> +int ixgbe_ptp_hwtstamp_set(struct net_device *netdev,
> +			   struct kernel_hwtstamp_config *config,
> +			   struct netlink_ext_ack *extack)
>  {
> -	struct hwtstamp_config config;
> +	struct ixgbe_adapter *adapter = netdev_priv(netdev);
>  	int err;


Same here, the current code would need ixgbe_from_netdev()

I think the right thing to do is submit a patch/fix which drops the
completely useless ixgbe_netdevice_priv structure, but I'll have to dig
into git history to see why it was ever there in the first place.



Download attachment "OpenPGP_signature.asc" of type "application/pgp-signature" (237 bytes)