* [PATCH v3 0/2] usb: f_fs: safe operation in ffs_epfile_io() @ 2022-06-01 4:15 Linyu Yuan 2022-06-01 4:15 ` [PATCH v3 1/2] usb: gadget: f_fs: change ep->status safe " Linyu Yuan 2022-06-01 4:15 ` [PATCH v3 2/2] usb: gadget: f_fs: change ep->ep " Linyu Yuan 0 siblings, 2 replies; 6+ messages in thread From: Linyu Yuan @ 2022-06-01 4:15 UTC (permalink / raw) To: Felipe Balbi, Greg Kroah-Hartman Cc: linux-usb, Michael Wu, John Keeping, Linyu Yuan Fix two possible issue in ffs_epfile_io() when operation at blocking mode. v1: https://lore.kernel.org/linux-usb/1653989775-14267-1-git-send-email-quic_linyyuan@quicinc.com/ v2: correct interrupted variable according comment from John Keeping v3: (v2: https://lore.kernel.org/linux-usb/1654006119-23869-1-git-send-email-quic_linyyuan@quicinc.com/) add Revived-by from John keeping, after offline discussion, add Reported-by from Michael Wu Linyu Yuan (2): usb: gadget: f_fs: change ep->status safe in ffs_epfile_io() usb: gadget: f_fs: change ep->ep safe in ffs_epfile_io() drivers/usb/gadget/function/f_fs.c | 38 +++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3 1/2] usb: gadget: f_fs: change ep->status safe in ffs_epfile_io() 2022-06-01 4:15 [PATCH v3 0/2] usb: f_fs: safe operation in ffs_epfile_io() Linyu Yuan @ 2022-06-01 4:15 ` Linyu Yuan 2022-06-01 4:15 ` [PATCH v3 2/2] usb: gadget: f_fs: change ep->ep " Linyu Yuan 1 sibling, 0 replies; 6+ messages in thread From: Linyu Yuan @ 2022-06-01 4:15 UTC (permalink / raw) To: Felipe Balbi, Greg Kroah-Hartman Cc: linux-usb, Michael Wu, John Keeping, Linyu Yuan 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. Reported-by: Michael Wu <michael@allwinnertech.com> Reviewed-by: John Keeping <john@metanate.com> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> --- v2: correct interrupted assignment v3: add Reviewed-by and Reported-by 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..d4d8940 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 = io_data->status < 0; } 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v3 2/2] usb: gadget: f_fs: change ep->ep safe in ffs_epfile_io() 2022-06-01 4:15 [PATCH v3 0/2] usb: f_fs: safe operation in ffs_epfile_io() Linyu Yuan 2022-06-01 4:15 ` [PATCH v3 1/2] usb: gadget: f_fs: change ep->status safe " Linyu Yuan @ 2022-06-01 4:15 ` Linyu Yuan 2022-06-02 10:39 ` Michael Wu 1 sibling, 1 reply; 6+ messages in thread From: Linyu Yuan @ 2022-06-01 4:15 UTC (permalink / raw) To: Felipe Balbi, Greg Kroah-Hartman Cc: linux-usb, Michael Wu, John Keeping, Linyu Yuan In ffs_epfile_io(), when read/write data in blocking mode, it will wait the completion in interruptible mode, if task receive a signal, it will terminate the wait, at same time, if function unbind occurs, ffs_func_unbind() will kfree all eps, ffs_epfile_io() still try to dequeue request by dereferencing ep which may become invalid. Fix it by add ep spinlock and will not dereference ep if it is not valid. Reported-by: Michael Wu <michael@allwinnertech.com> Reviewed-by: John Keeping <john@metanate.com> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> --- v2: add Reviewed-by from John keeping v3: add Reported-by from Michael Wu drivers/usb/gadget/function/f_fs.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c index d4d8940..9bf9287 100644 --- a/drivers/usb/gadget/function/f_fs.c +++ b/drivers/usb/gadget/function/f_fs.c @@ -1077,6 +1077,11 @@ 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(&io_data->done)) { + spin_lock_irq(&epfile->ffs->eps_lock); + if (epfile->ep != ep) { + ret = -ESHUTDOWN; + goto error_lock; + } /* * To avoid race condition with ffs_epfile_io_complete, * dequeue the request first then check @@ -1084,6 +1089,7 @@ 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); + spin_unlock_irq(&epfile->ffs->eps_lock); wait_for_completion(&io_data->done); interrupted = io_data->status < 0; } -- 2.7.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 2/2] usb: gadget: f_fs: change ep->ep safe in ffs_epfile_io() 2022-06-01 4:15 ` [PATCH v3 2/2] usb: gadget: f_fs: change ep->ep " Linyu Yuan @ 2022-06-02 10:39 ` Michael Wu 2022-06-02 13:06 ` John Keeping 0 siblings, 1 reply; 6+ messages in thread From: Michael Wu @ 2022-06-02 10:39 UTC (permalink / raw) To: Linyu Yuan, Felipe Balbi, Greg Kroah-Hartman; +Cc: linux-usb, John Keeping On 6/1/2022 12:15 PM, Linyu Yuan wrote: > In ffs_epfile_io(), when read/write data in blocking mode, it will wait > the completion in interruptible mode, if task receive a signal, it will > terminate the wait, at same time, if function unbind occurs, > ffs_func_unbind() will kfree all eps, ffs_epfile_io() still try to > dequeue request by dereferencing ep which may become invalid. > > Fix it by add ep spinlock and will not dereference ep if it is not valid. > > Reported-by: Michael Wu <michael@allwinnertech.com> > Reviewed-by: John Keeping <john@metanate.com> > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> > --- > v2: add Reviewed-by from John keeping > v3: add Reported-by from Michael Wu > > drivers/usb/gadget/function/f_fs.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c > index d4d8940..9bf9287 100644 > --- a/drivers/usb/gadget/function/f_fs.c > +++ b/drivers/usb/gadget/function/f_fs.c > @@ -1077,6 +1077,11 @@ 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(&io_data->done)) { > + spin_lock_irq(&epfile->ffs->eps_lock); > + if (epfile->ep != ep) { > + ret = -ESHUTDOWN; > + goto error_lock; > + } > /* > * To avoid race condition with ffs_epfile_io_complete, > * dequeue the request first then check > @@ -1084,6 +1089,7 @@ 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); > + spin_unlock_irq(&epfile->ffs->eps_lock); > wait_for_completion(&io_data->done); > interrupted = io_data->status < 0; > } Tested-by: Michael Wu <michael@allwinnertech.com> I've tested Linyu's patches [PATCH v3 1/2] [PATCH v3 2/2]. I believe it fixes the bug I reported. What's more, John's solution [1] also works in my tests. It looks simpler. I'm not sure if it's as complete as Linyu's solution. [1] https://lore.kernel.org/linux-usb/YpUJkxWBNuZiW7Xk@donbot/ -- Regards, Michael Wu ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 2/2] usb: gadget: f_fs: change ep->ep safe in ffs_epfile_io() 2022-06-02 10:39 ` Michael Wu @ 2022-06-02 13:06 ` John Keeping 2022-06-06 4:49 ` Michael Wu 0 siblings, 1 reply; 6+ messages in thread From: John Keeping @ 2022-06-02 13:06 UTC (permalink / raw) To: Michael Wu; +Cc: Linyu Yuan, Felipe Balbi, Greg Kroah-Hartman, linux-usb On Thu, Jun 02, 2022 at 06:39:30PM +0800, Michael Wu wrote: > On 6/1/2022 12:15 PM, Linyu Yuan wrote: > > In ffs_epfile_io(), when read/write data in blocking mode, it will wait > > the completion in interruptible mode, if task receive a signal, it will > > terminate the wait, at same time, if function unbind occurs, > > ffs_func_unbind() will kfree all eps, ffs_epfile_io() still try to > > dequeue request by dereferencing ep which may become invalid. > > > > Fix it by add ep spinlock and will not dereference ep if it is not valid. > > > > Reported-by: Michael Wu <michael@allwinnertech.com> > > Reviewed-by: John Keeping <john@metanate.com> > > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> > > --- > > v2: add Reviewed-by from John keeping > > v3: add Reported-by from Michael Wu > > > > drivers/usb/gadget/function/f_fs.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c > > index d4d8940..9bf9287 100644 > > --- a/drivers/usb/gadget/function/f_fs.c > > +++ b/drivers/usb/gadget/function/f_fs.c > > @@ -1077,6 +1077,11 @@ 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(&io_data->done)) { > > + spin_lock_irq(&epfile->ffs->eps_lock); > > + if (epfile->ep != ep) { > > + ret = -ESHUTDOWN; > > + goto error_lock; > > + } > > /* > > * To avoid race condition with ffs_epfile_io_complete, > > * dequeue the request first then check > > @@ -1084,6 +1089,7 @@ 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); > > + spin_unlock_irq(&epfile->ffs->eps_lock); > > wait_for_completion(&io_data->done); > > interrupted = io_data->status < 0; > > } > > Tested-by: Michael Wu <michael@allwinnertech.com> > > I've tested Linyu's patches [PATCH v3 1/2] [PATCH v3 2/2]. I believe it > fixes the bug I reported. > > What's more, John's solution [1] also works in my tests. It looks simpler. > I'm not sure if it's as complete as Linyu's solution. It's not as comprehensive, let's focus on this thread. > [1] https://lore.kernel.org/linux-usb/YpUJkxWBNuZiW7Xk@donbot/ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 2/2] usb: gadget: f_fs: change ep->ep safe in ffs_epfile_io() 2022-06-02 13:06 ` John Keeping @ 2022-06-06 4:49 ` Michael Wu 0 siblings, 0 replies; 6+ messages in thread From: Michael Wu @ 2022-06-06 4:49 UTC (permalink / raw) To: John Keeping; +Cc: Linyu Yuan, Felipe Balbi, Greg Kroah-Hartman, linux-usb On 6/2/2022 9:06 PM, John Keeping wrote: > On Thu, Jun 02, 2022 at 06:39:30PM +0800, Michael Wu wrote: >> Tested-by: Michael Wu <michael@allwinnertech.com> >> >> I've tested Linyu's patches [PATCH v3 1/2] [PATCH v3 2/2]. I believe it >> fixes the bug I reported. >> >> What's more, John's solution [1] also works in my tests. It looks simpler. >> I'm not sure if it's as complete as Linyu's solution. > > It's not as comprehensive, let's focus on this thread. > No problem. Thank you. -- Regards, Michael Wu ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-06-06 5:26 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-06-01 4:15 [PATCH v3 0/2] usb: f_fs: safe operation in ffs_epfile_io() Linyu Yuan 2022-06-01 4:15 ` [PATCH v3 1/2] usb: gadget: f_fs: change ep->status safe " Linyu Yuan 2022-06-01 4:15 ` [PATCH v3 2/2] usb: gadget: f_fs: change ep->ep " Linyu Yuan 2022-06-02 10:39 ` Michael Wu 2022-06-02 13:06 ` John Keeping 2022-06-06 4:49 ` Michael Wu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.