[PATCH 0/3] usb: gadget: f_fs: cancellation and mm fixes

[PATCH 0/3] usb: gadget: f_fs: cancellation and mm fixes

[Ingo Rohloff]

This 3 part patch series does some mini refactoring and
fixes two previous commits.

Ingo Rohloff (3):
  usb: gadget: f_fs: simplify error handling using goto
  usb: gadget: f_fs: register cancellation handler only after request
    queueing
  usb: gadget: f_fs: Fix mmgrab()/mmdrop() balance.

 drivers/usb/gadget/function/f_fs.c | 132 +++++++++++++++--------------
 1 file changed, 68 insertions(+), 64 deletions(-)

--
2.55.0

[PATCH 1/3] usb: gadget: f_fs: simplify error handling using goto

[Ingo Rohloff]

Refactor conditional checks to use direct error returns/gotos instead of a
long if/else chain. This makes failure paths more obvious.

At the end of the function AIO request handling is done.

Signed-off-by: Ingo Rohloff <ingo.rohloff@lauterbach.com>
---
 drivers/usb/gadget/function/f_fs.c | 80 ++++++++++++++++++------------
 1 file changed, 47 insertions(+), 33 deletions(-)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 43962e05eacf..76fba3085664 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -1104,11 +1104,17 @@ 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;
-	} else if (halt) {
+		goto error_lock;
+	}
+
+	if (halt) {
 		ret = usb_ep_set_halt(ep->ep);
 		if (!ret)
 			ret = -EBADMSG;
-	} else if (data_len == -EINVAL) {
+		goto error_lock;
+	}
+
+	if (data_len == -EINVAL) {
 		/*
 		 * Sanity Check: even though data_len can't be used
 		 * uninitialized at the time I write this comment, some
@@ -1122,7 +1128,10 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
 		 */
 		WARN(1, "%s: data_len == -EINVAL\n", __func__);
 		ret = -EINVAL;
-	} else if (!io_data->aio) {
+		goto error_lock;
+	}
+
+	if (!io_data->aio) {
 		bool interrupted = false;
 
 		req = ep->req;
@@ -1176,44 +1185,49 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
 		else
 			ret = io_data->status;
 		goto error_mutex;
-	} else if (!(req = usb_ep_alloc_request(ep->ep, GFP_ATOMIC))) {
+	}
+
+	// It's an AIO request
+	req = usb_ep_alloc_request(ep->ep, GFP_ATOMIC);
+	if (!req) {
 		ret = -ENOMEM;
-	} else {
-		if (io_data->use_sg) {
-			req->buf = NULL;
-			req->sg	= io_data->sgt.sgl;
-			req->num_sgs = io_data->sgt.nents;
-		} else {
-			req->buf = data;
-			req->num_sgs = 0;
-		}
+		goto error_lock;
+	}
 
-		req->zero = !io_data->read ? epfile->zlp_enabled : 0;
-		req->length = data_len;
+	if (io_data->use_sg) {
+		req->buf = NULL;
+		req->sg = io_data->sgt.sgl;
+		req->num_sgs = io_data->sgt.nents;
+	} else {
+		req->buf = data;
+		req->num_sgs = 0;
+	}
 
-		io_data->buf = data;
-		io_data->ep = ep->ep;
-		io_data->req = req;
-		io_data->ffs = epfile->ffs;
+	req->zero = !io_data->read ? epfile->zlp_enabled : 0;
+	req->length = data_len;
 
-		req->context  = io_data;
-		req->complete = ffs_epfile_async_io_complete;
+	io_data->buf = data;
+	io_data->ep = ep->ep;
+	io_data->req = req;
+	io_data->ffs = epfile->ffs;
 
-		ret = usb_ep_queue(ep->ep, req, GFP_ATOMIC);
-		if (ret) {
-			io_data->req = NULL;
-			usb_ep_free_request(ep->ep, req);
-			goto error_lock;
-		}
+	req->context  = io_data;
+	req->complete = ffs_epfile_async_io_complete;
 
-		ret = -EIOCBQUEUED;
-		/*
-		 * Do not kfree the buffer in this function.  It will be freed
-		 * by ffs_user_copy_worker.
-		 */
-		data = NULL;
+	ret = usb_ep_queue(ep->ep, req, GFP_ATOMIC);
+	if (ret) {
+		io_data->req = NULL;
+		usb_ep_free_request(ep->ep, req);
+		goto error_lock;
 	}
 
+	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:
-- 
2.55.0

[PATCH 2/3] usb: gadget: f_fs: register cancellation handler only after request queueing

[Ingo Rohloff]

Call kiocb_set_cancel_fn() only after usb_ep_queue() completes
successfully.

