[Intel-wired-lan] [PATCH iwl-net v2 1/5] i40e: serialize Tx timestamp skb ownership

From: Xuanqiang Luo <[email protected]>

The Tx timestamp interrupt, watchdog, transmit error path, and PTP stop
path can all release ptp_tx_skb. The existing in-progress bit serializes
slot reservation, but does not protect the skb pointer. One path can
therefore free the skb while another is still using it, causing a
use-after-free or double free.

Add ptp_tx_lock to protect the timestamp slot. With all accesses
serialized, use a non-NULL ptp_tx_skb as the single indication that the
slot is occupied and remove the redundant in-progress bit. Publish and
claim the skb under the lock, then complete or free it after unlocking.

Make the transmit error path claim the slot only when it still points to
the skb whose transmission failed. This prevents delayed error cleanup
from releasing a newer timestamp request after another path recycled the
slot.

Fixes: 0bc0706b46cd ("i40e: check for Tx timestamp timeouts during watchdog")
Signed-off-by: Xuanqiang Luo <[email protected]>
---
 drivers/net/ethernet/intel/i40e/i40e.h      |  3 +-
 drivers/net/ethernet/intel/i40e/i40e_main.c |  1 +
 drivers/net/ethernet/intel/i40e/i40e_ptp.c  | 55 ++++++++++++++-------
 drivers/net/ethernet/intel/i40e/i40e_txrx.c | 38 ++++++++++----
 4 files changed, 67 insertions(+), 30 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e.h b/drivers/net/ethernet/intel/i40e/i40e.h
index 83e780919ac97..4c8bcaa9dd192 100644
--- a/drivers/net/ethernet/intel/i40e/i40e.h
+++ b/drivers/net/ethernet/intel/i40e/i40e.h
@@ -101,7 +101,6 @@ enum i40e_state {
 	__I40E_GLOBAL_RESET_REQUESTED,
 	__I40E_EMP_RESET_INTR_RECEIVED,
 	__I40E_SUSPENDED,
-	__I40E_PTP_TX_IN_PROGRESS,
 	__I40E_BAD_EEPROM,
 	__I40E_DOWN_REQUESTED,
 	__I40E_FD_FLUSH_REQUESTED,
@@ -675,6 +674,8 @@ struct i40e_pf {
 	u32 tx_hwtstamp_skipped;
 	u32 rx_hwtstamp_cleared;
 	u32 latch_event_flags;
+	/* Protects ptp_tx_skb, which also indicates the slot state. */
+	spinlock_t ptp_tx_lock;
 	spinlock_t ptp_rx_lock; /* Used to protect Rx timestamp registers. */
 	unsigned long latch_events[4];
 	bool ptp_tx;
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index a04683004a567..0a259351c127e 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -12819,6 +12819,7 @@ static int i40e_sw_init(struct i40e_pf *pf)
 			 "total-port-shutdown was enabled, link-down-on-close is forced on\n");
 	}
 	mutex_init(&pf->switch_mutex);
+	spin_lock_init(&pf->ptp_tx_lock);
 
 sw_init_done:
 	return err;
