| Message ID | 1357098567-8558-1-git-send-email-Larry.Finger@lwfinger.net |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
Larry Finger wrote: > diff --git a/drivers/net/wireless/ath/ath9k/htc_hst.c b/drivers/net/wireless/ath/ath9k/htc_hst.c > index 4a9570d..a304748 100644 > --- a/drivers/net/wireless/ath/ath9k/htc_hst.c > +++ b/drivers/net/wireless/ath/ath9k/htc_hst.c > @@ -278,10 +278,12 @@ int htc_connect_service(struct htc_target *target, > if (!time_left) { > dev_err(target->dev, "Service connection timeout for: %d\n", > service_connreq->service_id); > - return -ETIMEDOUT; > + ret = -ETIMEDOUT; > + goto err; > } > > *conn_rsp_epid = target->conn_rsp_epid; > + kfree_skb(skb); > return 0; > err: > kfree_skb(skb); The allocated skb should be freed in the TX completion hander, ath9k_htc_txcompletion_cb() - doing it in htc_connect_service() is wrong, I think. Sujith -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/01/2013 10:30 PM, Sujith Manoharan wrote: > Larry Finger wrote: >> diff --git a/drivers/net/wireless/ath/ath9k/htc_hst.c b/drivers/net/wireless/ath/ath9k/htc_hst.c >> index 4a9570d..a304748 100644 >> --- a/drivers/net/wireless/ath/ath9k/htc_hst.c >> +++ b/drivers/net/wireless/ath/ath9k/htc_hst.c >> @@ -278,10 +278,12 @@ int htc_connect_service(struct htc_target *target, >> if (!time_left) { >> dev_err(target->dev, "Service connection timeout for: %d\n", >> service_connreq->service_id); >> - return -ETIMEDOUT; >> + ret = -ETIMEDOUT; >> + goto err; >> } >> >> *conn_rsp_epid = target->conn_rsp_epid; >> + kfree_skb(skb); >> return 0; >> err: >> kfree_skb(skb); > ath9k_htc_txcompletion_cb() > The allocated skb should be freed in the TX completion hander, > ath9k_htc_txcompletion_cb() - doing it in htc_connect_service() is wrong, I think. My only counter argument is that none of the other paths that get to ath9k_htc_txcompletion_cb() leak the skb. It only happens for the path that goes through htc_connect_service(). Larry -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Larry Finger wrote: > My only counter argument is that none of the other paths that get to > ath9k_htc_txcompletion_cb() leak the skb. It only happens for the path that goes > through htc_connect_service(). Sure, but the TX completion handler would be invoked for every skb that is passed to the USB layer, including the ones that are allocated in htc_hst.c. When this skb goes down to hif_usb.c, a URB is allocated and I am not sure how freeing the skb before the USB layer invokes the completion handler for the URB is correct. I'll come up with a patch and see if kmemleak still complains. Sujith -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/01/2013 11:26 PM, Sujith Manoharan wrote: > Larry Finger wrote: >> My only counter argument is that none of the other paths that get to >> ath9k_htc_txcompletion_cb() leak the skb. It only happens for the path that goes >> through htc_connect_service(). > > Sure, but the TX completion handler would be invoked for every skb that is passed > to the USB layer, including the ones that are allocated in htc_hst.c. When this > skb goes down to hif_usb.c, a URB is allocated and I am not sure how freeing > the skb before the USB layer invokes the completion handler for the URB is correct. > > I'll come up with a patch and see if kmemleak still complains. Did I miss your posting on this issue? Larry -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Larry Finger wrote: > > I'll come up with a patch and see if kmemleak still complains. > > Did I miss your posting on this issue? Nope, I haven't got to this yet, sorry. I'll try to send a patch by today. Sujith -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/wireless/ath/ath9k/htc_hst.c b/drivers/net/wireless/ath/ath9k/htc_hst.c index 4a9570d..a304748 100644 --- a/drivers/net/wireless/ath/ath9k/htc_hst.c +++ b/drivers/net/wireless/ath/ath9k/htc_hst.c @@ -278,10 +278,12 @@ int htc_connect_service(struct htc_target *target, if (!time_left) { dev_err(target->dev, "Service connection timeout for: %d\n", service_connreq->service_id); - return -ETIMEDOUT; + ret = -ETIMEDOUT; + goto err; } *conn_rsp_epid = target->conn_rsp_epid; + kfree_skb(skb); return 0; err: kfree_skb(skb);
Running with kmemleak shows that this driver leaks every skb allocated in routine htc_connect_service() whether the service connection succeeds or fails. Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net> Cc: Stable <stable@vger.kernel.org> Cc: "Luis R. Rodriguez" <mcgrof@qca.qualcomm.com> Cc: Vasanthakumar Thiagarajan <vthiagar@qca.qualcomm.com> Cc: Senthil Balasubramanian <senthilb@qca.qualcomm.com> Cc: linux-wireless@vger.kernel.org Cc: ath9k-devel@lists.ath9k.org Cc: netdev@vger.kernel.org Cc: linux-kernel@vger.kernel.org --- drivers/net/wireless/ath/ath9k/htc_hst.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)