2013-01-09 10:38:49

by Sujith Manoharan

Subject: [PATCH] ath9k_htc: Fix memory leak

From: Sujith Manoharan <[email protected]>

SKBs that are allocated in the HTC layer do not have callbacks
registered and hence ended up not being freed, Fix this by freeing
them properly in the TX completion routine.

Cc: <[email protected]>
Reported-by: Larry Finger <[email protected]>
Signed-off-by: Sujith Manoharan <[email protected]>
---
drivers/net/wireless/ath/ath9k/htc_hst.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/drivers/net/wireless/ath/ath9k/htc_hst.c b/drivers/net/wireless/ath/ath9k/htc_hst.c
index 4a9570d..aac4a40 100644
--- a/drivers/net/wireless/ath/ath9k/htc_hst.c
+++ b/drivers/net/wireless/ath/ath9k/htc_hst.c
@@ -344,6 +344,8 @@ void ath9k_htc_txcompletion_cb(struct htc_target *htc_handle,
endpoint->ep_callbacks.tx(endpoint->ep_callbacks.priv,
skb, htc_hdr->endpoint_id,
txok);
+ } else {
+ kfree_skb(skb);
}
}

--
1.8.1

2013-01-09 16:44:47

by Larry Finger

Subject: Re: [PATCH] ath9k_htc: Fix memory leak

On 01/09/2013 04:37 AM, Sujith Manoharan wrote:
> From: Sujith Manoharan <[email protected]>
>
> SKBs that are allocated in the HTC layer do not have callbacks
> registered and hence ended up not being freed, Fix this by freeing
> them properly in the TX completion routine.
>
> Cc: <[email protected]>
> Reported-by: Larry Finger <[email protected]>
> Signed-off-by: Sujith Manoharan <[email protected]>
> ---
> drivers/net/wireless/ath/ath9k/htc_hst.c | 2 ++
> 1 file changed, 2 insertions(+)

Yes, your fix handles the normal case, and you can add a
Tested-by: Larry Finger <[email protected]>

I still have one remaining question. It is possible for the timeout code in
htc_config_pipe_credits() to be executed, as shown below:

time_left = wait_for_completion_timeout(&target->cmd_wait, HZ);
if (!time_left) {
dev_err(target->dev, "HTC credit config timeout\n");
return -ETIMEDOUT;
}

When this timeout happens, is the skb leaked?

Larry

2013-01-09 16:54:37

by Sujith Manoharan

Subject: Re: [PATCH] ath9k_htc: Fix memory leak

Larry Finger wrote:
> Yes, your fix handles the normal case, and you can add a
> Tested-by: Larry Finger <[email protected]>

Thanks.

> I still have one remaining question. It is possible for the timeout code in
> htc_config_pipe_credits() to be executed, as shown below:
>
> time_left = wait_for_completion_timeout(&target->cmd_wait, HZ);
> if (!time_left) {
> dev_err(target->dev, "HTC credit config timeout\n");
> return -ETIMEDOUT;
> }
>
> When this timeout happens, is the skb leaked?

No, the submitted SKB (via htc_issue_send()) will always have a TX completion
invocation. The actual completion of target->cmd_wait happens via a different
code-path, htc_process_conn_rsp() in ath9k_htc_rx_msg().

Sujith