Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1301118 > unrolled thread

[PATCH 1/5] usb: f_fs: fix memory leak when ep changes during transfer

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/5] usb: f_fs: fix memory leak when ep changes during transfer Michal Nazarewicz <mina86@mina86.com> - 2016-01-04 22:50 +0100
    [PATCH 3/5] usb: f_fs: replace unnecessary goto with a return Michal Nazarewicz <mina86@mina86.com> - 2016-01-04 22:50 +0100
    [PATCH 4/5] usb: f_fs: refactor ffs_epfile_io Michal Nazarewicz <mina86@mina86.com> - 2016-01-04 22:50 +0100
    [PATCH 2/5] usb: f_fs: fix ffs_epfile_io returning success on req alloc failure Michal Nazarewicz <mina86@mina86.com> - 2016-01-04 22:50 +0100
    [PATCH 5/5] usb: f_fs: avoid race condition with ffs_epfile_io_complete Michal Nazarewicz <mina86@mina86.com> - 2016-01-04 22:50 +0100

#1301118 — [PATCH 1/5] usb: f_fs: fix memory leak when ep changes during transfer

In the ffs_epfile_io function, data buffer is allocated for non-halt
requests.  Later, after grabing a mutex, the function checks that
epfile->ep is still ep and if it’s not, it set ret to -ESHUTDOWN and
follow a path including spin_unlock_irq (just after ‘ret = -ESHUTDOWN’),
mutex_unlock (after if-else-if-else chain) and returns ret.  Noticeably,
this does not include freeing of the data buffer.

