| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ef2a89f5-f68c-e7e2-9338-78e70dc41701@gmail.com>
Date: Fri, 17 Sep 2021 18:03:52 +0300
From: Pavel Skripkin <paskripkin@...il.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Larry Finger <Larry.Finger@...inger.net>,
Phillip Potter <phil@...lpotter.co.uk>
Cc: linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org,
David Laight <david.Laight@...lab.com>,
Dan Carpenter <dan.carpenter@...cle.com>,
"Fabio M. De Francesco" <fmdefrancesco@...il.com>
Subject: Re: [PATCH v7 19/19] staging: r8188eu: remove shared buffer for usb
requests
On 9/17/21 17:55, Greg Kroah-Hartman wrote:
> On Fri, Sep 17, 2021 at 09:18:37AM +0200, Fabio M. De Francesco wrote:
>> From: Pavel Skripkin <paskripkin@...il.com>
>>
>> This driver used shared buffer for usb requests. It led to using
>> mutexes, i.e no usb requests can be done in parallel.
>>
>> USB requests can be fired in parallel since USB Core allows it. In
>> order to allow them, remove usb_vendor_req_buf from dvobj_priv (since
>> USB I/O is the only user of it) and remove also usb_vendor_req_mutex
>> (since there is nothing to protect).
>
> Ah, you are removing this buffer, nice!
>
> But, just because the USB core allows multiple messages to be sent to a
> device at the same time, does NOT mean that the device itself can handle
> that sort of a thing.
>
> Keeping that lock might be a good idea, until you can prove otherwise.
> You never know, maybe there's never any contention at all for it because
> these accesses are all done in a serial fashion and the lock
> grab/release is instant. But if that is not the case, you might really
> get a device confused here by throwing multiple control messages at it
> in ways that it is not set up to handle at all.
>
> So please do not drop the lock.
>
> More comments below.
>
We have tested this change. I've tested it in qemu with TP-Link
TL-WN722N v2 / v3 [Realtek RTL8188EUS], and Fabio has tested it on his
host for like a whole evening.
I agree, that our testing does not cover all possible cases and I can't
say it was "good stress testing", so, I think, we need some comments
from maintainers.
@Larry, @Phillip, does this change looks reasonable for this chip?
With regards,
Pavel Skripkin
>
>>
>> Co-developed-by: Fabio M. De Francesco <fmdefrancesco@...il.com>
>> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@...il.com>
>> Signed-off-by: Pavel Skripkin <paskripkin@...il.com>
>> ---
>> drivers/staging/r8188eu/hal/usb_ops_linux.c | 29 ++++++++-------
>> drivers/staging/r8188eu/include/drv_types.h | 5 ---
>> drivers/staging/r8188eu/os_dep/usb_intf.c | 40 ++-------------------
>> 3 files changed, 16 insertions(+), 58 deletions(-)
>>
>> diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
>> index 656f3a774e48..0ed4e6c8b1f5 100644
>> --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
>> +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
>> @@ -19,9 +19,9 @@ static int usb_read(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size)
>> if (adapt->bSurpriseRemoved || adapt->pwrctrlpriv.pnp_bstop_trx)
>> return -EPERM;
>>
>> - mutex_lock(&dvobjpriv->usb_vendor_req_mutex);
>> -
>> - io_buf = dvobjpriv->usb_vendor_req_buf;
>> + io_buf = kmalloc(size, GFP_KERNEL);
>> + if (!io_buf)
>> + return -ENOMEM;
>
> Please read the docs for usb_control_msg_recv(). It can allow data off
> of the stack, so no need to allocate/free the buffer like this all the
> time.
>
> Note, the usb_control_msg() call does require the data to be allocated
> dynamically, like the code does today. Which is why you probably got
> confused here.
>
> Same for usb_control_msg_send(), it can take data off of the stack.
>
>
>>
>> status = usb_control_msg_recv(udev, 0, REALTEK_USB_VENQT_CMD_REQ,
>> REALTEK_USB_VENQT_READ, addr,
>> @@ -39,7 +39,7 @@ static int usb_read(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size)
>> * exist or is not enabled.
>> */
>> adapt->bSurpriseRemoved = true;
>> - goto mutex_unlock;
>> + goto end;
>> }
>>
>> if (status < 0) {
>> @@ -49,15 +49,14 @@ static int usb_read(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size)
>> if (rtw_inc_and_chk_continual_urb_error(dvobjpriv))
>> adapt->bSurpriseRemoved = true;
>>
>> - goto mutex_unlock;
>> + goto end;
>> }
>>
>> rtw_reset_continual_urb_error(dvobjpriv);
>> memcpy(data, io_buf, size);
>>
>> -mutex_unlock:
>> - mutex_unlock(&dvobjpriv->usb_vendor_req_mutex);
>> -
>> +end:
>> + kfree(io_buf);
>> return status;
>> }
>>
>> @@ -72,9 +71,10 @@ static int usb_write(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size)
>> if (adapt->bSurpriseRemoved || adapt->pwrctrlpriv.pnp_bstop_trx)
>> return -EPERM;
>>
>> - mutex_lock(&dvobjpriv->usb_vendor_req_mutex);
>> + io_buf = kmalloc(size, GFP_KERNEL);
>> + if (!io_buf)
>> + return -ENOMEM;
>>
>> - io_buf = dvobjpriv->usb_vendor_req_buf;
>> memcpy(io_buf, data, size);
>>
>> status = usb_control_msg_send(udev, 0, REALTEK_USB_VENQT_CMD_REQ,
>> @@ -93,7 +93,7 @@ static int usb_write(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size)
>> * exist or is not enabled.
>> */
>> adapt->bSurpriseRemoved = true;
>> - goto mutex_unlock;
>> + goto end;
>> }
>>
>> if (status < 0) {
>> @@ -103,14 +103,13 @@ static int usb_write(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size)
>> if (rtw_inc_and_chk_continual_urb_error(dvobjpriv))
>> adapt->bSurpriseRemoved = true;
>>
>> - goto mutex_unlock;
>> + goto end;
>> }
>>
>> rtw_reset_continual_urb_error(dvobjpriv);
>>
>> -mutex_unlock:
>> - mutex_unlock(&dvobjpriv->usb_vendor_req_mutex);
>> -
>> +end:
>> + kfree(io_buf);
>> return status;
>> }
>>
>> diff --git a/drivers/staging/r8188eu/include/drv_types.h b/drivers/staging/r8188eu/include/drv_types.h
>> index 626c6273be6f..499b2bce8cbe 100644
>> --- a/drivers/staging/r8188eu/include/drv_types.h
>> +++ b/drivers/staging/r8188eu/include/drv_types.h
>> @@ -168,11 +168,6 @@ struct dvobj_priv {
>> int ep_num[5]; /* endpoint number */
>> int RegUsbSS;
>> struct semaphore usb_suspend_sema;
>> - struct mutex usb_vendor_req_mutex;
>> -
>> - u8 *usb_alloc_vendor_req_buf;
>> - u8 *usb_vendor_req_buf;
>
> I do like removing these buffers, and I think that is all that this
> change should be doing.
>
> thanks,
>
> greg k-h
>
Powered by blists - more mailing lists