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: <20250630205639.ayydzdmh6et2zlyb@skbuf>
Date: Mon, 30 Jun 2025 23:56:39 +0300
From: Vladimir Oltean <vladimir.oltean@....com>
To: Jacob Keller <jacob.e.keller@...el.com>
Cc: intel-wired-lan@...ts.osuosl.org,
	Tony Nguyen <anthony.l.nguyen@...el.com>, 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 Mon, Jun 30, 2025 at 01:47:07PM -0700, Jacob Keller wrote:
> 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.

Ugh :-/ sorry for the trouble, and thanks for doing the hard work of
characterizing this.

Indeed, my first conversion of ixgbe was in August 2023, as this commit
can attest:
https://github.com/vladimiroltean/linux/commit/0351ebf1eee3381ccfba9d31a924d1dd887a316f

At that time, Przemyslaw's commit fd5ef5203ce6 ("ixgbe: wrap
netdev_priv() usage") didn't exist, and "struct ixgbe_adapter *adapter =
netdev_priv(netdev);" was the de facto idiom in the driver, which I then
replicated two more times, in the new ixgbe_ptp_hwtstamp_set() and
ixgbe_ptp_hwtstamp_get() functions. Not only did I not notice that this
change took place, but it also compiled just fine, making me completely
unsuspecting...

Tony, let me know how you would like to proceed.