Fix by introducing a goto which moves control flow to the the end of the
function where spin_unlock_irq, mutex_unlock and kfree are all called.

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 drivers/usb/gadget/function/f_fs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index cf43e9e..d1a4a86 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -763,7 +763,7 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
 	if (epfile->ep != ep) {
 		/* In the meantime, endpoint got disabled or changed. */
 		ret = -ESHUTDOWN;
-		spin_unlock_irq(&epfile->ffs->eps_lock);
+		goto error_lock;
 	} else if (halt) {
 		/* Halt */
 		if (likely(epfile->ep == ep) && !WARN_ON(!ep->ep))
-- 
2.6.0.rc2.230.g3dd15c0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1301120 — [PATCH 3/5] usb: f_fs: replace unnecessary goto with a return

In ffs_epfile_io error label points to a return path which includes
a kfree(data) call.  However, at the beginning of the function data is
always NULL so some of the early ‘goto error’ can safely be replaced
with a trivial return statement.

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 drivers/usb/gadget/function/f_fs.c | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 1384220..c4e6395 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -690,32 +690,24 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
 	int halt;
 
 	/* Are we still active? */
-	if (WARN_ON(epfile->ffs->state != FFS_ACTIVE)) {
-		ret = -ENODEV;
-		goto error;
-	}
+	if (WARN_ON(epfile->ffs->state != FFS_ACTIVE))
+		return -ENODEV;
 
 	/* Wait for endpoint to be enabled */
 	ep = epfile->ep;
 	if (!ep) {
-		if (file->f_flags & O_NONBLOCK) {
-			ret = -EAGAIN;
-			goto error;
-		}
+		if (file->f_flags & O_NONBLOCK)
+			return -EAGAIN;
 
 		ret = wait_event_interruptible(epfile->wait, (ep = epfile->ep));
-		if (ret) {
-			ret = -EINTR;
-			goto error;
-		}
+		if (ret)
+			return -EINTR;
 	}
 
 	/* Do we halt? */
 	halt = (!io_data->read == !epfile->in);
-	if (halt && epfile->isoc) {
-		ret = -EINVAL;
-		goto error;
-	}
+	if (halt && epfile->isoc)
+		return -EINVAL;
 
 	/* Allocate & copy */
 	if (!halt) {
-- 
2.6.0.rc2.230.g3dd15c0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1301121 — [PATCH 4/5] usb: f_fs: refactor ffs_epfile_io

Eliminate one of the return paths by using a ‘goto error_mutex’ and
rearrange some if-bodies which results in reduction of the indention level
and thus hopefully makes the function easier to read and reason about.

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 drivers/usb/gadget/function/f_fs.c | 127 +++++++++++++++++--------------------
 1 file changed, 58 insertions(+), 69 deletions(-)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index c4e6395..63fe693 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -684,6 +684,7 @@ static void ffs_epfile_async_io_complete(struct usb_ep *_ep,
 static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
 {
 	struct ffs_epfile *epfile = file->private_data;
+	struct usb_request *req;
 	struct ffs_ep *ep;
 	char *data = NULL;
 	ssize_t ret, data_len = -EINVAL;
@@ -713,7 +714,8 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
 	if (!halt) {
 		/*
 		 * if we _do_ wait above, the epfile->ffs->gadget might be NULL
-		 * before the waiting completes, so do not assign to 'gadget' earlier
+		 * before the waiting completes, so do not assign to 'gadget'
+		 * earlier
 		 */
 		struct usb_gadget *gadget = epfile->ffs->gadget;
 		size_t copied;
@@ -755,17 +757,12 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
 	if (epfile->ep != ep) {
 		/* In the meantime, endpoint got disabled or changed. */
 		ret = -ESHUTDOWN;
-		goto error_lock;
 	} else if (halt) {
 		/* Halt */
 		if (likely(epfile->ep == ep) && !WARN_ON(!ep->ep))
 			usb_ep_set_halt(ep->ep);
-		spin_unlock_irq(&epfile->ffs->eps_lock);
 		ret = -EBADMSG;
-	} else {
-		/* Fire the request */
-		struct usb_request *req;
-
+	} else if (unlikely(data_len == -EINVAL)) {
 		/*
 		 * Sanity Check: even though data_len can't be used
 		 * uninitialized at the time I write this comment, some
@@ -777,82 +774,74 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
 		 * For such reason, we're adding this redundant sanity check
 		 * here.
 		 */
-		if (unlikely(data_len == -EINVAL)) {
-			WARN(1, "%s: data_len == -EINVAL\n", __func__);
-			ret = -EINVAL;
-			goto error_lock;
-		}
-
-		if (io_data->aio) {
-			req = usb_ep_alloc_request(ep->ep, GFP_KERNEL);
-			if (unlikely(!req)) {
-				ret = -ENOMEM;
-				goto error_lock;
-			}
-
-			req->buf      = data;
-			req->length   = data_len;
+		WARN(1, "%s: data_len == -EINVAL\n", __func__);
+		ret = -EINVAL;
+	} else if (!io_data->aio) {
+		DECLARE_COMPLETION_ONSTACK(done);
 
-			io_data->buf = data;
-			io_data->ep = ep->ep;
-			io_data->req = req;
-			io_data->ffs = epfile->ffs;
+		req = ep->req;
+		req->buf      = data;
+		req->length   = data_len;
 
-			req->context  = io_data;
-			req->complete = ffs_epfile_async_io_complete;
+		req->context  = &done;
+		req->complete = ffs_epfile_io_complete;
 
-			ret = usb_ep_queue(ep->ep, req, GFP_ATOMIC);
-			if (unlikely(ret)) {
-				usb_ep_free_request(ep->ep, req);
-				goto error_lock;
-			}
-			ret = -EIOCBQUEUED;
+		ret = usb_ep_queue(ep->ep, req, GFP_ATOMIC);
+		if (unlikely(ret < 0))
+			goto error_lock;
 
-			spin_unlock_irq(&epfile->ffs->eps_lock);
-		} else {
-			DECLARE_COMPLETION_ONSTACK(done);
+		spin_unlock_irq(&epfile->ffs->eps_lock);
 
-			req = ep->req;
-			req->buf      = data;
-			req->length   = data_len;
+		if (unlikely(wait_for_completion_interruptible(&done))) {
+			ret = -EINTR;
+			usb_ep_dequeue(ep->ep, req);
+			goto error_mutex;
+		}
 
-			req->context  = &done;
-			req->complete = ffs_epfile_io_complete;
+		/*
+		 * XXX We may end up silently droping data here.  Since data_len
+		 * (i.e. req->length) may be bigger than len (after being
+		 * rounded up to maxpacketsize), we may end up with more data
+		 * then user space has space for.
+		 */
+		ret = ep->status;
+		if (io_data->read && ret > 0) {
+			ret = copy_to_iter(data, ret, &io_data->data);
+			if (!ret)
+				ret = -EFAULT;
+		}
+		goto error_mutex;
+	} else if (!(req = usb_ep_alloc_request(ep->ep, GFP_KERNEL))) {
+		ret = -ENOMEM;
+	} else {
+		req->buf      = data;
+		req->length   = data_len;
 
-			ret = usb_ep_queue(ep->ep, req, GFP_ATOMIC);
+		io_data->buf = data;
+		io_data->ep = ep->ep;
+		io_data->req = req;
+		io_data->ffs = epfile->ffs;
 
-			spin_unlock_irq(&epfile->ffs->eps_lock);
+		req->context  = io_data;
+		req->complete = ffs_epfile_async_io_complete;
 
-			if (unlikely(ret < 0)) {
-				/* nop */
-			} else if (unlikely(
-				   wait_for_completion_interruptible(&done))) {
-				ret = -EINTR;
-				usb_ep_dequeue(ep->ep, req);
-			} else {
-				/*
-				 * XXX We may end up silently droping data
-				 * here.  Since data_len (i.e. req->length) may
-				 * be bigger than len (after being rounded up
-				 * to maxpacketsize), we may end up with more
-				 * data then user space has space for.
-				 */
-				ret = ep->status;
-				if (io_data->read && ret > 0) {
-					ret = copy_to_iter(data, ret, &io_data->data);
-					if (!ret)
-						ret = -EFAULT;
-				}
-			}
-			kfree(data);
+		ret = usb_ep_queue(ep->ep, req, GFP_ATOMIC);
+		if (unlikely(ret)) {
+			usb_ep_free_request(ep->ep, req);
+			goto error_lock;
 		}
-	}
 
-	mutex_unlock(&epfile->mutex);
-	return ret;
+		ret = -EIOCBQUEUED;
+		/*
+		 * Do not kfree the buffer in this function.  It will be freed
+		 * by ffs_user_copy_worker.
+		 */
+		data = NULL;
+	}
 
 error_lock:
 	spin_unlock_irq(&epfile->ffs->eps_lock);
+error_mutex:
 	mutex_unlock(&epfile->mutex);
 error:
 	kfree(data);
-- 
2.6.0.rc2.230.g3dd15c0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1301122 — [PATCH 2/5] usb: f_fs: fix ffs_epfile_io returning success on req alloc failure

In the AIO path, if allocating of a request failse, the function simply
goes to the error_lock path whose end result is returning value of ret.
However, at this point ret’s value is zero (assigned as return value from
ffs_mutex_lock).

Fix by adding ‘ret = -ENOMEM’ statement.

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 drivers/usb/gadget/function/f_fs.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index d1a4a86..1384220 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -793,8 +793,10 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
 
 		if (io_data->aio) {
 			req = usb_ep_alloc_request(ep->ep, GFP_KERNEL);
-			if (unlikely(!req))
+			if (unlikely(!req)) {
+				ret = -ENOMEM;
 				goto error_lock;
+			}
 
 			req->buf      = data;
 			req->length   = data_len;
-- 
2.6.0.rc2.230.g3dd15c0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1301126 — [PATCH 5/5] usb: f_fs: avoid race condition with ffs_epfile_io_complete

From: "Du, Changbin" <changbin.du@intel.com>

ffs_epfile_io and ffs_epfile_io_complete runs in different context, but
there is no synchronization between them.

consider the following scenario:
1) ffs_epfile_io interrupted by sigal while
wait_for_completion_interruptible
2) then ffs_epfile_io set ret to -EINTR
3) just before or during usb_ep_dequeue, the request completed
4) ffs_epfile_io return with -EINTR

In this case, ffs_epfile_io tell caller no transfer success but actually
it may has been done. This break the caller's pipe.

Below script can help test it (adbd is the process which lies on f_fs).
while true
do
   pkill -19 adbd #SIGSTOP
   pkill -18 adbd #SIGCONT
   sleep 0.1
done

To avoid this, just dequeue the request first. After usb_ep_dequeue, the
request must be done or canceled.

With this change, we can ensure no race condition in f_fs driver. But
actually I found some of the udc driver has analogical issue in its
dequeue implementation. For example,
1) the dequeue function hold the controller's lock.
2) before driver request controller  to stop transfer, a request
   completed.
