| 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
| ||
|
Date: Mon, 11 Nov 2013 15:21:58 -0800
From: David Cohen <david.a.cohen@...ux.intel.com>
To: David Cohen <david.a.cohen@...ux.intel.com>
CC: balbi@...com, gregkh@...uxfoundation.org,
stern@...land.harvard.edu, mina86@...a86.com,
linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 3/5] usb: gadget: f_fs: remove loop from I/O function
On 11/11/2013 12:16 PM, David Cohen wrote:
> From: Michal Nazarewicz <mina86@...a86.com>
>
> When endpoint changes (due to it being disabled or alt setting changed),
> mimic the action as if the change happened after the request has been
> queued, instead of retrying with the new endpoint.
>
> Signed-off-by: Michal Nazarewicz <mina86@...a86.com>
> Cc: David Cohen <david.a.cohen@...ux.intel.com>
> ---
> drivers/usb/gadget/f_fs.c | 94 +++++++++++++++++++++--------------------------
> 1 file changed, 41 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
> index 75e4b7846a8d..f1563c47e0c2 100644
> --- a/drivers/usb/gadget/f_fs.c
> +++ b/drivers/usb/gadget/f_fs.c
> @@ -756,74 +756,61 @@ static ssize_t ffs_epfile_io(struct file *file,
> {
> struct ffs_epfile *epfile = file->private_data;
> struct ffs_ep *ep;
> - char *data = NULL;
> ssize_t ret;
> + char *data;
> int halt;
>
> - goto first_try;
> - do {
> - spin_unlock_irq(&epfile->ffs->eps_lock);
> - mutex_unlock(&epfile->mutex);
> + /* Are we still active? */
> + if (WARN_ON(epfile->ffs->state != FFS_ACTIVE)) {
> + ret = -ENODEV;
> + goto error;
> + }
>
> -first_try:
> - /* Are we still active? */
> - if (WARN_ON(epfile->ffs->state != FFS_ACTIVE)) {
> - ret = -ENODEV;
> + /* Wait for endpoint to be enabled */
> + ep = epfile->ep;
> + if (!ep) {
> + if (file->f_flags & O_NONBLOCK) {
> + ret = -EAGAIN;
> goto error;
> }
>
> - /* Wait for endpoint to be enabled */
> - ep = epfile->ep;
> - if (!ep) {
> - if (file->f_flags & O_NONBLOCK) {
> - ret = -EAGAIN;
> - goto error;
> - }
> -
> - if (wait_event_interruptible(epfile->wait,
> - (ep = epfile->ep))) {
> - ret = -EINTR;
> - goto error;
> - }
> - }
> -
> - /* Do we halt? */
> - halt = !read == !epfile->in;
> - if (halt && epfile->isoc) {
> - ret = -EINVAL;
> + if (wait_event_interruptible(epfile->wait, (ep = epfile->ep))) {
> + ret = -EINTR;
> goto error;
> }
> + }
>
> - /* Allocate & copy */
> - if (!halt && !data) {
> - data = kzalloc(len, GFP_KERNEL);
> - if (unlikely(!data))
> - return -ENOMEM;
> + /* Do we halt? */
> + halt = !read == !epfile->in;
One curiosity here. This patch prints the following warning:
In file included from (...)/drivers/usb/gadget/g_ffs.c:55:0:
(...)/drivers/usb/gadget/f_fs.c: In function 'ffs_epfile_io.isra.18':
(...)/drivers/usb/gadget/f_fs.c:837:15: warning: 'data_len' may be used
uninitialized in this function [-Wmaybe-uninitialized]
But if we do this dummy change on it:
@@ -782,7 +782,10 @@ static ssize_t ffs_epfile_io(struct file *file,
}
/* Do we halt? */
- halt = !read == !epfile->in;
+ if (!read == !epfile->in)
+ halt = 1;
+ else
+ halt = 0;
if (halt && epfile->isoc) {
ret = -EINVAL;
goto error;
The warning goes away. This false-positive warning comes out of this
gcc version:
$ i686-linux-android-gcc --version
i686-linux-android-gcc (GCC) 4.7
Br, David Cohen
> + if (halt && epfile->isoc) {
> + ret = -EINVAL;
> + goto error;
> + }
>
> - if (!read &&
> - unlikely(__copy_from_user(data, buf, len))) {
> - ret = -EFAULT;
> - goto error;
> - }
> - }
> + /* Allocate & copy */
> + if (!halt) {
> + data = kmalloc(len, GFP_KERNEL);
> + if (unlikely(!data))
> + return -ENOMEM;
>
> - /* We will be using request */
> - ret = ffs_mutex_lock(&epfile->mutex,
> - file->f_flags & O_NONBLOCK);
> - if (unlikely(ret))
> + if (!read && unlikely(copy_from_user(data, buf, len))) {
> + ret = -EFAULT;
> goto error;
> + }
> + }
>
> - /*
> - * We're called from user space, we can use _irq rather then
> - * _irqsave
> - */
> - spin_lock_irq(&epfile->ffs->eps_lock);
> + /* We will be using request */
> + ret = ffs_mutex_lock(&epfile->mutex, file->f_flags & O_NONBLOCK);
> + if (unlikely(ret))
> + goto error;
> + spin_lock_irq(&epfile->ffs->eps_lock);
>
> - /*
> - * While we were acquiring mutex endpoint got disabled
> - * or changed?
> - */
> - } while (unlikely(epfile->ep != ep));
> + /* While we were acquiring mutex endpoint got disabled or changed. */
> + if (epfile->ep != ep) {
> + ret = -ESHUTDOWN;
> + spin_unlock_irq(&epfile->ffs->eps_lock);
> + goto error_unlock;
> + }
>
> /* Halt */
> if (unlikely(halt)) {
> @@ -858,6 +845,7 @@ first_try:
> }
> }
>
> +error_unlock:
> mutex_unlock(&epfile->mutex);
> error:
> kfree(data);
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists