[PATCH 1/4] ath6kl: Fix htc_packet leak in ath6kl_htc_rx_process_packets()

[PATCH 1/4] ath6kl: Fix htc_packet leak in ath6kl_htc_rx_process_packets()

[Vasanthakumar Thiagarajan]

Packet is not reclaimed when ath6kl_htc_rx_process_hdr() fails.
Fix this by deferring the packet deletion from comp_pktq till
ath6kl_htc_rx_process_hdr() returns success.

Signed-off-by: Vasanthakumar Thiagarajan <vthiagar@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath6kl/htc.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/htc.c b/drivers/net/wireless/ath/ath6kl/htc.c
index f88a7c9..7bc9884 100644
--- a/drivers/net/wireless/ath/ath6kl/htc.c
+++ b/drivers/net/wireless/ath/ath6kl/htc.c
@@ -1643,7 +1643,6 @@ static int ath6kl_htc_rx_process_packets(struct htc_target *target,
 	int status = 0;
 
 	list_for_each_entry_safe(packet, tmp_pkt, comp_pktq, list) {
-		list_del(&packet->list);
 		ep = &target->endpoint[packet->endpoint];
 
 		/* process header for each of the recv packet */
@@ -1652,6 +1651,8 @@ static int ath6kl_htc_rx_process_packets(struct htc_target *target,
 		if (status)
 			return status;
 
+		list_del(&packet->list);
+
 		if (list_empty(comp_pktq)) {
 			/*
 			 * Last packet's more packet flag is set
-- 
1.7.0.4

[PATCH 2/4] ath6kl: Fix htc_packet leak in ath6kl_htc_rx_fetch()

[Vasanthakumar Thiagarajan]

Signed-off-by: Vasanthakumar Thiagarajan <vthiagar@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath6kl/htc.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/htc.c b/drivers/net/wireless/ath/ath6kl/htc.c
index 7bc9884..4a03dac 100644
--- a/drivers/net/wireless/ath/ath6kl/htc.c
+++ b/drivers/net/wireless/ath/ath6kl/htc.c
@@ -1715,12 +1715,10 @@ static int ath6kl_htc_rx_fetch(struct htc_target *target,
 			packet = list_first_entry(rx_pktq, struct htc_packet,
 						   list);
 
-			list_del(&packet->list);
-
 			/* fully synchronous */
 			packet->completion = NULL;
 
-			if (!list_empty(rx_pktq))
+			if (!list_is_singular(rx_pktq))
 				/*
 				 * look_aheads in all packet
 				 * except the last one in the
@@ -1735,7 +1733,7 @@ static int ath6kl_htc_rx_fetch(struct htc_target *target,
 			if (status)
 				return status;
 
-			list_add_tail(&packet->list, comp_pktq);
+			list_move_tail(&packet->list, comp_pktq);
 		}
 	}
 
-- 
1.7.0.4

[PATCH 3/4] ath6kl: Avoid processing failed rx packets

[Vasanthakumar Thiagarajan]

It is not necessary to process an htc_packet which is allocated for rx
but failed in sdio rx.

Signed-off-by: Vasanthakumar Thiagarajan <vthiagar@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath6kl/htc.c |   48 +++++++++++++++++++++++---------
 1 files changed, 34 insertions(+), 14 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/htc.c b/drivers/net/wireless/ath/ath6kl/htc.c
index 4a03dac..ca3d084 100644
--- a/drivers/net/wireless/ath/ath6kl/htc.c
+++ b/drivers/net/wireless/ath/ath6kl/htc.c
@@ -1687,11 +1687,15 @@ static int ath6kl_htc_rx_fetch(struct htc_target *target,
 	int fetched_pkts;
 	bool part_bundle = false;
 	int status = 0;
+	struct list_head tmp_rxq;
+	struct htc_packet *packet, *tmp_pkt;
 
 	/* now go fetch the list of HTC packets */
 	while (!list_empty(rx_pktq)) {
 		fetched_pkts = 0;
 
+		INIT_LIST_HEAD(&tmp_rxq);
+
 		if (target->rx_bndl_enable && (get_queue_depth(rx_pktq) > 1)) {
 			/*
 			 * There are enough packets to attempt a
@@ -1699,18 +1703,19 @@ static int ath6kl_htc_rx_fetch(struct htc_target *target,
 			 * allowed.
 			 */
 			status = ath6kl_htc_rx_bundle(target, rx_pktq,
-						      comp_pktq,
+						      &tmp_rxq,
 						      &fetched_pkts,
 						      part_bundle);
 			if (status)
-				return status;
+				goto fail_rx;
 
 			if (!list_empty(rx_pktq))
 				part_bundle = true;
+
+			list_splice_tail_init(&tmp_rxq, comp_pktq);
 		}
 
 		if (!fetched_pkts) {
-			struct htc_packet *packet;
 
 			packet = list_first_entry(rx_pktq, struct htc_packet,
 						   list);
@@ -1730,13 +1735,37 @@ static int ath6kl_htc_rx_fetch(struct htc_target *target,
 			/* go fetch the packet */
 			status = ath6kl_htc_rx_packet(target, packet,
 						      packet->act_len);
+
+			list_move_tail(&packet->list, &tmp_rxq);
+
 			if (status)
-				return status;
+				goto fail_rx;
 
-			list_move_tail(&packet->list, comp_pktq);
+			list_splice_tail_init(&tmp_rxq, comp_pktq);
 		}
 	}
 
+	return 0;
+
+fail_rx:
+
+	/*
+	 * Cleanup any packets we allocated but didn't use to
+	 * actually fetch any packets.
+	 */
+
+	list_for_each_entry_safe(packet, tmp_pkt, rx_pktq, list) {
+		list_del(&packet->list);
+		htc_reclaim_rxbuf(target, packet,
+				&target->endpoint[packet->endpoint]);
+	}
+
+	list_for_each_entry_safe(packet, tmp_pkt, &tmp_rxq, list) {
+		list_del(&packet->list);
+		htc_reclaim_rxbuf(target, packet,
+				&target->endpoint[packet->endpoint]);
+	}
+
 	return status;
 }
 
@@ -1826,15 +1855,6 @@ int ath6kl_htc_rxmsg_pending_handler(struct htc_target *target,
 	if (status) {
 		ath6kl_err("failed to get pending recv messages: %d\n",
 			   status);
-		/*
-		 * Cleanup any packets we allocated but didn't use to
-		 * actually fetch any packets.
-		 */
-		list_for_each_entry_safe(packets, tmp_pkt, &rx_pktq, list) {
-			list_del(&packets->list);
-			htc_reclaim_rxbuf(target, packets,
-					&target->endpoint[packets->endpoint]);
-		}
 
 		/* cleanup any packets in sync completion queue */
 		list_for_each_entry_safe(packets, tmp_pkt, &comp_pktq, list) {
-- 
1.7.0.4

[PATCH 4/4] ath6kl: Minor cleanup in msg_look_ahead parameter in ath6kl_htc_rxmsg_pending_handler()

[Vasanthakumar Thiagarajan]

It is just a four byte information of the received message
from ath6kl_htc_rxmsg_pending_handler(). Remove unnecessary
array representaion.

Signed-off-by: Vasanthakumar Thiagarajan <vthiagar@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath6kl/htc.c     |    4 ++--
 drivers/net/wireless/ath/ath6kl/htc.h     |    2 +-
 drivers/net/wireless/ath/ath6kl/htc_hif.c |    2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/htc.c b/drivers/net/wireless/ath/ath6kl/htc.c
index ca3d084..9a9eae5 100644
--- a/drivers/net/wireless/ath/ath6kl/htc.c
+++ b/drivers/net/wireless/ath/ath6kl/htc.c
@@ -1770,7 +1770,7 @@ fail_rx:
 }
 
 int ath6kl_htc_rxmsg_pending_handler(struct htc_target *target,
-				     u32 msg_look_ahead[], int *num_pkts)
+				     u32 msg_look_ahead, int *num_pkts)
 {
 	struct htc_packet *packets, *tmp_pkt;
 	struct htc_endpoint *endpoint;
@@ -1787,7 +1787,7 @@ int ath6kl_htc_rxmsg_pending_handler(struct htc_target *target,
 	 * On first entry copy the look_aheads into our temp array for
 	 * processing
 	 */
-	memcpy(look_aheads, msg_look_ahead, sizeof(look_aheads));
+	look_aheads[0] = msg_look_ahead;
 
 	while (true) {
 
diff --git a/drivers/net/wireless/ath/ath6kl/htc.h b/drivers/net/wireless/ath/ath6kl/htc.h
index 8ce0c2c..69d44e3 100644
--- a/drivers/net/wireless/ath/ath6kl/htc.h
+++ b/drivers/net/wireless/ath/ath6kl/htc.h
@@ -563,7 +563,7 @@ int ath6kl_htc_get_rxbuf_num(struct htc_target *target,
 int ath6kl_htc_add_rxbuf_multiple(struct htc_target *target,
 				  struct list_head *pktq);
 int ath6kl_htc_rxmsg_pending_handler(struct htc_target *target,
-				     u32 msg_look_ahead[], int *n_pkts);
+				     u32 msg_look_ahead, int *n_pkts);
 
 static inline void set_htc_pkt_info(struct htc_packet *packet, void *context,
 				    u8 *buf, unsigned int len,
diff --git a/drivers/net/wireless/ath/ath6kl/htc_hif.c b/drivers/net/wireless/ath/ath6kl/htc_hif.c
index 86b1cc7..37a13f0 100644
--- a/drivers/net/wireless/ath/ath6kl/htc_hif.c
+++ b/drivers/net/wireless/ath/ath6kl/htc_hif.c
@@ -417,7 +417,7 @@ static int proc_pending_irqs(struct ath6kl_device *dev, bool *done)
 		 * we rapidly pull packets.
 		 */
 		status = ath6kl_htc_rxmsg_pending_handler(dev->htc_cnxt,
-							  &lk_ahd, &fetched);
+							  lk_ahd, &fetched);
 		if (status)
 			goto out;
 
-- 
1.7.0.4

Re: [PATCH 1/4] ath6kl: Fix htc_packet leak in ath6kl_htc_rx_process_packets()

[Kalle Valo]

On 09/29/2011 06:01 PM, Vasanthakumar Thiagarajan wrote:
> Packet is not reclaimed when ath6kl_htc_rx_process_hdr() fails.
> Fix this by deferring the packet deletion from comp_pktq till
> ath6kl_htc_rx_process_hdr() returns success.

Does this fix a user visible bug? Or is this just something you found
while doing code review?

Kalle

Re: [PATCH 3/4] ath6kl: Avoid processing failed rx packets

[Kalle Valo]

On 09/29/2011 06:01 PM, Vasanthakumar Thiagarajan wrote:
> It is not necessary to process an htc_packet which is allocated for rx
> but failed in sdio rx.

Same question here: does it fix a visible bug or is it more like a
theoretical issue?

I would prefer that fixes would also state the impact of the bug: is it
a theoretical issue, a random bug which almost nobody sees or really bad
bug visible almost everywhere? That way it's easier to decide which
trees it should be sent.

Kalle

Re: [PATCH 1/4] ath6kl: Fix htc_packet leak in ath6kl_htc_rx_process_packets()

[Vasanthakumar Thiagarajan]

On Mon, Oct 03, 2011 at 02:11:50PM +0300, Kalle Valo wrote:
> On 09/29/2011 06:01 PM, Vasanthakumar Thiagarajan wrote:
> > Packet is not reclaimed when ath6kl_htc_rx_process_hdr() fails.
> > Fix this by deferring the packet deletion from comp_pktq till
> > ath6kl_htc_rx_process_hdr() returns success.
> 
> Does this fix a user visible bug? Or is this just something you found
> while doing code review?

This one is found in code review, it does not fix any visible bug.

Vasanth

Re: [PATCH 3/4] ath6kl: Avoid processing failed rx packets

[Vasanthakumar Thiagarajan]

On Mon, Oct 03, 2011 at 02:18:01PM +0300, Kalle Valo wrote:
> On 09/29/2011 06:01 PM, Vasanthakumar Thiagarajan wrote:
> > It is not necessary to process an htc_packet which is allocated for rx
> > but failed in sdio rx.
> 
> Same question here: does it fix a visible bug or is it more like a
> theoretical issue?
> 
> I would prefer that fixes would also state the impact of the bug: is it
> a theoretical issue, a random bug which almost nobody sees or really bad
> bug visible almost everywhere? That way it's easier to decide which
> trees it should be sent.

Ok, I can add this to commit log. I'll resend everything with V2.

Vasanth

Re: [PATCH 3/4] ath6kl: Avoid processing failed rx packets

[Kalle Valo]

On 10/03/2011 02:41 PM, Vasanthakumar Thiagarajan wrote:
> On Mon, Oct 03, 2011 at 02:18:01PM +0300, Kalle Valo wrote:
>> On 09/29/2011 06:01 PM, Vasanthakumar Thiagarajan wrote:
>>> It is not necessary to process an htc_packet which is allocated for rx
>>> but failed in sdio rx.
>>
>> Same question here: does it fix a visible bug or is it more like a
>> theoretical issue?
>>
>> I would prefer that fixes would also state the impact of the bug: is it
>> a theoretical issue, a random bug which almost nobody sees or really bad
>> bug visible almost everywhere? That way it's easier to decide which
>> trees it should be sent.
> 
> Ok, I can add this to commit log. I'll resend everything with V2.

Excellent, thanks a lot.

Kalle

[PATCH 1/4] ath6kl: Fix htc_packet leak in ath6kl_htc_rx_process_packets()

[Vasanthakumar Thiagarajan]

Packet is not reclaimed when ath6kl_htc_rx_process_hdr() fails.
Fix this by deferring the packet deletion from comp_pktq till
ath6kl_htc_rx_process_hdr() returns success. This bug is found
in code review, impact is not easily visible as the leak happens
only in failure cases.

Signed-off-by: Vasanthakumar Thiagarajan <vthiagar@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath6kl/htc.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

V2 --- Commit log change

diff --git a/drivers/net/wireless/ath/ath6kl/htc.c b/drivers/net/wireless/ath/ath6kl/htc.c
index f88a7c9..7bc9884 100644
--- a/drivers/net/wireless/ath/ath6kl/htc.c
+++ b/drivers/net/wireless/ath/ath6kl/htc.c
@@ -1643,7 +1643,6 @@ static int ath6kl_htc_rx_process_packets(struct htc_target *target,
 	int status = 0;
 
 	list_for_each_entry_safe(packet, tmp_pkt, comp_pktq, list) {
-		list_del(&packet->list);
 		ep = &target->endpoint[packet->endpoint];
 
 		/* process header for each of the recv packet */
@@ -1652,6 +1651,8 @@ static int ath6kl_htc_rx_process_packets(struct htc_target *target,
 		if (status)
 			return status;
 
+		list_del(&packet->list);
+
 		if (list_empty(comp_pktq)) {
 			/*
 			 * Last packet's more packet flag is set
-- 
1.7.0.4