3) the controller trigger a interrupt, but its irq handler need wait
   dequeue function to release the lock.
4) dequeue function give back the request with negative status, and
   release lock.
5) irq handler get lock but the request has already been given back.

So, the dequeue implementation should take care of this case. IMO, it
can be done as below steps to dequeue a already started request,
1) request controller to stop transfer on the given ep. HW know the
   actual transfer status.
2) after hw stop transfer, driver scan if there are any completed one.
3) if found, process it with real status. if no, the request can
   canceled.

Signed-off-by: "Du, Changbin" <changbin.du@intel.com>
[mina86@mina86.com: rebased on top of refactoring patches]
Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 drivers/usb/gadget/function/f_fs.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 63fe693..8cfce10 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -778,6 +778,7 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
 		ret = -EINVAL;
 	} else if (!io_data->aio) {
 		DECLARE_COMPLETION_ONSTACK(done);
+		bool interrupted = false;
 
 		req = ep->req;
 		req->buf      = data;
@@ -793,9 +794,14 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
 		spin_unlock_irq(&epfile->ffs->eps_lock);
 
 		if (unlikely(wait_for_completion_interruptible(&done))) {
-			ret = -EINTR;
+			/*
+			 * To avoid race condition with ffs_epfile_io_complete,
+			 * dequeue the request first then check
+			 * status. usb_ep_dequeue API should guarantee no race
+			 * condition with req->complete callback.
+			 */
 			usb_ep_dequeue(ep->ep, req);
-			goto error_mutex;
+			interrupted = ep->status < 0;
 		}
 
 		/*
@@ -804,7 +810,7 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
 		 * rounded up to maxpacketsize), we may end up with more data
 		 * then user space has space for.
 		 */
-		ret = ep->status;
+		ret = interrupted ? -EINTR : ep->status;
 		if (io_data->read && ret > 0) {
 			ret = copy_to_iter(data, ret, &io_data->data);
 			if (!ret)
-- 
2.6.0.rc2.230.g3dd15c0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web