| Message ID | 1653989775-14267-2-git-send-email-quic_linyyuan@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: f_fs: safe operation in ffs_epfile_io() | expand |
On Tue, May 31, 2022 at 05:36:14PM +0800, Linyu Yuan wrote: > If a task read/write data in blocking mode, it will wait the completion > in ffs_epfile_io(), if function unbind occurs, ffs_func_unbind() will > kfree ffs ep, once the task wake up, it still dereference the ffs ep to > obtain the request status. > > Fix it by moving the request status to io_data which is stack-safe. > > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> One minor issue below, but otherwise I like this approach and it will help solving a similar issue on the AIO path which I spotted while looking into this (ffs_func_unbind() calls ffs_func_eps_disable() setting epfile->ep = NULL before draining the completion workqueue where ffs_user_copy_worker() relies on epfile->ep being non-null). > --- > drivers/usb/gadget/function/f_fs.c | 32 +++++++++++++++++--------------- > 1 file changed, 17 insertions(+), 15 deletions(-) > > diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c > index 4585ee3..dcba835 100644 > --- a/drivers/usb/gadget/function/f_fs.c > +++ b/drivers/usb/gadget/function/f_fs.c > @@ -122,8 +122,6 @@ struct ffs_ep { > struct usb_endpoint_descriptor *descs[3]; > > u8 num; > - > - int status; /* P: epfile->mutex */ > }; > > struct ffs_epfile { > @@ -227,6 +225,9 @@ struct ffs_io_data { > bool use_sg; > > struct ffs_data *ffs; > + > + int status; > + struct completion done; > }; > > struct ffs_desc_helper { > @@ -707,12 +708,12 @@ static const struct file_operations ffs_ep0_operations = { > > static void ffs_epfile_io_complete(struct usb_ep *_ep, struct usb_request *req) > { > + struct ffs_io_data *io_data = req->context; > + > ENTER(); > - if (req->context) { > - struct ffs_ep *ep = _ep->driver_data; > - ep->status = req->status ? req->status : req->actual; > - complete(req->context); > - } > + > + io_data->status = req->status ? req->status : req->actual; > + complete(&io_data->done); > } > > static ssize_t ffs_copy_to_iter(void *data, int data_len, struct iov_iter *iter) > @@ -1050,7 +1051,6 @@ 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) { > - DECLARE_COMPLETION_ONSTACK(done); > bool interrupted = false; > > req = ep->req; > @@ -1066,7 +1066,8 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data) > > io_data->buf = data; > > - req->context = &done; > + init_completion(&io_data->done); > + req->context = io_data; > req->complete = ffs_epfile_io_complete; > > ret = usb_ep_queue(ep->ep, req, GFP_ATOMIC); > @@ -1075,7 +1076,7 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data) > > spin_unlock_irq(&epfile->ffs->eps_lock); > > - if (wait_for_completion_interruptible(&done)) { > + if (wait_for_completion_interruptible(&io_data->done)) { > /* > * To avoid race condition with ffs_epfile_io_complete, > * dequeue the request first then check > @@ -1083,17 +1084,18 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data) > * condition with req->complete callback. > */ > usb_ep_dequeue(ep->ep, req); > - wait_for_completion(&done); > - interrupted = ep->status < 0; > + wait_for_completion(&io_data->done); > + interrupted = true; This is a change in behaviour - shouldn't this be: interrupted = io_data->status; ? Otherwise data is lost unnecessarily if the request completes successfully in the small window between cancellation and being dequeued. > } > > if (interrupted) > ret = -EINTR; > - else if (io_data->read && ep->status > 0) > - ret = __ffs_epfile_read_data(epfile, data, ep->status, > + else if (io_data->read && io_data->status > 0) > + ret = __ffs_epfile_read_data(epfile, data, io_data->status, > &io_data->data); > else > - ret = ep->status; > + ret = io_data->status; > + > goto error_mutex; > } else if (!(req = usb_ep_alloc_request(ep->ep, GFP_ATOMIC))) { > ret = -ENOMEM; > -- > 2.7.4 >
On 5/31/2022 7:44 PM, John Keeping wrote: > On Tue, May 31, 2022 at 05:36:14PM +0800, Linyu Yuan wrote: >> If a task read/write data in blocking mode, it will wait the completion >> in ffs_epfile_io(), if function unbind occurs, ffs_func_unbind() will >> kfree ffs ep, once the task wake up, it still dereference the ffs ep to >> obtain the request status. >> >> Fix it by moving the request status to io_data which is stack-safe. >> >> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> > One minor issue below, but otherwise I like this approach and it will > help solving a similar issue on the AIO path which I spotted while > looking into this (ffs_func_unbind() calls ffs_func_eps_disable() > setting epfile->ep = NULL before draining the completion workqueue where > ffs_user_copy_worker() relies on epfile->ep being non-null). > >> --- >> drivers/usb/gadget/function/f_fs.c | 32 +++++++++++++++++--------------- >> 1 file changed, 17 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c >> index 4585ee3..dcba835 100644 >> --- a/drivers/usb/gadget/function/f_fs.c >> +++ b/drivers/usb/gadget/function/f_fs.c >> @@ -122,8 +122,6 @@ struct ffs_ep { >> struct usb_endpoint_descriptor *descs[3]; >> >> u8 num; >> - >> - int status; /* P: epfile->mutex */ >> }; >> >> struct ffs_epfile { >> @@ -227,6 +225,9 @@ struct ffs_io_data { >> bool use_sg; >> >> struct ffs_data *ffs; >> + >> + int status; >> + struct completion done; >> }; >> >> struct ffs_desc_helper { >> @@ -707,12 +708,12 @@ static const struct file_operations ffs_ep0_operations = { >> >> static void ffs_epfile_io_complete(struct usb_ep *_ep, struct usb_request *req) >> { >> + struct ffs_io_data *io_data = req->context; >> + >> ENTER(); >> - if (req->context) { >> - struct ffs_ep *ep = _ep->driver_data; >> - ep->status = req->status ? req->status : req->actual; >> - complete(req->context); >> - } >> + >> + io_data->status = req->status ? req->status : req->actual; >> + complete(&io_data->done); >> } >> >> static ssize_t ffs_copy_to_iter(void *data, int data_len, struct iov_iter *iter) >> @@ -1050,7 +1051,6 @@ 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) { >> - DECLARE_COMPLETION_ONSTACK(done); >> bool interrupted = false; >> >> req = ep->req; >> @@ -1066,7 +1066,8 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data) >> >> io_data->buf = data; >> >> - req->context = &done; >> + init_completion(&io_data->done); >> + req->context = io_data; >> req->complete = ffs_epfile_io_complete; >> >> ret = usb_ep_queue(ep->ep, req, GFP_ATOMIC); >> @@ -1075,7 +1076,7 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data) >> >> spin_unlock_irq(&epfile->ffs->eps_lock); >> >> - if (wait_for_completion_interruptible(&done)) { >> + if (wait_for_completion_interruptible(&io_data->done)) { >> /* >> * To avoid race condition with ffs_epfile_io_complete, >> * dequeue the request first then check >> @@ -1083,17 +1084,18 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data) >> * condition with req->complete callback. >> */ >> usb_ep_dequeue(ep->ep, req); >> - wait_for_completion(&done); >> - interrupted = ep->status < 0; >> + wait_for_completion(&io_data->done); >> + interrupted = true; > This is a change in behaviour - shouldn't this be: > > interrupted = io_data->status; > > ? > > Otherwise data is lost unnecessarily if the request completes > successfully in the small window between cancellation and being > dequeued. thanks for explanation, will follow your suggestion in V2. >> } >> >> if (interrupted) >> ret = -EINTR; >> - else if (io_data->read && ep->status > 0) >> - ret = __ffs_epfile_read_data(epfile, data, ep->status, >> + else if (io_data->read && io_data->status > 0) >> + ret = __ffs_epfile_read_data(epfile, data, io_data->status, >> &io_data->data); >> else >> - ret = ep->status; >> + ret = io_data->status; >> + >> goto error_mutex; >> } else if (!(req = usb_ep_alloc_request(ep->ep, GFP_ATOMIC))) { >> ret = -ENOMEM; >> -- >> 2.7.4 >>
diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c index 4585ee3..dcba835 100644 --- a/drivers/usb/gadget/function/f_fs.c +++ b/drivers/usb/gadget/function/f_fs.c @@ -122,8 +122,6 @@ struct ffs_ep { struct usb_endpoint_descriptor *descs[3]; u8 num; - - int status; /* P: epfile->mutex */ }; struct ffs_epfile { @@ -227,6 +225,9 @@ struct ffs_io_data { bool use_sg; struct ffs_data *ffs; + + int status; + struct completion done; }; struct ffs_desc_helper { @@ -707,12 +708,12 @@ static const struct file_operations ffs_ep0_operations = { static void ffs_epfile_io_complete(struct usb_ep *_ep, struct usb_request *req) { + struct ffs_io_data *io_data = req->context; + ENTER(); - if (req->context) { - struct ffs_ep *ep = _ep->driver_data; - ep->status = req->status ? req->status : req->actual; - complete(req->context); - } + + io_data->status = req->status ? req->status : req->actual; + complete(&io_data->done); } static ssize_t ffs_copy_to_iter(void *data, int data_len, struct iov_iter *iter) @@ -1050,7 +1051,6 @@ 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) { - DECLARE_COMPLETION_ONSTACK(done); bool interrupted = false; req = ep->req; @@ -1066,7 +1066,8 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data) io_data->buf = data; - req->context = &done; + init_completion(&io_data->done); + req->context = io_data; req->complete = ffs_epfile_io_complete; ret = usb_ep_queue(ep->ep, req, GFP_ATOMIC); @@ -1075,7 +1076,7 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data) spin_unlock_irq(&epfile->ffs->eps_lock); - if (wait_for_completion_interruptible(&done)) { + if (wait_for_completion_interruptible(&io_data->done)) { /* * To avoid race condition with ffs_epfile_io_complete, * dequeue the request first then check @@ -1083,17 +1084,18 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data) * condition with req->complete callback. */ usb_ep_dequeue(ep->ep, req); - wait_for_completion(&done); - interrupted = ep->status < 0; + wait_for_completion(&io_data->done); + interrupted = true; } if (interrupted) ret = -EINTR; - else if (io_data->read && ep->status > 0) - ret = __ffs_epfile_read_data(epfile, data, ep->status, + else if (io_data->read && io_data->status > 0) + ret = __ffs_epfile_read_data(epfile, data, io_data->status, &io_data->data); else - ret = ep->status; + ret = io_data->status; + goto error_mutex; } else if (!(req = usb_ep_alloc_request(ep->ep, GFP_ATOMIC))) { ret = -ENOMEM;
If a task read/write data in blocking mode, it will wait the completion in ffs_epfile_io(), if function unbind occurs, ffs_func_unbind() will kfree ffs ep, once the task wake up, it still dereference the ffs ep to obtain the request status. Fix it by moving the request status to io_data which is stack-safe. Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> --- drivers/usb/gadget/function/f_fs.c | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-)