Am Samstag, den 30.05.2020, 13:58 +0900 schrieb Tetsuo Handa: Hi, sorry for taking this long. There has been a family emergency. > > The kernel most definitely does need to protect itself against > > misbehaving hardware. Let's just leave it at that. If you don't > > believe me, ask Greg KH. > > I've made many locations killable (in order to reduce damage caused by OOM > condition). But I can't make locations killable where handling SIGKILL case is > too difficult to implement. We can make flush interruptible. But that will not do the job. We would get a file that cannot be closed. > "struct file_operations"->flush() is called from filp_close() when there is > something which has to be done before "struct file_operations"->release() is > called. Yes, in particular error reporting. Without flush() there is no way to know whether the last write() has actually worked. > As far as I read this thread, what you are trying to do sounds like allow > "not waiting for completion of wdm_out_callback()" with only > 's/wait_event/wait_event_intrruptible/' in wdm_flush(). Then, please do remove > wdm_flush() call itself. That would break error reporting. That flush() waits for IO to complete is basically a side effect. You can know whether IO has worked after it is finished. > Therefore, again, please show me as a patch first. Sure, attached. The difficulty here is that I see three possible interacting errors, two of which are races. Regards Oliver
From 27cd2e25b37af973b61b77217fa2dad822889ff8 Mon Sep 17 00:00:00 2001 From: Oliver Neukum <oneukum@xxxxxxxx> Date: Wed, 24 Jun 2020 10:52:03 +0200 Subject: [PATCH] CDC-WDM: fix hangs in flush() When flushing a task needs to wait a bounded time, as a hardware failure could mean eternal sleep. So an arbitrary timeout is introduced. Simply making the syscall interruptible will not do the job, as while the syscall would not hang, the fd would be unclosable. In addition a flush() and a write() may be waiting for the same IO to complete. Hence completion of output must use wake_up_all(), even in error handling. Reported-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx> Signed-off-by: Oliver Neukum <oneukum@xxxxxxxx> --- drivers/usb/class/cdc-wdm.c | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c index e3db6fbeadef..ec5412773c57 100644 --- a/drivers/usb/class/cdc-wdm.c +++ b/drivers/usb/class/cdc-wdm.c @@ -58,6 +58,9 @@ MODULE_DEVICE_TABLE (usb, wdm_ids); #define WDM_MAX 16 +/* flush() needs to be uninterruptible, but we cannot wait forever */ +#define WDM_FLUSH_TIMEOUT (30 * HZ) + /* CDC-WMC r1.1 requires wMaxCommand to be "at least 256 decimal (0x100)" */ #define WDM_DEFAULT_BUFSIZE 256 @@ -151,7 +154,7 @@ static void wdm_out_callback(struct urb *urb) kfree(desc->outbuf); desc->outbuf = NULL; clear_bit(WDM_IN_USE, &desc->flags); - wake_up(&desc->wait); + wake_up_all(&desc->wait); } static void wdm_in_callback(struct urb *urb) @@ -424,6 +427,7 @@ static ssize_t wdm_write if (rv < 0) { desc->outbuf = NULL; clear_bit(WDM_IN_USE, &desc->flags); + wake_up_all(&desc->wait); /* for flush() */ dev_err(&desc->intf->dev, "Tx URB error: %d\n", rv); rv = usb_translate_errors(rv); goto out_free_mem_pm; @@ -586,8 +590,9 @@ static ssize_t wdm_read static int wdm_flush(struct file *file, fl_owner_t id) { struct wdm_device *desc = file->private_data; + int rv; - wait_event(desc->wait, + rv = wait_event_interruptible_timeout(desc->wait, /* * needs both flags. We cannot do with one * because resetting it would cause a race @@ -595,11 +600,16 @@ static int wdm_flush(struct file *file, fl_owner_t id) * a disconnect */ !test_bit(WDM_IN_USE, &desc->flags) || - test_bit(WDM_DISCONNECTING, &desc->flags)); + test_bit(WDM_DISCONNECTING, &desc->flags), + WDM_FLUSH_TIMEOUT); /* cannot dereference desc->intf if WDM_DISCONNECTING */ if (test_bit(WDM_DISCONNECTING, &desc->flags)) return -ENODEV; + if (!rv) + return -EIO; + if (rv < 0) + return -EINTR; if (desc->werr < 0) dev_err(&desc->intf->dev, "Error in flush path: %d\n", desc->werr); @@ -656,6 +666,14 @@ static int wdm_open(struct inode *inode, struct file *file) goto out; } + /* + * in case flush() had timed out + */ + usb_kill_urb(desc->command); + spin_lock_irq(&desc->iuspin); + desc->werr = 0; + spin_unlock_irq(&desc->iuspin); + /* using write lock to protect desc->count */ mutex_lock(&desc->wlock); if (!desc->count++) { -- 2.16.4