| Message ID | 20160301103612.GC29736@c50.bag.software (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
Hi, On Tue, Mar 01, 2016 at 11:36:13AM +0100, Vishal Thanki wrote: > I observed a NULL pointer access crash during my testing on a custom AM33xx > based board with RT5572 USB wifi module. The kernel log is attached with > the mail. With initial debugging, I think that the USB disconnect > event was triggered while there was an pending/incomplete URB request > present. As a part of USB disconnect, the driver cleanup deallocated > queues. However the completion of pending URB tried to access the queue, > which resulted in the NULL pointer crash. > > I added a check in the queue helper routines and with that I did not see > the problem. The patch for the same is also attached with the email. > Please suggest if that is the right way to address the problem. Fix is not correct as we can crash at any other point if we get callback from pending urb after resources are freed. What should be done is create a list of pending urbs (possibly using usb_anchor structure and primitives) and kill urb's before freeing resources. Stanislaw -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 7, 2016 at 10:59 AM, Stanislaw Gruszka <sgruszka@redhat.com> wrote: > Hi, > > On Tue, Mar 01, 2016 at 11:36:13AM +0100, Vishal Thanki wrote: >> I observed a NULL pointer access crash during my testing on a custom AM33xx >> based board with RT5572 USB wifi module. The kernel log is attached with >> the mail. With initial debugging, I think that the USB disconnect >> event was triggered while there was an pending/incomplete URB request >> present. As a part of USB disconnect, the driver cleanup deallocated >> queues. However the completion of pending URB tried to access the queue, >> which resulted in the NULL pointer crash. >> >> I added a check in the queue helper routines and with that I did not see >> the problem. The patch for the same is also attached with the email. >> Please suggest if that is the right way to address the problem. > > Fix is not correct as we can crash at any other point if we get callback > from pending urb after resources are freed. What should be done is > create a list of pending urbs (possibly using usb_anchor structure and > primitives) and kill urb's before freeing resources. > Thank you for the reply. I will prepare the patch as suggested. Vishal > Stanislaw -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" 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/rt2x00/rt2x00queue.h b/drivers/net/wireless/rt2x00/rt2x00queue.h index 2233b91..96ea5ba 100644 --- a/drivers/net/wireless/rt2x00/rt2x00queue.h +++ b/drivers/net/wireless/rt2x00/rt2x00queue.h @@ -532,7 +532,7 @@ struct data_queue { */ #define queue_loop(__entry, __start, __end) \ for ((__entry) = (__start); \ - prefetch(queue_next(__entry)), (__entry) != (__end);\ + prefetch(queue_next(__entry)), __entry && (__entry) != (__end);\ (__entry) = queue_next(__entry)) /**