| From foo@baz Mon Apr 9 17:09:24 CEST 2018 |
| From: Jacob Keller <jacob.e.keller@intel.com> |
| Date: Wed, 3 May 2017 10:28:50 -0700 |
| Subject: e1000e: fix race condition around skb_tstamp_tx() |
| |
| From: Jacob Keller <jacob.e.keller@intel.com> |
| |
| |
| [ Upstream commit 5012863b7347866764c4a4e58b62fb05346b0d06 ] |
| |
| The e1000e driver and related hardware has a limitation on Tx PTP |
| packets which requires we limit to timestamping a single packet at once. |
| We do this by verifying that we never request a new Tx timestamp while |
| we still have a tx_hwtstamp_skb pointer. |
| |
| Unfortunately the driver suffers from a race condition around this. The |
| tx_hwtstamp_skb pointer is not set to NULL until after skb_tstamp_tx() |
| is called. This function notifies the stack and applications of a new |
| timestamp. Even a well behaved application that only sends a new request |
| when the first one is finished might be woken up and possibly send |
| a packet before we can free the timestamp in the driver again. The |
| result is that we needlessly ignore some Tx timestamp requests in this |
| corner case. |
| |
| Fix this by assigning the tx_hwtstamp_skb pointer prior to calling |
| skb_tstamp_tx() and use a temporary pointer to hold the timestamped skb |
| until that function finishes. This ensures that the application is not |
| woken up until the driver is ready to begin timestamping a new packet. |
| |
| This ensures that well behaved applications do not accidentally race |
| with condition to skip Tx timestamps. 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. |
| |
| Reported-by: David Mirabito <davidm@metamako.com> |
| Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> |
| Tested-by: Aaron Brown <aaron.f.brown@intel.com> |
| Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com> |
| Signed-off-by: Sasha Levin <alexander.levin@microsoft.com> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| --- |
| drivers/net/ethernet/intel/e1000e/netdev.c | 10 ++++++++-- |
| 1 file changed, 8 insertions(+), 2 deletions(-) |
| |
| --- a/drivers/net/ethernet/intel/e1000e/netdev.c |
| +++ b/drivers/net/ethernet/intel/e1000e/netdev.c |
| @@ -1182,6 +1182,7 @@ static void e1000e_tx_hwtstamp_work(stru |
| 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; |
| |
| @@ -1190,9 +1191,14 @@ static void e1000e_tx_hwtstamp_work(stru |
| |
| 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); |