[ath9k-devel] [RFT] ath10k: serialize CE completions properly

[ath9k-devel] [RFT] ath10k: serialize CE completions properly

[Michal Kazior]

This fixes memory leak when doing heavy TX.

ath10k_pci_check_process_ce() was checking
`compl_processing` and would call
ath10k_pci_process_ce() if its false. The
ath10k_pci_process_ce() would then set it to true
if there's at least one completion. The lock
protecting the variable is not held between
checking and setting the variable, meaning it is a
possible race condition.

If we then assume ath10k_pci_process_ce() is
called simultaneusly from different contexts we
have a race. And apparently this was possible:

ath10k_pci_check_process_ce() was called from
ath10k_ce_per_engine_service(). This in turn could
be called either from multiple tasklets (MSI-X) or
a tasklet (MSI, Legacy INTR) or
ath10k_htc_send_complete_check().

The race itself would cause corruption of
htt.refcount (ath10k_skb_cb) and we'd leak
multiple skbuffs per tx.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---

I'm unable to reproduce the memleak.
Can someone verify this patch, please?

 drivers/net/wireless/ath/ath10k/pci.c |   39 +++++++++++++--------------------
 1 file changed, 15 insertions(+), 24 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index e348fd9..4b1d8f7 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -506,24 +506,6 @@ exit:
 	return compl;
 }
 
-static void ath10k_pci_check_process_ce(struct ath10k *ar)
-{
-	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
-
-	/*
-	 * Check if another tasklet is already processing
-	 * the completion list. This could happen in multiple-MSI.
-	 */
-	spin_lock_bh(&ar_pci->compl_lock);
-	if (ar_pci->compl_processing) {
-		spin_unlock_bh(&ar_pci->compl_lock);
-		return;
-	}
-	spin_unlock_bh(&ar_pci->compl_lock);
-
-	ath10k_pci_process_ce(ar);
-}
-
 /* Called by lower (CE) layer when a send to Target completes. */
 static void ath10k_pci_ce_send_done(struct ce_state *ce_state,
 				    void *transfer_context,
@@ -583,7 +565,7 @@ static void ath10k_pci_ce_send_done(struct ce_state *ce_state,
 	if (!process)
 		return;
 
-	ath10k_pci_check_process_ce(ar);
+	ath10k_pci_process_ce(ar);
 }
 
 /* Called by lower (CE) layer when data is received from the Target. */
@@ -629,7 +611,7 @@ static void ath10k_pci_ce_recv_data(struct ce_state *ce_state,
 							   &transfer_id,
 							   &flags) == 0);
 
-	ath10k_pci_check_process_ce(ar);
+	ath10k_pci_process_ce(ar);
 }
 
 /* Send the first nbytes bytes of the buffer */
@@ -888,7 +870,18 @@ static void ath10k_pci_process_ce(struct ath10k *ar)
 	unsigned int nbytes;
 	int ret, send_done = 0;
 
