Patch Detail
get:
Show a patch.
patch:
Update a patch.
put:
Update a patch.
GET /api/patches/756519/?format=api
{ "id": 756519, "url": "http://patchwork.ozlabs.org/api/patches/756519/?format=api", "web_url": "http://patchwork.ozlabs.org/project/intel-wired-lan/patch/20170428184421.20216-2-jacob.e.keller@intel.com/", "project": { "id": 46, "url": "http://patchwork.ozlabs.org/api/projects/46/?format=api", "name": "Intel Wired Ethernet development", "link_name": "intel-wired-lan", "list_id": "intel-wired-lan.osuosl.org", "list_email": "intel-wired-lan@osuosl.org", "web_url": "", "scm_url": "", "webscm_url": "", "list_archive_url": "", "list_archive_url_format": "", "commit_url_format": "" }, "msgid": "<20170428184421.20216-2-jacob.e.keller@intel.com>", "list_archive_url": null, "date": "2017-04-28T18:44:17", "name": "[v3,1/5] net-intel: fix race condition with PTP_TX_IN_PROGRESS bits", "commit_ref": null, "pull_url": null, "state": "superseded", "archived": false, "hash": "6c70529b80efecaeb53ec2423736bf24ca14eab3", "submitter": { "id": 9784, "url": "http://patchwork.ozlabs.org/api/people/9784/?format=api", "name": "Jacob Keller", "email": "jacob.e.keller@intel.com" }, "delegate": { "id": 68, "url": "http://patchwork.ozlabs.org/api/users/68/?format=api", "username": "jtkirshe", "first_name": "Jeff", "last_name": "Kirsher", "email": "jeffrey.t.kirsher@intel.com" }, "mbox": "http://patchwork.ozlabs.org/project/intel-wired-lan/patch/20170428184421.20216-2-jacob.e.keller@intel.com/mbox/", "series": [], "comments": "http://patchwork.ozlabs.org/api/patches/756519/comments/", "check": "pending", "checks": "http://patchwork.ozlabs.org/api/patches/756519/checks/", "tags": {}, "related": [], "headers": { "Return-Path": "<intel-wired-lan-bounces@lists.osuosl.org>", "X-Original-To": [ "incoming@patchwork.ozlabs.org", "intel-wired-lan@lists.osuosl.org" ], "Delivered-To": [ "patchwork-incoming@bilbo.ozlabs.org", "intel-wired-lan@lists.osuosl.org" ], "Received": [ "from fraxinus.osuosl.org (smtp4.osuosl.org [140.211.166.137])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3wF2mM2lHPz9s7y\n\tfor <incoming@patchwork.ozlabs.org>;\n\tSat, 29 Apr 2017 04:44:35 +1000 (AEST)", "from localhost (localhost [127.0.0.1])\n\tby fraxinus.osuosl.org (Postfix) with ESMTP id D37E888505;\n\tFri, 28 Apr 2017 18:44:33 +0000 (UTC)", "from fraxinus.osuosl.org ([127.0.0.1])\n\tby localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024)\n\twith ESMTP id 9dn-sQwHf5AI; Fri, 28 Apr 2017 18:44:30 +0000 (UTC)", "from ash.osuosl.org (ash.osuosl.org [140.211.166.34])\n\tby fraxinus.osuosl.org (Postfix) with ESMTP id 537E0884C3;\n\tFri, 28 Apr 2017 18:44:29 +0000 (UTC)", "from hemlock.osuosl.org (smtp2.osuosl.org [140.211.166.133])\n\tby ash.osuosl.org (Postfix) with ESMTP id 1B5F91C26F3\n\tfor <intel-wired-lan@lists.osuosl.org>;\n\tFri, 28 Apr 2017 18:44:27 +0000 (UTC)", "from localhost (localhost [127.0.0.1])\n\tby hemlock.osuosl.org (Postfix) with ESMTP id 157668A23D\n\tfor <intel-wired-lan@lists.osuosl.org>;\n\tFri, 28 Apr 2017 18:44:27 +0000 (UTC)", "from hemlock.osuosl.org ([127.0.0.1])\n\tby localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024)\n\twith ESMTP id sYXO4px2B4z3 for <intel-wired-lan@lists.osuosl.org>;\n\tFri, 28 Apr 2017 18:44:25 +0000 (UTC)", "from mga07.intel.com (mga07.intel.com [134.134.136.100])\n\tby hemlock.osuosl.org (Postfix) with ESMTPS id EAF758A268\n\tfor <intel-wired-lan@lists.osuosl.org>;\n\tFri, 28 Apr 2017 18:44:24 +0000 (UTC)", "from fmsmga001.fm.intel.com ([10.253.24.23])\n\tby orsmga105.jf.intel.com with ESMTP; 28 Apr 2017 11:44:23 -0700", "from jekeller-desk.amr.corp.intel.com (HELO\n\tjekeller-desk.jekeller.internal) ([10.166.35.158])\n\tby fmsmga001.fm.intel.com with ESMTP; 28 Apr 2017 11:44:23 -0700" ], "X-Virus-Scanned": [ "amavisd-new at osuosl.org", "amavisd-new at osuosl.org" ], "X-Greylist": "domain auto-whitelisted by SQLgrey-1.7.6", "X-ExtLoop1": "1", "X-IronPort-AV": "E=Sophos; i=\"5.37,389,1488873600\"; d=\"scan'208\";\n\ta=\"1141484695\"", "From": "Jacob Keller <jacob.e.keller@intel.com>", "To": "Intel Wired LAN <intel-wired-lan@lists.osuosl.org>", "Date": "Fri, 28 Apr 2017 11:44:17 -0700", "Message-Id": "<20170428184421.20216-2-jacob.e.keller@intel.com>", "X-Mailer": "git-send-email 2.13.0.rc0.317.gcc792a6cad5a", "In-Reply-To": "<20170428184421.20216-1-jacob.e.keller@intel.com>", "References": "<20170428184421.20216-1-jacob.e.keller@intel.com>", "Cc": "Richard Cochran <richardcochran@gmail.com>", "Subject": "[Intel-wired-lan] [PATCH v3 1/5] net-intel: fix race condition with\n\tPTP_TX_IN_PROGRESS bits", "X-BeenThere": "intel-wired-lan@lists.osuosl.org", "X-Mailman-Version": "2.1.18-1", "Precedence": "list", "List-Id": "Intel Wired Ethernet Linux Kernel Driver Development\n\t<intel-wired-lan.lists.osuosl.org>", "List-Unsubscribe": "<http://lists.osuosl.org/mailman/options/intel-wired-lan>, \n\t<mailto:intel-wired-lan-request@lists.osuosl.org?subject=unsubscribe>", "List-Archive": "<http://lists.osuosl.org/pipermail/intel-wired-lan/>", "List-Post": "<mailto:intel-wired-lan@lists.osuosl.org>", "List-Help": "<mailto:intel-wired-lan-request@lists.osuosl.org?subject=help>", "List-Subscribe": "<http://lists.osuosl.org/mailman/listinfo/intel-wired-lan>, \n\t<mailto:intel-wired-lan-request@lists.osuosl.org?subject=subscribe>", "MIME-Version": "1.0", "Content-Type": "text/plain; charset=\"us-ascii\"", "Content-Transfer-Encoding": "7bit", "Errors-To": "intel-wired-lan-bounces@lists.osuosl.org", "Sender": "\"Intel-wired-lan\" <intel-wired-lan-bounces@lists.osuosl.org>" }, "content": "Several Intel drivers have a hardware limitation on Tx PTP packets which\nrequires them to limit to a single Tx timestamp at once. THey do this\nmostly using a state bit lock which enforces that only one timestamp\nrequest is honored at a time.\n\nUnfortunately they all suffer from a similar race condition. The bit\nlock is not cleared until after skb_tstamp_tx() is called notifying\napplications of a new Tx timestamp. Even a well behaved application\nsending only one packet at a time and waiting for a response can wake up\nand send a new packet before the bit lock is cleared. This results in\nneedlessly dropping some Tx timestamp requests.\n\nWe can fix this by unlocking the state bit as soon as we read the\nTimestamp register, as this is the first point at which it is safe to\nunlock.\n\nTo avoid issues with the skb pointer, we'll use a copy of the pointer\nand set the global variable in the driver structure to NULL first. This\nensures that the next timestamp request does not modify our local copy\nof the skb pointer.\n\nThis ensures that well behaved applications do not accidentally race\nwith the unlock bit. Obviously an application which sends multiple Tx\ntimestamp requests at once will still only timestamp one packet at\na time. Unfortunately there is nothing we can do about this.\n\nSince the fix is almost the same for each driver, this patch combines\nthe fixes for e1000e, igb, ixgbe and i40e.\n\nReported-by: David Mirabito <davidm@metamako.com>\nSigned-off-by: Jacob Keller <jacob.e.keller@intel.com>\n---\n drivers/net/ethernet/intel/e1000e/netdev.c | 10 ++++++++--\n drivers/net/ethernet/intel/i40e/i40e_ptp.c | 14 +++++++++++---\n drivers/net/ethernet/intel/igb/igb_ptp.c | 12 ++++++++++--\n drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c | 15 ++++++++++++---\n 4 files changed, 41 insertions(+), 10 deletions(-)", "diff": "diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c\nindex b3679728caac..ec9a50a72550 100644\n--- a/drivers/net/ethernet/intel/e1000e/netdev.c\n+++ b/drivers/net/ethernet/intel/e1000e/netdev.c\n@@ -1183,6 +1183,7 @@ static void e1000e_tx_hwtstamp_work(struct work_struct *work)\n \tstruct e1000_hw *hw = &adapter->hw;\n \n \tif (er32(TSYNCTXCTL) & E1000_TSYNCTXCTL_VALID) {\n+\t\tstruct sk_buff *skb = adapter->tx_hwtstamp_skb;\n \t\tstruct skb_shared_hwtstamps shhwtstamps;\n \t\tu64 txstmp;\n \n@@ -1191,9 +1192,14 @@ static void e1000e_tx_hwtstamp_work(struct work_struct *work)\n \n \t\te1000e_systim_to_hwtstamp(adapter, &shhwtstamps, txstmp);\n \n-\t\tskb_tstamp_tx(adapter->tx_hwtstamp_skb, &shhwtstamps);\n-\t\tdev_kfree_skb_any(adapter->tx_hwtstamp_skb);\n+\t\t/* Clear the global tx_hwtstamp_skb pointer and force writes\n+\t\t * prior to notifying the stack of a Tx timestamp.\n+\t\t */\n \t\tadapter->tx_hwtstamp_skb = NULL;\n+\t\twmb(); /* force write prior to skb_tstamp_tx */\n+\n+\t\tskb_tstamp_tx(skb, &shhwtstamps);\n+\t\tdev_kfree_skb_any(skb);\n \t} else if (time_after(jiffies, adapter->tx_hwtstamp_start\n \t\t\t + adapter->tx_timeout_factor * HZ)) {\n \t\tdev_kfree_skb_any(adapter->tx_hwtstamp_skb);\ndiff --git a/drivers/net/ethernet/intel/i40e/i40e_ptp.c b/drivers/net/ethernet/intel/i40e/i40e_ptp.c\nindex 18c1cc08da97..5a4854e9fd29 100644\n--- a/drivers/net/ethernet/intel/i40e/i40e_ptp.c\n+++ b/drivers/net/ethernet/intel/i40e/i40e_ptp.c\n@@ -338,6 +338,7 @@ void i40e_ptp_rx_hang(struct i40e_vsi *vsi)\n void i40e_ptp_tx_hwtstamp(struct i40e_pf *pf)\n {\n \tstruct skb_shared_hwtstamps shhwtstamps;\n+\tstruct sk_buff *skb = pf->ptp_tx_skb;\n \tstruct i40e_hw *hw = &pf->hw;\n \tu32 hi, lo;\n \tu64 ns;\n@@ -353,12 +354,19 @@ void i40e_ptp_tx_hwtstamp(struct i40e_pf *pf)\n \thi = rd32(hw, I40E_PRTTSYN_TXTIME_H);\n \n \tns = (((u64)hi) << 32) | lo;\n-\n \ti40e_ptp_convert_to_hwtstamp(&shhwtstamps, ns);\n-\tskb_tstamp_tx(pf->ptp_tx_skb, &shhwtstamps);\n-\tdev_kfree_skb_any(pf->ptp_tx_skb);\n+\n+\t/* Clear the bit lock as soon as possible after reading the register,\n+\t * and prior to notifying the stack via skb_tstamp_tx(). Otherwise\n+\t * applications might wake up and attempt to request another transmit\n+\t * timestamp prior to the bit lock being cleared.\n+\t */\n \tpf->ptp_tx_skb = NULL;\n \tclear_bit_unlock(__I40E_PTP_TX_IN_PROGRESS, pf->state);\n+\n+\t/* Notify the stack and free the skb after we've unlocked */\n+\tskb_tstamp_tx(skb, &shhwtstamps);\n+\tdev_kfree_skb_any(skb);\n }\n \n /**\ndiff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c\nindex 7a3fd4d74592..a2da5738bfb5 100644\n--- a/drivers/net/ethernet/intel/igb/igb_ptp.c\n+++ b/drivers/net/ethernet/intel/igb/igb_ptp.c\n@@ -721,6 +721,7 @@ void igb_ptp_rx_hang(struct igb_adapter *adapter)\n **/\n static void igb_ptp_tx_hwtstamp(struct igb_adapter *adapter)\n {\n+\tstruct sk_buff *skb = adapter->ptp_tx_skb;\n \tstruct e1000_hw *hw = &adapter->hw;\n \tstruct skb_shared_hwtstamps shhwtstamps;\n \tu64 regval;\n@@ -748,10 +749,17 @@ static void igb_ptp_tx_hwtstamp(struct igb_adapter *adapter)\n \tshhwtstamps.hwtstamp =\n \t\tktime_add_ns(shhwtstamps.hwtstamp, adjust);\n \n-\tskb_tstamp_tx(adapter->ptp_tx_skb, &shhwtstamps);\n-\tdev_kfree_skb_any(adapter->ptp_tx_skb);\n+\t/* Clear the lock early before calling skb_tstamp_tx so that\n+\t * applications are not woken up before the lock bit is clear. We use\n+\t * a copy of the skb pointer to ensure other threads can't change it\n+\t * while we're notifying the stack.\n+\t */\n \tadapter->ptp_tx_skb = NULL;\n \tclear_bit_unlock(__IGB_PTP_TX_IN_PROGRESS, &adapter->state);\n+\n+\t/* Notify the stack and free the skb after we've unlocked */\n+\tskb_tstamp_tx(skb, &shhwtstamps);\n+\tdev_kfree_skb_any(skb);\n }\n \n /**\ndiff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c\nindex ef0635e0918c..9270e6f4fcff 100644\n--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c\n+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c\n@@ -672,17 +672,26 @@ static void ixgbe_ptp_clear_tx_timestamp(struct ixgbe_adapter *adapter)\n */\n static void ixgbe_ptp_tx_hwtstamp(struct ixgbe_adapter *adapter)\n {\n+\tstruct sk_buff *skb = adapter->ptp_tx_skb;\n \tstruct ixgbe_hw *hw = &adapter->hw;\n \tstruct skb_shared_hwtstamps shhwtstamps;\n \tu64 regval = 0;\n \n \tregval |= (u64)IXGBE_READ_REG(hw, IXGBE_TXSTMPL);\n \tregval |= (u64)IXGBE_READ_REG(hw, IXGBE_TXSTMPH) << 32;\n-\n \tixgbe_ptp_convert_to_hwtstamp(adapter, &shhwtstamps, regval);\n-\tskb_tstamp_tx(adapter->ptp_tx_skb, &shhwtstamps);\n \n-\tixgbe_ptp_clear_tx_timestamp(adapter);\n+\t/* Handle cleanup of the ptp_tx_skb ourselves, and unlock the state\n+\t * bit prior to notifying the stack via skb_tstamp_tx(). This prevents\n+\t * well behaved applications from attempting to timestamp again prior\n+\t * to the lock bit being clear.\n+\t */\n+\tadapter->ptp_tx_skb = NULL;\n+\tclear_bit_unlock(__IXGBE_PTP_TX_IN_PROGRESS, &adapter->state);\n+\n+\t/* Notify the stack and then free the skb after we've unlocked */\n+\tskb_tstamp_tx(skb, &shhwtstamps);\n+\tdev_kfree_skb_any(skb);\n }\n \n /**\n", "prefixes": [ "v3", "1/5" ] }