[PATCH net] igb: initialize PTP lock before registering PHC

[PATCH net] igb: initialize PTP lock before registering PHC

[Runyu Xiao]

igb_ptp_init() registers the PHC before initializing
adapter->tmreg_lock.  ptp_clock_register() publishes the PHC device,
so a userspace PTP operation can enter a callback and take the lock
before it has been initialized.

Initialize tmreg_lock before registering the PHC so all published PTP
callbacks see a valid lock.

Fixes: b888c510f7b3 ("igb: Avoid starting unnecessary workqueues")
Cc: stable@vger.kernel.org
Assisted-by: Codex:GPT-5
Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
---
 drivers/net/ethernet/intel/igb/igb_ptp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
index 638d8242b..2a4ea327a 100644
--- a/drivers/net/ethernet/intel/igb/igb_ptp.c
+++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
@@ -1378,6 +1378,7 @@ void igb_ptp_init(struct igb_adapter *adapter)
 		return;
 	}
 
+	spin_lock_init(&adapter->tmreg_lock);
 	adapter->ptp_clock = ptp_clock_register(&adapter->ptp_caps,
 						&adapter->pdev->dev);
 	if (IS_ERR(adapter->ptp_clock)) {
@@ -1388,7 +1389,6 @@ void igb_ptp_init(struct igb_adapter *adapter)
 			 adapter->netdev->name);
 		adapter->ptp_flags |= IGB_PTP_ENABLED;
 
-		spin_lock_init(&adapter->tmreg_lock);
 		INIT_WORK(&adapter->ptp_tx_work, igb_ptp_tx_work);
 
 		if (adapter->ptp_flags & IGB_PTP_OVERFLOW_CHECK)
-- 
2.34.1

Re: [PATCH net] igb: initialize PTP lock before registering PHC

[Tony Nguyen]

+ Alessio

On 8/30/2026 8:49 AM, Runyu Xiao wrote:
> igb_ptp_init() registers the PHC before initializing
> adapter->tmreg_lock.  ptp_clock_register() publishes the PHC device,
> so a userspace PTP operation can enter a callback and take the lock
> before it has been initialized.
> 
> Initialize tmreg_lock before registering the PHC so all published PTP
> callbacks see a valid lock.
> 
> Fixes: b888c510f7b3 ("igb: Avoid starting unnecessary workqueues")
> Cc: stable@vger.kernel.org
> Assisted-by: Codex:GPT-5
> Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
> ---
>   drivers/net/ethernet/intel/igb/igb_ptp.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
> index 638d8242b..2a4ea327a 100644
> --- a/drivers/net/ethernet/intel/igb/igb_ptp.c
> +++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
> @@ -1378,6 +1378,7 @@ void igb_ptp_init(struct igb_adapter *adapter)
>   		return;
>   	}
>   
> +	spin_lock_init(&adapter->tmreg_lock);
>   	adapter->ptp_clock = ptp_clock_register(&adapter->ptp_caps,
>   						&adapter->pdev->dev);

Sashiko says:
This isn't a bug introduced by this patch, but does this expose the PTP
character device to userspace before the timecounter is fully initialized?
If userspace immediately opens the device and issues the PTP_CLOCK_GETTIME
ioctl, callbacks like igb_ptp_gettimex_82576() are invoked. These callbacks
call timecounter_cyc2time() on adapter->tc. Because igb_ptp_reset() has not
yet executed at this point to initialize the timecounter, adapter->tc.cc is
NULL, which can cause a kernel panic due to a NULL pointer dereference.

>   	if (IS_ERR(adapter->ptp_clock)) {
> @@ -1388,7 +1389,6 @@ void igb_ptp_init(struct igb_adapter *adapter)
>   			 adapter->netdev->name);
>   		adapter->ptp_flags |= IGB_PTP_ENABLED;
>   
> -		spin_lock_init(&adapter->tmreg_lock);
>   		INIT_WORK(&adapter->ptp_tx_work, igb_ptp_tx_work);

and:
This is a pre-existing issue, but does initializing this work struct here
leave a race window for list corruption?
Since register_netdev() is called in igb_probe() before igb_ptp_init()
executes, the network interface is already visible to userspace. If
userspace brings the interface up and transmits a packet with 
SO_TIMESTAMPING
requested, igb_xmit_frame_ring() can execute concurrently and call
schedule_work(&adapter->ptp_tx_work).
Because the work struct is uninitialized at that time, enqueuing it can
cause list corruption and a kernel panic. Should INIT_WORK() and other PTP
initializations be performed before register_netdev() in igb_probe()?

Tony:
Even though these report here as pre-existing, it seems these originate 
from the Fixes commit. Everything needs to be setup and ready to run 
before calling ptp_clock_register() so we can't defer starting these 
until after the PTP register. I think we need to back out the fixes 
commit and add a proper CONFIG_PTP check and unwind path for register fail.

Thanks,
Tony

>   
>   		if (adapter->ptp_flags & IGB_PTP_OVERFLOW_CHECK)

[PATCH net v2] igb: initialize PTP state before registering PHC

[Runyu Xiao]

igb_ptp_init() currently initializes the PTP lock, work items,
timestamp configuration, and timecounter after ptp_clock_register().
The PHC is published by that call, so PTP callbacks and timestamp
interrupts can run before those objects are ready.

