[PATCH 1/1] HID: fix tty<->hid deadlock

[PATCH 1/1] HID: fix tty<->hid deadlock

[Jiri Slaby]

Jiri Slaby wrote:
> Valdis.Kletnieks@vt.edu napsal(a):
>> (Adding Alan Cox to the cc: in case he can shed light on this one - it
>> appears that HID_COMPAT only puts the bullet in the chamber, and doesn't
>> actually cause the hang...)
>>
>> On Tue, 02 Sep 2008 23:02:42 +0200, Jiri Slaby said:
>>> On 09/02/2008 06:11 PM, Valdis.Kletnieks@vt.edu wrote:
>>>> The following 3 lines don't go to console by default due to loglevel setting.
>>>> So I'm not sure exactly where it hangs. But it's somewhere in here.
>> Right around here, we kick off a modprobe for 'hid_dummy'.
>>>>>> [    1.959193] usbcore: registered new interface driver usbhid
>>>>>> [    1.973037] usbhid: v2.6:USB HID core driver
>> And since my initrd doesn't include any modules (since until now, I've had
>> a kernel that can everything builtin so it can boot far enough to do the
>> whole udev/modprobe off my root filesystem, and hid_dummy is a new one on
>> me), this modprobe spits out a:
>>
>> modprobe: FATAL: Could not open '/lib/modules/2.6.27-rc5-mmotm0829/modules.dep': No such file or directory
>>
>> Well, yeah.. No modules on the initrd, so no modules.dep.  But having spewed
>> its error message, modprobe apparently decides to go off in a snit and hang.
>> Eventually, the usermode_helper call does a wait() on the modprobe, and then
>> *that* hangs because modprobe isn't returning.  And eventually the whole
>> level of initcalls comes to a screeching halt...
>>
>> And here's the totally unexpected kernel traceback for the modprobe:
>>
>> schedule_timeout+0x22/0xb4
>> ? _raw_spin_lock+0xce/0x186
>> ? _raw_spin_unlock+0xb7/0xe0
>> wait_for_common+-xb2/0xfb
>> ? default_wake_function+0x0/0xf
>> wait_for_completion+0x18/0x1a
>> flush_cpu_workqueue+0x6b/0x77
>> ? wq_barrior_func+0x0/0xf
>> flush_workqueue+0x4f/0x68
>> flush_scheduled_work+0x10/0x12
>> tty_ldisc_release+0x4a/0x21e
>> ? _raw_pin_lock+0xce/0x186
>> ? debug_mutex_unlock+0x127/0x14d
>> ? mutex_unlock_slowpath+0x14a/0x15c
>> tty_release_dev+0x4da/0x508
>> ? get_parent_ip+0x11/0x41
>> ? get_parent_ip+0x11/0x41
>> tty_release+0x19/0x24
>> __fput+0xd9/0x198
>> fput+0x15/0x17
>> filp_close+0x67/0x72
>> sys_close+0xa9/0x104
>> system_call_fastpath+0x16/0x1b
>>
>> WTF? We hang trying to close a tty??!?
> 
> Hmm, *if* we stuck in request_module in hid, workqueue cannot be flushed and
> tty waits... Could you stick 2 printks into hid_compat_load if it finishes?

Ah, this is deadlock:
hid_compat_load() runs on the default workqueue, it request_module(), it
execs modprobe, it exits, tty flushes default workqueue, it hangs, because
we are still in it.

I haven't run into it since there always was at least one opener of the tty,
I guess. Could you try the patch below?

--

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
---
 drivers/hid/hid-core.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 92c16e1..18d952a 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1663,6 +1663,7 @@ static void hid_compat_load(struct work_struct *ws)
 	request_module("hid-dummy");
 }
 static DECLARE_WORK(hid_compat_work, hid_compat_load);
+static struct workqueue_struct *hid_compat_wq;
 #endif
 
 static int __init hid_init(void)
@@ -1680,7 +1681,12 @@ static int __init hid_init(void)
 		goto err_bus;
 
 #ifdef CONFIG_HID_COMPAT
-	schedule_work(&hid_compat_work);
+	hid_compat_wq = create_workqueue("hid_compat");
+	if (!hid_compat_wq) {
+		hidraw_exit();
+		goto err;
+	}
+	queue_work(hid_compat_wq, &hid_compat_work);
 #endif
 
 	return 0;
