| Message ID | 20130216230016.15041.99979.stgit@localhost6.localdomain6 (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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: <stable@vger.kernel.org> > Signed-off-by: Jussi Kivilinna <jussi.kivilinna@mbnet.fi> 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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- 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 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. -- 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
Quoting Dave Kilroy <kilroyd@googlemail.com>: > 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. > > -- 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
Quoting Larry Finger <Larry.Finger@lwfinger.net>: > 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: <stable@vger.kernel.org> >> Signed-off-by: Jussi Kivilinna <jussi.kivilinna@mbnet.fi> > > 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 majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > > > -- 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/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; }
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: <stable@vger.kernel.org> Signed-off-by: Jussi Kivilinna <jussi.kivilinna@mbnet.fi> --- drivers/net/wireless/rtlwifi/usb.c | 44 +++++++++++++++++++++++------------- 1 file changed, 28 insertions(+), 16 deletions(-) -- 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