| 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: <IA1PR11MB621916FD8AB1FE45D1BDCC209296A@IA1PR11MB6219.namprd11.prod.outlook.com>
Date: Wed, 21 Jan 2026 14:20:07 +0000
From: "Nitka, Grzegorz" <grzegorz.nitka@...el.com>
To: Jakub Kicinski <kuba@...nel.org>, "Vecera, Ivan" <ivecera@...hat.com>
CC: "conor+dt@...nel.org" <conor+dt@...nel.org>, "Oros, Petr"
<poros@...hat.com>, "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>,
"linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
"tariqt@...dia.com" <tariqt@...dia.com>, "robh@...nel.org" <robh@...nel.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "Lobakin,
Aleksander" <aleksander.lobakin@...el.com>, "mbloch@...dia.com"
<mbloch@...dia.com>, "jiri@...nulli.us" <jiri@...nulli.us>,
"Prathosh.Satish@...rochip.com" <Prathosh.Satish@...rochip.com>,
"krzk+dt@...nel.org" <krzk+dt@...nel.org>, "saeedm@...dia.com"
<saeedm@...dia.com>, "devicetree@...r.kernel.org"
<devicetree@...r.kernel.org>, "davem@...emloft.net" <davem@...emloft.net>,
"pabeni@...hat.com" <pabeni@...hat.com>, "Kitszel, Przemyslaw"
<przemyslaw.kitszel@...el.com>, "Kubalewski, Arkadiusz"
<arkadiusz.kubalewski@...el.com>, "jonathan.lemon@...il.com"
<jonathan.lemon@...il.com>, "saravanak@...nel.org" <saravanak@...nel.org>,
"Loktionov, Aleksandr" <aleksandr.loktionov@...el.com>, "Schmidt, Michal"
<mschmidt@...hat.com>, "edumazet@...gle.com" <edumazet@...gle.com>,
"leon@...nel.org" <leon@...nel.org>, "vadim.fedorenko@...ux.dev"
<vadim.fedorenko@...ux.dev>, "intel-wired-lan@...ts.osuosl.org"
<intel-wired-lan@...ts.osuosl.org>, "richardcochran@...il.com"
<richardcochran@...il.com>, "andrew+netdev@...n.ch" <andrew+netdev@...n.ch>
Subject: RE: [net-next,v2,12/12] ice: dpll: Support E825-C SyncE and dynamic
pin discovery
> -----Original Message-----
> From: Jakub Kicinski <kuba@...nel.org>
> Sent: Wednesday, January 21, 2026 1:19 AM
> To: Vecera, Ivan <ivecera@...hat.com>
> Cc: Jakub Kicinski <kuba@...nel.org>; conor+dt@...nel.org; Oros, Petr
> <poros@...hat.com>; Nguyen, Anthony L <anthony.l.nguyen@...el.com>;
> linux-rdma@...r.kernel.org; tariqt@...dia.com; robh@...nel.org;
> netdev@...r.kernel.org; linux-kernel@...r.kernel.org; Lobakin, Aleksander
> <aleksander.lobakin@...el.com>; mbloch@...dia.com; jiri@...nulli.us;
> Prathosh.Satish@...rochip.com; krzk+dt@...nel.org; saeedm@...dia.com;
> devicetree@...r.kernel.org; davem@...emloft.net; pabeni@...hat.com;
> Kitszel, Przemyslaw <przemyslaw.kitszel@...el.com>; Kubalewski, Arkadiusz
> <arkadiusz.kubalewski@...el.com>; jonathan.lemon@...il.com;
> saravanak@...nel.org; Loktionov, Aleksandr
> <aleksandr.loktionov@...el.com>; Schmidt, Michal
> <mschmidt@...hat.com>; edumazet@...gle.com; leon@...nel.org;
> vadim.fedorenko@...ux.dev; Nitka, Grzegorz <grzegorz.nitka@...el.com>;
> intel-wired-lan@...ts.osuosl.org; richardcochran@...il.com;
> andrew+netdev@...n.ch
> Subject: Re: [net-next,v2,12/12] ice: dpll: Support E825-C SyncE and dynamic
> pin discovery
>
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
>
> For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-
> local.html
> ---
> ice: dpll: Support E825-C SyncE and dynamic pin discovery
>
> This patch adds DPLL support for the Intel E825-C Ethernet controller.
> It implements dynamic pin discovery via firmware nodes, asynchronous
> registration using a DPLL notifier chain, and hardware configuration
> for CGU registers.
>
> > diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c
> b/drivers/net/ethernet/intel/ice/ice_dpll.c
> > index 4eca62688d83..9d82748810a3 100644
>
> [ ... ]
>
> > +static void ice_dpll_pin_notify_work(struct work_struct *work)
> > +{
> > + struct ice_dpll_pin_work *w = container_of(work,
> > + struct ice_dpll_pin_work,
> > + work);
> > + struct ice_dpll_pin *pin, *parent = w->pin;
> > + struct ice_pf *pf = parent->pf;
> > + int ret;
> > +
> > + switch (w->action) {
> > + case DPLL_PIN_CREATED:
> > + if (!IS_ERR_OR_NULL(parent->pin)) {
> > + /* We have already our pin registered */
> > + goto out;
> > + }
> > +
> > + /* Grab reference on fwnode pin */
> > + parent->pin = fwnode_dpll_pin_find(parent->fwnode,
> > + &parent->tracker);
> > + if (IS_ERR_OR_NULL(parent->pin)) {
> > + dev_err(ice_pf_to_dev(pf),
> > + "Cannot get fwnode pin reference\n");
> > + goto out;
> > + }
> > +
> > + /* Register rclk pin */
> > + pin = &pf->dplls.rclk;
> > + ret = dpll_pin_on_pin_register(parent->pin, pin->pin,
> > + &ice_dpll_rclk_ops, pin);
>
> Can pin->pin (pf->dplls.rclk.pin) be NULL here? Looking at the init
> order in ice_dpll_init_pins_e825():
>
> ret = ice_dpll_init_fwnode_pins(pf, pf->dplls.inputs, 0);
> ...
> ret = ice_dpll_init_rclk_pin(pf, DPLL_PIN_IDX_UNSPEC, ...);
>
> The fwnode pins are initialized first, which registers notifiers. If a
> DPLL_PIN_CREATED notification fires before ice_dpll_init_rclk_pin()
> completes, this work handler will run while pf->dplls.rclk.pin is still
> NULL (the structure is zeroed by kcalloc). Passing NULL to
> dpll_pin_on_pin_register() causes a NULL dereference in
> dpll_xa_ref_pin_add() when it accesses pin->parent_refs.
>
This is a good catch! Yes, it's possible to go with the flow described above.
And this can lead to NULL pointer dereference.
To be fixed in the next iteration.
I believe, NULL checker and/or acquiring dpll mutex in notifier callback is needed
to let the driver complete DPLL initialization.
> > + if (ret) {
> > + dev_err(ice_pf_to_dev(pf),
> > + "Failed to register pin: %pe\n",
> ERR_PTR(ret));
> > + dpll_pin_put(parent->pin, &parent->tracker);
> > + parent->pin = NULL;
> > + goto out;
> > + }
> > + break;
>
> [ ... ]
>
> > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c
> b/drivers/net/ethernet/intel/ice/ice_ptp.c
> > index 4c8d20f2d2c0..0eb8a9e29a52 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> > @@ -1341,6 +1341,35 @@ void ice_ptp_link_change(struct ice_pf *pf, bool
> linkup)
> > if (pf->hw.reset_ongoing)
> > return;
> >
> > + if (hw->mac_type == ICE_MAC_GENERIC_3K_E825) {
> > + int pin, err;
> > +
> > + mutex_lock(&pf->dplls.lock);
>
> Should this check ICE_FLAG_DPLL before using pf->dplls.lock? Looking at
> ice_dpll_init_e825(), if initialization fails the mutex is destroyed:
>
> err = ice_dpll_init_info_e825c(pf);
> if (err)
> goto err_exit;
> ...
> err_exit:
> mutex_destroy(&d->lock);
>
> But ICE_FLAG_DPLL is only set on success. If DPLL init fails and a link
> change event occurs, this code will use a destroyed mutex.
>
This is also a valid comment. Yes, using dpll mutex should be conditioned by
mentioned flag.
> > + for (pin = 0; pin < ICE_SYNCE_CLK_NUM; pin++) {
> > + enum ice_synce_clk clk_pin;
> > + bool active;
> > + u8 port_num;
> > +
> > + port_num = ptp_port->port_num;
> > + clk_pin = (enum ice_synce_clk)pin;
> > + err = ice_tspll_bypass_mux_active_e825c(hw,
> > + port_num,
> > + &active,
> > + clk_pin);
> > + if (WARN_ON_ONCE(err)) {
> > + mutex_unlock(&pf->dplls.lock);
> > + return;
> > + }
> > +
> > + err = ice_tspll_cfg_synce_ethdiv_e825c(hw, clk_pin);
> > + if (active && WARN_ON_ONCE(err)) {
> > + mutex_unlock(&pf->dplls.lock);
> > + return;
> > + }
> > + }
> > + mutex_unlock(&pf->dplls.lock);
> > + }
Powered by blists - more mailing lists