[Intel-wired-lan] [PATCH v2 1/5] net-intel: fix race condition with PTP_TX_IN_PROGRESS bits
Jacob Keller
jacob.e.keller at intel.com
Fri Apr 28 17:16:51 UTC 2017
Several Intel drivers have a hardware limitation on Tx PTP packets which
requires them to limit to a single Tx timestamp at once. THey do this
mostly using a state bit lock which enforces that only one timestamp
request is honored at a time.
Unfortunately they all suffer from a similar race condition. The bit
lock is not cleared until after skb_tstamp_tx() is called notifying
applications of a new Tx timestamp. Even a well behaved application
sending only one packet at a time and waiting for a response can wake up
and send a new packet before the bit lock is cleared. This results in
needlessly dropping some Tx timestamp requests.
We can fix this by unlocking the state bit as soon as we read the
Timestamp register, as this is the first point at which it is safe to
unlock.
To avoid issues with the skb pointer, we'll use a copy of the pointer
and set the global variable in the driver structure to NULL first. This
ensures that the next timestamp request does not modify our local copy
of the skb pointer.
This ensures that well behaved applications do not accidentally race
with the unlock bit. Obviously an application which sends multiple Tx
timestamp requests at once will still only timestamp one packet at
a time. Unfortunately there is nothing we can do about this.
Since the fix is almost the same for each driver, this patch combines
the fixes for e1000e, igb, ixgbe and i40e.
Reported-by: David Mirabito <davidm at metamako.com>
Signed-off-by: Jacob Keller <jacob.e.keller at intel.com>
---
drivers/net/ethernet/intel/e1000e/netdev.c | 10 ++++++++--
drivers/net/ethernet/intel/i40e/i40e_ptp.c | 14 +++++++++++---
drivers/net/ethernet/intel/igb/igb_ptp.c | 12 ++++++++++--
drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c | 15 ++++++++++++---
4 files changed, 41 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index b3679728caac..ec9a50a72550 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -1183,6 +1183,7 @@ static void e1000e_tx_hwtstamp_work(struct work_struct *work)
struct e1000_hw *hw = &adapter->hw;
if (er32(TSYNCTXCTL) & E1000_TSYNCTXCTL_VALID) {
+ struct sk_buff *skb = adapter->tx_hwtstamp_skb;
struct skb_shared_hwtstamps shhwtstamps;
u64 txstmp;
@@ -1191,9 +1192,14 @@ static void e1000e_tx_hwtstamp_work(struct work_struct *work)
e1000e_systim_to_hwtstamp(adapter, &shhwtstamps, txstmp);
- skb_tstamp_tx(adapter->tx_hwtstamp_skb, &shhwtstamps);
- dev_kfree_skb_any(adapter->tx_hwtstamp_skb);
+ /* Clear the global tx_hwtstamp_skb pointer and force writes
+ * prior to notifying the stack of a Tx timestamp.
+ */
adapter->tx_hwtstamp_skb = NULL;
+ wmb(); /* force write prior to skb_tstamp_tx */
+
+ skb_tstamp_tx(skb, &shhwtstamps);
+ dev_kfree_skb_any(skb);
} else if (time_after(jiffies, adapter->tx_hwtstamp_start
+ adapter->tx_timeout_factor * HZ)) {
dev_kfree_skb_any(adapter->tx_hwtstamp_skb);
diff --git a/drivers/net/ethernet/intel/i40e/i40e_ptp.c b/drivers/net/ethernet/intel/i40e/i40e_ptp.c
index 18c1cc08da97..5a4854e9fd29 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ptp.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_ptp.c
@@ -338,6 +338,7 @@ void i40e_ptp_rx_hang(struct i40e_vsi *vsi)
void i40e_ptp_tx_hwtstamp(struct i40e_pf *pf)
{
struct skb_shared_hwtstamps shhwtstamps;
+ struct sk_buff *skb = pf->ptp_tx_skb;
struct i40e_hw *hw = &pf->hw;
u32 hi, lo;
u64 ns;
@@ -353,12 +354,19 @@ void i40e_ptp_tx_hwtstamp(struct i40e_pf *pf)
hi = rd32(hw, I40E_PRTTSYN_TXTIME_H);
ns = (((u64)hi) << 32) | lo;
-
i40e_ptp_convert_to_hwtstamp(&shhwtstamps, ns);
- skb_tstamp_tx(pf->ptp_tx_skb, &shhwtstamps);
- dev_kfree_skb_any(pf->ptp_tx_skb);
+
+ /* Clear the bit lock as soon as possible after reading the register,
+ * and prior to notifying the stack via skb_tstamp_tx(). Otherwise
+ * applications might wake up and attempt to request another transmit
+ * timestamp prior to the bit lock being cleared.
+ */
pf->ptp_tx_skb = NULL;
clear_bit_unlock(__I40E_PTP_TX_IN_PROGRESS, pf->state);
+
+ /* Notify the stack and free the skb after we've unlocked */
+ skb_tstamp_tx(skb, &shhwtstamps);
+ dev_kfree_skb_any(skb);
}
/**
diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
index 7a3fd4d74592..a2da5738bfb5 100644
--- a/drivers/net/ethernet/intel/igb/igb_ptp.c
+++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
@@ -721,6 +721,7 @@ void igb_ptp_rx_hang(struct igb_adapter *adapter)
**/
static void igb_ptp_tx_hwtstamp(struct igb_adapter *adapter)
{
+ struct sk_buff *skb = adapter->ptp_tx_skb;
struct e1000_hw *hw = &adapter->hw;
struct skb_shared_hwtstamps shhwtstamps;
u64 regval;
@@ -748,10 +749,17 @@ static void igb_ptp_tx_hwtstamp(struct igb_adapter *adapter)
shhwtstamps.hwtstamp =
ktime_add_ns(shhwtstamps.hwtstamp, adjust);
- skb_tstamp_tx(adapter->ptp_tx_skb, &shhwtstamps);
- dev_kfree_skb_any(adapter->ptp_tx_skb);
+ /* 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(__IGB_PTP_TX_IN_PROGRESS, &adapter->state);
+
+ /* Notify the stack and free the skb after we've unlocked */
+ skb_tstamp_tx(skb, &shhwtstamps);
+ dev_kfree_skb_any(skb);
}
/**
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
index ef0635e0918c..9270e6f4fcff 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
@@ -672,17 +672,26 @@ static void ixgbe_ptp_clear_tx_timestamp(struct ixgbe_adapter *adapter)
*/
static void ixgbe_ptp_tx_hwtstamp(struct ixgbe_adapter *adapter)
{
+ struct sk_buff *skb = adapter->ptp_tx_skb;
struct ixgbe_hw *hw = &adapter->hw;
struct skb_shared_hwtstamps shhwtstamps;
u64 regval = 0;
regval |= (u64)IXGBE_READ_REG(hw, IXGBE_TXSTMPL);
regval |= (u64)IXGBE_READ_REG(hw, IXGBE_TXSTMPH) << 32;
-
ixgbe_ptp_convert_to_hwtstamp(adapter, &shhwtstamps, regval);
- skb_tstamp_tx(adapter->ptp_tx_skb, &shhwtstamps);
- ixgbe_ptp_clear_tx_timestamp(adapter);
+ /* Handle cleanup of the ptp_tx_skb ourselves, and unlock the state
+ * bit prior to notifying the stack via skb_tstamp_tx(). This prevents
+ * well behaved applications from attempting to timestamp again prior
+ * to the lock bit being clear.
+ */
+ adapter->ptp_tx_skb = NULL;
+ clear_bit_unlock(__IXGBE_PTP_TX_IN_PROGRESS, &adapter->state);
+
+ /* Notify the stack and then free the skb after we've unlocked */
+ skb_tstamp_tx(skb, &shhwtstamps);
+ dev_kfree_skb_any(skb);
}
/**
--
2.13.0.rc0.317.gcc792a6cad5a
More information about the Intel-wired-lan
mailing list