@@ -1692,6 +1698,9 @@ err:
 
 static void __exit hid_exit(void)
 {
+#ifdef CONFIG_HID_COMPAT
+	destroy_workqueue(hid_compat_wq);
+#endif
 	hidraw_exit();
 	bus_unregister(&hid_bus_type);
 }
-- 
1.6.0

Re: [PATCH 1/1] HID: fix tty<->hid deadlock

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 924 bytes --]

On Thu, 04 Sep 2008 17:16:07 +0200, Jiri Slaby said:

> hid_compat_load() runs on the default workqueue, it request_module(), it
> execs modprobe, it exits, tty flushes default workqueue, it hangs, because
> we are still in it.
> 
> I haven't run into it since there always was at least one opener of the tty,
> I guess. Could you try the patch below?
> 
> --
> 
> Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
> ---
>  drivers/hid/hid-core.c |   11 ++++++++++-
>  1 files changed, 10 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 92c16e1..18d952a 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c

With this patch applied, the kernel behaves as expected for my config: it
requests a 'modprobe hid_dummy', which prints a nasty message about being
unable to find it on the initrd, and then we continue with the boot.

Thanks for the quick fix...

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [PATCH 1/1] HID: fix tty<->hid deadlock

[Jiri Slaby]

Valdis.Kletnieks@vt.edu napsal(a):
> On Thu, 04 Sep 2008 17:16:07 +0200, Jiri Slaby said:
> 
>> hid_compat_load() runs on the default workqueue, it request_module(), it
>> execs modprobe, it exits, tty flushes default workqueue, it hangs, because
>> we are still in it.
>>
>> I haven't run into it since there always was at least one opener of the tty,
>> I guess. Could you try the patch below?
>>
>> --
>>
>> Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
>> ---
>>  drivers/hid/hid-core.c |   11 ++++++++++-
>>  1 files changed, 10 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> index 92c16e1..18d952a 100644
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
> 
> With this patch applied, the kernel behaves as expected for my config: it
> requests a 'modprobe hid_dummy', which prints a nasty message about being
> unable to find it on the initrd, and then we continue with the boot.

Thanks for testing, I can't think of any method which would solve this
problem. As this compat module is a temporary solution for those who have
(very) old module init tools (not supporting aliases), I wouldn't do
anything else until somebody complains.

hidraw_exit in discarded section (was: Re: [PATCH 1/1] HID: fix tty<->hid deadlock)

[Geert Uytterhoeven]

On Thu, 4 Sep 2008, Jiri Slaby wrote:
> hid_compat_load() runs on the default workqueue, it request_module(), it
> execs modprobe, it exits, tty flushes default workqueue, it hangs, because
> we are still in it.
> 
> I haven't run into it since there always was at least one opener of the tty,
> I guess. Could you try the patch below?
> 
> --
> 
> Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
> ---
>  drivers/hid/hid-core.c |   11 ++++++++++-
>  1 files changed, 10 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 92c16e1..18d952a 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1663,6 +1663,7 @@ static void hid_compat_load(struct work_struct *ws)
>  	request_module("hid-dummy");
>  }
>  static DECLARE_WORK(hid_compat_work, hid_compat_load);
> +static struct workqueue_struct *hid_compat_wq;
>  #endif
>  
>  static int __init hid_init(void)
> @@ -1680,7 +1681,12 @@ static int __init hid_init(void)
>  		goto err_bus;
>  
>  #ifdef CONFIG_HID_COMPAT
> -	schedule_work(&hid_compat_work);
> +	hid_compat_wq = create_workqueue("hid_compat");
> +	if (!hid_compat_wq) {
> +		hidraw_exit();
> +		goto err;
> +	}
> +	queue_work(hid_compat_wq, &hid_compat_work);
>  #endif
>  
>  	return 0;
> @@ -1692,6 +1698,9 @@ err:
>  
>  static void __exit hid_exit(void)
>  {
> +#ifdef CONFIG_HID_COMPAT
> +	destroy_workqueue(hid_compat_wq);
> +#endif
>  	hidraw_exit();
>  	bus_unregister(&hid_bus_type);
>  }

hid_init() is marked __init.
hidraw_exit() is marked __exit.

Hence I get:

    `hidraw_exit' referenced in section `.init.text' of drivers/built-in.o: defined in discarded section `.exit.text' of drivers/built-in.o

and my `enable everything' m68k kernel fails to link.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds