This is a note to let you know that I've just added the patch titled
igc: Fix race condition in PTP tx code
to the 5.10-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
The filename of the patch is:
igc-fix-race-condition-in-ptp-tx-code.patch
and it can be found in the queue-5.10 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree,
please let <stable@xxxxxxxxxxxxxxx> know about it.
commit 36e6af181b7d5be161661f87e7e94c93c932e1c5
Author: Vinicius Costa Gomes <vinicius.gomes@xxxxxxxxx>
Date: Wed Jun 7 14:32:29 2023 -0700
igc: Fix race condition in PTP tx code
[ Upstream commit 9c50e2b150c8ee0eee5f8154e2ad168cdd748877 ]
Currently, the igc driver supports timestamping only one tx packet at a
time. During the transmission flow, the skb that requires hardware
timestamping is saved in adapter->ptp_tx_skb. Once hardware has the
timestamp, an interrupt is delivered, and adapter->ptp_tx_work is
scheduled. In igc_ptp_tx_work(), we read the timestamp register, update
adapter->ptp_tx_skb, and notify the network stack.
While the thread executing the transmission flow (the user process
running in kernel mode) and the thread executing ptp_tx_work don't
access adapter->ptp_tx_skb concurrently, there are two other places
where adapter->ptp_tx_skb is accessed: igc_ptp_tx_hang() and
igc_ptp_suspend().
igc_ptp_tx_hang() is executed by the adapter->watchdog_task worker
thread which runs periodically so it is possible we have two threads
accessing ptp_tx_skb at the same time. Consider the following scenario:
right after __IGC_PTP_TX_IN_PROGRESS is set in igc_xmit_frame_ring(),
igc_ptp_tx_hang() is executed. Since adapter->ptp_tx_start hasn't been
written yet, this is considered a timeout and adapter->ptp_tx_skb is
cleaned up.
This patch fixes the issue described above by adding the ptp_tx_lock to
protect access to ptp_tx_skb and ptp_tx_start fields from igc_adapter.
Since igc_xmit_frame_ring() called in atomic context by the networking
stack, ptp_tx_lock is defined as a spinlock, and the irq safe variants
of lock/unlock are used.
With the introduction of the ptp_tx_lock, the __IGC_PTP_TX_IN_PROGRESS
flag doesn't provide much of a use anymore so this patch gets rid of it.
Fixes: 2c344ae24501 ("igc: Add support for TX timestamping")
Signed-off-by: Andre Guedes <andre.guedes@xxxxxxxxx>
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@xxxxxxxxx>
Reviewed-by: Kurt Kanzenbach <kurt@xxxxxxxxxxxxx>
Tested-by: Naama Meir <naamax.meir@xxxxxxxxxxxxxxx>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@xxxxxxxxx>
Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>
diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index 47ba1eafcdc7b..33f64c80335d3 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -210,6 +210,10 @@ struct igc_adapter {
struct ptp_clock *ptp_clock;
struct ptp_clock_info ptp_caps;
struct work_struct ptp_tx_work;
+ /* Access to ptp_tx_skb and ptp_tx_start are protected by the
+ * ptp_tx_lock.
+ */
+ spinlock_t ptp_tx_lock;
struct sk_buff *ptp_tx_skb;
struct hwtstamp_config tstamp_config;
unsigned long ptp_tx_start;
@@ -389,7 +393,6 @@ enum igc_state_t {
__IGC_TESTING,
__IGC_RESETTING,
__IGC_DOWN,
- __IGC_PTP_TX_IN_PROGRESS,
};
enum igc_tx_flags {
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 72d7d2cf126d1..a15e4b6d7fa40 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -1467,9 +1467,10 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
* the other timer registers before skipping the
* timestamping request.
*/
- if (adapter->tstamp_config.tx_type == HWTSTAMP_TX_ON &&
- !test_and_set_bit_lock(__IGC_PTP_TX_IN_PROGRESS,
- &adapter->state)) {
+ unsigned long flags;
+
+ spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
+ if (adapter->tstamp_config.tx_type == HWTSTAMP_TX_ON && !adapter->ptp_tx_skb) {
skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
tx_flags |= IGC_TX_FLAGS_TSTAMP;
@@ -1478,6 +1479,8 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
} else {
adapter->tx_hwtstamp_skipped++;
}
+
+ spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
}
/* record initial flags and protocol */
diff --git a/drivers/net/ethernet/intel/igc/igc_ptp.c b/drivers/net/ethernet/intel/igc/igc_ptp.c
index ef53f7665b58c..25b238c6a675c 100644
--- a/drivers/net/ethernet/intel/igc/igc_ptp.c
+++ b/drivers/net/ethernet/intel/igc/igc_ptp.c
@@ -323,6 +323,7 @@ static int igc_ptp_set_timestamp_mode(struct igc_adapter *adapter,
return 0;
}
+/* Requires adapter->ptp_tx_lock held by caller. */
static void igc_ptp_tx_timeout(struct igc_adapter *adapter)
{
struct igc_hw *hw = &adapter->hw;
@@ -330,7 +331,6 @@ static void igc_ptp_tx_timeout(struct igc_adapter *adapter)
dev_kfree_skb_any(adapter->ptp_tx_skb);
adapter->ptp_tx_skb = NULL;
adapter->tx_hwtstamp_timeouts++;
- clear_bit_unlock(__IGC_PTP_TX_IN_PROGRESS, &adapter->state);
/* Clear the tx valid bit in TSYNCTXCTL register to enable interrupt. */
rd32(IGC_TXSTMPH);
netdev_warn(adapter->netdev, "Tx timestamp timeout\n");
@@ -338,20 +338,20 @@ static void igc_ptp_tx_timeout(struct igc_adapter *adapter)
void igc_ptp_tx_hang(struct igc_adapter *adapter)
{
- bool timeout = time_is_before_jiffies(adapter->ptp_tx_start +
- IGC_PTP_TX_TIMEOUT);
+ unsigned long flags;
- if (!test_bit(__IGC_PTP_TX_IN_PROGRESS, &adapter->state))
- return;
+ spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
- /* If we haven't received a timestamp within the timeout, it is
- * reasonable to assume that it will never occur, so we can unlock the
- * timestamp bit when this occurs.
- */
- if (timeout) {
- cancel_work_sync(&adapter->ptp_tx_work);
- igc_ptp_tx_timeout(adapter);
- }
+ if (!adapter->ptp_tx_skb)
+ goto unlock;
+
+ if (time_is_after_jiffies(adapter->ptp_tx_start + IGC_PTP_TX_TIMEOUT))
+ goto unlock;
+
+ igc_ptp_tx_timeout(adapter);
+
+unlock:
+ spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
}
/**
@@ -361,6 +361,8 @@ void igc_ptp_tx_hang(struct igc_adapter *adapter)
* If we were asked to do hardware stamping and such a time stamp is
* available, then it must have been for this skb here because we only
* allow only one such packet into the queue.
+ *
+ * Context: Expects adapter->ptp_tx_lock to be held by caller.
*/
static void igc_ptp_tx_hwtstamp(struct igc_adapter *adapter)
{
@@ -396,13 +398,7 @@ static void igc_ptp_tx_hwtstamp(struct igc_adapter *adapter)
shhwtstamps.hwtstamp =
ktime_add_ns(shhwtstamps.hwtstamp, adjust);
- /* Clear the lock early before calling skb_tstamp_tx so that
- * applications are not woken up before the lock bit is clear. We use
- * a copy of the skb pointer to ensure other threads can't change it
- * while we're notifying the stack.
- */
adapter->ptp_tx_skb = NULL;
- clear_bit_unlock(__IGC_PTP_TX_IN_PROGRESS, &adapter->state);
/* Notify the stack and free the skb after we've unlocked */
skb_tstamp_tx(skb, &shhwtstamps);
@@ -413,24 +409,33 @@ static void igc_ptp_tx_hwtstamp(struct igc_adapter *adapter)
* igc_ptp_tx_work
* @work: pointer to work struct
*
- * This work function polls the TSYNCTXCTL valid bit to determine when a
- * timestamp has been taken for the current stored skb.
+ * This work function checks the TSYNCTXCTL valid bit to determine when
+ * a timestamp has been taken for the current stored skb.
*/
static void igc_ptp_tx_work(struct work_struct *work)
{
struct igc_adapter *adapter = container_of(work, struct igc_adapter,
ptp_tx_work);
struct igc_hw *hw = &adapter->hw;
+ unsigned long flags;
u32 tsynctxctl;
- if (!test_bit(__IGC_PTP_TX_IN_PROGRESS, &adapter->state))
- return;
+ spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
+
+ if (!adapter->ptp_tx_skb)
+ goto unlock;
tsynctxctl = rd32(IGC_TSYNCTXCTL);
- if (WARN_ON_ONCE(!(tsynctxctl & IGC_TSYNCTXCTL_TXTT_0)))
- return;
+ tsynctxctl &= IGC_TSYNCTXCTL_TXTT_0;
+ if (!tsynctxctl) {
+ WARN_ONCE(1, "Received a TSTAMP interrupt but no TSTAMP is ready.\n");
+ goto unlock;
+ }
igc_ptp_tx_hwtstamp(adapter);
+
+unlock:
+ spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
}
/**
@@ -506,6 +511,7 @@ void igc_ptp_init(struct igc_adapter *adapter)
return;
}
+ spin_lock_init(&adapter->ptp_tx_lock);
spin_lock_init(&adapter->tmreg_lock);
INIT_WORK(&adapter->ptp_tx_work, igc_ptp_tx_work);
@@ -559,7 +565,6 @@ void igc_ptp_suspend(struct igc_adapter *adapter)
cancel_work_sync(&adapter->ptp_tx_work);
dev_kfree_skb_any(adapter->ptp_tx_skb);
adapter->ptp_tx_skb = NULL;
- clear_bit_unlock(__IGC_PTP_TX_IN_PROGRESS, &adapter->state);
if (pci_device_is_present(adapter->pdev))
igc_ptp_time_save(adapter);
