[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

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

[Ingo Rohloff]

Hello Neill,

On Fri, 6 Nov 2026 22:47:48 +0000
Neill Kapron <nkapron@google.com> wrote:

> 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.
> > ...
>
> Unfortunately, I don't think this is safe. The cancel handler has to be
> set before the request is queued. 

Why do you think, that the cancel handler has to be set before 
the request is queued?

I can't find any reason for this.

I will describe this in terms of an AIO write (read is the same),
referring to the current kernel (without my patch):

The request enters the kernel via
SYSCALL_DEFINE3(io_submit
...
  io_submit_one(ctx, user_iocb, false);
    __io_submit_one(ctx, &iocb, user_iocb, req, compat);   
      ...
        case IOCB_CMD_PWRITE:
          return aio_write(&req->rw, iocb, false, compat);
      aio_write
          ...
          aio_rw_done(req, file->f_op->write_iter(req, &iter));
            file->f_op->write_iter == ffs_epfile_write_iter
        ffs_epfile_write_iter(struct kiocb *kiocb, struct iov_iter *from)
            kiocb_set_cancel_fn(...)
            ffs_epfile_io(...)
              ret = usb_ep_queue(ep->ep, req, GFP_ATOMIC);


I can't see why you should need to call "kiocb_set_cancel_fn(...)"
before you call "usb_ep_queue()".
In fact I think that's exactly wrong:
You should **NOT** call "kiocb_set_cancel_fn(...)",
before the request is successfully queued via "usb_ep_queue()".

Unfortunately f_fs.c seems the only place where "kiocb_set_cancel_fn()"
is used, so there is no other code to compare to.

If "usb_ep_queue()" fails, then "ffs_epfile_io()"
will **NOT** return -EIOCBQUEUED.

This in turn means the code in "aio_rw_done(...)"
will call "req->ki_complete(req, ret)", because that's
what "aio_rw_done(...)" does, if you **DO NOT** return -EIOCBQUEUED.

As far as I can tell, this is exactly what we want:
The request is completed with an error,
which indicates that the request was not queued.

Because the request is completed, why should you ever
set a cancel handler for it?
So why should you call kiocb_set_cancel_fn before the
request is queued successfully?

with best regards
  Ingo

-- 

-------------------------------------------------------------------------
Dipl.-Inform.
Ingo ROHLOFF
Senior Staff Engineer
phone +49 8102 9876-142 - ingo.rohloff@lauterbach.com

Lauterbach Engineering GmbH & Co. KG
Altlaufstrasse 40, 85635 Hoehenkirchen-Siegertsbrunn, GERMANY
www.lauterbach.com

Registered Office: Hoehenkirchen-Siegertsbrunn, Germany,
Local Court: Munich, HRA 87406, VAT ID: DE246770537, 
Managing Directors: Stephan Lauterbach, Dr. Thomas Ullmann

-------------------------------------------------------------------------

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

[Ingo Rohloff]

Hello Neill,

> On Fri, 6 Nov 2026 22:47:48 +0000
> Neill Kapron <nkapron@google.com> wrote:
> 
> > 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.
> > > ...  
> >
> > Unfortunately, I don't think this is safe. The cancel handler has to be
> > set before the request is queued.  
> ...
> You should **NOT** call "kiocb_set_cancel_fn(...)",
> before the request is successfully queued via "usb_ep_queue()".

I just had another look at the code and I am even more convinced than
before that commit  ("usb: gadget: f_fs: Fix Use-After-Free in
AIO error path") is wrong. Especially this code part

	if (res == -EIOCBQUEUED)
		return res;
	/* Comment by Ingo: Code here menans there was an error */
	if (p->aio) {
		kiocb->ki_complete(kiocb, res);
		mmdrop(p->mm);
		kfree(p);
		return -EIOCBQUEUED;
	}

Returning "-EIOCBQUEUED" when in fact nothing has been queued
(because of an error) doesn't make any sense to me.

As far as I can tell, the whole point of returning "-EIOCBQUEUED" is  to tell
aio.c that something is "in progress"; which is exactly  the opposite of
what's happening here.

Returning -EIOCBQUEUED is just a hack here, to prevent
"aio_rw_done(...)" (in aio.c) from calling "kiocb->ki_complete(...)" after 
ffs_epfile_write_iter/ffs_epfile_read_iter
has returned.

with best regards
  Ingo

On Mon, 9 Nov 2026 12:31:40 +0100
Ingo Rohloff <ingo.rohloff@lauterbach.com> wrote:

> Hello Neill,
> 
> On Fri, 6 Nov 2026 22:47:48 +0000
> Neill Kapron <nkapron@google.com> wrote:
> 
> > 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.
> > > ...  
> >
> > Unfortunately, I don't think this is safe. The cancel handler has to be
> > set before the request is queued.   
> 
> Why do you think, that the cancel handler has to be set before 
> the request is queued?
> 
> I can't find any reason for this.
> 
> I will describe this in terms of an AIO write (read is the same),
> referring to the current kernel (without my patch):
> 
> The request enters the kernel via
> SYSCALL_DEFINE3(io_submit
> ...
>   io_submit_one(ctx, user_iocb, false);
>     __io_submit_one(ctx, &iocb, user_iocb, req, compat);   
>       ...
>         case IOCB_CMD_PWRITE:
>           return aio_write(&req->rw, iocb, false, compat);
>       aio_write
>           ...
>           aio_rw_done(req, file->f_op->write_iter(req, &iter));
>             file->f_op->write_iter == ffs_epfile_write_iter
>         ffs_epfile_write_iter(struct kiocb *kiocb, struct iov_iter *from)
>             kiocb_set_cancel_fn(...)
>             ffs_epfile_io(...)
>               ret = usb_ep_queue(ep->ep, req, GFP_ATOMIC);
> 
> 
> I can't see why you should need to call "kiocb_set_cancel_fn(...)"
> before you call "usb_ep_queue()".
> In fact I think that's exactly wrong:
> You should **NOT** call "kiocb_set_cancel_fn(...)",
> before the request is successfully queued via "usb_ep_queue()".
> 
> Unfortunately f_fs.c seems the only place where "kiocb_set_cancel_fn()"
> is used, so there is no other code to compare to.
> 
> If "usb_ep_queue()" fails, then "ffs_epfile_io()"
> will **NOT** return -EIOCBQUEUED.
> 
> This in turn means the code in "aio_rw_done(...)"
> will call "req->ki_complete(req, ret)", because that's
> what "aio_rw_done(...)" does, if you **DO NOT** return -EIOCBQUEUED.
> 
> As far as I can tell, this is exactly what we want:
> The request is completed with an error,
> which indicates that the request was not queued.
> 
> Because the request is completed, why should you ever
> set a cancel handler for it?
> So why should you call kiocb_set_cancel_fn before the
> request is queued successfully?
> 
> with best regards
>   Ingo
> 



-- 

-------------------------------------------------------------------------
Dipl.-Inform.
Ingo ROHLOFF
Senior Staff Engineer
phone +49 8102 9876-142 - ingo.rohloff@lauterbach.com

Lauterbach Engineering GmbH & Co. KG
Altlaufstrasse 40, 85635 Hoehenkirchen-Siegertsbrunn, GERMANY
www.lauterbach.com

Registered Office: Hoehenkirchen-Siegertsbrunn, Germany,
Local Court: Munich, HRA 87406, VAT ID: DE246770537, 
Managing Directors: Stephan Lauterbach, Dr. Thomas Ullmann

-------------------------------------------------------------------------

[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