diff --git a/drivers/net/ethernet/intel/i40e/i40e_ptp.c b/drivers/net/ethernet/intel/i40e/i40e_ptp.c
index ff62b5f2c8150..224568a3af085 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ptp.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_ptp.c
@@ -728,14 +728,17 @@ void i40e_ptp_rx_hang(struct i40e_pf *pf)
  **/
 void i40e_ptp_tx_hang(struct i40e_pf *pf)
 {
+	unsigned long flags;
 	struct sk_buff *skb;
 
 	if (!test_bit(I40E_FLAG_PTP_ENA, pf->flags) || !pf->ptp_tx)
 		return;
 
+	spin_lock_irqsave(&pf->ptp_tx_lock, flags);
+
 	/* Nothing to do if we're not already waiting for a timestamp */
-	if (!test_bit(__I40E_PTP_TX_IN_PROGRESS, pf->state))
-		return;
+	if (!pf->ptp_tx_skb)
+		goto out_unlock;
 
 	/* We already have a handler routine which is run when we are notified
 	 * of a Tx timestamp in the hardware. If we don't get an interrupt
@@ -744,12 +747,16 @@ void i40e_ptp_tx_hang(struct i40e_pf *pf)
 	if (time_is_before_jiffies(pf->ptp_tx_start + HZ)) {
 		skb = pf->ptp_tx_skb;
 		pf->ptp_tx_skb = NULL;
-		clear_bit_unlock(__I40E_PTP_TX_IN_PROGRESS, pf->state);
+		spin_unlock_irqrestore(&pf->ptp_tx_lock, flags);
 
-		/* Free the skb after we clear the bitlock */
+		/* Free the skb after releasing the lock. */
 		dev_kfree_skb_any(skb);
 		pf->tx_hwtstamp_timeouts++;
+		return;
 	}
+
+out_unlock:
+	spin_unlock_irqrestore(&pf->ptp_tx_lock, flags);
 }
 
 /**
@@ -763,17 +770,21 @@ void i40e_ptp_tx_hang(struct i40e_pf *pf)
 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;
+	unsigned long flags;
+	struct sk_buff *skb;
 	u32 hi, lo;
 	u64 ns;
 
 	if (!test_bit(I40E_FLAG_PTP_ENA, pf->flags) || !pf->ptp_tx)
 		return;
 
-	/* don't attempt to timestamp if we don't have an skb */
-	if (!pf->ptp_tx_skb)
-		return;
+	spin_lock_irqsave(&pf->ptp_tx_lock, flags);
+
+	/* Take ownership of the skb before another cleanup path can free it. */
+	skb = pf->ptp_tx_skb;
+	if (!skb)
+		goto out_unlock;
 
 	lo = rd32(hw, I40E_PRTTSYN_TXTIME_L);
 	hi = rd32(hw, I40E_PRTTSYN_TXTIME_H);
@@ -781,17 +792,21 @@ void i40e_ptp_tx_hwtstamp(struct i40e_pf *pf)
 	ns = (((u64)hi) << 32) | lo;
 	i40e_ptp_convert_to_hwtstamp(&shhwtstamps, ns);
 
-	/* 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.
+	/* Mark the slot available as soon as possible after reading the
+	 * register and before notifying the stack via skb_tstamp_tx().
+	 * Otherwise, applications might wake up and request another transmit
+	 * timestamp before the slot is available.
 	 */
 	pf->ptp_tx_skb = NULL;
-	clear_bit_unlock(__I40E_PTP_TX_IN_PROGRESS, pf->state);
+	spin_unlock_irqrestore(&pf->ptp_tx_lock, flags);
 
 	/* Notify the stack and free the skb after we've unlocked */
 	skb_tstamp_tx(skb, &shhwtstamps);
 	dev_kfree_skb_any(skb);
