[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

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

[Neill Kapron]

On Tue, Sep 01, 2026 at 02:09:06PM +0200, Ingo Rohloff wrote:

> +	// It's an AIO request

Nit: please use /* */ style comments to match the surrounding code.

>  
> -		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;

Nit: since we're already changing this line, can you cleanup the double
spacing?


Other that the two minor nits above, this looks reasonable. Since patch
2 shouldn't go in, can you resend this separately with the above items
addressed?

Thanks,
Neill

[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

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

[Neill Kapron]

On Tue, Sep 01, 2026 at 02:09:07PM +0200, Ingo Rohloff wrote:
> 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.

Hi Ingo,

Unfortunately, I don't think this is safe. The cancel handler has to be
set before the request is queued. 

Moving the registration to just before usb_ep_queue() would close the
potential race, but if the queue fails, you still have to unwind- the
kiocb is already on active_reqs, so it has to be removed before io_data
is freed. 

That ends up more complex than what we have today, so I'm going to NAK
this patch.

Thanks,
Neill

[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

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

[Hongyu Xie]

Hi Ingo,

I ran into the same imbalance while carrying a backport of 5eb5c72c72fe in
a 5.10-based product tree, and your analysis matches what I see.

mmgrab() is taken in both ffs_epfile_read_iter() and ffs_epfile_write_iter()
when the request is queued for AIO, but ffs_user_copy_worker() drops the
reference only inside

	if (io_data->read && ret > 0)

so the reference is lost on every completed AIO write, and also on an AIO
read that completes with ret <= 0. Moving mmdrop() out of the block fixes
both cases, and it stays balanced with the mmgrab() in the iter functions.

The released stable trees carry the same code: 6.6.160, 6.12.111, 7.2.y and
rolling-lts (6.18.y) all still have mmdrop() inside the if. 5.10.y and
5.15.y are not affected, they still use the older kthread_use_mm() code
without mmgrab().

It would help to add

	Fixes: 5eb5c72c72fe ("USB: gadget: ffs: fix mm lifetime handling")
	Cc: stable@vger.kernel.org

so the stable trees pick it up; the change itself applies there with only
offset differences.

Reviewed-by: Hongyu Xie <xiehongyu1@kylinos.cn>

Thanks,
Hongyu

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

[Neill Kapron]

On Tue, Sep 01, 2026 at 02:09:08PM +0200, Ingo Rohloff wrote:
> 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
>

Hi Ingo,

This looks correct to me, and has been reported to me separately. Can
you resend this change on its own ahead of the rest of the series and
include the Fixes: and Cc: stable tags as Hongyu already pointed out?

With that:

Reviewed-by: Neill Kapron <nkapron@google.com>

Thanks,
Neill