[PATCH 0/7] [RESEND] [net] intel: Use smp_rmb rather than read_barrier_depends

[PATCH 0/7] [RESEND] [net] intel: Use smp_rmb rather than read_barrier_depends

[Brian King]

Resending as the first attempt is not showing up in the list archive.

This patch converts several network drivers to use smp_rmb
rather than read_barrier_depends. The initial issue was
discovered with ixgbe on a Power machine which resulted
in skb list corruption due to fetching a stale skb pointer.
More details can be found in the ixgbe patch description.

Brian King (7):
  ixgbe: Fix skb list corruption on Power systems
  i40e: Use smp_rmb rather than read_barrier_depends
  ixgbevf: Use smp_rmb rather than read_barrier_depends
  igbvf: Use smp_rmb rather than read_barrier_depends
  igb: Use smp_rmb rather than read_barrier_depends
  fm10k: Use smp_rmb rather than read_barrier_depends
  i40evf: Use smp_rmb rather than read_barrier_depends

 drivers/net/ethernet/intel/fm10k/fm10k_main.c     | 2 +-
 drivers/net/ethernet/intel/i40e/i40e_main.c       | 2 +-
 drivers/net/ethernet/intel/i40e/i40e_txrx.c       | 2 +-
 drivers/net/ethernet/intel/i40evf/i40e_txrx.c     | 2 +-
 drivers/net/ethernet/intel/igb/igb_main.c         | 2 +-
 drivers/net/ethernet/intel/igbvf/netdev.c         | 2 +-
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c     | 3 ++-
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 2 +-
 8 files changed, 9 insertions(+), 8 deletions(-)

-- 
1.8.3.1

[Intel-wired-lan] [PATCH 0/7] [RESEND] [net] intel: Use smp_rmb rather than read_barrier_depends

[Brian King]

Resending as the first attempt is not showing up in the list archive.

This patch converts several network drivers to use smp_rmb
rather than read_barrier_depends. The initial issue was
discovered with ixgbe on a Power machine which resulted
in skb list corruption due to fetching a stale skb pointer.
More details can be found in the ixgbe patch description.

Brian King (7):
  ixgbe: Fix skb list corruption on Power systems
  i40e: Use smp_rmb rather than read_barrier_depends
  ixgbevf: Use smp_rmb rather than read_barrier_depends
  igbvf: Use smp_rmb rather than read_barrier_depends
  igb: Use smp_rmb rather than read_barrier_depends
  fm10k: Use smp_rmb rather than read_barrier_depends
  i40evf: Use smp_rmb rather than read_barrier_depends

 drivers/net/ethernet/intel/fm10k/fm10k_main.c     | 2 +-
 drivers/net/ethernet/intel/i40e/i40e_main.c       | 2 +-
 drivers/net/ethernet/intel/i40e/i40e_txrx.c       | 2 +-
 drivers/net/ethernet/intel/i40evf/i40e_txrx.c     | 2 +-
 drivers/net/ethernet/intel/igb/igb_main.c         | 2 +-
 drivers/net/ethernet/intel/igbvf/netdev.c         | 2 +-
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c     | 3 ++-
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 2 +-
 8 files changed, 9 insertions(+), 8 deletions(-)

-- 
1.8.3.1

[PATCH 1/7] ixgbe: Fix skb list corruption on Power systems

[Brian King]

This patch fixes an issue seen on Power systems with ixgbe which results
in skb list corruption and an eventual kernel oops. The following is what
was observed:

CPU 1                                   CPU2
============================            ============================
1: ixgbe_xmit_frame_ring                ixgbe_clean_tx_irq
2:  first->skb = skb                     eop_desc = tx_buffer->next_to_watch
3:  ixgbe_tx_map                         read_barrier_depends()
4:   wmb                                 check adapter written status bit
5:   first->next_to_watch = tx_desc      napi_consume_skb(tx_buffer->skb ..);
6:   writel(i, tx_ring->tail);

The read_barrier_depends is insufficient to ensure that tx_buffer->skb does not
get loaded prior to tx_buffer->next_to_watch, which then results in loading
a stale skb pointer, since we aren't zeroing it, like is done in other
similar code in other networking drivers. This patch addresses both of these
issues, replacing the read_barrier_depends with an smp_rmb, which will ensure
the load of tx_buffer->skb will not occur until after the load from
tx_buffer->next_to_watch. Secondly, it zeroes tx_buffer->skb to make this
consistent with other Intel ethernet drivers and elso ensure we don't leave
a stale skb pointer sitting around.

