By submitting completed transfers to the system workqueue there is no
guarantee that completion events will be queued up in the correct order,
as in multi-processor systems there is a thread running for each
processor and the work items are not bound to a particular core.
This means that several completions are in the queue at the same time,
they may be processed in parallel and complete out of order, resulting
in data appearing corrupt when read by userspace.
Create a single-threaded workqueue for FunctionFS so that data completed
requests is passed to userspace in the order in which they complete.
Signed-off-by: John Keeping <[email protected]>
---
I originally sent a version of this patch back in July [1] without any
response. Since then, I've improved the commit message and switched
from create_singlethread_workqueue() to alloc_ordered_workqueue(), so
I've marked this as v2.
[1] https://patchwork.kernel.org/patch/9838441/
drivers/usb/gadget/function/f_fs.c | 17 +++++++++++++----
drivers/usb/gadget/function/u_fs.h | 1 +
2 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 9990944a7245..8b342587f8ad 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -46,7 +46,8 @@
static void ffs_data_get(struct ffs_data *ffs);
static void ffs_data_put(struct ffs_data *ffs);
/* Creates new ffs_data object. */
-static struct ffs_data *__must_check ffs_data_new(void) __attribute__((malloc));
+static struct ffs_data *__must_check ffs_data_new(const char *dev_name)
+ __attribute__((malloc));
/* Opened counter handling. */
static void ffs_data_opened(struct ffs_data *ffs);
@@ -780,11 +781,12 @@ static void ffs_epfile_async_io_complete(struct usb_ep *_ep,
struct usb_request *req)
{
struct ffs_io_data *io_data = req->context;
+ struct ffs_data *ffs = io_data->ffs;
ENTER();
INIT_WORK(&io_data->work, ffs_user_copy_worker);
- schedule_work(&io_data->work);
+ queue_work(ffs->io_completion_wq, &io_data->work);
}
static void __ffs_epfile_read_buffer_free(struct ffs_epfile *epfile)
@@ -1500,7 +1502,7 @@ ffs_fs_mount(struct file_system_type *t, int flags,
if (unlikely(ret < 0))
return ERR_PTR(ret);
- ffs = ffs_data_new();
+ ffs = ffs_data_new(dev_name);
if (unlikely(!ffs))
return ERR_PTR(-ENOMEM);
ffs->file_perms = data.perms;
@@ -1610,6 +1612,7 @@ static void ffs_data_put(struct ffs_data *ffs)
BUG_ON(waitqueue_active(&ffs->ev.waitq) ||
waitqueue_active(&ffs->ep0req_completion.wait) ||
waitqueue_active(&ffs->wait));
+ destroy_workqueue(ffs->io_completion_wq);
kfree(ffs->dev_name);
kfree(ffs);
}
@@ -1642,7 +1645,7 @@ static void ffs_data_closed(struct ffs_data *ffs)
ffs_data_put(ffs);
}
-static struct ffs_data *ffs_data_new(void)
+static struct ffs_data *ffs_data_new(const char *dev_name)
{
struct ffs_data *ffs = kzalloc(sizeof *ffs, GFP_KERNEL);
if (unlikely(!ffs))
@@ -1650,6 +1653,12 @@ static struct ffs_data *ffs_data_new(void)
ENTER();
+ ffs->io_completion_wq = alloc_ordered_workqueue("%s", 0, dev_name);
+ if (!ffs->io_completion_wq) {
+ kfree(ffs);
+ return NULL;
+ }
+
refcount_set(&ffs->ref, 1);
atomic_set(&ffs->opened, 0);
ffs->state = FFS_READ_DESCRIPTORS;
diff --git a/drivers/usb/gadget/function/u_fs.h b/drivers/usb/gadget/function/u_fs.h
index 540f1c48c1a8..79f70ebf85dc 100644
--- a/drivers/usb/gadget/function/u_fs.h
+++ b/drivers/usb/gadget/function/u_fs.h
@@ -279,6 +279,7 @@ struct ffs_data {
} file_perms;
struct eventfd_ctx *ffs_eventfd;
+ struct workqueue_struct *io_completion_wq;
bool no_disconnect;
struct work_struct reset_work;
--
2.14.1
On Tue, Sep 12 2017, John Keeping wrote:
> By submitting completed transfers to the system workqueue there is no
> guarantee that completion events will be queued up in the correct order,
> as in multi-processor systems there is a thread running for each
> processor and the work items are not bound to a particular core.
>
> This means that several completions are in the queue at the same time,
> they may be processed in parallel and complete out of order, resulting
> in data appearing corrupt when read by userspace.
>
> Create a single-threaded workqueue for FunctionFS so that data completed
> requests is passed to userspace in the order in which they complete.
>
> Signed-off-by: John Keeping <[email protected]>
Acked-by: Michal Nazarewicz <[email protected]>
> ---
> I originally sent a version of this patch back in July [1] without any
> response. Since then, I've improved the commit message and switched
> from create_singlethread_workqueue() to alloc_ordered_workqueue(), so
> I've marked this as v2.
>
> [1] https://patchwork.kernel.org/patch/9838441/
>
> drivers/usb/gadget/function/f_fs.c | 17 +++++++++++++----
> drivers/usb/gadget/function/u_fs.h | 1 +
> 2 files changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index 9990944a7245..8b342587f8ad 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -46,7 +46,8 @@
> static void ffs_data_get(struct ffs_data *ffs);
> static void ffs_data_put(struct ffs_data *ffs);
> /* Creates new ffs_data object. */
> -static struct ffs_data *__must_check ffs_data_new(void) __attribute__((malloc));
> +static struct ffs_data *__must_check ffs_data_new(const char *dev_name)
> + __attribute__((malloc));
>
> /* Opened counter handling. */
> static void ffs_data_opened(struct ffs_data *ffs);
> @@ -780,11 +781,12 @@ static void ffs_epfile_async_io_complete(struct usb_ep *_ep,
> struct usb_request *req)
> {
> struct ffs_io_data *io_data = req->context;
> + struct ffs_data *ffs = io_data->ffs;
>
> ENTER();
>
> INIT_WORK(&io_data->work, ffs_user_copy_worker);
> - schedule_work(&io_data->work);
> + queue_work(ffs->io_completion_wq, &io_data->work);
> }
>
> static void __ffs_epfile_read_buffer_free(struct ffs_epfile *epfile)
> @@ -1500,7 +1502,7 @@ ffs_fs_mount(struct file_system_type *t, int flags,
> if (unlikely(ret < 0))
> return ERR_PTR(ret);
>
> - ffs = ffs_data_new();
> + ffs = ffs_data_new(dev_name);
> if (unlikely(!ffs))
> return ERR_PTR(-ENOMEM);
> ffs->file_perms = data.perms;
> @@ -1610,6 +1612,7 @@ static void ffs_data_put(struct ffs_data *ffs)
> BUG_ON(waitqueue_active(&ffs->ev.waitq) ||
> waitqueue_active(&ffs->ep0req_completion.wait) ||
> waitqueue_active(&ffs->wait));
> + destroy_workqueue(ffs->io_completion_wq);
> kfree(ffs->dev_name);
> kfree(ffs);
> }
> @@ -1642,7 +1645,7 @@ static void ffs_data_closed(struct ffs_data *ffs)
> ffs_data_put(ffs);
> }
>
> -static struct ffs_data *ffs_data_new(void)
> +static struct ffs_data *ffs_data_new(const char *dev_name)
> {
> struct ffs_data *ffs = kzalloc(sizeof *ffs, GFP_KERNEL);
> if (unlikely(!ffs))
> @@ -1650,6 +1653,12 @@ static struct ffs_data *ffs_data_new(void)
>
> ENTER();
>
> + ffs->io_completion_wq = alloc_ordered_workqueue("%s", 0, dev_name);
> + if (!ffs->io_completion_wq) {
> + kfree(ffs);
> + return NULL;
> + }
> +
> refcount_set(&ffs->ref, 1);
> atomic_set(&ffs->opened, 0);
> ffs->state = FFS_READ_DESCRIPTORS;
> diff --git a/drivers/usb/gadget/function/u_fs.h b/drivers/usb/gadget/function/u_fs.h
> index 540f1c48c1a8..79f70ebf85dc 100644
> --- a/drivers/usb/gadget/function/u_fs.h
> +++ b/drivers/usb/gadget/function/u_fs.h
> @@ -279,6 +279,7 @@ struct ffs_data {
> } file_perms;
>
> struct eventfd_ctx *ffs_eventfd;
> + struct workqueue_struct *io_completion_wq;
> bool no_disconnect;
> struct work_struct reset_work;
--
Best regards
ミハウ “????????????????86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»