rtlwifi allocates both setup_packet and data buffer of control message urb,
using shared kmalloc in _usbctrl_vendorreq_async_write. Structure used for
allocating is:
struct {
u8 data[254];
struct usb_ctrlrequest dr;
};
Because 'struct usb_ctrlrequest' is __packed, setup packet is unaligned and
DMA mapping of both 'data' and 'dr' confuses ARM/sunxi, leading to memory
corruptions and freezes.
Patch changes setup packet to be allocated separately.
Cc: <[email protected]>
Signed-off-by: Jussi Kivilinna <[email protected]>
---
drivers/net/wireless/rtlwifi/usb.c | 44 +++++++++++++++++++++++-------------
1 file changed, 28 insertions(+), 16 deletions(-)
diff --git a/drivers/net/wireless/rtlwifi/usb.c b/drivers/net/wireless/rtlwifi/usb.c
index 476eaef..d98fcdb 100644
--- a/drivers/net/wireless/rtlwifi/usb.c
+++ b/drivers/net/wireless/rtlwifi/usb.c
@@ -42,8 +42,12 @@
static void usbctrl_async_callback(struct urb *urb)
{
- if (urb)
- kfree(urb->context);
+ if (urb) {
+ /* free dr */
+ kfree(urb->setup_packet);
+ /* free databuf */
+ kfree(urb->transfer_buffer);
+ }
}
static int _usbctrl_vendorreq_async_write(struct usb_device *udev, u8 request,
@@ -55,39 +59,47 @@ static int _usbctrl_vendorreq_async_write(struct usb_device *udev, u8 request,
u8 reqtype;
struct usb_ctrlrequest *dr;
struct urb *urb;
- struct rtl819x_async_write_data {
- u8 data[REALTEK_USB_VENQT_MAX_BUF_SIZE];
- struct usb_ctrlrequest dr;
- } *buf;
+ const u16 databuf_maxlen = REALTEK_USB_VENQT_MAX_BUF_SIZE;
+ u8 *databuf;
+
+ if (WARN_ON(len > databuf_maxlen))
+ len = databuf_maxlen;
pipe = usb_sndctrlpipe(udev, 0); /* write_out */
reqtype = REALTEK_USB_VENQT_WRITE;
- buf = kmalloc(sizeof(*buf), GFP_ATOMIC);
- if (!buf)
+ dr = kmalloc(sizeof(*dr), GFP_ATOMIC);
+ if (!dr)
return -ENOMEM;
+ databuf = kmalloc(databuf_maxlen, GFP_ATOMIC);
+ if (!databuf) {
+ kfree(dr);
+ return -ENOMEM;
+ }
+
urb = usb_alloc_urb(0, GFP_ATOMIC);
if (!urb) {
- kfree(buf);
+ kfree(databuf);
+ kfree(dr);
return -ENOMEM;
}
- dr = &buf->dr;
-
dr->bRequestType = reqtype;
dr->bRequest = request;
dr->wValue = cpu_to_le16(value);
dr->wIndex = cpu_to_le16(index);
dr->wLength = cpu_to_le16(len);
/* data are already in little-endian order */
- memcpy(buf, pdata, len);
+ memcpy(databuf, pdata, len);
usb_fill_control_urb(urb, udev, pipe,
- (unsigned char *)dr, buf, len,
- usbctrl_async_callback, buf);
+ (unsigned char *)dr, databuf, len,
+ usbctrl_async_callback, NULL);
rc = usb_submit_urb(urb, GFP_ATOMIC);
- if (rc < 0)
- kfree(buf);
+ if (rc < 0) {
+ kfree(databuf);
+ kfree(dr);
+ }
usb_free_urb(urb);
return rc;
}
Quoting Larry Finger <[email protected]>:
> On 02/16/2013 05:00 PM, Jussi Kivilinna wrote:
>> rtlwifi allocates both setup_packet and data buffer of control message urb,
>> using shared kmalloc in _usbctrl_vendorreq_async_write. Structure used for
>> allocating is:
>> struct {
>> u8 data[254];
>> struct usb_ctrlrequest dr;
>> };
>>
>> Because 'struct usb_ctrlrequest' is __packed, setup packet is unaligned and
>> DMA mapping of both 'data' and 'dr' confuses ARM/sunxi, leading to memory
>> corruptions and freezes.
>>
>> Patch changes setup packet to be allocated separately.
>>
>> Cc: <[email protected]>
>> Signed-off-by: Jussi Kivilinna <[email protected]>
>
> The only change I would make is to convert the WARN_ON to
> WARN_ON_ONCE. In case something goes wrong, there is no need to spam
> the logs.
Ok.
>
> I am not crazy about the overhead in allocating two different blocks
> for each output packet, but if it is necessary, then it has to happen.
>
It already happens for '*_sync_read' calls, where setup_packet is
alloced in 'usb_control_message'.
-Jussi
> Larry
>
>> ---
>> drivers/net/wireless/rtlwifi/usb.c | 44
>> +++++++++++++++++++++++-------------
>> 1 file changed, 28 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/net/wireless/rtlwifi/usb.c
>> b/drivers/net/wireless/rtlwifi/usb.c
>> index 476eaef..d98fcdb 100644
>> --- a/drivers/net/wireless/rtlwifi/usb.c
>> +++ b/drivers/net/wireless/rtlwifi/usb.c
>> @@ -42,8 +42,12 @@
>>
>> static void usbctrl_async_callback(struct urb *urb)
>> {
>> - if (urb)
>> - kfree(urb->context);
>> + if (urb) {
>> + /* free dr */
>> + kfree(urb->setup_packet);
>> + /* free databuf */
>> + kfree(urb->transfer_buffer);
>> + }
>> }
>>
>> static int _usbctrl_vendorreq_async_write(struct usb_device *udev,
>> u8 request,
>> @@ -55,39 +59,47 @@ static int
>> _usbctrl_vendorreq_async_write(struct usb_device *udev, u8 request,
>> u8 reqtype;
>> struct usb_ctrlrequest *dr;
>> struct urb *urb;
>> - struct rtl819x_async_write_data {
>> - u8 data[REALTEK_USB_VENQT_MAX_BUF_SIZE];
>> - struct usb_ctrlrequest dr;
>> - } *buf;
>> + const u16 databuf_maxlen = REALTEK_USB_VENQT_MAX_BUF_SIZE;
>> + u8 *databuf;
>> +
>> + if (WARN_ON(len > databuf_maxlen))
>> + len = databuf_maxlen;
>>
>> pipe = usb_sndctrlpipe(udev, 0); /* write_out */
>> reqtype = REALTEK_USB_VENQT_WRITE;
>>
>> - buf = kmalloc(sizeof(*buf), GFP_ATOMIC);
>> - if (!buf)
>> + dr = kmalloc(sizeof(*dr), GFP_ATOMIC);
>> + if (!dr)
>> return -ENOMEM;
>>
>> + databuf = kmalloc(databuf_maxlen, GFP_ATOMIC);
>> + if (!databuf) {
>> + kfree(dr);
>> + return -ENOMEM;
>> + }
>> +
>> urb = usb_alloc_urb(0, GFP_ATOMIC);
>> if (!urb) {
>> - kfree(buf);
>> + kfree(databuf);
>> + kfree(dr);
>> return -ENOMEM;
>> }
>>
>> - dr = &buf->dr;
>> -
>> dr->bRequestType = reqtype;
>> dr->bRequest = request;
>> dr->wValue = cpu_to_le16(value);
>> dr->wIndex = cpu_to_le16(index);
>> dr->wLength = cpu_to_le16(len);
>> /* data are already in little-endian order */
>> - memcpy(buf, pdata, len);
>> + memcpy(databuf, pdata, len);
>> usb_fill_control_urb(urb, udev, pipe,
>> - (unsigned char *)dr, buf, len,
>> - usbctrl_async_callback, buf);
>> + (unsigned char *)dr, databuf, len,
>> + usbctrl_async_callback, NULL);
>> rc = usb_submit_urb(urb, GFP_ATOMIC);
>> - if (rc < 0)
>> - kfree(buf);
>> + if (rc < 0) {
>> + kfree(databuf);
>> + kfree(dr);
>> + }
>> usb_free_urb(urb);
>> return rc;
>> }
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
>
>
On 16/02/2013 23:48, Larry Finger wrote:
> On 02/16/2013 05:00 PM, Jussi Kivilinna wrote:
>> rtlwifi allocates both setup_packet and data buffer of control
>> message urb,
>> using shared kmalloc in _usbctrl_vendorreq_async_write. Structure
>> used for
>> allocating is:
>> struct {
>> u8 data[254];
>> struct usb_ctrlrequest dr;
>> };
>>
>> Because 'struct usb_ctrlrequest' is __packed, setup packet is
>> unaligned and
>> DMA mapping of both 'data' and 'dr' confuses ARM/sunxi, leading to
>> memory
>> corruptions and freezes.
>>
>> Patch changes setup packet to be allocated separately.
>
> I am not crazy about the overhead in allocating two different blocks
> for each output packet, but if it is necessary, then it has to happen.
>> static int _usbctrl_vendorreq_async_write(struct usb_device *udev,
>> u8 request,
>> @@ -55,39 +59,47 @@ static int _usbctrl_vendorreq_async_write(struct
>> usb_device *udev, u8 request,
>> u8 reqtype;
>> struct usb_ctrlrequest *dr;
>> struct urb *urb;
>> - struct rtl819x_async_write_data {
>> - u8 data[REALTEK_USB_VENQT_MAX_BUF_SIZE];
>> - struct usb_ctrlrequest dr;
>> - } *buf;
>
If dr isn't at the appropriate alignment, wouldn't padding out the data
buffer achieve the same thing? Something like:
struct rtl819x_async_write_data {
- u8 data[REALTEK_USB_VENQT_MAX_BUF_SIZE];
+ u8 data[ALIGN(REALTEK_USB_VENQT_MAX_BUF_SIZE, ALIGNMENT)];
struct usb_ctrlrequest dr;
} *buf;
Where ALIGNMENT is whatever the required alignment is.
Dave.
Quoting Dave Kilroy <[email protected]>:
> On 16/02/2013 23:48, Larry Finger wrote:
>> On 02/16/2013 05:00 PM, Jussi Kivilinna wrote:
>>> rtlwifi allocates both setup_packet and data buffer of control message urb,
>>> using shared kmalloc in _usbctrl_vendorreq_async_write. Structure used for
>>> allocating is:
>>> struct {
>>> u8 data[254];
>>> struct usb_ctrlrequest dr;
>>> };
>>>
>>> Because 'struct usb_ctrlrequest' is __packed, setup packet is unaligned and
>>> DMA mapping of both 'data' and 'dr' confuses ARM/sunxi, leading to memory
>>> corruptions and freezes.
>>>
>>> Patch changes setup packet to be allocated separately.
>>
>> I am not crazy about the overhead in allocating two different
>> blocks for each output packet, but if it is necessary, then it has
>> to happen.
>>> static int _usbctrl_vendorreq_async_write(struct usb_device
>>> *udev, u8 request,
>>> @@ -55,39 +59,47 @@ static int
>>> _usbctrl_vendorreq_async_write(struct usb_device *udev, u8 request,
>>> u8 reqtype;
>>> struct usb_ctrlrequest *dr;
>>> struct urb *urb;
>>> - struct rtl819x_async_write_data {
>>> - u8 data[REALTEK_USB_VENQT_MAX_BUF_SIZE];
>>> - struct usb_ctrlrequest dr;
>>> - } *buf;
>>
>
> If dr isn't at the appropriate alignment, wouldn't padding out the
> data buffer achieve the same thing? Something like:
>
> struct rtl819x_async_write_data {
> - u8 data[REALTEK_USB_VENQT_MAX_BUF_SIZE];
> + u8 data[ALIGN(REALTEK_USB_VENQT_MAX_BUF_SIZE, ALIGNMENT)];
> struct usb_ctrlrequest dr;
> } *buf;
>
> Where ALIGNMENT is whatever the required alignment is.
Either it needs to be ARCH_DMA_MINALIGN (need to use ifdefs) or
KMALLOC_MIN_ALIGN, which appearently is defined only for SLUB, not
SLAB or SLOB.
-Jussi
>
>
> Dave.
>
>
On 02/16/2013 05:00 PM, Jussi Kivilinna wrote:
> rtlwifi allocates both setup_packet and data buffer of control message urb,
> using shared kmalloc in _usbctrl_vendorreq_async_write. Structure used for
> allocating is:
> struct {
> u8 data[254];
> struct usb_ctrlrequest dr;
> };
>
> Because 'struct usb_ctrlrequest' is __packed, setup packet is unaligned and
> DMA mapping of both 'data' and 'dr' confuses ARM/sunxi, leading to memory
> corruptions and freezes.
>
> Patch changes setup packet to be allocated separately.
>
> Cc: <[email protected]>
> Signed-off-by: Jussi Kivilinna <[email protected]>
The only change I would make is to convert the WARN_ON to WARN_ON_ONCE. In case
something goes wrong, there is no need to spam the logs.
I am not crazy about the overhead in allocating two different blocks for each
output packet, but if it is necessary, then it has to happen.
Larry
> ---
> drivers/net/wireless/rtlwifi/usb.c | 44 +++++++++++++++++++++++-------------
> 1 file changed, 28 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/net/wireless/rtlwifi/usb.c b/drivers/net/wireless/rtlwifi/usb.c
> index 476eaef..d98fcdb 100644
> --- a/drivers/net/wireless/rtlwifi/usb.c
> +++ b/drivers/net/wireless/rtlwifi/usb.c
> @@ -42,8 +42,12 @@
>
> static void usbctrl_async_callback(struct urb *urb)
> {
> - if (urb)
> - kfree(urb->context);
> + if (urb) {
> + /* free dr */
> + kfree(urb->setup_packet);
> + /* free databuf */
> + kfree(urb->transfer_buffer);
> + }
> }
>
> static int _usbctrl_vendorreq_async_write(struct usb_device *udev, u8 request,
> @@ -55,39 +59,47 @@ static int _usbctrl_vendorreq_async_write(struct usb_device *udev, u8 request,
> u8 reqtype;
> struct usb_ctrlrequest *dr;
> struct urb *urb;
> - struct rtl819x_async_write_data {
> - u8 data[REALTEK_USB_VENQT_MAX_BUF_SIZE];
> - struct usb_ctrlrequest dr;
> - } *buf;
> + const u16 databuf_maxlen = REALTEK_USB_VENQT_MAX_BUF_SIZE;
> + u8 *databuf;
> +
> + if (WARN_ON(len > databuf_maxlen))
> + len = databuf_maxlen;
>
> pipe = usb_sndctrlpipe(udev, 0); /* write_out */
> reqtype = REALTEK_USB_VENQT_WRITE;
>
> - buf = kmalloc(sizeof(*buf), GFP_ATOMIC);
> - if (!buf)
> + dr = kmalloc(sizeof(*dr), GFP_ATOMIC);
> + if (!dr)
> return -ENOMEM;
>
> + databuf = kmalloc(databuf_maxlen, GFP_ATOMIC);
> + if (!databuf) {
> + kfree(dr);
> + return -ENOMEM;
> + }
> +
> urb = usb_alloc_urb(0, GFP_ATOMIC);
> if (!urb) {
> - kfree(buf);
> + kfree(databuf);
> + kfree(dr);
> return -ENOMEM;
> }
>
> - dr = &buf->dr;
> -
> dr->bRequestType = reqtype;
> dr->bRequest = request;
> dr->wValue = cpu_to_le16(value);
> dr->wIndex = cpu_to_le16(index);
> dr->wLength = cpu_to_le16(len);
> /* data are already in little-endian order */
> - memcpy(buf, pdata, len);
> + memcpy(databuf, pdata, len);
> usb_fill_control_urb(urb, udev, pipe,
> - (unsigned char *)dr, buf, len,
> - usbctrl_async_callback, buf);
> + (unsigned char *)dr, databuf, len,
> + usbctrl_async_callback, NULL);
> rc = usb_submit_urb(urb, GFP_ATOMIC);
> - if (rc < 0)
> - kfree(buf);
> + if (rc < 0) {
> + kfree(databuf);
> + kfree(dr);
> + }
> usb_free_urb(urb);
> return rc;
> }
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>