diff mbox series

[iwl-net,2/3] ice: Don't process extts if PTP is disabled

Message ID 20240618104310.1429515-3-karol.kolacinski@intel.com
State Superseded
Headers show
Series ice: Fix incorrect input/output pin behavior | expand

Commit Message

Karol Kolacinski June 18, 2024, 10:41 a.m. UTC
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. To ensure that the IRQ
sees the state change, call synchronize_irq() before removing the PTP
clock.

Another potential fix would be to ensure that all the GPIO configuration
gets disabled during release of the driver. This is currently not
trivial as each device family has its own set of configuration which is
not shared across all devices. In addition, only some of the device
families use the pin configuration interface. For now, relying on the
state flag is the simpler solution.

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>
---
 drivers/net/ethernet/intel/ice/ice_ptp.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Comments

Simon Horman June 19, 2024, 4:40 p.m. UTC | #1
On Tue, Jun 18, 2024 at 12:41:37PM +0200, Karol Kolacinski 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. To ensure that the IRQ
> sees the state change, call synchronize_irq() before removing the PTP
> clock.

Hi Karol and Jacob,

After pf->ptp.state is set in ptp_clock_event(),
ice_ptp_disable_all_extts() is called which in turn calls
synchronize_irq(). Which I assume is what the last sentence above refers
to. But the way it is worded it sounds like a call to synchronize_irq() is
being added by this patch, which is not the case.

I suppose it is not a big deal, but this did confuse me.
So perhaps the wording could be enhanced?

> Another potential fix would be to ensure that all the GPIO configuration
> gets disabled during release of the driver. This is currently not
> trivial as each device family has its own set of configuration which is
> not shared across all devices. In addition, only some of the device
> families use the pin configuration interface. For now, relying on the
> state flag is the simpler solution.
> 
> 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>
> ---
>  drivers/net/ethernet/intel/ice/ice_ptp.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index 30f1f910e6d9..b952cad42f92 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
> @@ -1573,10 +1577,8 @@ void ice_ptp_extts_event(struct ice_pf *pf)
>  			event.timestamp = (((u64)hi) << 32) | lo;
>  			event.type = PTP_CLOCK_EXTTS;
>  			event.index = chan;
> -
> -			/* Fire event */
> -			ptp_clock_event(pf->ptp.clock, &event);
>  			pf->ptp.ext_ts_irq &= ~(1 << chan);
> +			ptp_clock_event(pf->ptp.clock, &event);
>  		}
>  	}
>  }

I'm also confused (often, TBH!) as to how the last hunk of this
patch relates to the problem at hand.
Keller, Jacob E July 8, 2024, 10:50 p.m. UTC | #2
> -----Original Message-----
> From: Simon Horman <horms@kernel.org>
> Sent: Wednesday, June 19, 2024 9:41 AM
> To: Kolacinski, Karol <karol.kolacinski@intel.com>
> Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; Kitszel, Przemyslaw
> <przemyslaw.kitszel@intel.com>; Keller, Jacob E <jacob.e.keller@intel.com>
> Subject: Re: [PATCH iwl-net 2/3] ice: Don't process extts if PTP is disabled
> 
> On Tue, Jun 18, 2024 at 12:41:37PM +0200, Karol Kolacinski 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. To ensure that the IRQ
> > sees the state change, call synchronize_irq() before removing the PTP
> > clock.
> 
> Hi Karol and Jacob,
> 
> After pf->ptp.state is set in ptp_clock_event(),
> ice_ptp_disable_all_extts() is called which in turn calls
> synchronize_irq(). Which I assume is what the last sentence above refers
> to. But the way it is worded it sounds like a call to synchronize_irq() is
> being added by this patch, which is not the case.
> 
> I suppose it is not a big deal, but this did confuse me.
> So perhaps the wording could be enhanced?
> 

I believe the call to synchronize_irq() predates this as the same IRQ is used for other timestamping/PTP related events.

This could be clarified in the commit message

> > Another potential fix would be to ensure that all the GPIO configuration
> > gets disabled during release of the driver. This is currently not
> > trivial as each device family has its own set of configuration which is
> > not shared across all devices. In addition, only some of the device
> > families use the pin configuration interface. For now, relying on the
> > state flag is the simpler solution.
> >
> > 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>
> > ---
> >  drivers/net/ethernet/intel/ice/ice_ptp.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c
> b/drivers/net/ethernet/intel/ice/ice_ptp.c
> > index 30f1f910e6d9..b952cad42f92 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
> > @@ -1573,10 +1577,8 @@ void ice_ptp_extts_event(struct ice_pf *pf)
> >  			event.timestamp = (((u64)hi) << 32) | lo;
> >  			event.type = PTP_CLOCK_EXTTS;
> >  			event.index = chan;
> > -
> > -			/* Fire event */
> > -			ptp_clock_event(pf->ptp.clock, &event);
> >  			pf->ptp.ext_ts_irq &= ~(1 << chan);
> > +			ptp_clock_event(pf->ptp.clock, &event);
> >  		}
> >  	}
> >  }
> 
> I'm also confused (often, TBH!) as to how the last hunk of this
> patch relates to the problem at hand.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index 30f1f910e6d9..b952cad42f92 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
@@ -1573,10 +1577,8 @@  void ice_ptp_extts_event(struct ice_pf *pf)
 			event.timestamp = (((u64)hi) << 32) | lo;
 			event.type = PTP_CLOCK_EXTTS;
 			event.index = chan;
-
-			/* Fire event */
-			ptp_clock_event(pf->ptp.clock, &event);
 			pf->ptp.ext_ts_irq &= ~(1 << chan);
+			ptp_clock_event(pf->ptp.clock, &event);
 		}
 	}
 }