This also undoes commit e78dcb1f7ec2 ("usb: gadget: f_fs: Fix
Use-After-Free in AIO error path"): The described error is avoided by
making sure the cancellation handler is only registered, if there is
something to cancel.

Registering the cancellation handler adds the kiocb to the context's
active request list (active_reqs in aio.c).  If usb_ep_queue() fails, the
request was never actually submitted to the hardware, which means the
request is not active.  The aio framework already deals with calling the
completion function ki_complete() automatically if you do not return
-EIOCBQUEUED.

Signed-off-by: Ingo Rohloff <ingo.rohloff@lauterbach.com>
---
 drivers/usb/gadget/function/f_fs.c | 50 ++++++++++++------------------
 1 file changed, 20 insertions(+), 30 deletions(-)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 76fba3085664..9dec09d06ca9 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -1014,8 +1014,21 @@ static struct ffs_ep *ffs_epfile_wait_ep(struct ffs_epfile *epfile, struct file
 	return ep;
 }
 
-static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
+static int ffs_aio_cancel(struct kiocb *kiocb)
 {
+	struct ffs_io_data *io_data = kiocb->private;
+	int value;
+
+	if (!io_data || !io_data->ep || !io_data->req)
+		return -EINVAL;
+
+	value = usb_ep_dequeue(io_data->ep, io_data->req);
+	return value;
+}
+
+static ssize_t ffs_epfile_io(struct kiocb *kiocb, struct ffs_io_data *io_data)
+{
+	struct file *file = kiocb->ki_filp;
 	struct ffs_epfile *epfile = file->private_data;
 	struct usb_request *req;
 	struct ffs_ep *ep;
@@ -1227,6 +1240,8 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data)
 	 * by ffs_user_copy_worker.
 	 */
 	data = NULL;
+	kiocb->private = io_data;
+	kiocb_set_cancel_fn(kiocb, ffs_aio_cancel);
 
 error_lock:
 	spin_unlock_irq(&epfile->ffs->eps_lock);
@@ -1266,19 +1281,6 @@ ffs_epfile_open(struct inode *inode, struct file *file)
 	return stream_open(inode, file);
 }
 
-static int ffs_aio_cancel(struct kiocb *kiocb)
-{
-	struct ffs_io_data *io_data = kiocb->private;
-	int value;
-
-	if (io_data && io_data->ep && io_data->req)
-		value = usb_ep_dequeue(io_data->ep, io_data->req);
-	else
-		value = -EINVAL;
-
-	return value;
-}
-
 static ssize_t ffs_epfile_write_iter(struct kiocb *kiocb, struct iov_iter *from)
 {
 	struct ffs_io_data io_data, *p = &io_data;
@@ -1299,21 +1301,15 @@ static ssize_t ffs_epfile_write_iter(struct kiocb *kiocb, struct iov_iter *from)
 	p->data = *from;
 	p->mm = current->mm;
 
-	kiocb->private = p;
-
-	if (p->aio) {
+	if (p->aio)
 		mmgrab(p->mm);
-		kiocb_set_cancel_fn(kiocb, ffs_aio_cancel);
-	}
 
-	res = ffs_epfile_io(kiocb->ki_filp, p);
+	res = ffs_epfile_io(kiocb, p);
 	if (res == -EIOCBQUEUED)
 		return res;
 	if (p->aio) {
-		kiocb->ki_complete(kiocb, res);
 		mmdrop(p->mm);
 		kfree(p);
-		return -EIOCBQUEUED;
 	} else {
 		*from = p->data;
 	}
@@ -1349,23 +1345,17 @@ static ssize_t ffs_epfile_read_iter(struct kiocb *kiocb, struct iov_iter *to)
 	}
 	p->mm = current->mm;
 
-	kiocb->private = p;
-
-	if (p->aio) {
+	if (p->aio)
 		mmgrab(p->mm);
-		kiocb_set_cancel_fn(kiocb, ffs_aio_cancel);
-	}
 
-	res = ffs_epfile_io(kiocb->ki_filp, p);
+	res = ffs_epfile_io(kiocb, p);
 	if (res == -EIOCBQUEUED)
 		return res;
 
 	if (p->aio) {
-		kiocb->ki_complete(kiocb, res);
 		mmdrop(p->mm);
 		kfree(p->to_free);
 		kfree(p);
-		return -EIOCBQUEUED;
 	} else {
 		*to = p->data;
 	}
-- 
2.55.0

[PATCH 3/3] usb: gadget: f_fs: Fix mmgrab()/mmdrop() balance.

[Ingo Rohloff]

This fixes commit 5eb5c72c72fe ("USB: gadget: ffs: fix mm lifetime
handling").  The problem is that the previous patch calls mmgrab() for
reads and writes, but it only calls mmdrop() for reads.  So with each
completed async IO write (via an USB IN endpoint) the number of mm_count
increases.

Signed-off-by: Ingo Rohloff <ingo.rohloff@lauterbach.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 9dec09d06ca9..0be989c7c23f 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -884,8 +884,8 @@ static void ffs_user_copy_worker(struct work_struct *work)
 		} else {
 			ret = -EFAULT;
 		}
-		mmdrop(io_data->mm);
 	}
+	mmdrop(io_data->mm);
 
 	io_data->kiocb->ki_complete(io_data->kiocb, ret);
 
-- 
2.55.0