| 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
| ||
|
Message-ID: <BL3PR11MB643584D780C953B0060E29E6C646A@BL3PR11MB6435.namprd11.prod.outlook.com>
Date: Mon, 30 Jun 2025 20:58:57 +0000
From: "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>
To: Vladimir Oltean <vladimir.oltean@....com>, "Keller, Jacob E"
<jacob.e.keller@...el.com>
CC: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>, "Kitszel, Przemyslaw"
<przemyslaw.kitszel@...el.com>, "Gomes, Vinicius" <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()
> -----Original Message-----
> From: Vladimir Oltean <vladimir.oltean@....com>
> Sent: Monday, June 30, 2025 1:57 PM
> To: Keller, Jacob E <jacob.e.keller@...el.com>
> Cc: intel-wired-lan@...ts.osuosl.org; Nguyen, Anthony L
> <anthony.l.nguyen@...el.com>; netdev@...r.kernel.org; Kitszel, Przemyslaw
> <przemyslaw.kitszel@...el.com>; Gomes, Vinicius
> <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/0351ebf1eee3381ccfba9d3
> 1a924d1dd887a316f
>
> 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.
I think it's easiest for me to make the change.
Thanks,
Tony
Powered by blists - more mailing lists