Initialize the complete PTP state before registering the PHC. Skip the
setup when CONFIG_PTP_1588_CLOCK is disabled, since the PTP registration
helper is then a no-op. If PHC registration fails, cancel the work items
queued by igb_ptp_reset() or a timestamp interrupt before returning.

Fixes: b888c510f7b3 ("igb: Avoid starting unnecessary workqueues")
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/netdev/20260830154912.2712900-1-runyu.xiao@seu.edu.cn/
Assisted-by: LLM
Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>

---
v2:
- Restore all PTP state initialization before PHC registration.
- Skip PTP setup when CONFIG_PTP_1588_CLOCK is disabled.
- Cancel both PTP work items when PHC registration fails.
---
 drivers/net/ethernet/intel/igb/igb_ptp.c | 31 +++++++++++++++---------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
index 638d8242b..1c5581451 100644
--- a/drivers/net/ethernet/intel/igb/igb_ptp.c
+++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
@@ -1307,6 +1307,9 @@ void igb_ptp_init(struct igb_adapter *adapter)
 	struct e1000_hw *hw = &adapter->hw;
 	struct net_device *netdev = adapter->netdev;
 
+	if (!IS_ENABLED(CONFIG_PTP_1588_CLOCK))
+		return;
+
 	switch (hw->mac.type) {
 	case e1000_82576:
 		snprintf(adapter->ptp_caps.name, 16, "%pm", netdev->dev_addr);
@@ -1378,27 +1381,31 @@ void igb_ptp_init(struct igb_adapter *adapter)
 		return;
 	}
 
+	spin_lock_init(&adapter->tmreg_lock);
+	INIT_WORK(&adapter->ptp_tx_work, igb_ptp_tx_work);
+
+	if (adapter->ptp_flags & IGB_PTP_OVERFLOW_CHECK)
+		INIT_DELAYED_WORK(&adapter->ptp_overflow_work,
+				  igb_ptp_overflow_check);
+
+	adapter->tstamp_config.rx_filter = HWTSTAMP_FILTER_NONE;
+	adapter->tstamp_config.tx_type = HWTSTAMP_TX_OFF;
+
+	igb_ptp_reset(adapter);
+
 	adapter->ptp_clock = ptp_clock_register(&adapter->ptp_caps,
 						&adapter->pdev->dev);
 	if (IS_ERR(adapter->ptp_clock)) {
+		if (adapter->ptp_flags & IGB_PTP_OVERFLOW_CHECK)
+			cancel_delayed_work_sync(&adapter->ptp_overflow_work);
+		cancel_work_sync(&adapter->ptp_tx_work);
+
 		adapter->ptp_clock = NULL;
 		dev_err(&adapter->pdev->dev, "ptp_clock_register failed\n");
 	} else if (adapter->ptp_clock) {
 		dev_info(&adapter->pdev->dev, "added PHC on %s\n",
 			 adapter->netdev->name);
 		adapter->ptp_flags |= IGB_PTP_ENABLED;
-
-		spin_lock_init(&adapter->tmreg_lock);
-		INIT_WORK(&adapter->ptp_tx_work, igb_ptp_tx_work);
-
-		if (adapter->ptp_flags & IGB_PTP_OVERFLOW_CHECK)
-			INIT_DELAYED_WORK(&adapter->ptp_overflow_work,
-					  igb_ptp_overflow_check);
-
-		adapter->tstamp_config.rx_filter = HWTSTAMP_FILTER_NONE;
-		adapter->tstamp_config.tx_type = HWTSTAMP_TX_OFF;
-
-		igb_ptp_reset(adapter);
 	}
 }
 
-- 
2.34.1

Re: [PATCH net v2] igb: initialize PTP state before registering PHC

[Simon Horman]

On Thu, Sep 17, 2026 at 11:02:47PM +0800, Runyu Xiao wrote:
> igb_ptp_init() currently initializes the PTP lock, work items,
> timestamp configuration, and timecounter after ptp_clock_register().
> The PHC is published by that call, so PTP callbacks and timestamp
> interrupts can run before those objects are ready.
> 
> Initialize the complete PTP state before registering the PHC. Skip the
> setup when CONFIG_PTP_1588_CLOCK is disabled, since the PTP registration
> helper is then a no-op. If PHC registration fails, cancel the work items
> queued by igb_ptp_reset() or a timestamp interrupt before returning.
> 
> Fixes: b888c510f7b3 ("igb: Avoid starting unnecessary workqueues")
> Cc: stable@vger.kernel.org
> Link: https://lore.kernel.org/netdev/20260830154912.2712900-1-runyu.xiao@seu.edu.cn/
> Assisted-by: LLM
> Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
> 
> ---
> v2:
> - Restore all PTP state initialization before PHC registration.
> - Skip PTP setup when CONFIG_PTP_1588_CLOCK is disabled.
> - Cancel both PTP work items when PHC registration fails.

Reviewed-by: Simon Horman <horms@kernel.org>

For future reference: please post new patch revisions as new
email threads, rather than as a response to an earlier revision.

Link: https://docs.kernel.org/process/maintainer-netdev.html#resending-after-review

Thanks!