-	do {
+	/* Upper layers aren't ready to handle tx/rx completions in parallel so
+	 * we must serialize all completion processing. */
+
+	spin_lock_bh(&ar_pci->compl_lock);
+	if (ar_pci->compl_processing) {
+		spin_unlock_bh(&ar_pci->compl_lock);
+		return;
+	}
+	ar_pci->compl_processing = true;
+	spin_unlock_bh(&ar_pci->compl_lock);
+
+	for (;;) {
 		spin_lock_bh(&ar_pci->compl_lock);
 		if (list_empty(&ar_pci->compl_process)) {
 			spin_unlock_bh(&ar_pci->compl_lock);
@@ -897,7 +890,6 @@ static void ath10k_pci_process_ce(struct ath10k *ar)
 		compl = list_first_entry(&ar_pci->compl_process,
 					 struct ath10k_pci_compl, list);
 		list_del(&compl->list);
-		ar_pci->compl_processing = true;
 		spin_unlock_bh(&ar_pci->compl_lock);
 
 		if (compl->send_or_recv == HIF_CE_COMPLETE_SEND) {
@@ -943,8 +935,7 @@ static void ath10k_pci_process_ce(struct ath10k *ar)
 		list_add_tail(&compl->list, &compl->pipe_info->compl_free);
 		compl->pipe_info->num_sends_allowed += send_done;
 		spin_unlock_bh(&compl->pipe_info->pipe_lock);
-
-	} while (1);
+	}
 
 	spin_lock_bh(&ar_pci->compl_lock);
 	ar_pci->compl_processing = false;
-- 
1.7.9.5

[ath9k-devel] [RFT] ath10k: serialize CE completions properly

[Sujith Manoharan]

Michal Kazior wrote:
> This fixes memory leak when doing heavy TX.
> 
> ath10k_pci_check_process_ce() was checking
> `compl_processing` and would call
> ath10k_pci_process_ce() if its false. The
> ath10k_pci_process_ce() would then set it to true
> if there's at least one completion. The lock
> protecting the variable is not held between
> checking and setting the variable, meaning it is a
> possible race condition.
> 
> If we then assume ath10k_pci_process_ce() is
> called simultaneusly from different contexts we
> have a race. And apparently this was possible:
> 
> ath10k_pci_check_process_ce() was called from
> ath10k_ce_per_engine_service(). This in turn could
> be called either from multiple tasklets (MSI-X) or
> a tasklet (MSI, Legacy INTR) or
> ath10k_htc_send_complete_check().
> 
> The race itself would cause corruption of
> htt.refcount (ath10k_skb_cb) and we'd leak
> multiple skbuffs per tx.
> 
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>

Looks good to me.

Sujith

[ath9k-devel] [PATCH] ath10k: serialize CE completions properly

[Michal Kazior]

This fixes memory leak when doing heavy TX.

ath10k_pci_check_process_ce() was checking
`compl_processing` and would call
ath10k_pci_process_ce() if its false. The
ath10k_pci_process_ce() would then set it to true
if there's at least one completion. The lock
protecting the variable is not held between
checking and setting the variable, meaning it is a
possible race condition.

If we then assume ath10k_pci_process_ce() is
called simultaneusly from different contexts we
have a race. And apparently this was possible:

ath10k_pci_check_process_ce() was called from
ath10k_ce_per_engine_service(). This in turn could
be called either from multiple tasklets (MSI-X) or
a tasklet (MSI, Legacy INTR) or
ath10k_htc_send_complete_check().

The race itself would cause corruption of
htt.refcount (ath10k_skb_cb) and we'd leak
multiple skbuffs per tx.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/pci.c |   39 +++++++++++++--------------------
 1 file changed, 15 insertions(+), 24 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index e348fd9..4b1d8f7 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -506,24 +506,6 @@ exit:
 	return compl;
 }
 
-static void ath10k_pci_check_process_ce(struct ath10k *ar)
-{
-	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
-
-	/*
-	 * Check if another tasklet is already processing
-	 * the completion list. This could happen in multiple-MSI.
-	 */
-	spin_lock_bh(&ar_pci->compl_lock);
-	if (ar_pci->compl_processing) {
-		spin_unlock_bh(&ar_pci->compl_lock);
-		return;
-	}
-	spin_unlock_bh(&ar_pci->compl_lock);
-
-	ath10k_pci_process_ce(ar);
-}
-
 /* Called by lower (CE) layer when a send to Target completes. */
 static void ath10k_pci_ce_send_done(struct ce_state *ce_state,
 				    void *transfer_context,
@@ -583,7 +565,7 @@ static void ath10k_pci_ce_send_done(struct ce_state *ce_state,
 	if (!process)
 		return;
 
-	ath10k_pci_check_process_ce(ar);
+	ath10k_pci_process_ce(ar);
 }
 
 /* Called by lower (CE) layer when data is received from the Target. */
@@ -629,7 +611,7 @@ static void ath10k_pci_ce_recv_data(struct ce_state *ce_state,
 							   &transfer_id,
 							   &flags) == 0);
 
-	ath10k_pci_check_process_ce(ar);
+	ath10k_pci_process_ce(ar);
 }
 
 /* Send the first nbytes bytes of the buffer */
@@ -888,7 +870,18 @@ static void ath10k_pci_process_ce(struct ath10k *ar)
 	unsigned int nbytes;
 	int ret, send_done = 0;
 
-	do {
+	/* Upper layers aren't ready to handle tx/rx completions in parallel so
+	 * we must serialize all completion processing. */
+
+	spin_lock_bh(&ar_pci->compl_lock);
+	if (ar_pci->compl_processing) {
+		spin_unlock_bh(&ar_pci->compl_lock);
+		return;
+	}
+	ar_pci->compl_processing = true;
+	spin_unlock_bh(&ar_pci->compl_lock);
+
+	for (;;) {
 		spin_lock_bh(&ar_pci->compl_lock);
 		if (list_empty(&ar_pci->compl_process)) {
 			spin_unlock_bh(&ar_pci->compl_lock);
@@ -897,7 +890,6 @@ static void ath10k_pci_process_ce(struct ath10k *ar)
 		compl = list_first_entry(&ar_pci->compl_process,
 					 struct ath10k_pci_compl, list);
 		list_del(&compl->list);
-		ar_pci->compl_processing = true;
 		spin_unlock_bh(&ar_pci->compl_lock);
 
 		if (compl->send_or_recv == HIF_CE_COMPLETE_SEND) {
@@ -943,8 +935,7 @@ static void ath10k_pci_process_ce(struct ath10k *ar)
 		list_add_tail(&compl->list, &compl->pipe_info->compl_free);
 		compl->pipe_info->num_sends_allowed += send_done;
 		spin_unlock_bh(&compl->pipe_info->pipe_lock);
-
-	} while (1);
+	}
 
 	spin_lock_bh(&ar_pci->compl_lock);
 	ar_pci->compl_processing = false;
-- 
1.7.9.5

[ath9k-devel] [PATCH] ath10k: serialize CE completions properly

[Kalle Valo]

Michal Kazior <michal.kazior@tieto.com> writes:

> This fixes memory leak when doing heavy TX.
>
> ath10k_pci_check_process_ce() was checking
> `compl_processing` and would call
> ath10k_pci_process_ce() if its false. The
> ath10k_pci_process_ce() would then set it to true
> if there's at least one completion. The lock
> protecting the variable is not held between
> checking and setting the variable, meaning it is a
> possible race condition.
>
> If we then assume ath10k_pci_process_ce() is
> called simultaneusly from different contexts we
> have a race. And apparently this was possible:
>
> ath10k_pci_check_process_ce() was called from
> ath10k_ce_per_engine_service(). This in turn could
> be called either from multiple tasklets (MSI-X) or
> a tasklet (MSI, Legacy INTR) or
> ath10k_htc_send_complete_check().
>
> The race itself would cause corruption of
> htt.refcount (ath10k_skb_cb) and we'd leak
> multiple skbuffs per tx.
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>

Thanks, applied.

-- 
Kalle Valo