| Message ID | 20240625170248.199162-3-anthony.l.nguyen@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Intel Wired LAN Driver Updates 2024-06-25 (ice) | expand |
On 6/25/2024 10:02 AM, Tony Nguyen wrote: > From: Jacob Keller <jacob.e.keller@intel.com> > > The ice_ptp_extts_event() function can race with ice_ptp_release() and > result in a NULL pointer dereference which leads to a kernel panic. > > Panic occurs because the ice_ptp_extts_event() function calls > ptp_clock_event() with a NULL pointer. The ice driver has already > released the PTP clock by the time the interrupt for the next external > timestamp event occurs. > > To fix this, modify the ice_ptp_extts_event() function to check the > PTP state and bail early if PTP is not ready. > > Fixes: 172db5f91d5f ("ice: add support for auxiliary input/output pins") > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> > Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com> > Reviewed-by: Simon Horman <horms@kernel.org> > Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel) > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com> > --- > drivers/net/ethernet/intel/ice/ice_ptp.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c > index d8ff9f26010c..0500ced1adf8 100644 > --- a/drivers/net/ethernet/intel/ice/ice_ptp.c > +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c > @@ -1559,6 +1559,10 @@ void ice_ptp_extts_event(struct ice_pf *pf) > u8 chan, tmr_idx; > u32 hi, lo; > > + /* Don't process timestamp events if PTP is not ready */ > + if (pf->ptp.state != ICE_PTP_READY) > + return; > + If this is a potential race problem, is there some sort of locking that assures this stays true while running through your for-loop below here? sln > tmr_idx = hw->func_caps.ts_func_info.tmr_index_owned; > /* Event time is captured by one of the two matched registers > * GLTSYN_EVNT_L: 32 LSB of sampled time event > -- > 2.41.0 > >
On 6/25/2024 7:57 PM, Nelson Shannon wrote: > > From: Jacob Keller <jacob.e.keller@intel.com> > > > > The ice_ptp_extts_event() function can race with ice_ptp_release() and > > result in a NULL pointer dereference which leads to a kernel panic. > > > > Panic occurs because the ice_ptp_extts_event() function calls > > ptp_clock_event() with a NULL pointer. The ice driver has already > > released the PTP clock by the time the interrupt for the next external > > timestamp event occurs. > > > > To fix this, modify the ice_ptp_extts_event() function to check the > > PTP state and bail early if PTP is not ready. > > > > Fixes: 172db5f91d5f ("ice: add support for auxiliary input/output pins") > > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> > > Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com> > > Reviewed-by: Simon Horman <horms@kernel.org> > > Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel) > > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com> > > --- > > drivers/net/ethernet/intel/ice/ice_ptp.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c > > index d8ff9f26010c..0500ced1adf8 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_ptp.c > > +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c > > @@ -1559,6 +1559,10 @@ void ice_ptp_extts_event(struct ice_pf *pf) > > u8 chan, tmr_idx; > > u32 hi, lo; > > > > + /* Don't process timestamp events if PTP is not ready */ > > + if (pf->ptp.state != ICE_PTP_READY) > > + return; > > + > > If this is a potential race problem, is there some sort of locking that > assures this stays true while running through your for-loop below here? > > sln Currently, we have no locking around PTP state. The code above happens only in the top half of the interrupt and race can happen when ice_ptp_release() is called and the driver starts to release PTP structures, but hasn't stopped EXTTS yet. Thanks, Karol
> -----Original Message----- > From: Nelson, Shannon <shannon.nelson@amd.com> > Sent: Tuesday, June 25, 2024 10:58 AM > To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net; > kuba@kernel.org; pabeni@redhat.com; edumazet@google.com; > netdev@vger.kernel.org > Cc: Keller, Jacob E <jacob.e.keller@intel.com>; richardcochran@gmail.com; Kitszel, > Przemyslaw <przemyslaw.kitszel@intel.com>; Kolacinski, Karol > <karol.kolacinski@intel.com>; Simon Horman <horms@kernel.org>; Pucha, > HimasekharX Reddy <himasekharx.reddy.pucha@intel.com> > Subject: Re: [PATCH net 2/4] ice: Don't process extts if PTP is disabled > > On 6/25/2024 10:02 AM, Tony Nguyen wrote: > > From: Jacob Keller <jacob.e.keller@intel.com> > > > > The ice_ptp_extts_event() function can race with ice_ptp_release() and > > result in a NULL pointer dereference which leads to a kernel panic. > > > > Panic occurs because the ice_ptp_extts_event() function calls > > ptp_clock_event() with a NULL pointer. The ice driver has already > > released the PTP clock by the time the interrupt for the next external > > timestamp event occurs. > > > > To fix this, modify the ice_ptp_extts_event() function to check the > > PTP state and bail early if PTP is not ready. > > > > Fixes: 172db5f91d5f ("ice: add support for auxiliary input/output pins") > > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> > > Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com> > > Reviewed-by: Simon Horman <horms@kernel.org> > > Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A > Contingent worker at Intel) > > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com> > > --- > > drivers/net/ethernet/intel/ice/ice_ptp.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c > b/drivers/net/ethernet/intel/ice/ice_ptp.c > > index d8ff9f26010c..0500ced1adf8 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_ptp.c > > +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c > > @@ -1559,6 +1559,10 @@ void ice_ptp_extts_event(struct ice_pf *pf) > > u8 chan, tmr_idx; > > u32 hi, lo; > > > > + /* Don't process timestamp events if PTP is not ready */ > > + if (pf->ptp.state != ICE_PTP_READY) > > + return; > > + > > If this is a potential race problem, is there some sort of locking that > assures this stays true while running through your for-loop below here? > > sln > This function is called by the IRQ when it gets an extts event. During shutdown, we set the state to say its shutdown and then call synchronize_irq on the miscellaneous vector which will wait until the misc IRQ handler completes. From that point the state will always be not ready and the function will have completed execution. > > tmr_idx = hw->func_caps.ts_func_info.tmr_index_owned; > > /* Event time is captured by one of the two matched registers > > * GLTSYN_EVNT_L: 32 LSB of sampled time event > > -- > > 2.41.0 > > > >
> -----Original Message----- > From: Kolacinski, Karol <karol.kolacinski@intel.com> > Sent: Thursday, June 27, 2024 3:43 AM > To: Nelson, Shannon <shannon.nelson@amd.com>; Nguyen, Anthony L > <anthony.l.nguyen@intel.com>; davem@davemloft.net; kuba@kernel.org; > pabeni@redhat.com; edumazet@google.com; netdev@vger.kernel.org > Cc: Keller, Jacob E <jacob.e.keller@intel.com>; richardcochran@gmail.com; Kitszel, > Przemyslaw <przemyslaw.kitszel@intel.com>; Simon Horman > <horms@kernel.org>; Pucha, HimasekharX Reddy > <himasekharx.reddy.pucha@intel.com> > Subject: Re: [PATCH net 2/4] ice: Don't process extts if PTP is disabled > > On 6/25/2024 7:57 PM, Nelson Shannon wrote: > > > From: Jacob Keller <jacob.e.keller@intel.com> > > > > > > The ice_ptp_extts_event() function can race with ice_ptp_release() and > > > result in a NULL pointer dereference which leads to a kernel panic. > > > > > > Panic occurs because the ice_ptp_extts_event() function calls > > > ptp_clock_event() with a NULL pointer. The ice driver has already > > > released the PTP clock by the time the interrupt for the next external > > > timestamp event occurs. > > > > > > To fix this, modify the ice_ptp_extts_event() function to check the > > > PTP state and bail early if PTP is not ready. > > > > > > Fixes: 172db5f91d5f ("ice: add support for auxiliary input/output pins") > > > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > > > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> > > > Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com> > > > Reviewed-by: Simon Horman <horms@kernel.org> > > > Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> > (A Contingent worker at Intel) > > > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com> > > > --- > > > drivers/net/ethernet/intel/ice/ice_ptp.c | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c > b/drivers/net/ethernet/intel/ice/ice_ptp.c > > > index d8ff9f26010c..0500ced1adf8 100644 > > > --- a/drivers/net/ethernet/intel/ice/ice_ptp.c > > > +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c > > > @@ -1559,6 +1559,10 @@ void ice_ptp_extts_event(struct ice_pf *pf) > > > u8 chan, tmr_idx; > > > u32 hi, lo; > > > > > > + /* Don't process timestamp events if PTP is not ready */ > > > + if (pf->ptp.state != ICE_PTP_READY) > > > + return; > > > + > > > > If this is a potential race problem, is there some sort of locking that > > assures this stays true while running through your for-loop below here? > > > > sln > > Currently, we have no locking around PTP state. > The code above happens only in the top half of the interrupt and race > can happen when ice_ptp_release() is called and the driver starts to > release PTP structures, but hasn't stopped EXTTS yet. > > Thanks, > Karol My understanding is this is serialized via synchronize_irq on the miscellaneous IRQ vector. Thanks, Jake
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c index d8ff9f26010c..0500ced1adf8 100644 --- a/drivers/net/ethernet/intel/ice/ice_ptp.c +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c @@ -1559,6 +1559,10 @@ void ice_ptp_extts_event(struct ice_pf *pf) u8 chan, tmr_idx; u32 hi, lo; + /* Don't process timestamp events if PTP is not ready */ + if (pf->ptp.state != ICE_PTP_READY) + return; + tmr_idx = hw->func_caps.ts_func_info.tmr_index_owned; /* Event time is captured by one of the two matched registers * GLTSYN_EVNT_L: 32 LSB of sampled time event