+	return;
+
+out_unlock:
+	spin_unlock_irqrestore(&pf->ptp_tx_lock, flags);
 }
 
 /**
@@ -1539,19 +1554,21 @@ void i40e_ptp_stop(struct i40e_pf *pf)
 {
 	struct i40e_vsi *main_vsi = i40e_pf_get_main_vsi(pf);
 	struct i40e_hw *hw = &pf->hw;
+	unsigned long flags;
+	struct sk_buff *skb;
 	u32 regval;
 
+	spin_lock_irqsave(&pf->ptp_tx_lock, flags);
 	clear_bit(I40E_FLAG_PTP_ENA, pf->flags);
 	pf->ptp_tx = false;
-	pf->ptp_rx = false;
+	skb = pf->ptp_tx_skb;
+	pf->ptp_tx_skb = NULL;
+	spin_unlock_irqrestore(&pf->ptp_tx_lock, flags);
 
-	if (pf->ptp_tx_skb) {
-		struct sk_buff *skb = pf->ptp_tx_skb;
+	pf->ptp_rx = false;
 
-		pf->ptp_tx_skb = NULL;
-		clear_bit_unlock(__I40E_PTP_TX_IN_PROGRESS, pf->state);
+	if (skb)
 		dev_kfree_skb_any(skb);
-	}
 
 	if (pf->ptp_clock) {
 		ptp_clock_unregister(pf->ptp_clock);
diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
index 894f2d06d39d0..f430546acf422 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
@@ -3198,7 +3198,9 @@ static int i40e_tso(struct i40e_tx_buffer *first, u8 *hdr_len,
 static int i40e_tsyn(struct i40e_ring *tx_ring, struct sk_buff *skb,
 		     u32 tx_flags, u64 *cd_type_cmd_tso_mss)
 {
+	unsigned long flags;
 	struct i40e_pf *pf;
+	int tsyn = 0;
 
 	if (likely(!(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)))
 		return 0;
@@ -3211,23 +3213,28 @@ static int i40e_tsyn(struct i40e_ring *tx_ring, struct sk_buff *skb,
 	 * we are not already transmitting a packet to be timestamped
 	 */
 	pf = i40e_netdev_to_pf(tx_ring->netdev);
+	spin_lock_irqsave(&pf->ptp_tx_lock, flags);
+
 	if (!test_bit(I40E_FLAG_PTP_ENA, pf->flags))
-		return 0;
+		goto out_unlock;
 
-	if (pf->ptp_tx &&
-	    !test_and_set_bit_lock(__I40E_PTP_TX_IN_PROGRESS, pf->state)) {
+	if (pf->ptp_tx && !pf->ptp_tx_skb) {
 		skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
 		pf->ptp_tx_start = jiffies;
 		pf->ptp_tx_skb = skb_get(skb);
+		tsyn = 1;
 	} else {
 		pf->tx_hwtstamp_skipped++;
-		return 0;
 	}
 
-	*cd_type_cmd_tso_mss |= (u64)I40E_TX_CTX_DESC_TSYN <<
-				I40E_TXD_CTX_QW1_CMD_SHIFT;
+out_unlock:
+	spin_unlock_irqrestore(&pf->ptp_tx_lock, flags);
 
-	return 1;
+	if (tsyn)
+		*cd_type_cmd_tso_mss |= (u64)I40E_TX_CTX_DESC_TSYN <<
+					I40E_TXD_CTX_QW1_CMD_SHIFT;
+
+	return tsyn;
 }
 
 /**
@@ -3957,10 +3964,21 @@ static netdev_tx_t i40e_xmit_frame_ring(struct sk_buff *skb,
 cleanup_tx_tstamp:
 	if (unlikely(tx_flags & I40E_TX_FLAGS_TSYN)) {
 		struct i40e_pf *pf = i40e_netdev_to_pf(tx_ring->netdev);
+		struct sk_buff *ptp_skb = NULL;
+		unsigned long flags;
+
+		spin_lock_irqsave(&pf->ptp_tx_lock, flags);
+		/* Another cleanup path may have handed the slot
+		 * to a new skb.
+		 */
+		if (pf->ptp_tx_skb == skb) {
+			ptp_skb = pf->ptp_tx_skb;
+			pf->ptp_tx_skb = NULL;
+		}
+		spin_unlock_irqrestore(&pf->ptp_tx_lock, flags);
 
-		dev_kfree_skb_any(pf->ptp_tx_skb);
-		pf->ptp_tx_skb = NULL;
-		clear_bit_unlock(__I40E_PTP_TX_IN_PROGRESS, pf->state);
+		if (ptp_skb)
+			dev_kfree_skb_any(ptp_skb);
 	}
 
 	return NETDEV_TX_OK;
-- 
2.43.0