Cc: stable<stable@vger.kernel.org>
Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 6d5f31e..4d8c7bb 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -1192,7 +1192,7 @@ static bool ixgbe_clean_tx_irq(struct ixgbe_q_vector *q_vector,
 			break;
 
 		/* prevent any other reads prior to eop_desc */
-		read_barrier_depends();
+		smp_rmb();
 
 		/* if DD is not set pending work has not been completed */
 		if (!(eop_desc->wb.status & cpu_to_le32(IXGBE_TXD_STAT_DD)))
@@ -1218,6 +1218,7 @@ static bool ixgbe_clean_tx_irq(struct ixgbe_q_vector *q_vector,
 				 DMA_TO_DEVICE);
 
 		/* clear tx_buffer data */
+		tx_buffer->skb = NULL;
 		dma_unmap_len_set(tx_buffer, len, 0);
 
 		/* unmap remaining buffers */
-- 
1.8.3.1

[Intel-wired-lan] [PATCH 1/7] ixgbe: Fix skb list corruption on Power systems

[Brian King]

This patch fixes an issue seen on Power systems with ixgbe which results
in skb list corruption and an eventual kernel oops. The following is what
was observed:

CPU 1                                   CPU2
============================            ============================
1: ixgbe_xmit_frame_ring                ixgbe_clean_tx_irq
2:  first->skb = skb                     eop_desc = tx_buffer->next_to_watch
3:  ixgbe_tx_map                         read_barrier_depends()
4:   wmb                                 check adapter written status bit
5:   first->next_to_watch = tx_desc      napi_consume_skb(tx_buffer->skb ..);
6:   writel(i, tx_ring->tail);

The read_barrier_depends is insufficient to ensure that tx_buffer->skb does not
get loaded prior to tx_buffer->next_to_watch, which then results in loading
a stale skb pointer, since we aren't zeroing it, like is done in other
similar code in other networking drivers. This patch addresses both of these
issues, replacing the read_barrier_depends with an smp_rmb, which will ensure
the load of tx_buffer->skb will not occur until after the load from
tx_buffer->next_to_watch. Secondly, it zeroes tx_buffer->skb to make this
consistent with other Intel ethernet drivers and elso ensure we don't leave
a stale skb pointer sitting around.

Cc: stable<stable@vger.kernel.org>
Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 6d5f31e..4d8c7bb 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -1192,7 +1192,7 @@ static bool ixgbe_clean_tx_irq(struct ixgbe_q_vector *q_vector,
 			break;
 
 		/* prevent any other reads prior to eop_desc */
-		read_barrier_depends();
+		smp_rmb();
 
 		/* if DD is not set pending work has not been completed */
 		if (!(eop_desc->wb.status & cpu_to_le32(IXGBE_TXD_STAT_DD)))
@@ -1218,6 +1218,7 @@ static bool ixgbe_clean_tx_irq(struct ixgbe_q_vector *q_vector,
 				 DMA_TO_DEVICE);
 
 		/* clear tx_buffer data */
+		tx_buffer->skb = NULL;
 		dma_unmap_len_set(tx_buffer, len, 0);
 
 		/* unmap remaining buffers */
-- 
1.8.3.1

[PATCH 2/7] i40e: Use smp_rmb rather than read_barrier_depends

[Brian King]

The original issue being fixed in this patch was seen with the ixgbe
driver, but the same issue exists with i40e as well, as the code is
very similar. read_barrier_depends is not sufficient to ensure
loads following it are not speculatively loaded out of order
by the CPU, which can result in stale data being loaded, causing
potential system crashes.

Cc: stable<stable@vger.kernel.org>
Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
---
 drivers/net/ethernet/intel/i40e/i40e_main.c | 2 +-
 drivers/net/ethernet/intel/i40e/i40e_txrx.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 6498da8..ea20aac 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -3760,7 +3760,7 @@ static bool i40e_clean_fdir_tx_irq(struct i40e_ring *tx_ring, int budget)
 			break;
 
 		/* prevent any other reads prior to eop_desc */
-		read_barrier_depends();
+		smp_rmb();
 
 		/* if the descriptor isn't done, no work yet to do */
 		if (!(eop_desc->cmd_type_offset_bsz &
diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
index 120c68f..3c07ff1 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
@@ -759,7 +759,7 @@ static bool i40e_clean_tx_irq(struct i40e_vsi *vsi,
 			break;
 
 		/* prevent any other reads prior to eop_desc */
-		read_barrier_depends();
+		smp_rmb();
 
 		i40e_trace(clean_tx_irq, tx_ring, tx_desc, tx_buf);
 		/* we have caught up to head, no work left to do */
-- 
1.8.3.1

[Intel-wired-lan] [PATCH 2/7] i40e: Use smp_rmb rather than read_barrier_depends

[Brian King]

The original issue being fixed in this patch was seen with the ixgbe
driver, but the same issue exists with i40e as well, as the code is
very similar. read_barrier_depends is not sufficient to ensure
loads following it are not speculatively loaded out of order
by the CPU, which can result in stale data being loaded, causing
potential system crashes.

Cc: stable<stable@vger.kernel.org>
Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
---
 drivers/net/ethernet/intel/i40e/i40e_main.c | 2 +-
 drivers/net/ethernet/intel/i40e/i40e_txrx.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 6498da8..ea20aac 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -3760,7 +3760,7 @@ static bool i40e_clean_fdir_tx_irq(struct i40e_ring *tx_ring, int budget)
 			break;
 
 		/* prevent any other reads prior to eop_desc */
-		read_barrier_depends();
+		smp_rmb();
 
 		/* if the descriptor isn't done, no work yet to do */
 		if (!(eop_desc->cmd_type_offset_bsz &
diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
index 120c68f..3c07ff1 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
@@ -759,7 +759,7 @@ static bool i40e_clean_tx_irq(struct i40e_vsi *vsi,
 			break;
 
 		/* prevent any other reads prior to eop_desc */
-		read_barrier_depends();
+		smp_rmb();
 
 		i40e_trace(clean_tx_irq, tx_ring, tx_desc, tx_buf);
 		/* we have caught up to head, no work left to do */
-- 
1.8.3.1

[PATCH 3/7] ixgbevf: Use smp_rmb rather than read_barrier_depends

[Brian King]

The original issue being fixed in this patch was seen with the ixgbe
driver, but the same issue exists with ixgbevf as well, as the code is
very similar. read_barrier_depends is not sufficient to ensure
loads following it are not speculatively loaded out of order
by the CPU, which can result in stale data being loaded, causing
potential system crashes.

Cc: stable<stable@vger.kernel.org>
Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
---
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
index 032f8ac..90ecc4b 100644
--- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
+++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
@@ -326,7 +326,7 @@ static bool ixgbevf_clean_tx_irq(struct ixgbevf_q_vector *q_vector,
 			break;
 
 		/* prevent any other reads prior to eop_desc */
-		read_barrier_depends();
+		smp_rmb();
 
 		/* if DD is not set pending work has not been completed */
 		if (!(eop_desc->wb.status & cpu_to_le32(IXGBE_TXD_STAT_DD)))
-- 
1.8.3.1

[Intel-wired-lan] [PATCH 3/7] ixgbevf: Use smp_rmb rather than read_barrier_depends

[Brian King]

The original issue being fixed in this patch was seen with the ixgbe
driver, but the same issue exists with ixgbevf as well, as the code is
very similar. read_barrier_depends is not sufficient to ensure
loads following it are not speculatively loaded out of order
by the CPU, which can result in stale data being loaded, causing
potential system crashes.

Cc: stable<stable@vger.kernel.org>
Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
---
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
index 032f8ac..90ecc4b 100644
--- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
+++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
@@ -326,7 +326,7 @@ static bool ixgbevf_clean_tx_irq(struct ixgbevf_q_vector *q_vector,
 			break;
 
 		/* prevent any other reads prior to eop_desc */
-		read_barrier_depends();
+		smp_rmb();
 
 		/* if DD is not set pending work has not been completed */
 		if (!(eop_desc->wb.status & cpu_to_le32(IXGBE_TXD_STAT_DD)))
-- 
1.8.3.1

[PATCH 4/7] igbvf: Use smp_rmb rather than read_barrier_depends

[Brian King]

The original issue being fixed in this patch was seen with the ixgbe
driver, but the same issue exists with igbvf as well, as the code is
very similar. read_barrier_depends is not sufficient to ensure
loads following it are not speculatively loaded out of order
by the CPU, which can result in stale data being loaded, causing
potential system crashes.

Cc: stable<stable@vger.kernel.org>
Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
---
 drivers/net/ethernet/intel/igbvf/netdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igbvf/netdev.c b/drivers/net/ethernet/intel/igbvf/netdev.c
index 1ed5569..6f5888b 100644
--- a/drivers/net/ethernet/intel/igbvf/netdev.c
+++ b/drivers/net/ethernet/intel/igbvf/netdev.c
@@ -810,7 +810,7 @@ static bool igbvf_clean_tx_irq(struct igbvf_ring *tx_ring)
 			break;
 
 		/* prevent any other reads prior to eop_desc */
-		read_barrier_depends();
+		smp_rmb();
 
 		/* if DD is not set pending work has not been completed */
 		if (!(eop_desc->wb.status & cpu_to_le32(E1000_TXD_STAT_DD)))
-- 
1.8.3.1

[Intel-wired-lan] [PATCH 4/7] igbvf: Use smp_rmb rather than read_barrier_depends

[Brian King]

The original issue being fixed in this patch was seen with the ixgbe
driver, but the same issue exists with igbvf as well, as the code is
very similar. read_barrier_depends is not sufficient to ensure
loads following it are not speculatively loaded out of order
by the CPU, which can result in stale data being loaded, causing
potential system crashes.

Cc: stable<stable@vger.kernel.org>
Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
---
 drivers/net/ethernet/intel/igbvf/netdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igbvf/netdev.c b/drivers/net/ethernet/intel/igbvf/netdev.c
index 1ed5569..6f5888b 100644
--- a/drivers/net/ethernet/intel/igbvf/netdev.c
+++ b/drivers/net/ethernet/intel/igbvf/netdev.c
@@ -810,7 +810,7 @@ static bool igbvf_clean_tx_irq(struct igbvf_ring *tx_ring)
 			break;
 
 		/* prevent any other reads prior to eop_desc */
-		read_barrier_depends();
+		smp_rmb();
 
 		/* if DD is not set pending work has not been completed */
 		if (!(eop_desc->wb.status & cpu_to_le32(E1000_TXD_STAT_DD)))
-- 
1.8.3.1

[PATCH 5/7] igb: Use smp_rmb rather than read_barrier_depends

[Brian King]

The original issue being fixed in this patch was seen with the ixgbe
driver, but the same issue exists with igb as well, as the code is
very similar. read_barrier_depends is not sufficient to ensure
loads following it are not speculatively loaded out of order
by the CPU, which can result in stale data being loaded, causing
potential system crashes.

Cc: stable<stable@vger.kernel.org>
Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index ea69af2..b0031c5 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -6970,7 +6970,7 @@ static bool igb_clean_tx_irq(struct igb_q_vector *q_vector, int napi_budget)
 			break;
 
 		/* prevent any other reads prior to eop_desc */
-		read_barrier_depends();
+		smp_rmb();
 
 		/* if DD is not set pending work has not been completed */
 		if (!(eop_desc->wb.status & cpu_to_le32(E1000_TXD_STAT_DD)))
-- 
1.8.3.1

[Intel-wired-lan] [PATCH 5/7] igb: Use smp_rmb rather than read_barrier_depends

[Brian King]

The original issue being fixed in this patch was seen with the ixgbe
driver, but the same issue exists with igb as well, as the code is
very similar. read_barrier_depends is not sufficient to ensure
loads following it are not speculatively loaded out of order
by the CPU, which can result in stale data being loaded, causing
potential system crashes.

Cc: stable<stable@vger.kernel.org>
Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index ea69af2..b0031c5 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -6970,7 +6970,7 @@ static bool igb_clean_tx_irq(struct igb_q_vector *q_vector, int napi_budget)
 			break;
 
 		/* prevent any other reads prior to eop_desc */
-		read_barrier_depends();
+		smp_rmb();
 
 		/* if DD is not set pending work has not been completed */
 		if (!(eop_desc->wb.status & cpu_to_le32(E1000_TXD_STAT_DD)))
-- 
1.8.3.1

[PATCH 6/7] fm10k: Use smp_rmb rather than read_barrier_depends

[Brian King]

The original issue being fixed in this patch was seen with the ixgbe
driver, but the same issue exists with fm10k as well, as the code is
very similar. read_barrier_depends is not sufficient to ensure
loads following it are not speculatively loaded out of order
by the CPU, which can result in stale data being loaded, causing
potential system crashes.

Cc: stable<stable@vger.kernel.org>
Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
---
 drivers/net/ethernet/intel/fm10k/fm10k_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_main.c b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
index 9dffaba..103c0a7 100644
--- a/drivers/net/ethernet/intel/fm10k/fm10k_main.c
+++ b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
@@ -1229,7 +1229,7 @@ static bool fm10k_clean_tx_irq(struct fm10k_q_vector *q_vector,
 			break;
 
 		/* prevent any other reads prior to eop_desc */
-		read_barrier_depends();
+		smp_rmb();
 
 		/* if DD is not set pending work has not been completed */
 		if (!(eop_desc->flags & FM10K_TXD_FLAG_DONE))
-- 
1.8.3.1

[Intel-wired-lan] [PATCH 6/7] fm10k: Use smp_rmb rather than read_barrier_depends

[Brian King]

The original issue being fixed in this patch was seen with the ixgbe
driver, but the same issue exists with fm10k as well, as the code is
very similar. read_barrier_depends is not sufficient to ensure
loads following it are not speculatively loaded out of order
by the CPU, which can result in stale data being loaded, causing
potential system crashes.

Cc: stable<stable@vger.kernel.org>
Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
---
 drivers/net/ethernet/intel/fm10k/fm10k_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_main.c b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
index 9dffaba..103c0a7 100644
--- a/drivers/net/ethernet/intel/fm10k/fm10k_main.c
+++ b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
@@ -1229,7 +1229,7 @@ static bool fm10k_clean_tx_irq(struct fm10k_q_vector *q_vector,
 			break;
 
 		/* prevent any other reads prior to eop_desc */
-		read_barrier_depends();
+		smp_rmb();
 
 		/* if DD is not set pending work has not been completed */
 		if (!(eop_desc->flags & FM10K_TXD_FLAG_DONE))
-- 
1.8.3.1

[PATCH 7/7] i40evf: Use smp_rmb rather than read_barrier_depends

[Brian King]

The original issue being fixed in this patch was seen with the ixgbe
driver, but the same issue exists with i40evf as well, as the code is
very similar. read_barrier_depends is not sufficient to ensure
loads following it are not speculatively loaded out of order
by the CPU, which can result in stale data being loaded, causing
potential system crashes.

Cc: stable<stable@vger.kernel.org>
Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
---
 drivers/net/ethernet/intel/i40evf/i40e_txrx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/i40evf/i40e_txrx.c b/drivers/net/ethernet/intel/i40evf/i40e_txrx.c
index c32c624..07a4e6e 100644
--- a/drivers/net/ethernet/intel/i40evf/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40evf/i40e_txrx.c
@@ -179,7 +179,7 @@ static bool i40e_clean_tx_irq(struct i40e_vsi *vsi,
 			break;
 
 		/* prevent any other reads prior to eop_desc */
-		read_barrier_depends();
+		smp_rmb();
 
 		i40e_trace(clean_tx_irq, tx_ring, tx_desc, tx_buf);
 		/* if the descriptor isn't done, no work yet to do */
-- 
1.8.3.1

[Intel-wired-lan] [PATCH 7/7] i40evf: Use smp_rmb rather than read_barrier_depends

[Brian King]

The original issue being fixed in this patch was seen with the ixgbe
driver, but the same issue exists with i40evf as well, as the code is
very similar. read_barrier_depends is not sufficient to ensure
loads following it are not speculatively loaded out of order
by the CPU, which can result in stale data being loaded, causing
potential system crashes.

Cc: stable<stable@vger.kernel.org>
Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
---
 drivers/net/ethernet/intel/i40evf/i40e_txrx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/i40evf/i40e_txrx.c b/drivers/net/ethernet/intel/i40evf/i40e_txrx.c
index c32c624..07a4e6e 100644
--- a/drivers/net/ethernet/intel/i40evf/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40evf/i40e_txrx.c
@@ -179,7 +179,7 @@ static bool i40e_clean_tx_irq(struct i40e_vsi *vsi,
 			break;
 
 		/* prevent any other reads prior to eop_desc */
-		read_barrier_depends();
+		smp_rmb();
 
 		i40e_trace(clean_tx_irq, tx_ring, tx_desc, tx_buf);
 		/* if the descriptor isn't done, no work yet to do */
-- 
1.8.3.1

Re: [Intel-wired-lan] [PATCH 0/7] [RESEND] [net] intel: Use smp_rmb rather than read_barrier_depends

[Jesse Brandeburg]

On Thu, 16 Nov 2017 09:37:48 -0600
Brian King <brking@linux.vnet.ibm.com> wrote:

> Resending as the first attempt is not showing up in the list archive.
> 
> This patch converts several network drivers to use smp_rmb
> rather than read_barrier_depends. The initial issue was
> discovered with ixgbe on a Power machine which resulted
> in skb list corruption due to fetching a stale skb pointer.
> More details can be found in the ixgbe patch description.

Thanks for the fix Brian, I bet it was a tough debug.

The only users in the entire kernel of read_barrier_depends() (not
smp_read_barrier_depends) are the Intel network drivers.

Wouldn't it be better for power to just fix read_barrier_depends to do
the right thing on power? The question I'm not sure of the answer to is:
Is it really the wrong barrier to be using or is the implementation in
the kernel powerpc wrong?

So I think the right thing might actually to be to:
Fix arch powerpc read_barrier_depends to not be a noop, as the
semantics of the read_barrier_depends seems to be sufficient to solve
this problem, but it seems not to work for powerpc?

Alex Duyck may have the most clarity on this problem, and possible
solutions so I made sure to CC him.

[Intel-wired-lan] [PATCH 0/7] [RESEND] [net] intel: Use smp_rmb rather than read_barrier_depends

[Jesse Brandeburg]

On Thu, 16 Nov 2017 09:37:48 -0600
Brian King <brking@linux.vnet.ibm.com> wrote:

> Resending as the first attempt is not showing up in the list archive.
> 
> This patch converts several network drivers to use smp_rmb
> rather than read_barrier_depends. The initial issue was
> discovered with ixgbe on a Power machine which resulted
> in skb list corruption due to fetching a stale skb pointer.
> More details can be found in the ixgbe patch description.

Thanks for the fix Brian, I bet it was a tough debug.

The only users in the entire kernel of read_barrier_depends() (not
smp_read_barrier_depends) are the Intel network drivers.

Wouldn't it be better for power to just fix read_barrier_depends to do
the right thing on power? The question I'm not sure of the answer to is:
Is it really the wrong barrier to be using or is the implementation in
the kernel powerpc wrong?

So I think the right thing might actually to be to:
Fix arch powerpc read_barrier_depends to not be a noop, as the
semantics of the read_barrier_depends seems to be sufficient to solve
this problem, but it seems not to work for powerpc?

Alex Duyck may have the most clarity on this problem, and possible
solutions so I made sure to CC him.

Re: [Intel-wired-lan] [PATCH 0/7] [RESEND] [net] intel: Use smp_rmb rather than read_barrier_depends

[Brian King]

On 11/16/2017 01:33 PM, Jesse Brandeburg wrote:
> On Thu, 16 Nov 2017 09:37:48 -0600
> Brian King <brking@linux.vnet.ibm.com> wrote:
> 
>> Resending as the first attempt is not showing up in the list archive.
>>
>> This patch converts several network drivers to use smp_rmb
>> rather than read_barrier_depends. The initial issue was
>> discovered with ixgbe on a Power machine which resulted
>> in skb list corruption due to fetching a stale skb pointer.
>> More details can be found in the ixgbe patch description.
> 
> Thanks for the fix Brian, I bet it was a tough debug.
> 
> The only users in the entire kernel of read_barrier_depends() (not
> smp_read_barrier_depends) are the Intel network drivers.
> 
> Wouldn't it be better for power to just fix read_barrier_depends to do
> the right thing on power? The question I'm not sure of the answer to is:
> Is it really the wrong barrier to be using or is the implementation in
> the kernel powerpc wrong?
> 
> So I think the right thing might actually to be to:
> Fix arch powerpc read_barrier_depends to not be a noop, as the
> semantics of the read_barrier_depends seems to be sufficient to solve
> this problem, but it seems not to work for powerpc?

Jesse,

Thanks for the quick response.

Cc'ing linuxppc-dev as well. 

I did think about changing the powerpc definition of read_barrier_depends,
but after reading up on that barrier, decided it was not the correct barrier
to be used in this context. Here is some good historical background on
read_barrier_depends that I found, along with an example.

https://lwn.net/Articles/5159/

Since there is no data-dependency in the code in question here, I think
the smp_rmb is the proper barrier to use.

For background, the code in question looks like this:

CPU 1                                   CPU2
============================            ============================
1: ixgbe_xmit_frame_ring                ixgbe_clean_tx_irq
2:  first->skb = skb                     eop_desc = tx_buffer->next_to_watch
                                         if (!eop_desc)
                                             break;
3:  ixgbe_tx_map                         read_barrier_depends()
                                         if (!(eop_desc->wb.status) ... )
                                             break;
4:   wmb                                 
5:   first->next_to_watch = tx_desc      napi_consume_skb(tx_buffer->skb ..);
6:   writel(i, tx_ring->tail);

What we see on powerpc is that tx_buffer->skb on CPU2 is getting loaded
prior to tx_buffer->next_to_watch. Changing the read_barrier_depends
to a smp_rmb solves this and prevents us from dereferencing old pointer.

-Brian


-- 
Brian King
Power Linux I/O
IBM Linux Technology Center

[Intel-wired-lan] [PATCH 0/7] [RESEND] [net] intel: Use smp_rmb rather than read_barrier_depends

[Brian King]

On 11/16/2017 01:33 PM, Jesse Brandeburg wrote:
> On Thu, 16 Nov 2017 09:37:48 -0600
> Brian King <brking@linux.vnet.ibm.com> wrote:
> 
>> Resending as the first attempt is not showing up in the list archive.
>>
>> This patch converts several network drivers to use smp_rmb
>> rather than read_barrier_depends. The initial issue was
>> discovered with ixgbe on a Power machine which resulted
>> in skb list corruption due to fetching a stale skb pointer.
>> More details can be found in the ixgbe patch description.
> 
> Thanks for the fix Brian, I bet it was a tough debug.
> 
> The only users in the entire kernel of read_barrier_depends() (not
> smp_read_barrier_depends) are the Intel network drivers.
> 
> Wouldn't it be better for power to just fix read_barrier_depends to do
> the right thing on power? The question I'm not sure of the answer to is:
> Is it really the wrong barrier to be using or is the implementation in
> the kernel powerpc wrong?
> 
> So I think the right thing might actually to be to:
> Fix arch powerpc read_barrier_depends to not be a noop, as the
> semantics of the read_barrier_depends seems to be sufficient to solve
> this problem, but it seems not to work for powerpc?

Jesse,

Thanks for the quick response.

Cc'ing linuxppc-dev as well. 

I did think about changing the powerpc definition of read_barrier_depends,
but after reading up on that barrier, decided it was not the correct barrier
to be used in this context. Here is some good historical background on
read_barrier_depends that I found, along with an example.

https://lwn.net/Articles/5159/

Since there is no data-dependency in the code in question here, I think
the smp_rmb is the proper barrier to use.

For background, the code in question looks like this:

CPU 1                                   CPU2
============================            ============================
1: ixgbe_xmit_frame_ring                ixgbe_clean_tx_irq
2:  first->skb = skb                     eop_desc = tx_buffer->next_to_watch
                                         if (!eop_desc)
                                             break;
3:  ixgbe_tx_map                         read_barrier_depends()
                                         if (!(eop_desc->wb.status) ... )
                                             break;
4:   wmb                                 
5:   first->next_to_watch = tx_desc      napi_consume_skb(tx_buffer->skb ..);
6:   writel(i, tx_ring->tail);

What we see on powerpc is that tx_buffer->skb on CPU2 is getting loaded
prior to tx_buffer->next_to_watch. Changing the read_barrier_depends
to a smp_rmb solves this and prevents us from dereferencing old pointer.

-Brian


-- 
Brian King
Power Linux I/O
IBM Linux Technology Center

Re: [Intel-wired-lan] [PATCH 0/7] [RESEND] [net] intel: Use smp_rmb rather than read_barrier_depends

[Duyck, Alexander H]

On Thu, 2017-11-16 at 14:03 -0600, Brian King wrote:
> On 11/16/2017 01:33 PM, Jesse Brandeburg wrote:
> > On Thu, 16 Nov 2017 09:37:48 -0600
> > Brian King <brking@linux.vnet.ibm.com> wrote:
> > 
> > > Resending as the first attempt is not showing up in the list archive.
> > > 
> > > This patch converts several network drivers to use smp_rmb
> > > rather than read_barrier_depends. The initial issue was
> > > discovered with ixgbe on a Power machine which resulted
> > > in skb list corruption due to fetching a stale skb pointer.
> > > More details can be found in the ixgbe patch description.
> > 
> > Thanks for the fix Brian, I bet it was a tough debug.
> > 
> > The only users in the entire kernel of read_barrier_depends() (not
> > smp_read_barrier_depends) are the Intel network drivers.
> > 
> > Wouldn't it be better for power to just fix read_barrier_depends to do
> > the right thing on power? The question I'm not sure of the answer to is:
> > Is it really the wrong barrier to be using or is the implementation in
> > the kernel powerpc wrong?
> > 
> > So I think the right thing might actually to be to:
> > Fix arch powerpc read_barrier_depends to not be a noop, as the
> > semantics of the read_barrier_depends seems to be sufficient to solve
> > this problem, but it seems not to work for powerpc?
> 
> Jesse,
> 
> Thanks for the quick response.
> 
> Cc'ing linuxppc-dev as well. 
> 
> I did think about changing the powerpc definition of read_barrier_depends,
> but after reading up on that barrier, decided it was not the correct barrier
> to be used in this context. Here is some good historical background on
> read_barrier_depends that I found, along with an example.
> 
> https://lwn.net/Articles/5159/
> 
> Since there is no data-dependency in the code in question here, I think
> the smp_rmb is the proper barrier to use.
> 
> For background, the code in question looks like this:
> 
> CPU 1                                   CPU2
> ============================            ============================
> 1: ixgbe_xmit_frame_ring                ixgbe_clean_tx_irq
> 2:  first->skb = skb                     eop_desc = tx_buffer->next_to_watch
>                                          if (!eop_desc)
>                                              break;
> 3:  ixgbe_tx_map                         read_barrier_depends()
>                                          if (!(eop_desc->wb.status) ... )
>                                              break;
> 4:   wmb                                 
> 5:   first->next_to_watch = tx_desc      napi_consume_skb(tx_buffer->skb ..);
> 6:   writel(i, tx_ring->tail);
> 
> What we see on powerpc is that tx_buffer->skb on CPU2 is getting loaded
> prior to tx_buffer->next_to_watch. Changing the read_barrier_depends
> to a smp_rmb solves this and prevents us from dereferencing old pointer.
> 
> -Brian

So the barrier part I am okay with for all the drivers. I hadn't
accounted for the skb being read before next_to_watch. I was more
concerned about the descriptor ring versus buffer_info structure at the
time I made use of that.

The updates to clear tx_buffer->skb in ixgbe I am not okay with.
Basically the tell-tale sign for skb present is next_to_watch being
non-null. The extra writes add overhead and I want to avoid that at all
costs since I want to avoid as much bouncing between the xmit path and
the Tx clean-up as possible.

- Alex

Re: [Intel-wired-lan] [PATCH 0/7] [RESEND] [net] intel: Use smp_rmb rather than read_barrier_depends

[Duyck, Alexander H]

T24gVGh1LCAyMDE3LTExLTE2IGF0IDE0OjAzIC0wNjAwLCBCcmlhbiBLaW5nIHdyb3RlOg0KPiBP
biAxMS8xNi8yMDE3IDAxOjMzIFBNLCBKZXNzZSBCcmFuZGVidXJnIHdyb3RlOg0KPiA+IE9uIFRo
dSwgMTYgTm92IDIwMTcgMDk6Mzc6NDggLTA2MDANCj4gPiBCcmlhbiBLaW5nIDxicmtpbmdAbGlu
dXgudm5ldC5pYm0uY29tPiB3cm90ZToNCj4gPiANCj4gPiA+IFJlc2VuZGluZyBhcyB0aGUgZmly
c3QgYXR0ZW1wdCBpcyBub3Qgc2hvd2luZyB1cCBpbiB0aGUgbGlzdCBhcmNoaXZlLg0KPiA+ID4g
DQo+ID4gPiBUaGlzIHBhdGNoIGNvbnZlcnRzIHNldmVyYWwgbmV0d29yayBkcml2ZXJzIHRvIHVz
ZSBzbXBfcm1iDQo+ID4gPiByYXRoZXIgdGhhbiByZWFkX2JhcnJpZXJfZGVwZW5kcy4gVGhlIGlu
aXRpYWwgaXNzdWUgd2FzDQo+ID4gPiBkaXNjb3ZlcmVkIHdpdGggaXhnYmUgb24gYSBQb3dlciBt
YWNoaW5lIHdoaWNoIHJlc3VsdGVkDQo+ID4gPiBpbiBza2IgbGlzdCBjb3JydXB0aW9uIGR1ZSB0
byBmZXRjaGluZyBhIHN0YWxlIHNrYiBwb2ludGVyLg0KPiA+ID4gTW9yZSBkZXRhaWxzIGNhbiBi
ZSBmb3VuZCBpbiB0aGUgaXhnYmUgcGF0Y2ggZGVzY3JpcHRpb24uDQo+ID4gDQo+ID4gVGhhbmtz
IGZvciB0aGUgZml4IEJyaWFuLCBJIGJldCBpdCB3YXMgYSB0b3VnaCBkZWJ1Zy4NCj4gPiANCj4g
PiBUaGUgb25seSB1c2VycyBpbiB0aGUgZW50aXJlIGtlcm5lbCBvZiByZWFkX2JhcnJpZXJfZGVw
ZW5kcygpIChub3QNCj4gPiBzbXBfcmVhZF9iYXJyaWVyX2RlcGVuZHMpIGFyZSB0aGUgSW50ZWwg
bmV0d29yayBkcml2ZXJzLg0KPiA+IA0KPiA+IFdvdWxkbid0IGl0IGJlIGJldHRlciBmb3IgcG93
ZXIgdG8ganVzdCBmaXggcmVhZF9iYXJyaWVyX2RlcGVuZHMgdG8gZG8NCj4gPiB0aGUgcmlnaHQg
dGhpbmcgb24gcG93ZXI/IFRoZSBxdWVzdGlvbiBJJ20gbm90IHN1cmUgb2YgdGhlIGFuc3dlciB0
byBpczoNCj4gPiBJcyBpdCByZWFsbHkgdGhlIHdyb25nIGJhcnJpZXIgdG8gYmUgdXNpbmcgb3Ig
aXMgdGhlIGltcGxlbWVudGF0aW9uIGluDQo+ID4gdGhlIGtlcm5lbCBwb3dlcnBjIHdyb25nPw0K
PiA+IA0KPiA+IFNvIEkgdGhpbmsgdGhlIHJpZ2h0IHRoaW5nIG1pZ2h0IGFjdHVhbGx5IHRvIGJl
IHRvOg0KPiA+IEZpeCBhcmNoIHBvd2VycGMgcmVhZF9iYXJyaWVyX2RlcGVuZHMgdG8gbm90IGJl
IGEgbm9vcCwgYXMgdGhlDQo+ID4gc2VtYW50aWNzIG9mIHRoZSByZWFkX2JhcnJpZXJfZGVwZW5k
cyBzZWVtcyB0byBiZSBzdWZmaWNpZW50IHRvIHNvbHZlDQo+ID4gdGhpcyBwcm9ibGVtLCBidXQg
aXQgc2VlbXMgbm90IHRvIHdvcmsgZm9yIHBvd2VycGM/DQo+IA0KPiBKZXNzZSwNCj4gDQo+IFRo
YW5rcyBmb3IgdGhlIHF1aWNrIHJlc3BvbnNlLg0KPiANCj4gQ2MnaW5nIGxpbnV4cHBjLWRldiBh
cyB3ZWxsLiANCj4gDQo+IEkgZGlkIHRoaW5rIGFib3V0IGNoYW5naW5nIHRoZSBwb3dlcnBjIGRl
ZmluaXRpb24gb2YgcmVhZF9iYXJyaWVyX2RlcGVuZHMsDQo+IGJ1dCBhZnRlciByZWFkaW5nIHVw
IG9uIHRoYXQgYmFycmllciwgZGVjaWRlZCBpdCB3YXMgbm90IHRoZSBjb3JyZWN0IGJhcnJpZXIN
Cj4gdG8gYmUgdXNlZCBpbiB0aGlzIGNvbnRleHQuIEhlcmUgaXMgc29tZSBnb29kIGhpc3Rvcmlj
YWwgYmFja2dyb3VuZCBvbg0KPiByZWFkX2JhcnJpZXJfZGVwZW5kcyB0aGF0IEkgZm91bmQsIGFs
b25nIHdpdGggYW4gZXhhbXBsZS4NCj4gDQo+IGh0dHBzOi8vbHduLm5ldC9BcnRpY2xlcy81MTU5
Lw0KPiANCj4gU2luY2UgdGhlcmUgaXMgbm8gZGF0YS1kZXBlbmRlbmN5IGluIHRoZSBjb2RlIGlu
IHF1ZXN0aW9uIGhlcmUsIEkgdGhpbmsNCj4gdGhlIHNtcF9ybWIgaXMgdGhlIHByb3BlciBiYXJy
aWVyIHRvIHVzZS4NCj4gDQo+IEZvciBiYWNrZ3JvdW5kLCB0aGUgY29kZSBpbiBxdWVzdGlvbiBs
b29rcyBsaWtlIHRoaXM6DQo+IA0KPiBDUFUgMSAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgQ1BVMg0KPiA9PT09PT09PT09PT09PT09PT09PT09PT09PT09ICAgICAgICAgICAgPT09
PT09PT09PT09PT09PT09PT09PT09PT09PQ0KPiAxOiBpeGdiZV94bWl0X2ZyYW1lX3JpbmcgICAg
ICAgICAgICAgICAgaXhnYmVfY2xlYW5fdHhfaXJxDQo+IDI6ICBmaXJzdC0+c2tiID0gc2tiICAg
ICAgICAgICAgICAgICAgICAgZW9wX2Rlc2MgPSB0eF9idWZmZXItPm5leHRfdG9fd2F0Y2gNCj4g
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBpZiAoIWVvcF9kZXNjKQ0K
PiAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBicmVhazsNCj4g
MzogIGl4Z2JlX3R4X21hcCAgICAgICAgICAgICAgICAgICAgICAgICByZWFkX2JhcnJpZXJfZGVw
ZW5kcygpDQo+ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgaWYgKCEo
ZW9wX2Rlc2MtPndiLnN0YXR1cykgLi4uICkNCj4gICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgYnJlYWs7DQo+IDQ6ICAgd21iICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgDQo+IDU6ICAgZmlyc3QtPm5leHRfdG9fd2F0Y2ggPSB0eF9kZXNjICAgICAg
bmFwaV9jb25zdW1lX3NrYih0eF9idWZmZXItPnNrYiAuLik7DQo+IDY6ICAgd3JpdGVsKGksIHR4
X3JpbmctPnRhaWwpOw0KPiANCj4gV2hhdCB3ZSBzZWUgb24gcG93ZXJwYyBpcyB0aGF0IHR4X2J1
ZmZlci0+c2tiIG9uIENQVTIgaXMgZ2V0dGluZyBsb2FkZWQNCj4gcHJpb3IgdG8gdHhfYnVmZmVy
LT5uZXh0X3RvX3dhdGNoLiBDaGFuZ2luZyB0aGUgcmVhZF9iYXJyaWVyX2RlcGVuZHMNCj4gdG8g
YSBzbXBfcm1iIHNvbHZlcyB0aGlzIGFuZCBwcmV2ZW50cyB1cyBmcm9tIGRlcmVmZXJlbmNpbmcg
b2xkIHBvaW50ZXIuDQo+IA0KPiAtQnJpYW4NCg0KU28gdGhlIGJhcnJpZXIgcGFydCBJIGFtIG9r
YXkgd2l0aCBmb3IgYWxsIHRoZSBkcml2ZXJzLiBJIGhhZG4ndA0KYWNjb3VudGVkIGZvciB0aGUg
c2tiIGJlaW5nIHJlYWQgYmVmb3JlIG5leHRfdG9fd2F0Y2guIEkgd2FzIG1vcmUNCmNvbmNlcm5l
ZCBhYm91dCB0aGUgZGVzY3JpcHRvciByaW5nIHZlcnN1cyBidWZmZXJfaW5mbyBzdHJ1Y3R1cmUg
YXQgdGhlDQp0aW1lIEkgbWFkZSB1c2Ugb2YgdGhhdC4NCg0KVGhlIHVwZGF0ZXMgdG8gY2xlYXIg
dHhfYnVmZmVyLT5za2IgaW4gaXhnYmUgSSBhbSBub3Qgb2theSB3aXRoLg0KQmFzaWNhbGx5IHRo
ZSB0ZWxsLXRhbGUgc2lnbiBmb3Igc2tiIHByZXNlbnQgaXMgbmV4dF90b193YXRjaCBiZWluZw0K
bm9uLW51bGwuIFRoZSBleHRyYSB3cml0ZXMgYWRkIG92ZXJoZWFkIGFuZCBJIHdhbnQgdG8gYXZv
aWQgdGhhdCBhdCBhbGwNCmNvc3RzIHNpbmNlIEkgd2FudCB0byBhdm9pZCBhcyBtdWNoIGJvdW5j
aW5nIGJldHdlZW4gdGhlIHhtaXQgcGF0aCBhbmQNCnRoZSBUeCBjbGVhbi11cCBhcyBwb3NzaWJs
ZS4NCg0KLSBBbGV4DQoNCg==

[Intel-wired-lan] [PATCH 0/7] [RESEND] [net] intel: Use smp_rmb rather than read_barrier_depends

[Duyck, Alexander H]

On Thu, 2017-11-16 at 14:03 -0600, Brian King wrote:
> On 11/16/2017 01:33 PM, Jesse Brandeburg wrote:
> > On Thu, 16 Nov 2017 09:37:48 -0600
> > Brian King <brking@linux.vnet.ibm.com> wrote:
> > 
> > > Resending as the first attempt is not showing up in the list archive.
> > > 
> > > This patch converts several network drivers to use smp_rmb
> > > rather than read_barrier_depends. The initial issue was
> > > discovered with ixgbe on a Power machine which resulted
> > > in skb list corruption due to fetching a stale skb pointer.
> > > More details can be found in the ixgbe patch description.
> > 
> > Thanks for the fix Brian, I bet it was a tough debug.
> > 
> > The only users in the entire kernel of read_barrier_depends() (not
> > smp_read_barrier_depends) are the Intel network drivers.
> > 
> > Wouldn't it be better for power to just fix read_barrier_depends to do
> > the right thing on power? The question I'm not sure of the answer to is:
> > Is it really the wrong barrier to be using or is the implementation in
> > the kernel powerpc wrong?
> > 
> > So I think the right thing might actually to be to:
> > Fix arch powerpc read_barrier_depends to not be a noop, as the
> > semantics of the read_barrier_depends seems to be sufficient to solve
> > this problem, but it seems not to work for powerpc?
> 
> Jesse,
> 
> Thanks for the quick response.
> 
> Cc'ing linuxppc-dev as well. 
> 
> I did think about changing the powerpc definition of read_barrier_depends,
> but after reading up on that barrier, decided it was not the correct barrier
> to be used in this context. Here is some good historical background on
> read_barrier_depends that I found, along with an example.
> 
> https://lwn.net/Articles/5159/
> 
> Since there is no data-dependency in the code in question here, I think
> the smp_rmb is the proper barrier to use.
> 
> For background, the code in question looks like this:
> 
> CPU 1                                   CPU2
> ============================            ============================
> 1: ixgbe_xmit_frame_ring                ixgbe_clean_tx_irq
> 2:  first->skb = skb                     eop_desc = tx_buffer->next_to_watch
>                                          if (!eop_desc)
>                                              break;
> 3:  ixgbe_tx_map                         read_barrier_depends()
>                                          if (!(eop_desc->wb.status) ... )
>                                              break;
> 4:   wmb                                 
> 5:   first->next_to_watch = tx_desc      napi_consume_skb(tx_buffer->skb ..);
> 6:   writel(i, tx_ring->tail);
> 
> What we see on powerpc is that tx_buffer->skb on CPU2 is getting loaded
> prior to tx_buffer->next_to_watch. Changing the read_barrier_depends
> to a smp_rmb solves this and prevents us from dereferencing old pointer.
> 
> -Brian

So the barrier part I am okay with for all the drivers. I hadn't
accounted for the skb being read before next_to_watch. I was more
concerned about the descriptor ring versus buffer_info structure at the
time I made use of that.

The updates to clear tx_buffer->skb in ixgbe I am not okay with.
Basically the tell-tale sign for skb present is next_to_watch being
non-null. The extra writes add overhead and I want to avoid that at all
costs since I want to avoid as much bouncing between the xmit path and
the Tx clean-up as possible.

- Alex

Re: [Intel-wired-lan] [PATCH 0/7] [RESEND] [net] intel: Use smp_rmb rather than read_barrier_depends

[Jesse Brandeburg]

On Thu, 16 Nov 2017 14:03:02 -0600
Brian King <brking@linux.vnet.ibm.com> wrote:
> I did think about changing the powerpc definition of read_barrier_depends,
> but after reading up on that barrier, decided it was not the correct barrier
> to be used in this context. Here is some good historical background on
> read_barrier_depends that I found, along with an example.
> 
> https://lwn.net/Articles/5159/
> 
> Since there is no data-dependency in the code in question here, I think
> the smp_rmb is the proper barrier to use.

Hey Brian, thanks for the explanation, I'll agree with you and Alex
that the smb_rmb replacement is okay.  Does your test still pass
without the ->skb NULLs?

[Intel-wired-lan] [PATCH 0/7] [RESEND] [net] intel: Use smp_rmb rather than read_barrier_depends

[Jesse Brandeburg]

On Thu, 16 Nov 2017 14:03:02 -0600
Brian King <brking@linux.vnet.ibm.com> wrote:
> I did think about changing the powerpc definition of read_barrier_depends,
> but after reading up on that barrier, decided it was not the correct barrier
> to be used in this context. Here is some good historical background on
> read_barrier_depends that I found, along with an example.
> 
> https://lwn.net/Articles/5159/
> 
> Since there is no data-dependency in the code in question here, I think
> the smp_rmb is the proper barrier to use.

Hey Brian, thanks for the explanation, I'll agree with you and Alex
that the smb_rmb replacement is okay.  Does your test still pass
without the ->skb NULLs?

Re: [Intel-wired-lan] [PATCH 0/7] [RESEND] [net] intel: Use smp_rmb rather than read_barrier_depends

[Michael Ellerman]

Brian King <brking@linux.vnet.ibm.com> writes:

> On 11/16/2017 01:33 PM, Jesse Brandeburg wrote:
>> On Thu, 16 Nov 2017 09:37:48 -0600
>> Brian King <brking@linux.vnet.ibm.com> wrote:
>> 
>>> Resending as the first attempt is not showing up in the list archive.
>>>
>>> This patch converts several network drivers to use smp_rmb
>>> rather than read_barrier_depends. The initial issue was
>>> discovered with ixgbe on a Power machine which resulted
>>> in skb list corruption due to fetching a stale skb pointer.
>>> More details can be found in the ixgbe patch description.
>> 
>> Thanks for the fix Brian, I bet it was a tough debug.
>> 
>> The only users in the entire kernel of read_barrier_depends() (not
>> smp_read_barrier_depends) are the Intel network drivers.
>> 
>> Wouldn't it be better for power to just fix read_barrier_depends to do
>> the right thing on power? The question I'm not sure of the answer to is:
>> Is it really the wrong barrier to be using or is the implementation in
>> the kernel powerpc wrong?
>> 
>> So I think the right thing might actually to be to:
>> Fix arch powerpc read_barrier_depends to not be a noop, as the
>> semantics of the read_barrier_depends seems to be sufficient to solve
>> this problem, but it seems not to work for powerpc?
>
> Jesse,
>
> Thanks for the quick response.
>
> Cc'ing linuxppc-dev as well. 
>
> I did think about changing the powerpc definition of read_barrier_depends,
> but after reading up on that barrier, decided it was not the correct barrier
> to be used in this context. Here is some good historical background on
> read_barrier_depends that I found, along with an example.
>
> https://lwn.net/Articles/5159/
>
> Since there is no data-dependency in the code in question here, I think
> the smp_rmb is the proper barrier to use.

Yes I agree.

The read_barrier_depends() is correct to order the load of eop_desc and
then the dependent load of eop_desc->wb.status, but it's only required
or does anything on Alpha.

> For background, the code in question looks like this:
>
> CPU 1                                   CPU2
> ============================            ============================
> 1: ixgbe_xmit_frame_ring                ixgbe_clean_tx_irq
> 2:  first->skb = skb                     eop_desc = tx_buffer->next_to_watch
>                                          if (!eop_desc)
>                                              break;
> 3:  ixgbe_tx_map                         read_barrier_depends()
>                                          if (!(eop_desc->wb.status) ... )
>                                              break;
> 4:   wmb                                 
> 5:   first->next_to_watch = tx_desc      napi_consume_skb(tx_buffer->skb ..);
> 6:   writel(i, tx_ring->tail);
>
> What we see on powerpc is that tx_buffer->skb on CPU2 is getting loaded
> prior to tx_buffer->next_to_watch. Changing the read_barrier_depends
> to a smp_rmb solves this and prevents us from dereferencing old pointer.

Right. Given that read_barrier_depends() is a nop, there's nothing there
to order the load of tx_buffer->skb vs anything else.

If it's actually the load of tx_buffer->skb that's the issue then the
smp_rmb() should really be immediately prior to that, rather than where
the read_barrier_depends() currently is.

cheers

[Intel-wired-lan] [PATCH 0/7] [RESEND] [net] intel: Use smp_rmb rather than read_barrier_depends

[Michael Ellerman]

Brian King <brking@linux.vnet.ibm.com> writes:

> On 11/16/2017 01:33 PM, Jesse Brandeburg wrote:
>> On Thu, 16 Nov 2017 09:37:48 -0600
>> Brian King <brking@linux.vnet.ibm.com> wrote:
>> 
>>> Resending as the first attempt is not showing up in the list archive.
>>>
>>> This patch converts several network drivers to use smp_rmb
>>> rather than read_barrier_depends. The initial issue was
>>> discovered with ixgbe on a Power machine which resulted
>>> in skb list corruption due to fetching a stale skb pointer.
>>> More details can be found in the ixgbe patch description.
>> 
>> Thanks for the fix Brian, I bet it was a tough debug.
>> 
>> The only users in the entire kernel of read_barrier_depends() (not
>> smp_read_barrier_depends) are the Intel network drivers.
>> 
>> Wouldn't it be better for power to just fix read_barrier_depends to do
>> the right thing on power? The question I'm not sure of the answer to is:
>> Is it really the wrong barrier to be using or is the implementation in
>> the kernel powerpc wrong?
>> 
>> So I think the right thing might actually to be to:
>> Fix arch powerpc read_barrier_depends to not be a noop, as the
>> semantics of the read_barrier_depends seems to be sufficient to solve
>> this problem, but it seems not to work for powerpc?
>
> Jesse,
>
> Thanks for the quick response.
>
> Cc'ing linuxppc-dev as well. 
>
> I did think about changing the powerpc definition of read_barrier_depends,
> but after reading up on that barrier, decided it was not the correct barrier
> to be used in this context. Here is some good historical background on
> read_barrier_depends that I found, along with an example.
>
> https://lwn.net/Articles/5159/
>
> Since there is no data-dependency in the code in question here, I think
> the smp_rmb is the proper barrier to use.

Yes I agree.

The read_barrier_depends() is correct to order the load of eop_desc and
then the dependent load of eop_desc->wb.status, but it's only required
or does anything on Alpha.

> For background, the code in question looks like this:
>
> CPU 1                                   CPU2
> ============================            ============================
> 1: ixgbe_xmit_frame_ring                ixgbe_clean_tx_irq
> 2:  first->skb = skb                     eop_desc = tx_buffer->next_to_watch
>                                          if (!eop_desc)
>                                              break;
> 3:  ixgbe_tx_map                         read_barrier_depends()
>                                          if (!(eop_desc->wb.status) ... )
>                                              break;
> 4:   wmb                                 
> 5:   first->next_to_watch = tx_desc      napi_consume_skb(tx_buffer->skb ..);
> 6:   writel(i, tx_ring->tail);
>
> What we see on powerpc is that tx_buffer->skb on CPU2 is getting loaded
> prior to tx_buffer->next_to_watch. Changing the read_barrier_depends
> to a smp_rmb solves this and prevents us from dereferencing old pointer.

Right. Given that read_barrier_depends() is a nop, there's nothing there
to order the load of tx_buffer->skb vs anything else.

If it's actually the load of tx_buffer->skb that's the issue then the
smp_rmb() should really be immediately prior to that, rather than where
the read_barrier_depends() currently is.

cheers

Re: [Intel-wired-lan] [PATCH 0/7] [RESEND] [net] intel: Use smp_rmb rather than read_barrier_depends

[Brian King]

On 11/16/2017 04:57 PM, Michael Ellerman wrote:
> Brian King <brking@linux.vnet.ibm.com> writes:
> 
>> On 11/16/2017 01:33 PM, Jesse Brandeburg wrote:
>>> On Thu, 16 Nov 2017 09:37:48 -0600
>>> Brian King <brking@linux.vnet.ibm.com> wrote:
>>>
>>>> Resending as the first attempt is not showing up in the list archive.
>>>>
>>>> This patch converts several network drivers to use smp_rmb
>>>> rather than read_barrier_depends. The initial issue was
>>>> discovered with ixgbe on a Power machine which resulted
>>>> in skb list corruption due to fetching a stale skb pointer.
>>>> More details can be found in the ixgbe patch description.
>>>
>>> Thanks for the fix Brian, I bet it was a tough debug.
>>>
>>> The only users in the entire kernel of read_barrier_depends() (not
>>> smp_read_barrier_depends) are the Intel network drivers.
>>>
>>> Wouldn't it be better for power to just fix read_barrier_depends to do
>>> the right thing on power? The question I'm not sure of the answer to is:
>>> Is it really the wrong barrier to be using or is the implementation in
>>> the kernel powerpc wrong?
>>>
>>> So I think the right thing might actually to be to:
>>> Fix arch powerpc read_barrier_depends to not be a noop, as the
>>> semantics of the read_barrier_depends seems to be sufficient to solve
>>> this problem, but it seems not to work for powerpc?
>>
>> Jesse,
>>
>> Thanks for the quick response.
>>
>> Cc'ing linuxppc-dev as well. 
>>
>> I did think about changing the powerpc definition of read_barrier_depends,
>> but after reading up on that barrier, decided it was not the correct barrier
>> to be used in this context. Here is some good historical background on
>> read_barrier_depends that I found, along with an example.
>>
>> https://lwn.net/Articles/5159/
>>
>> Since there is no data-dependency in the code in question here, I think
>> the smp_rmb is the proper barrier to use.
> 
> Yes I agree.
> 
> The read_barrier_depends() is correct to order the load of eop_desc and
> then the dependent load of eop_desc->wb.status, but it's only required
> or does anything on Alpha.
> 
>> For background, the code in question looks like this:
>>
>> CPU 1                                   CPU2
>> ============================            ============================
>> 1: ixgbe_xmit_frame_ring                ixgbe_clean_tx_irq
>> 2:  first->skb = skb                     eop_desc = tx_buffer->next_to_watch
>>                                          if (!eop_desc)
>>                                              break;
>> 3:  ixgbe_tx_map                         read_barrier_depends()
>>                                          if (!(eop_desc->wb.status) ... )
>>                                              break;
>> 4:   wmb                                 
>> 5:   first->next_to_watch = tx_desc      napi_consume_skb(tx_buffer->skb ..);
>> 6:   writel(i, tx_ring->tail);
>>
>> What we see on powerpc is that tx_buffer->skb on CPU2 is getting loaded
>> prior to tx_buffer->next_to_watch. Changing the read_barrier_depends
>> to a smp_rmb solves this and prevents us from dereferencing old pointer.
> 
> Right. Given that read_barrier_depends() is a nop, there's nothing there
> to order the load of tx_buffer->skb vs anything else.
> 
> If it's actually the load of tx_buffer->skb that's the issue then the
> smp_rmb() should really be immediately prior to that, rather than where
> the read_barrier_depends() currently is.

Alex,

How would you like to proceed? read_barrier_depends is a noop on all archs
except alpha and blackfin. On those two archs, read_barrier_depends and
smp_rmb end up resulting in the same code. So, I can either:

1. Remove the setting of tx_buffer->skb to NULL to address your concern and proceed
with the rest of the patch set unchanged.

2. Leave the read_barrier_depends, as it is the right barrier to order the load
of eop_desc with respect to eop_desc->wb.status, and then *add* an smp_rmb in
the same code path to address the speculative load of the skb that I was running into.
This is arguably more pure from the perspective of the use of the different
barriers, but has the downside of additional overhead on alpha and blackfin.

Do you have a preference? 

Thanks,

Brian

-- 
Brian King
Power Linux I/O
IBM Linux Technology Center

[Intel-wired-lan] [PATCH 0/7] [RESEND] [net] intel: Use smp_rmb rather than read_barrier_depends

[Brian King]

On 11/16/2017 04:57 PM, Michael Ellerman wrote:
> Brian King <brking@linux.vnet.ibm.com> writes:
> 
>> On 11/16/2017 01:33 PM, Jesse Brandeburg wrote:
>>> On Thu, 16 Nov 2017 09:37:48 -0600
>>> Brian King <brking@linux.vnet.ibm.com> wrote:
>>>
>>>> Resending as the first attempt is not showing up in the list archive.
>>>>
>>>> This patch converts several network drivers to use smp_rmb
>>>> rather than read_barrier_depends. The initial issue was
>>>> discovered with ixgbe on a Power machine which resulted
>>>> in skb list corruption due to fetching a stale skb pointer.
>>>> More details can be found in the ixgbe patch description.
>>>
>>> Thanks for the fix Brian, I bet it was a tough debug.
>>>
>>> The only users in the entire kernel of read_barrier_depends() (not
>>> smp_read_barrier_depends) are the Intel network drivers.
>>>
>>> Wouldn't it be better for power to just fix read_barrier_depends to do
>>> the right thing on power? The question I'm not sure of the answer to is:
>>> Is it really the wrong barrier to be using or is the implementation in
>>> the kernel powerpc wrong?
>>>
>>> So I think the right thing might actually to be to:
>>> Fix arch powerpc read_barrier_depends to not be a noop, as the
>>> semantics of the read_barrier_depends seems to be sufficient to solve
>>> this problem, but it seems not to work for powerpc?
>>
>> Jesse,
>>
>> Thanks for the quick response.
>>
>> Cc'ing linuxppc-dev as well. 
>>
>> I did think about changing the powerpc definition of read_barrier_depends,
>> but after reading up on that barrier, decided it was not the correct barrier
>> to be used in this context. Here is some good historical background on
>> read_barrier_depends that I found, along with an example.
>>
>> https://lwn.net/Articles/5159/
>>
>> Since there is no data-dependency in the code in question here, I think
>> the smp_rmb is the proper barrier to use.
> 
> Yes I agree.
> 
> The read_barrier_depends() is correct to order the load of eop_desc and
> then the dependent load of eop_desc->wb.status, but it's only required
> or does anything on Alpha.
> 
>> For background, the code in question looks like this:
>>
>> CPU 1                                   CPU2
>> ============================            ============================
>> 1: ixgbe_xmit_frame_ring                ixgbe_clean_tx_irq
>> 2:  first->skb = skb                     eop_desc = tx_buffer->next_to_watch
>>                                          if (!eop_desc)
>>                                              break;
>> 3:  ixgbe_tx_map                         read_barrier_depends()
>>                                          if (!(eop_desc->wb.status) ... )
>>                                              break;
>> 4:   wmb                                 
>> 5:   first->next_to_watch = tx_desc      napi_consume_skb(tx_buffer->skb ..);
>> 6:   writel(i, tx_ring->tail);
>>
>> What we see on powerpc is that tx_buffer->skb on CPU2 is getting loaded
>> prior to tx_buffer->next_to_watch. Changing the read_barrier_depends
>> to a smp_rmb solves this and prevents us from dereferencing old pointer.
> 
> Right. Given that read_barrier_depends() is a nop, there's nothing there
> to order the load of tx_buffer->skb vs anything else.
> 
> If it's actually the load of tx_buffer->skb that's the issue then the
> smp_rmb() should really be immediately prior to that, rather than where
> the read_barrier_depends() currently is.

Alex,

How would you like to proceed? read_barrier_depends is a noop on all archs
except alpha and blackfin. On those two archs, read_barrier_depends and
smp_rmb end up resulting in the same code. So, I can either:

1. Remove the setting of tx_buffer->skb to NULL to address your concern and proceed
with the rest of the patch set unchanged.

2. Leave the read_barrier_depends, as it is the right barrier to order the load
of eop_desc with respect to eop_desc->wb.status, and then *add* an smp_rmb in
the same code path to address the speculative load of the skb that I was running into.
This is arguably more pure from the perspective of the use of the different
barriers, but has the downside of additional overhead on alpha and blackfin.

Do you have a preference? 

Thanks,

Brian

-- 
Brian King
Power Linux I/O
IBM Linux Technology Center

Re: [Intel-wired-lan] [PATCH 0/7] [RESEND] [net] intel: Use smp_rmb rather than read_barrier_depends

[Duyck, Alexander H]

On Fri, 2017-11-17 at 10:16 -0600, Brian King wrote:
> On 11/16/2017 04:57 PM, Michael Ellerman wrote:
> > Brian King <brking@linux.vnet.ibm.com> writes:
> > 
> > > On 11/16/2017 01:33 PM, Jesse Brandeburg wrote:
> > > > On Thu, 16 Nov 2017 09:37:48 -0600
> > > > Brian King <brking@linux.vnet.ibm.com> wrote:
> > > > 
> > > > > Resending as the first attempt is not showing up in the list archive.
> > > > > 
> > > > > This patch converts several network drivers to use smp_rmb
> > > > > rather than read_barrier_depends. The initial issue was
> > > > > discovered with ixgbe on a Power machine which resulted
> > > > > in skb list corruption due to fetching a stale skb pointer.
> > > > > More details can be found in the ixgbe patch description.
> > > > 
> > > > Thanks for the fix Brian, I bet it was a tough debug.
> > > > 
> > > > The only users in the entire kernel of read_barrier_depends() (not
> > > > smp_read_barrier_depends) are the Intel network drivers.
> > > > 
> > > > Wouldn't it be better for power to just fix read_barrier_depends to do
> > > > the right thing on power? The question I'm not sure of the answer to is:
> > > > Is it really the wrong barrier to be using or is the implementation in
> > > > the kernel powerpc wrong?
> > > > 
> > > > So I think the right thing might actually to be to:
> > > > Fix arch powerpc read_barrier_depends to not be a noop, as the
> > > > semantics of the read_barrier_depends seems to be sufficient to solve
> > > > this problem, but it seems not to work for powerpc?
> > > 
> > > Jesse,
> > > 
> > > Thanks for the quick response.
> > > 
> > > Cc'ing linuxppc-dev as well. 
> > > 
> > > I did think about changing the powerpc definition of read_barrier_depends,
> > > but after reading up on that barrier, decided it was not the correct barrier
> > > to be used in this context. Here is some good historical background on
> > > read_barrier_depends that I found, along with an example.
> > > 
> > > https://lwn.net/Articles/5159/
> > > 
> > > Since there is no data-dependency in the code in question here, I think
> > > the smp_rmb is the proper barrier to use.
> > 
> > Yes I agree.
> > 
> > The read_barrier_depends() is correct to order the load of eop_desc and
> > then the dependent load of eop_desc->wb.status, but it's only required
> > or does anything on Alpha.
> > 
> > > For background, the code in question looks like this:
> > > 
> > > CPU 1                                   CPU2
> > > ============================            ============================
> > > 1: ixgbe_xmit_frame_ring                ixgbe_clean_tx_irq
> > > 2:  first->skb = skb                     eop_desc = tx_buffer->next_to_watch
> > >                                          if (!eop_desc)
> > >                                              break;
> > > 3:  ixgbe_tx_map                         read_barrier_depends()
> > >                                          if (!(eop_desc->wb.status) ... )
> > >                                              break;
> > > 4:   wmb                                 
> > > 5:   first->next_to_watch = tx_desc      napi_consume_skb(tx_buffer->skb ..);
> > > 6:   writel(i, tx_ring->tail);
> > > 
> > > What we see on powerpc is that tx_buffer->skb on CPU2 is getting loaded
> > > prior to tx_buffer->next_to_watch. Changing the read_barrier_depends
> > > to a smp_rmb solves this and prevents us from dereferencing old pointer.
> > 
> > Right. Given that read_barrier_depends() is a nop, there's nothing there
> > to order the load of tx_buffer->skb vs anything else.
> > 
> > If it's actually the load of tx_buffer->skb that's the issue then the
> > smp_rmb() should really be immediately prior to that, rather than where
> > the read_barrier_depends() currently is.
> 
> Alex,
> 
> How would you like to proceed? read_barrier_depends is a noop on all archs
> except alpha and blackfin. On those two archs, read_barrier_depends and
> smp_rmb end up resulting in the same code. So, I can either:
> 
> 1. Remove the setting of tx_buffer->skb to NULL to address your concern and proceed
> with the rest of the patch set unchanged.

I am good with this option. We just need to be certain that it solves
the original issue you saw.

> 2. Leave the read_barrier_depends, as it is the right barrier to order the load
> of eop_desc with respect to eop_desc->wb.status, and then *add* an smp_rmb in
> the same code path to address the speculative load of the skb that I was running into.
> This is arguably more pure from the perspective of the use of the different
> barriers, but has the downside of additional overhead on alpha and blackfin.
> 
> Do you have a preference? 

If you have the smp_rmb there is no need for the read_barrier_depends
as having both barriers would be redundant anyway. It was there as more
of a mental place holder than anything else since I suspect these
drivers would never be run on an alpha architecture anyway.

> Thanks,
> 
> Brian

Thanks for finding this issue and taking the time to resolve it.

- Alex

Re: [Intel-wired-lan] [PATCH 0/7] [RESEND] [net] intel: Use smp_rmb rather than read_barrier_depends

[Duyck, Alexander H]

T24gRnJpLCAyMDE3LTExLTE3IGF0IDEwOjE2IC0wNjAwLCBCcmlhbiBLaW5nIHdyb3RlOg0KPiBP
biAxMS8xNi8yMDE3IDA0OjU3IFBNLCBNaWNoYWVsIEVsbGVybWFuIHdyb3RlOg0KPiA+IEJyaWFu
IEtpbmcgPGJya2luZ0BsaW51eC52bmV0LmlibS5jb20+IHdyaXRlczoNCj4gPiANCj4gPiA+IE9u
IDExLzE2LzIwMTcgMDE6MzMgUE0sIEplc3NlIEJyYW5kZWJ1cmcgd3JvdGU6DQo+ID4gPiA+IE9u
IFRodSwgMTYgTm92IDIwMTcgMDk6Mzc6NDggLTA2MDANCj4gPiA+ID4gQnJpYW4gS2luZyA8YnJr
aW5nQGxpbnV4LnZuZXQuaWJtLmNvbT4gd3JvdGU6DQo+ID4gPiA+IA0KPiA+ID4gPiA+IFJlc2Vu
ZGluZyBhcyB0aGUgZmlyc3QgYXR0ZW1wdCBpcyBub3Qgc2hvd2luZyB1cCBpbiB0aGUgbGlzdCBh
cmNoaXZlLg0KPiA+ID4gPiA+IA0KPiA+ID4gPiA+IFRoaXMgcGF0Y2ggY29udmVydHMgc2V2ZXJh
bCBuZXR3b3JrIGRyaXZlcnMgdG8gdXNlIHNtcF9ybWINCj4gPiA+ID4gPiByYXRoZXIgdGhhbiBy
ZWFkX2JhcnJpZXJfZGVwZW5kcy4gVGhlIGluaXRpYWwgaXNzdWUgd2FzDQo+ID4gPiA+ID4gZGlz
Y292ZXJlZCB3aXRoIGl4Z2JlIG9uIGEgUG93ZXIgbWFjaGluZSB3aGljaCByZXN1bHRlZA0KPiA+
ID4gPiA+IGluIHNrYiBsaXN0IGNvcnJ1cHRpb24gZHVlIHRvIGZldGNoaW5nIGEgc3RhbGUgc2ti
IHBvaW50ZXIuDQo+ID4gPiA+ID4gTW9yZSBkZXRhaWxzIGNhbiBiZSBmb3VuZCBpbiB0aGUgaXhn
YmUgcGF0Y2ggZGVzY3JpcHRpb24uDQo+ID4gPiA+IA0KPiA+ID4gPiBUaGFua3MgZm9yIHRoZSBm
aXggQnJpYW4sIEkgYmV0IGl0IHdhcyBhIHRvdWdoIGRlYnVnLg0KPiA+ID4gPiANCj4gPiA+ID4g
VGhlIG9ubHkgdXNlcnMgaW4gdGhlIGVudGlyZSBrZXJuZWwgb2YgcmVhZF9iYXJyaWVyX2RlcGVu
ZHMoKSAobm90DQo+ID4gPiA+IHNtcF9yZWFkX2JhcnJpZXJfZGVwZW5kcykgYXJlIHRoZSBJbnRl
bCBuZXR3b3JrIGRyaXZlcnMuDQo+ID4gPiA+IA0KPiA+ID4gPiBXb3VsZG4ndCBpdCBiZSBiZXR0
ZXIgZm9yIHBvd2VyIHRvIGp1c3QgZml4IHJlYWRfYmFycmllcl9kZXBlbmRzIHRvIGRvDQo+ID4g
PiA+IHRoZSByaWdodCB0aGluZyBvbiBwb3dlcj8gVGhlIHF1ZXN0aW9uIEknbSBub3Qgc3VyZSBv
ZiB0aGUgYW5zd2VyIHRvIGlzOg0KPiA+ID4gPiBJcyBpdCByZWFsbHkgdGhlIHdyb25nIGJhcnJp
ZXIgdG8gYmUgdXNpbmcgb3IgaXMgdGhlIGltcGxlbWVudGF0aW9uIGluDQo+ID4gPiA+IHRoZSBr
ZXJuZWwgcG93ZXJwYyB3cm9uZz8NCj4gPiA+ID4gDQo+ID4gPiA+IFNvIEkgdGhpbmsgdGhlIHJp
Z2h0IHRoaW5nIG1pZ2h0IGFjdHVhbGx5IHRvIGJlIHRvOg0KPiA+ID4gPiBGaXggYXJjaCBwb3dl
cnBjIHJlYWRfYmFycmllcl9kZXBlbmRzIHRvIG5vdCBiZSBhIG5vb3AsIGFzIHRoZQ0KPiA+ID4g
PiBzZW1hbnRpY3Mgb2YgdGhlIHJlYWRfYmFycmllcl9kZXBlbmRzIHNlZW1zIHRvIGJlIHN1ZmZp
Y2llbnQgdG8gc29sdmUNCj4gPiA+ID4gdGhpcyBwcm9ibGVtLCBidXQgaXQgc2VlbXMgbm90IHRv
IHdvcmsgZm9yIHBvd2VycGM/DQo+ID4gPiANCj4gPiA+IEplc3NlLA0KPiA+ID4gDQo+ID4gPiBU
aGFua3MgZm9yIHRoZSBxdWljayByZXNwb25zZS4NCj4gPiA+IA0KPiA+ID4gQ2MnaW5nIGxpbnV4
cHBjLWRldiBhcyB3ZWxsLiANCj4gPiA+IA0KPiA+ID4gSSBkaWQgdGhpbmsgYWJvdXQgY2hhbmdp
bmcgdGhlIHBvd2VycGMgZGVmaW5pdGlvbiBvZiByZWFkX2JhcnJpZXJfZGVwZW5kcywNCj4gPiA+
IGJ1dCBhZnRlciByZWFkaW5nIHVwIG9uIHRoYXQgYmFycmllciwgZGVjaWRlZCBpdCB3YXMgbm90
IHRoZSBjb3JyZWN0IGJhcnJpZXINCj4gPiA+IHRvIGJlIHVzZWQgaW4gdGhpcyBjb250ZXh0LiBI
ZXJlIGlzIHNvbWUgZ29vZCBoaXN0b3JpY2FsIGJhY2tncm91bmQgb24NCj4gPiA+IHJlYWRfYmFy
cmllcl9kZXBlbmRzIHRoYXQgSSBmb3VuZCwgYWxvbmcgd2l0aCBhbiBleGFtcGxlLg0KPiA+ID4g
DQo+ID4gPiBodHRwczovL2x3bi5uZXQvQXJ0aWNsZXMvNTE1OS8NCj4gPiA+IA0KPiA+ID4gU2lu
Y2UgdGhlcmUgaXMgbm8gZGF0YS1kZXBlbmRlbmN5IGluIHRoZSBjb2RlIGluIHF1ZXN0aW9uIGhl
cmUsIEkgdGhpbmsNCj4gPiA+IHRoZSBzbXBfcm1iIGlzIHRoZSBwcm9wZXIgYmFycmllciB0byB1
c2UuDQo+ID4gDQo+ID4gWWVzIEkgYWdyZWUuDQo+ID4gDQo+ID4gVGhlIHJlYWRfYmFycmllcl9k
ZXBlbmRzKCkgaXMgY29ycmVjdCB0byBvcmRlciB0aGUgbG9hZCBvZiBlb3BfZGVzYyBhbmQNCj4g
PiB0aGVuIHRoZSBkZXBlbmRlbnQgbG9hZCBvZiBlb3BfZGVzYy0+d2Iuc3RhdHVzLCBidXQgaXQn
cyBvbmx5IHJlcXVpcmVkDQo+ID4gb3IgZG9lcyBhbnl0aGluZyBvbiBBbHBoYS4NCj4gPiANCj4g
PiA+IEZvciBiYWNrZ3JvdW5kLCB0aGUgY29kZSBpbiBxdWVzdGlvbiBsb29rcyBsaWtlIHRoaXM6
DQo+ID4gPiANCj4gPiA+IENQVSAxICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBD
UFUyDQo+ID4gPiA9PT09PT09PT09PT09PT09PT09PT09PT09PT09ICAgICAgICAgICAgPT09PT09
PT09PT09PT09PT09PT09PT09PT09PQ0KPiA+ID4gMTogaXhnYmVfeG1pdF9mcmFtZV9yaW5nICAg
ICAgICAgICAgICAgIGl4Z2JlX2NsZWFuX3R4X2lycQ0KPiA+ID4gMjogIGZpcnN0LT5za2IgPSBz
a2IgICAgICAgICAgICAgICAgICAgICBlb3BfZGVzYyA9IHR4X2J1ZmZlci0+bmV4dF90b193YXRj
aA0KPiA+ID4gICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBpZiAoIWVv
cF9kZXNjKQ0KPiA+ID4gICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgYnJlYWs7DQo+ID4gPiAzOiAgaXhnYmVfdHhfbWFwICAgICAgICAgICAgICAgICAgICAgICAg
IHJlYWRfYmFycmllcl9kZXBlbmRzKCkNCj4gPiA+ICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgaWYgKCEoZW9wX2Rlc2MtPndiLnN0YXR1cykgLi4uICkNCj4gPiA+ICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIGJyZWFrOw0KPiA+ID4g
NDogICB3bWIgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICANCj4gPiA+IDU6ICAgZmly
c3QtPm5leHRfdG9fd2F0Y2ggPSB0eF9kZXNjICAgICAgbmFwaV9jb25zdW1lX3NrYih0eF9idWZm
ZXItPnNrYiAuLik7DQo+ID4gPiA2OiAgIHdyaXRlbChpLCB0eF9yaW5nLT50YWlsKTsNCj4gPiA+
IA0KPiA+ID4gV2hhdCB3ZSBzZWUgb24gcG93ZXJwYyBpcyB0aGF0IHR4X2J1ZmZlci0+c2tiIG9u
IENQVTIgaXMgZ2V0dGluZyBsb2FkZWQNCj4gPiA+IHByaW9yIHRvIHR4X2J1ZmZlci0+bmV4dF90
b193YXRjaC4gQ2hhbmdpbmcgdGhlIHJlYWRfYmFycmllcl9kZXBlbmRzDQo+ID4gPiB0byBhIHNt
cF9ybWIgc29sdmVzIHRoaXMgYW5kIHByZXZlbnRzIHVzIGZyb20gZGVyZWZlcmVuY2luZyBvbGQg
cG9pbnRlci4NCj4gPiANCj4gPiBSaWdodC4gR2l2ZW4gdGhhdCByZWFkX2JhcnJpZXJfZGVwZW5k
cygpIGlzIGEgbm9wLCB0aGVyZSdzIG5vdGhpbmcgdGhlcmUNCj4gPiB0byBvcmRlciB0aGUgbG9h
ZCBvZiB0eF9idWZmZXItPnNrYiB2cyBhbnl0aGluZyBlbHNlLg0KPiA+IA0KPiA+IElmIGl0J3Mg
YWN0dWFsbHkgdGhlIGxvYWQgb2YgdHhfYnVmZmVyLT5za2IgdGhhdCdzIHRoZSBpc3N1ZSB0aGVu
IHRoZQ0KPiA+IHNtcF9ybWIoKSBzaG91bGQgcmVhbGx5IGJlIGltbWVkaWF0ZWx5IHByaW9yIHRv
IHRoYXQsIHJhdGhlciB0aGFuIHdoZXJlDQo+ID4gdGhlIHJlYWRfYmFycmllcl9kZXBlbmRzKCkg
Y3VycmVudGx5IGlzLg0KPiANCj4gQWxleCwNCj4gDQo+IEhvdyB3b3VsZCB5b3UgbGlrZSB0byBw
cm9jZWVkPyByZWFkX2JhcnJpZXJfZGVwZW5kcyBpcyBhIG5vb3Agb24gYWxsIGFyY2hzDQo+IGV4
Y2VwdCBhbHBoYSBhbmQgYmxhY2tmaW4uIE9uIHRob3NlIHR3byBhcmNocywgcmVhZF9iYXJyaWVy
X2RlcGVuZHMgYW5kDQo+IHNtcF9ybWIgZW5kIHVwIHJlc3VsdGluZyBpbiB0aGUgc2FtZSBjb2Rl
LiBTbywgSSBjYW4gZWl0aGVyOg0KPiANCj4gMS4gUmVtb3ZlIHRoZSBzZXR0aW5nIG9mIHR4X2J1
ZmZlci0+c2tiIHRvIE5VTEwgdG8gYWRkcmVzcyB5b3VyIGNvbmNlcm4gYW5kIHByb2NlZWQNCj4g
d2l0aCB0aGUgcmVzdCBvZiB0aGUgcGF0Y2ggc2V0IHVuY2hhbmdlZC4NCg0KSSBhbSBnb29kIHdp
dGggdGhpcyBvcHRpb24uIFdlIGp1c3QgbmVlZCB0byBiZSBjZXJ0YWluIHRoYXQgaXQgc29sdmVz
DQp0aGUgb3JpZ2luYWwgaXNzdWUgeW91IHNhdy4NCg0KPiAyLiBMZWF2ZSB0aGUgcmVhZF9iYXJy
aWVyX2RlcGVuZHMsIGFzIGl0IGlzIHRoZSByaWdodCBiYXJyaWVyIHRvIG9yZGVyIHRoZSBsb2Fk
DQo+IG9mIGVvcF9kZXNjIHdpdGggcmVzcGVjdCB0byBlb3BfZGVzYy0+d2Iuc3RhdHVzLCBhbmQg
dGhlbiAqYWRkKiBhbiBzbXBfcm1iIGluDQo+IHRoZSBzYW1lIGNvZGUgcGF0aCB0byBhZGRyZXNz
IHRoZSBzcGVjdWxhdGl2ZSBsb2FkIG9mIHRoZSBza2IgdGhhdCBJIHdhcyBydW5uaW5nIGludG8u
DQo+IFRoaXMgaXMgYXJndWFibHkgbW9yZSBwdXJlIGZyb20gdGhlIHBlcnNwZWN0aXZlIG9mIHRo
ZSB1c2Ugb2YgdGhlIGRpZmZlcmVudA0KPiBiYXJyaWVycywgYnV0IGhhcyB0aGUgZG93bnNpZGUg
b2YgYWRkaXRpb25hbCBvdmVyaGVhZCBvbiBhbHBoYSBhbmQgYmxhY2tmaW4uDQo+IA0KPiBEbyB5
b3UgaGF2ZSBhIHByZWZlcmVuY2U/IA0KDQpJZiB5b3UgaGF2ZSB0aGUgc21wX3JtYiB0aGVyZSBp
cyBubyBuZWVkIGZvciB0aGUgcmVhZF9iYXJyaWVyX2RlcGVuZHMNCmFzIGhhdmluZyBib3RoIGJh
cnJpZXJzIHdvdWxkIGJlIHJlZHVuZGFudCBhbnl3YXkuIEl0IHdhcyB0aGVyZSBhcyBtb3JlDQpv
ZiBhIG1lbnRhbCBwbGFjZSBob2xkZXIgdGhhbiBhbnl0aGluZyBlbHNlIHNpbmNlIEkgc3VzcGVj
dCB0aGVzZQ0KZHJpdmVycyB3b3VsZCBuZXZlciBiZSBydW4gb24gYW4gYWxwaGEgYXJjaGl0ZWN0
dXJlIGFueXdheS4NCg0KPiBUaGFua3MsDQo+IA0KPiBCcmlhbg0KDQpUaGFua3MgZm9yIGZpbmRp
bmcgdGhpcyBpc3N1ZSBhbmQgdGFraW5nIHRoZSB0aW1lIHRvIHJlc29sdmUgaXQuDQoNCi0gQWxl
eA==

[Intel-wired-lan] [PATCH 0/7] [RESEND] [net] intel: Use smp_rmb rather than read_barrier_depends

[Duyck, Alexander H]

On Fri, 2017-11-17 at 10:16 -0600, Brian King wrote:
> On 11/16/2017 04:57 PM, Michael Ellerman wrote:
> > Brian King <brking@linux.vnet.ibm.com> writes:
> > 
> > > On 11/16/2017 01:33 PM, Jesse Brandeburg wrote:
> > > > On Thu, 16 Nov 2017 09:37:48 -0600
> > > > Brian King <brking@linux.vnet.ibm.com> wrote:
> > > > 
> > > > > Resending as the first attempt is not showing up in the list archive.
> > > > > 
> > > > > This patch converts several network drivers to use smp_rmb
> > > > > rather than read_barrier_depends. The initial issue was
> > > > > discovered with ixgbe on a Power machine which resulted
> > > > > in skb list corruption due to fetching a stale skb pointer.
> > > > > More details can be found in the ixgbe patch description.
> > > > 
> > > > Thanks for the fix Brian, I bet it was a tough debug.
> > > > 
> > > > The only users in the entire kernel of read_barrier_depends() (not
> > > > smp_read_barrier_depends) are the Intel network drivers.
> > > > 
> > > > Wouldn't it be better for power to just fix read_barrier_depends to do
> > > > the right thing on power? The question I'm not sure of the answer to is:
> > > > Is it really the wrong barrier to be using or is the implementation in
> > > > the kernel powerpc wrong?
> > > > 
> > > > So I think the right thing might actually to be to:
> > > > Fix arch powerpc read_barrier_depends to not be a noop, as the
> > > > semantics of the read_barrier_depends seems to be sufficient to solve
> > > > this problem, but it seems not to work for powerpc?
> > > 
> > > Jesse,
> > > 
> > > Thanks for the quick response.
> > > 
> > > Cc'ing linuxppc-dev as well. 
> > > 
> > > I did think about changing the powerpc definition of read_barrier_depends,
> > > but after reading up on that barrier, decided it was not the correct barrier
> > > to be used in this context. Here is some good historical background on
> > > read_barrier_depends that I found, along with an example.
> > > 
> > > https://lwn.net/Articles/5159/
> > > 
> > > Since there is no data-dependency in the code in question here, I think
> > > the smp_rmb is the proper barrier to use.
> > 
> > Yes I agree.
> > 
> > The read_barrier_depends() is correct to order the load of eop_desc and
> > then the dependent load of eop_desc->wb.status, but it's only required
> > or does anything on Alpha.
> > 
> > > For background, the code in question looks like this:
> > > 
> > > CPU 1                                   CPU2
> > > ============================            ============================
> > > 1: ixgbe_xmit_frame_ring                ixgbe_clean_tx_irq
> > > 2:  first->skb = skb                     eop_desc = tx_buffer->next_to_watch
> > >                                          if (!eop_desc)
> > >                                              break;
> > > 3:  ixgbe_tx_map                         read_barrier_depends()
> > >                                          if (!(eop_desc->wb.status) ... )
> > >                                              break;
> > > 4:   wmb                                 
> > > 5:   first->next_to_watch = tx_desc      napi_consume_skb(tx_buffer->skb ..);
> > > 6:   writel(i, tx_ring->tail);
> > > 
> > > What we see on powerpc is that tx_buffer->skb on CPU2 is getting loaded
> > > prior to tx_buffer->next_to_watch. Changing the read_barrier_depends
> > > to a smp_rmb solves this and prevents us from dereferencing old pointer.
> > 
> > Right. Given that read_barrier_depends() is a nop, there's nothing there
> > to order the load of tx_buffer->skb vs anything else.
> > 
> > If it's actually the load of tx_buffer->skb that's the issue then the
> > smp_rmb() should really be immediately prior to that, rather than where
> > the read_barrier_depends() currently is.
> 
> Alex,
> 
> How would you like to proceed? read_barrier_depends is a noop on all archs
> except alpha and blackfin. On those two archs, read_barrier_depends and
> smp_rmb end up resulting in the same code. So, I can either:
> 
> 1. Remove the setting of tx_buffer->skb to NULL to address your concern and proceed
> with the rest of the patch set unchanged.

I am good with this option. We just need to be certain that it solves
the original issue you saw.

> 2. Leave the read_barrier_depends, as it is the right barrier to order the load
> of eop_desc with respect to eop_desc->wb.status, and then *add* an smp_rmb in
> the same code path to address the speculative load of the skb that I was running into.
> This is arguably more pure from the perspective of the use of the different
> barriers, but has the downside of additional overhead on alpha and blackfin.
> 
> Do you have a preference? 

If you have the smp_rmb there is no need for the read_barrier_depends
as having both barriers would be redundant anyway. It was there as more
of a mental place holder than anything else since I suspect these
drivers would never be run on an alpha architecture anyway.

> Thanks,
> 
> Brian

Thanks for finding this issue and taking the time to resolve it.

- Alex