On Fri, 20 Feb 2026 20:04:52 +0000, "Loktionov, Aleksandr" wrote:
> > -----Original Message----- > > From: Kohei Enju <[email protected]> > > Sent: Friday, February 20, 2026 7:40 PM > > To: [email protected]; [email protected] > > Cc: Nguyen, Anthony L <[email protected]>; Kitszel, > > Przemyslaw <[email protected]>; Andrew Lunn > > <[email protected]>; David S. Miller <[email protected]>; Eric > > Dumazet <[email protected]>; Jakub Kicinski <[email protected]>; Paolo > > Abeni <[email protected]>; Loktionov, Aleksandr > > <[email protected]>; Alice Michael > > <[email protected]>; Greenwalt, Paul <[email protected]>; > > Fijalkowski, Maciej <[email protected]>; > > [email protected]; Kohei Enju <[email protected]> > > Subject: [PATCH v1 iwl-net] ice: fix potential NULL pointer deref in > > error path of ice_set_ringparam() > >=20 > > ice_set_ringparam nullifies tstamp_ring of temporary tx_rings, without > > clearing ICE_TX_RING_FLAGS_TXTIME bit. > > When ICE_TX_RING_FLAGS_TXTIME is set and the subsequent > > ice_setup_tx_ring() call fails, a NULL pointer dereference could > > happen in the unwinding sequence: > >=20 > > ice_clean_tx_ring() > > -> ice_is_txtime_cfg() =3D=3D true (ICE_TX_RING_FLAGS_TXTIME is set) > > -> ice_free_tx_tstamp_ring() > > -> ice_free_tstamp_ring() > > -> tstamp_ring->desc (NULL deref) > >=20 > > Clear ICE_TX_RING_FLAGS_TXTIME bit to avoid the potential issue. > >=20 > > Note that this potential issue is found by manual code review. > > Compile test only since unfortunately I don't have E830 devices. > >=20 > > Fixes: ccde82e90946 ("ice: add E830 Earliest TxTime First Offload > > support") > If it's a fix, shouldn't it go to net? This fix relies on a commit 8a4e78094945 ("ice: fix race condition in TX timestamp ring cleanup"), which changed type of flags from u8 to BITMAP, as you advised in [1]. Since the commit only exists in Tony's tree, I chose iwl-net, not net. [1] https://lore.kernel.org/intel-wired-lan/ia3pr11mb8986eb459d2fd1697644cf98e5...@ia3pr11mb8986.namprd11.prod.outlook.com/ > > > Signed-off-by: Kohei Enju <[email protected]> > > --- > > drivers/net/ethernet/intel/ice/ice_ethtool.c | 1 + > > 1 file changed, 1 insertion(+) > >=20 > > diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c > > b/drivers/net/ethernet/intel/ice/ice_ethtool.c > > index 7f769a90dde1..5ed86648d0d6 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c > > +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c > > @@ -3290,6 +3290,7 @@ ice_set_ringparam(struct net_device *netdev, > > struct ethtool_ringparam *ring, > > tx_rings[i].desc =3D NULL; > > tx_rings[i].tx_buf =3D NULL; > > tx_rings[i].tstamp_ring =3D NULL; > > + clear_bit(ICE_TX_RING_FLAGS_TXTIME, tx_rings[i].flags); > > tx_rings[i].tx_tstamps =3D &pf->ptp.port.tx; > > err =3D ice_setup_tx_ring(&tx_rings[i]); > If ice_setup_tx_ring() internally reads ICE_TX_RING_FLAGS_TXTIME to decide = > whether to allocate the tstamp_ring, then clearing the bit first means ice_= > setup_tx_ring() skips TxTime setup even on success - leaving TxTime silentl= > y broken after ice_set_ringparam() completes normally. The crash is fixed o= > n the error path, but I'm afraid a functional regression is introduced on t= > he success path. > Please correct me if I'm wrong. I believe, in the successful path, tx_tstamps and the flag ICE_TX_RING_FLAGS_TXTIME are restored in ice_up(). ice_up() ice_vsi_cfg_lan() ice_vsi_cfg_lan_txqs() ice_vsi_cfg_txqs() ice_vsi_cfg_txq() ice_is_txtime_ena() ( == test_bit(ring->q_index, pf->txtime_txqs)) ice_alloc_setup_tstamp_ring() ice_alloc_tstamp_ring() - allocate tx_tstamps - set_bit(ICE_TX_RING_FLAGS_TXTIME, tx_ring->flags); Enablement of TxTime feature is managed by pf->txtime_txqs, and ring->flags represents only a 'result' of the enablement. Therefore I think it's not a problem, but please let me know if my understanding is wrong. Thank you for reviewing, Alex! > > Thank you > Alex > > > if (err) { > > -- > > 2.51.0
