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: <a9d50186-bffa-4b3c-9d97-831269a84fbe@intel.com>
Date: Mon, 30 Jun 2025 13:47:07 -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>, 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 6/30/2025 11:56 AM, Jacob Keller wrote:
> 
> 
> 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.
> 
This is an artifact of the work to refactor ixgbe to support devlink:

Both netdev and devlink want a private structure allocated as a flexible
array member of their parent structure. They cannot both directly be
ice_adapter, so we chose to have devlink be ice_adapter, and netdev gets
the wrapper structure. I suspect the patches you wrote were based on a
tree before this refactor, and/or you just did not spot the refactor
happened.

a0285236ab93 ("ixgbe: add initial devlink support") is where the change
took place, which merged relatively recently.

@Tony, I think this is a pretty trivial fixup on your tree if you want
to handle it instead of forcing Vladimir to make a v2?

its really just switching netdev_priv to ixgbe_from_netdev in these two
functions.


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