From 332a5b446b7916d272c2a659a3b20909ce34d2c1 Mon Sep 17 00:00:00 2001 From: Lars-Peter Clausen Date: Wed, 30 Mar 2016 13:49:14 +0200 Subject: usb: gadget: f_fs: Fix EFAULT generation for async read operations In the current implementation functionfs generates a EFAULT for async read operations if the read buffer size is larger than the URB data size. Since a application does not necessarily know how much data the host side is going to send it typically supplies a buffer larger than the actual data, which will then result in a EFAULT error. This behaviour was introduced while refactoring the code to use iov_iter interface in commit c993c39b8639 ("gadget/function/f_fs.c: use put iov_iter into io_data"). The original code took the minimum over the URB size and the user buffer size and then attempted to copy that many bytes using copy_to_user(). If copy_to_user() could not copy all data a EFAULT error was generated. Restore the original behaviour by only generating a EFAULT error when the number of bytes copied is not the size of the URB and the target buffer has not been fully filled. Commit 342f39a6c8d3 ("usb: gadget: f_fs: fix check in read operation") already fixed the same problem for the synchronous read path. Fixes: c993c39b8639 ("gadget/function/f_fs.c: use put iov_iter into io_data") Acked-by: Michal Nazarewicz Signed-off-by: Lars-Peter Clausen Signed-off-by: Felipe Balbi --- drivers/usb/gadget/function/f_fs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/usb/gadget/function') diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c index e21ca2bd6839..2c314c13f9a7 100644 --- a/drivers/usb/gadget/function/f_fs.c +++ b/drivers/usb/gadget/function/f_fs.c @@ -650,7 +650,7 @@ static void ffs_user_copy_worker(struct work_struct *work) if (io_data->read && ret > 0) { use_mm(io_data->mm); ret = copy_to_iter(io_data->buf, ret, &io_data->data); - if (iov_iter_count(&io_data->data)) + if (ret != io_data->req->actual && iov_iter_count(&io_data->data)) ret = -EFAULT; unuse_mm(io_data->mm); } -- cgit From f78bbcae86e676fad9e6c6bb6cd9d9868ba23696 Mon Sep 17 00:00:00 2001 From: Michal Nazarewicz Date: Fri, 8 Apr 2016 10:24:11 +0200 Subject: usb: f_mass_storage: test whether thread is running before starting another MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When binding the function to usb_configuration, check whether the thread is running before starting another one. Without that, when function instance is added to multiple configurations, fsg_bing starts multiple threads with all but the latest one being forgotten by the driver. This leads to obvious thread leaks, possible lockups when trying to halt the machine and possible more issues. This fixes issues with legacy/multi¹ gadget as well as configfs gadgets when mass_storage function is added to multiple configurations. This change also simplifies API since the legacy gadgets no longer need to worry about starting the thread by themselves (which was where bug in legacy/multi was in the first place). N.B., this patch doesn’t address adding single mass_storage function instance to a single configuration twice. Thankfully, there’s no legitimate reason for such setup plus, if I’m not mistaken, configfs gadget doesn’t even allow it to be expressed. ¹ I have no example failure though. Conclusion that legacy/multi has a bug is based purely on me reading the code. Acked-by: Alan Stern Signed-off-by: Michal Nazarewicz Tested-by: Ivaylo Dimitrov Cc: Alan Stern Cc: Signed-off-by: Felipe Balbi --- drivers/usb/gadget/function/f_mass_storage.c | 36 ++++++++++++---------------- drivers/usb/gadget/function/f_mass_storage.h | 2 -- 2 files changed, 15 insertions(+), 23 deletions(-) (limited to 'drivers/usb/gadget/function') diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c index acf210f16328..5c6d4d7ca605 100644 --- a/drivers/usb/gadget/function/f_mass_storage.c +++ b/drivers/usb/gadget/function/f_mass_storage.c @@ -2977,25 +2977,6 @@ void fsg_common_set_inquiry_string(struct fsg_common *common, const char *vn, } EXPORT_SYMBOL_GPL(fsg_common_set_inquiry_string); -int fsg_common_run_thread(struct fsg_common *common) -{ - common->state = FSG_STATE_IDLE; - /* Tell the thread to start working */ - common->thread_task = - kthread_create(fsg_main_thread, common, "file-storage"); - if (IS_ERR(common->thread_task)) { - common->state = FSG_STATE_TERMINATED; - return PTR_ERR(common->thread_task); - } - - DBG(common, "I/O thread pid: %d\n", task_pid_nr(common->thread_task)); - - wake_up_process(common->thread_task); - - return 0; -} -EXPORT_SYMBOL_GPL(fsg_common_run_thread); - static void fsg_common_release(struct kref *ref) { struct fsg_common *common = container_of(ref, struct fsg_common, ref); @@ -3005,6 +2986,7 @@ static void fsg_common_release(struct kref *ref) if (common->state != FSG_STATE_TERMINATED) { raise_exception(common, FSG_STATE_EXIT); wait_for_completion(&common->thread_notifier); + common->thread_task = NULL; } for (i = 0; i < ARRAY_SIZE(common->luns); ++i) { @@ -3050,9 +3032,21 @@ static int fsg_bind(struct usb_configuration *c, struct usb_function *f) if (ret) return ret; fsg_common_set_inquiry_string(fsg->common, NULL, NULL); - ret = fsg_common_run_thread(fsg->common); - if (ret) + } + + if (!common->thread_task) { + common->state = FSG_STATE_IDLE; + common->thread_task = + kthread_create(fsg_main_thread, common, "file-storage"); + if (IS_ERR(common->thread_task)) { + int ret = PTR_ERR(common->thread_task); + common->thread_task = NULL; + common->state = FSG_STATE_TERMINATED; return ret; + } + DBG(common, "I/O thread pid: %d\n", + task_pid_nr(common->thread_task)); + wake_up_process(common->thread_task); } fsg->gadget = gadget; diff --git a/drivers/usb/gadget/function/f_mass_storage.h b/drivers/usb/gadget/function/f_mass_storage.h index 445df6775609..b6a9918eaefb 100644 --- a/drivers/usb/gadget/function/f_mass_storage.h +++ b/drivers/usb/gadget/function/f_mass_storage.h @@ -153,8 +153,6 @@ int fsg_common_create_luns(struct fsg_common *common, struct fsg_config *cfg); void fsg_common_set_inquiry_string(struct fsg_common *common, const char *vn, const char *pn); -int fsg_common_run_thread(struct fsg_common *common); - void fsg_config_from_params(struct fsg_config *cfg, const struct fsg_module_parameters *params, unsigned int fsg_num_buffers); -- cgit