linux-serial.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] tty/vt: Fix possible deadlock in input_inject_event
@ 2025-09-28 13:08 pengyu
  2025-09-29  4:54 ` Dmitry Torokhov
  2025-09-30  6:31 ` kernel test robot
  0 siblings, 2 replies; 10+ messages in thread
From: pengyu @ 2025-09-28 13:08 UTC (permalink / raw)
  To: gregkh, jirislaby
  Cc: legion, mingo, myrrhperiwinkle, tglx, dmitry.torokhov,
	changlianzhi, linux-kernel, linux-serial, pengyu,
	syzbot+79c403850e6816dc39cf

syzkaller testing revealed a potential deadlock involving keyboard 
handling:

CPU0                       CPU1                      CPU2
----                       ----                      ----
read_lock(tasklist_lock);  evdev_write
                          input_inject_event     write_lock(tasklist_lock);
                         lock(&dev->event_lock);
                        read_lock(tasklist_lock);
<Interrupt>
kbd_bh() / kd_sound_helper()
input_inject_event
lock(&dev->event_lock); // Deadlock risk

The deadlock occurs because:
1. Both kbd_bh and kd_sound_helper run in interrupt context
2. tasklist_lock is interrupt-unsafe
3. When evdev_write holds both dev->event_lock and tasklist_lock,
   interrupt context attempts to acquire dev->event_lock create deadlock 
   risks

Convert both kbd_bh and kd_sound_helper to use workqueues. This moves
input_inject_event execution to process context, where it's safe to 
acquire locks that may be held by code using interrupt-unsafe locks.

Reported-by: syzbot+79c403850e6816dc39cf@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/all/66f6c8ce.050a0220.46d20.001c.GAE@google.com/T/#u
Fixes: fb09d0ac0772 ("tty: Fix the keyboard led light display problem")

Signed-off-by: pengyu <pengyu@kylinos.cn>
---
 drivers/tty/vt/keyboard.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
index ee1d9c448c7e..eb2afc86b502 100644
--- a/drivers/tty/vt/keyboard.c
+++ b/drivers/tty/vt/keyboard.c
@@ -131,8 +131,8 @@ static const unsigned char max_vals[] = {
 
 static const int NR_TYPES = ARRAY_SIZE(max_vals);
 
-static void kbd_bh(struct tasklet_struct *unused);
-static DECLARE_TASKLET_DISABLED(keyboard_tasklet, kbd_bh);
+static void kbd_bh(struct work_struct *unused);
+static DECLARE_WORK(keyboard_work, kbd_bh);
 
 static struct input_handler kbd_handler;
 static DEFINE_SPINLOCK(kbd_event_lock);
@@ -264,23 +264,23 @@ static int kd_sound_helper(struct input_handle *handle, void *data)
 	return 0;
 }
 
-static void kd_nosound(struct timer_list *unused)
+static void kd_nosound(struct work_struct *unused)
 {
 	static unsigned int zero;
 
 	input_handler_for_each_handle(&kbd_handler, &zero, kd_sound_helper);
 }
 
-static DEFINE_TIMER(kd_mksound_timer, kd_nosound);
+static DECLARE_DELAYED_WORK(kd_mksound_worker, kd_nosound);
 
 void kd_mksound(unsigned int hz, unsigned int ticks)
 {
-	timer_delete_sync(&kd_mksound_timer);
+	cancel_delayed_work_sync(&kd_mksound_worker);
 
 	input_handler_for_each_handle(&kbd_handler, &hz, kd_sound_helper);
 
 	if (hz && ticks)
-		mod_timer(&kd_mksound_timer, jiffies + ticks);
+		schedule_delayed_work(&kd_mksound_worker, ticks);
 }
 EXPORT_SYMBOL(kd_mksound);
 
@@ -390,7 +390,7 @@ static void put_queue_utf8(struct vc_data *vc, u32 value)
 /* FIXME: review locking for vt.c callers */
 static void set_leds(void)
 {
-	tasklet_schedule(&keyboard_tasklet);
+	schedule_work(&keyboard_work);
 }
 
 /*
@@ -1024,10 +1024,10 @@ static int kbd_led_trigger_activate(struct led_classdev *cdev)
 	struct kbd_led_trigger *trigger =
 		container_of(cdev->trigger, struct kbd_led_trigger, trigger);
 
-	tasklet_disable(&keyboard_tasklet);
+	cancel_work_sync(&keyboard_work);
 	if (ledstate != -1U)
 		led_set_brightness(cdev, ledstate & trigger->mask ? LED_FULL : LED_OFF);
-	tasklet_enable(&keyboard_tasklet);
+	enable_work(&keyboard_work);
 
 	return 0;
 }
@@ -1243,7 +1243,7 @@ void vt_kbd_con_stop(unsigned int console)
  * handle the scenario when keyboard handler is not registered yet
  * but we already getting updates from the VT to update led state.
  */
-static void kbd_bh(struct tasklet_struct *unused)
+static void kbd_bh(struct work_struct *unused)
 {
 	unsigned int leds;
 	unsigned long flags;
@@ -1535,7 +1535,7 @@ static void kbd_event(struct input_handle *handle, unsigned int event_type,
 
 	spin_unlock(&kbd_event_lock);
 
-	tasklet_schedule(&keyboard_tasklet);
+	schedule_work(&keyboard_work);
 	do_poke_blanked_console = 1;
 	schedule_console_callback();
 }
@@ -1607,12 +1607,12 @@ static void kbd_disconnect(struct input_handle *handle)
  */
 static void kbd_start(struct input_handle *handle)
 {
-	tasklet_disable(&keyboard_tasklet);
+	cancel_work_sync(&keyboard_work);
 
 	if (ledstate != -1U)
 		kbd_update_leds_helper(handle, &ledstate);
 
-	tasklet_enable(&keyboard_tasklet);
+	enable_work(&keyboard_work);
 }
 
 static const struct input_device_id kbd_ids[] = {
@@ -1662,8 +1662,8 @@ int __init kbd_init(void)
 	if (error)
 		return error;
 
-	tasklet_enable(&keyboard_tasklet);
-	tasklet_schedule(&keyboard_tasklet);
+	enable_work(&keyboard_work);
+	schedule_work(&keyboard_work);
 
 	return 0;
 }
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] tty/vt: Fix possible deadlock in input_inject_event
  2025-09-28 13:08 [PATCH] tty/vt: Fix possible deadlock in input_inject_event pengyu
@ 2025-09-29  4:54 ` Dmitry Torokhov
  2025-10-01 10:42   ` pengyu
  2025-09-30  6:31 ` kernel test robot
  1 sibling, 1 reply; 10+ messages in thread
From: Dmitry Torokhov @ 2025-09-29  4:54 UTC (permalink / raw)
  To: pengyu
  Cc: gregkh, jirislaby, legion, mingo, myrrhperiwinkle, tglx,
	changlianzhi, linux-kernel, linux-serial,
	syzbot+79c403850e6816dc39cf

Hi,

On Sun, Sep 28, 2025 at 09:08:19PM +0800, pengyu wrote:
> syzkaller testing revealed a potential deadlock involving keyboard 
> handling:
> 
> CPU0                       CPU1                      CPU2
> ----                       ----                      ----
> read_lock(tasklist_lock);  evdev_write
>                           input_inject_event     write_lock(tasklist_lock);
>                          lock(&dev->event_lock);
>                         read_lock(tasklist_lock);
> <Interrupt>
> kbd_bh() / kd_sound_helper()
> input_inject_event
> lock(&dev->event_lock); // Deadlock risk
> 
> The deadlock occurs because:
> 1. Both kbd_bh and kd_sound_helper run in interrupt context
> 2. tasklist_lock is interrupt-unsafe
> 3. When evdev_write holds both dev->event_lock and tasklist_lock,
>    interrupt context attempts to acquire dev->event_lock create deadlock 
>    risks
> 
> Convert both kbd_bh and kd_sound_helper to use workqueues. This moves
> input_inject_event execution to process context, where it's safe to 
> acquire locks that may be held by code using interrupt-unsafe locks.

So if we ignore the input code and instead look at the send_sigio()
(which input core ends up calling) and do_wait() we see that
send_sigio() disables interrupts and takes the owner's spinlock
before taking the tasklist_lock, while do_wait() takes the tasklist_lock
first, without disabling interrupts. This is root of the issue as far as
I can tell and no amount of changes to the keyboard handler (which is
just happens to be in the middle) will not solve for all potential cases
and code paths. 

I believe either do_exit() or send_sigio() have to be changed to fix
this properly. 

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] tty/vt: Fix possible deadlock in input_inject_event
  2025-09-28 13:08 [PATCH] tty/vt: Fix possible deadlock in input_inject_event pengyu
  2025-09-29  4:54 ` Dmitry Torokhov
@ 2025-09-30  6:31 ` kernel test robot
  2025-10-01 10:23   ` [PATCH v2 1/2] workqueue: Add initialization macro for work items that disabled by default pengyu
  1 sibling, 1 reply; 10+ messages in thread
From: kernel test robot @ 2025-09-30  6:31 UTC (permalink / raw)
  To: pengyu
  Cc: oe-lkp, lkp, linux-kernel, linux-serial, gregkh, jirislaby,
	legion, mingo, myrrhperiwinkle, tglx, dmitry.torokhov,
	changlianzhi, pengyu, syzbot+79c403850e6816dc39cf, oliver.sang



Hello,

kernel test robot noticed "WARNING:at_kernel/workqueue.c:#enable_work" on:

commit: 81d246ffd402b46338d8a3e0cd23f474195dd236 ("[PATCH] tty/vt: Fix possible deadlock in input_inject_event")
url: https://github.com/intel-lab-lkp/linux/commits/pengyu/tty-vt-Fix-possible-deadlock-in-input_inject_event/20250928-211353
base: https://git.kernel.org/cgit/linux/kernel/git/gregkh/tty.git tty-testing
patch link: https://lore.kernel.org/all/20250928130819.383808-1-pengyu@kylinos.cn/
patch subject: [PATCH] tty/vt: Fix possible deadlock in input_inject_event

in testcase: boot

config: x86_64-kexec
compiler: clang-20
test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G

(please refer to attached dmesg/kmsg for entire log/backtrace)


+--------------------------------------------+------------+------------+
|                                            | f4abab3508 | 81d246ffd4 |
+--------------------------------------------+------------+------------+
| WARNING:at_kernel/workqueue.c:#enable_work | 0          | 12         |
| RIP:enable_work                            | 0          | 12         |
+--------------------------------------------+------------+------------+


If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202509301323.34d956e1-lkp@intel.com


[    0.877354][    T1] ------------[ cut here ]------------
[    0.878411][    T1] workqueue: work disable count underflowed
[ 0.879704][ T1] WARNING: CPU: 0 PID: 1 at kernel/workqueue.c:4326 enable_work (kernel/workqueue.c:4326) 
[    0.881213][    T1] Modules linked in:
[    0.881232][    T1] CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.17.0-rc6-00045-g81d246ffd402 #1 PREEMPT(voluntary)
[    0.881232][    T1] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
[ 0.881232][ T1] RIP: 0010:enable_work (kernel/workqueue.c:4326) 
[ 0.881232][ T1] Code: 5e 5d e9 0c c1 ec 00 cc 0f 0b eb 9b 31 ed 80 3d 1e e8 c6 01 00 75 a0 c6 05 15 e8 c6 01 01 48 c7 c7 cd bf 6d 82 e8 89 26 f3 ff <0f> 0b eb 89 0f 0b eb 93 e8 cc c7 eb 00 66 66 66 2e 0f 1f 84 00 00
All code
========
   0:	5e                   	pop    %rsi
   1:	5d                   	pop    %rbp
   2:	e9 0c c1 ec 00       	jmp    0xecc113
   7:	cc                   	int3
   8:	0f 0b                	ud2
   a:	eb 9b                	jmp    0xffffffffffffffa7
   c:	31 ed                	xor    %ebp,%ebp
   e:	80 3d 1e e8 c6 01 00 	cmpb   $0x0,0x1c6e81e(%rip)        # 0x1c6e833
  15:	75 a0                	jne    0xffffffffffffffb7
  17:	c6 05 15 e8 c6 01 01 	movb   $0x1,0x1c6e815(%rip)        # 0x1c6e833
  1e:	48 c7 c7 cd bf 6d 82 	mov    $0xffffffff826dbfcd,%rdi
  25:	e8 89 26 f3 ff       	call   0xfffffffffff326b3
  2a:*	0f 0b                	ud2		<-- trapping instruction
  2c:	eb 89                	jmp    0xffffffffffffffb7
  2e:	0f 0b                	ud2
  30:	eb 93                	jmp    0xffffffffffffffc5
  32:	e8 cc c7 eb 00       	call   0xebc803
  37:	66                   	data16
  38:	66                   	data16
  39:	66                   	data16
  3a:	2e                   	cs
  3b:	0f                   	.byte 0xf
  3c:	1f                   	(bad)
  3d:	84 00                	test   %al,(%rax)
	...

Code starting with the faulting instruction
===========================================
   0:	0f 0b                	ud2
   2:	eb 89                	jmp    0xffffffffffffff8d
   4:	0f 0b                	ud2
   6:	eb 93                	jmp    0xffffffffffffff9b
   8:	e8 cc c7 eb 00       	call   0xebc7d9
   d:	66                   	data16
   e:	66                   	data16
   f:	66                   	data16
  10:	2e                   	cs
  11:	0f                   	.byte 0xf
  12:	1f                   	(bad)
  13:	84 00                	test   %al,(%rax)
	...
[    0.881232][    T1] RSP: 0000:ffffc90000013b28 EFLAGS: 00010046
[    0.881232][    T1] RAX: 63593ad70bfd2600 RBX: ffffffff82ead988 RCX: 63593ad70bfd2601
[    0.881232][    T1] RDX: ffffc90000013918 RSI: 00000000ffff7fff RDI: 0000000000000001
[    0.881232][    T1] RBP: 0000000000000000 R08: 0000000000000000 R09: ffffc90000013910
[    0.881232][    T1] R10: 00000000ffff7fff R11: 0000000000000001 R12: 0000000000000000
[    0.881232][    T1] R13: 0000000000000000 R14: 000fffffffe00001 R15: 0000000000000000
[    0.881232][    T1] FS:  0000000000000000(0000) GS:ffff8884ac411000(0000) knlGS:0000000000000000
[    0.881232][    T1] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    0.881232][    T1] CR2: ffff88843ffff000 CR3: 0000000002a30000 CR4: 00000000000406f0
[    0.881232][    T1] Call Trace:
[    0.881232][    T1]  <TASK>
[ 0.881232][ T1] kbd_init (include/linux/workqueue.h:730 drivers/tty/vt/keyboard.c:1666) 
[ 0.881232][ T1] vty_init (drivers/tty/vt/vt.c:3884) 
[ 0.881232][ T1] tty_init (drivers/tty/tty_io.c:3651) 
[ 0.881232][ T1] ? __pfx_chr_dev_init (drivers/char/mem.c:734) 
[ 0.881232][ T1] do_one_initcall (init/main.c:1269) 
[ 0.881232][ T1] ? __alloc_frozen_pages_noprof (mm/page_alloc.c:5148) 
[ 0.881232][ T1] ? alloc_pages_mpol (mm/mempolicy.c:2416) 
[ 0.881232][ T1] ? new_slab (mm/slub.c:560 mm/slub.c:2698 mm/slub.c:2714) 
[ 0.881232][ T1] ? __proc_create (fs/proc/generic.c:453) 
[ 0.881232][ T1] ? ida_alloc_range (include/linux/xarray.h:1437 lib/idr.c:461) 
[ 0.881232][ T1] ? parameq (kernel/params.c:90 kernel/params.c:99) 
[ 0.881232][ T1] ? __pfx_ignore_unknown_bootoption (init/main.c:1315) 
[ 0.881232][ T1] ? parse_args (kernel/params.c:153 kernel/params.c:186) 
[ 0.881232][ T1] do_initcall_level (init/main.c:1330) 
[ 0.881232][ T1] do_initcalls (init/main.c:1344) 
[ 0.881232][ T1] kernel_init_freeable (init/main.c:1583) 
[ 0.881232][ T1] ? __pfx_kernel_init (init/main.c:1461) 
[ 0.881232][ T1] kernel_init (init/main.c:1471) 
[ 0.881232][ T1] ret_from_fork (arch/x86/kernel/process.c:154) 
[ 0.881232][ T1] ? __pfx_kernel_init (init/main.c:1461) 
[ 0.881232][ T1] ret_from_fork_asm (arch/x86/entry/entry_64.S:258) 
[    0.881232][    T1]  </TASK>
[    0.881232][    T1] ---[ end trace 0000000000000000 ]---


The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20250930/202509301323.34d956e1-lkp@intel.com



-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 1/2] workqueue: Add initialization macro for work items that disabled by default
  2025-09-30  6:31 ` kernel test robot
@ 2025-10-01 10:23   ` pengyu
  2025-10-01 10:23     ` [PATCH v2 2/2] tty/vt: Fix possible deadlock in input_inject_event pengyu
  2025-10-07 19:18     ` [PATCH v2 1/2] workqueue: Add initialization macro for work items that disabled by default Tejun Heo
  0 siblings, 2 replies; 10+ messages in thread
From: pengyu @ 2025-10-01 10:23 UTC (permalink / raw)
  To: tj, jiangshanlai, oliver.sang
  Cc: changlianzhi, dmitry.torokhov, gregkh, jirislaby, legion,
	linux-kernel, linux-serial, lkp, mingo, myrrhperiwinkle, oe-lkp,
	pengyu, syzbot+79c403850e6816dc39cf, tglx

In certain scenarios, workqueue tasks that are disabled by default are
required. Similar to DECLARE_TASKLET_DISABLED, the DECLARE_WORK_DISABLED
macro is added to achieve this functionality.

Signed-off-by: pengyu <pengyu@kylinos.cn>
---
 include/linux/workqueue.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index 45d5dd470ff6..b6c72d59351b 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -102,6 +102,7 @@ enum wq_misc_consts {
 /* Convenience constants - of type 'unsigned long', not 'enum'! */
 #define WORK_OFFQ_BH		(1ul << WORK_OFFQ_BH_BIT)
 #define WORK_OFFQ_FLAG_MASK	(((1ul << WORK_OFFQ_FLAG_BITS) - 1) << WORK_OFFQ_FLAG_SHIFT)
+#define WORK_OFFQ_DISABLED	(1ul  << WORK_OFFQ_DISABLE_SHIFT)
 #define WORK_OFFQ_DISABLE_MASK	(((1ul << WORK_OFFQ_DISABLE_BITS) - 1) << WORK_OFFQ_DISABLE_SHIFT)
 #define WORK_OFFQ_POOL_NONE	((1ul << WORK_OFFQ_POOL_BITS) - 1)
 #define WORK_STRUCT_NO_POOL	(WORK_OFFQ_POOL_NONE << WORK_OFFQ_POOL_SHIFT)
@@ -110,6 +111,8 @@ enum wq_misc_consts {
 #define WORK_DATA_INIT()	ATOMIC_LONG_INIT((unsigned long)WORK_STRUCT_NO_POOL)
 #define WORK_DATA_STATIC_INIT()	\
 	ATOMIC_LONG_INIT((unsigned long)(WORK_STRUCT_NO_POOL | WORK_STRUCT_STATIC))
+#define WORK_DATA_DISABLED_INIT()	\
+		ATOMIC_LONG_INIT((unsigned long)(WORK_STRUCT_NO_POOL | WORK_STRUCT_STATIC | WORK_OFFQ_DISABLED))
 
 struct delayed_work {
 	struct work_struct work;
@@ -242,6 +245,13 @@ struct execute_work {
 	__WORK_INIT_LOCKDEP_MAP(#n, &(n))				\
 	}
 
+#define __WORK_DISABLED_INITIALIZER(n, f) {					\
+	.data = WORK_DATA_DISABLED_INIT(),				\
+	.entry	= { &(n).entry, &(n).entry },				\
+	.func = (f),							\
+	__WORK_INIT_LOCKDEP_MAP(#n, &(n))				\
+	}
+
 #define __DELAYED_WORK_INITIALIZER(n, f, tflags) {			\
 	.work = __WORK_INITIALIZER((n).work, (f)),			\
 	.timer = __TIMER_INITIALIZER(delayed_work_timer_fn,\
@@ -251,6 +261,9 @@ struct execute_work {
 #define DECLARE_WORK(n, f)						\
 	struct work_struct n = __WORK_INITIALIZER(n, f)
 
+#define DECLARE_WORK_DISABLED(n, f)						\
+	struct work_struct n = __WORK_DISABLED_INITIALIZER(n, f)
+
 #define DECLARE_DELAYED_WORK(n, f)					\
 	struct delayed_work n = __DELAYED_WORK_INITIALIZER(n, f, 0)
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 2/2] tty/vt: Fix possible deadlock in input_inject_event
  2025-10-01 10:23   ` [PATCH v2 1/2] workqueue: Add initialization macro for work items that disabled by default pengyu
@ 2025-10-01 10:23     ` pengyu
  2025-10-07 19:18     ` [PATCH v2 1/2] workqueue: Add initialization macro for work items that disabled by default Tejun Heo
  1 sibling, 0 replies; 10+ messages in thread
From: pengyu @ 2025-10-01 10:23 UTC (permalink / raw)
  To: tj, jiangshanlai, oliver.sang
  Cc: changlianzhi, dmitry.torokhov, gregkh, jirislaby, legion,
	linux-kernel, linux-serial, lkp, mingo, myrrhperiwinkle, oe-lkp,
	pengyu, syzbot+79c403850e6816dc39cf, tglx

syzkaller testing revealed a potential deadlock involving keyboard
handling:

CPU0                       CPU1                      CPU2
----                       ----                      ----
read_lock(tasklist_lock);  evdev_write
                          input_inject_event     write_lock(tasklist_lock);
                         lock(&dev->event_lock);
                        read_lock(tasklist_lock);
<Interrupt>
kbd_bh() / kd_sound_helper()
input_inject_event
lock(&dev->event_lock); // Deadlock risk

The deadlock occurs because:
1. Both kbd_bh and kd_sound_helper run in interrupt context
2. tasklist_lock is interrupt-unsafe
3. When evdev_write holds both dev->event_lock and tasklist_lock,
   interrupt context attempts to acquire dev->event_lock create deadlock
   risks

Convert both kbd_bh and kd_sound_helper to use workqueues. This moves
input_inject_event execution to process context, where it's safe to
acquire locks that may be held by code using interrupt-unsafe locks.

Reported-by: syzbot+79c403850e6816dc39cf@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/all/66f6c8ce.050a0220.46d20.001c.GAE@google.com/T/#u
Fixes: fb09d0ac0772 ("tty: Fix the keyboard led light display problem")

Signed-off-by: pengyu <pengyu@kylinos.cn>
---
Changes in v2:
  - enable_work needs to be used in pairs with disable_work_sync,
    not with cancel_work_sync.
  - use work items that diabled by default.
---
 drivers/tty/vt/keyboard.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
index ee1d9c448c7e..d3d9c2fda467 100644
--- a/drivers/tty/vt/keyboard.c
+++ b/drivers/tty/vt/keyboard.c
@@ -131,8 +131,8 @@ static const unsigned char max_vals[] = {
 
 static const int NR_TYPES = ARRAY_SIZE(max_vals);
 
-static void kbd_bh(struct tasklet_struct *unused);
-static DECLARE_TASKLET_DISABLED(keyboard_tasklet, kbd_bh);
+static void kbd_bh(struct work_struct *unused);
+static DECLARE_WORK_DISABLED(keyboard_work, kbd_bh);
 
 static struct input_handler kbd_handler;
 static DEFINE_SPINLOCK(kbd_event_lock);
@@ -264,23 +264,23 @@ static int kd_sound_helper(struct input_handle *handle, void *data)
 	return 0;
 }
 
-static void kd_nosound(struct timer_list *unused)
+static void kd_nosound(struct work_struct *unused)
 {
 	static unsigned int zero;
 
 	input_handler_for_each_handle(&kbd_handler, &zero, kd_sound_helper);
 }
 
-static DEFINE_TIMER(kd_mksound_timer, kd_nosound);
+static DECLARE_DELAYED_WORK(kd_mksound_worker, kd_nosound);
 
 void kd_mksound(unsigned int hz, unsigned int ticks)
 {
-	timer_delete_sync(&kd_mksound_timer);
+	cancel_delayed_work_sync(&kd_mksound_worker);
 
 	input_handler_for_each_handle(&kbd_handler, &hz, kd_sound_helper);
 
 	if (hz && ticks)
-		mod_timer(&kd_mksound_timer, jiffies + ticks);
+		schedule_delayed_work(&kd_mksound_worker, ticks);
 }
 EXPORT_SYMBOL(kd_mksound);
 
@@ -390,7 +390,7 @@ static void put_queue_utf8(struct vc_data *vc, u32 value)
 /* FIXME: review locking for vt.c callers */
 static void set_leds(void)
 {
-	tasklet_schedule(&keyboard_tasklet);
+	schedule_work(&keyboard_work);
 }
 
 /*
@@ -1024,10 +1024,10 @@ static int kbd_led_trigger_activate(struct led_classdev *cdev)
 	struct kbd_led_trigger *trigger =
 		container_of(cdev->trigger, struct kbd_led_trigger, trigger);
 
-	tasklet_disable(&keyboard_tasklet);
+	disable_work_sync(&keyboard_work);
 	if (ledstate != -1U)
 		led_set_brightness(cdev, ledstate & trigger->mask ? LED_FULL : LED_OFF);
-	tasklet_enable(&keyboard_tasklet);
+	enable_work(&keyboard_work);
 
 	return 0;
 }
@@ -1243,7 +1243,7 @@ void vt_kbd_con_stop(unsigned int console)
  * handle the scenario when keyboard handler is not registered yet
  * but we already getting updates from the VT to update led state.
  */
-static void kbd_bh(struct tasklet_struct *unused)
+static void kbd_bh(struct work_struct *unused)
 {
 	unsigned int leds;
 	unsigned long flags;
@@ -1535,7 +1535,7 @@ static void kbd_event(struct input_handle *handle, unsigned int event_type,
 
 	spin_unlock(&kbd_event_lock);
 
-	tasklet_schedule(&keyboard_tasklet);
+	schedule_work(&keyboard_work);
 	do_poke_blanked_console = 1;
 	schedule_console_callback();
 }
@@ -1607,12 +1607,12 @@ static void kbd_disconnect(struct input_handle *handle)
  */
 static void kbd_start(struct input_handle *handle)
 {
-	tasklet_disable(&keyboard_tasklet);
+	disable_work_sync(&keyboard_work);
 
 	if (ledstate != -1U)
 		kbd_update_leds_helper(handle, &ledstate);
 
-	tasklet_enable(&keyboard_tasklet);
+	enable_work(&keyboard_work);
 }
 
 static const struct input_device_id kbd_ids[] = {
@@ -1662,8 +1662,8 @@ int __init kbd_init(void)
 	if (error)
 		return error;
 
-	tasklet_enable(&keyboard_tasklet);
-	tasklet_schedule(&keyboard_tasklet);
+	enable_work(&keyboard_work);
+	schedule_work(&keyboard_work);
 
 	return 0;
 }
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] tty/vt: Fix possible deadlock in input_inject_event
  2025-09-29  4:54 ` Dmitry Torokhov
@ 2025-10-01 10:42   ` pengyu
  0 siblings, 0 replies; 10+ messages in thread
From: pengyu @ 2025-10-01 10:42 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: gregkh, jirislaby, legion, mingo, myrrhperiwinkle, tglx,
	changlianzhi, linux-kernel, linux-serial,
	syzbot+79c403850e6816dc39cf, jiangshanlai, tj

在 2025/9/29 12:54, Dmitry Torokhov 写道:
> Hi,
> 
> On Sun, Sep 28, 2025 at 09:08:19PM +0800, pengyu wrote:
>> syzkaller testing revealed a potential deadlock involving keyboard
>> handling:
>>
>> CPU0                       CPU1                      CPU2
>> ----                       ----                      ----
>> read_lock(tasklist_lock);  evdev_write
>>                            input_inject_event     write_lock(tasklist_lock);
>>                           lock(&dev->event_lock);
>>                          read_lock(tasklist_lock);
>> <Interrupt>
>> kbd_bh() / kd_sound_helper()
>> input_inject_event
>> lock(&dev->event_lock); // Deadlock risk
>>
>> The deadlock occurs because:
>> 1. Both kbd_bh and kd_sound_helper run in interrupt context
>> 2. tasklist_lock is interrupt-unsafe
>> 3. When evdev_write holds both dev->event_lock and tasklist_lock,
>>     interrupt context attempts to acquire dev->event_lock create deadlock
>>     risks
>>
>> Convert both kbd_bh and kd_sound_helper to use workqueues. This moves
>> input_inject_event execution to process context, where it's safe to
>> acquire locks that may be held by code using interrupt-unsafe locks.
> 
> So if we ignore the input code and instead look at the send_sigio()
> (which input core ends up calling) and do_wait() we see that
> send_sigio() disables interrupts and takes the owner's spinlock
> before taking the tasklist_lock, while do_wait() takes the tasklist_lock
> first, without disabling interrupts. This is root of the issue as far as
> I can tell and no amount of changes to the keyboard handler (which is
> just happens to be in the middle) will not solve for all potential cases
> and code paths.
> 
> I believe either do_exit() or send_sigio() have to be changed to fix
> this properly.
> 
> Thanks.
> 

Hi,

I noticed that besides do_wait, there are many places in the kernel 
where read_lock(tasklist_lock) is used without disabling interrupts. 
Addressing this solely through tasklist_lock may not fully resolve the 
issue.

This involves tasklist_lock, evdev_write, and various input device 
drivers. The only approach I can think of is to move functions like 
input_[inject]_event in the input drivers out of the interrupt context. 
This could affect many code paths, so I plan to start by modifying the 
keyboard code first.

-- 
Thanks,
Yu Peng


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/2] workqueue: Add initialization macro for work items that disabled by default
  2025-10-01 10:23   ` [PATCH v2 1/2] workqueue: Add initialization macro for work items that disabled by default pengyu
  2025-10-01 10:23     ` [PATCH v2 2/2] tty/vt: Fix possible deadlock in input_inject_event pengyu
@ 2025-10-07 19:18     ` Tejun Heo
  2025-10-17  7:56       ` [PATCH v3 " Yu Peng
  1 sibling, 1 reply; 10+ messages in thread
From: Tejun Heo @ 2025-10-07 19:18 UTC (permalink / raw)
  To: pengyu
  Cc: jiangshanlai, oliver.sang, changlianzhi, dmitry.torokhov, gregkh,
	jirislaby, legion, linux-kernel, linux-serial, lkp, mingo,
	myrrhperiwinkle, oe-lkp, syzbot+79c403850e6816dc39cf, tglx

On Wed, Oct 01, 2025 at 06:23:40PM +0800, pengyu wrote:
> In certain scenarios, workqueue tasks that are disabled by default are
> required. Similar to DECLARE_TASKLET_DISABLED, the DECLARE_WORK_DISABLED
> macro is added to achieve this functionality.
> 
> Signed-off-by: pengyu <pengyu@kylinos.cn>

Can you please use FIRST_NAME LAST_NAME <EMAIL> for the From and SOB tags?

Thanks.

-- 
tejun

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v3 1/2] workqueue: Add initialization macro for work items that disabled by default
  2025-10-07 19:18     ` [PATCH v2 1/2] workqueue: Add initialization macro for work items that disabled by default Tejun Heo
@ 2025-10-17  7:56       ` Yu Peng
  2025-10-17  7:56         ` [PATCH v3 2/2] tty/vt: Fix possible deadlock in input_inject_event Yu Peng
  2025-10-17 15:35         ` [PATCH v3 1/2] workqueue: Add initialization macro for work items that disabled by default Tejun Heo
  0 siblings, 2 replies; 10+ messages in thread
From: Yu Peng @ 2025-10-17  7:56 UTC (permalink / raw)
  To: tj
  Cc: changlianzhi, dmitry.torokhov, gregkh, jiangshanlai, jirislaby,
	legion, linux-kernel, linux-serial, lkp, mingo, myrrhperiwinkle,
	oe-lkp, oliver.sang, pengyu, syzbot+79c403850e6816dc39cf, tglx

In certain scenarios, workqueue tasks that are disabled by default are
required. Similar to DECLARE_TASKLET_DISABLED, the DECLARE_WORK_DISABLED
macro is added to achieve this functionality.

Signed-off-by: Yu Peng <pengyu@kylinos.cn>
---
 include/linux/workqueue.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index 45d5dd470ff6..b6c72d59351b 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -102,6 +102,7 @@ enum wq_misc_consts {
 /* Convenience constants - of type 'unsigned long', not 'enum'! */
 #define WORK_OFFQ_BH		(1ul << WORK_OFFQ_BH_BIT)
 #define WORK_OFFQ_FLAG_MASK	(((1ul << WORK_OFFQ_FLAG_BITS) - 1) << WORK_OFFQ_FLAG_SHIFT)
+#define WORK_OFFQ_DISABLED	(1ul  << WORK_OFFQ_DISABLE_SHIFT)
 #define WORK_OFFQ_DISABLE_MASK	(((1ul << WORK_OFFQ_DISABLE_BITS) - 1) << WORK_OFFQ_DISABLE_SHIFT)
 #define WORK_OFFQ_POOL_NONE	((1ul << WORK_OFFQ_POOL_BITS) - 1)
 #define WORK_STRUCT_NO_POOL	(WORK_OFFQ_POOL_NONE << WORK_OFFQ_POOL_SHIFT)
@@ -110,6 +111,8 @@ enum wq_misc_consts {
 #define WORK_DATA_INIT()	ATOMIC_LONG_INIT((unsigned long)WORK_STRUCT_NO_POOL)
 #define WORK_DATA_STATIC_INIT()	\
 	ATOMIC_LONG_INIT((unsigned long)(WORK_STRUCT_NO_POOL | WORK_STRUCT_STATIC))
+#define WORK_DATA_DISABLED_INIT()	\
+		ATOMIC_LONG_INIT((unsigned long)(WORK_STRUCT_NO_POOL | WORK_STRUCT_STATIC | WORK_OFFQ_DISABLED))
 
 struct delayed_work {
 	struct work_struct work;
@@ -242,6 +245,13 @@ struct execute_work {
 	__WORK_INIT_LOCKDEP_MAP(#n, &(n))				\
 	}
 
+#define __WORK_DISABLED_INITIALIZER(n, f) {					\
+	.data = WORK_DATA_DISABLED_INIT(),				\
+	.entry	= { &(n).entry, &(n).entry },				\
+	.func = (f),							\
+	__WORK_INIT_LOCKDEP_MAP(#n, &(n))				\
+	}
+
 #define __DELAYED_WORK_INITIALIZER(n, f, tflags) {			\
 	.work = __WORK_INITIALIZER((n).work, (f)),			\
 	.timer = __TIMER_INITIALIZER(delayed_work_timer_fn,\
@@ -251,6 +261,9 @@ struct execute_work {
 #define DECLARE_WORK(n, f)						\
 	struct work_struct n = __WORK_INITIALIZER(n, f)
 
+#define DECLARE_WORK_DISABLED(n, f)						\
+	struct work_struct n = __WORK_DISABLED_INITIALIZER(n, f)
+
 #define DECLARE_DELAYED_WORK(n, f)					\
 	struct delayed_work n = __DELAYED_WORK_INITIALIZER(n, f, 0)
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v3 2/2] tty/vt: Fix possible deadlock in input_inject_event
  2025-10-17  7:56       ` [PATCH v3 " Yu Peng
@ 2025-10-17  7:56         ` Yu Peng
  2025-10-17 15:35         ` [PATCH v3 1/2] workqueue: Add initialization macro for work items that disabled by default Tejun Heo
  1 sibling, 0 replies; 10+ messages in thread
From: Yu Peng @ 2025-10-17  7:56 UTC (permalink / raw)
  To: tj
  Cc: changlianzhi, dmitry.torokhov, gregkh, jiangshanlai, jirislaby,
	legion, linux-kernel, linux-serial, lkp, mingo, myrrhperiwinkle,
	oe-lkp, oliver.sang, pengyu, syzbot+79c403850e6816dc39cf, tglx

syzkaller testing revealed a potential deadlock involving keyboard
handling:

CPU0                       CPU1                      CPU2
----                       ----                      ----
read_lock(tasklist_lock);  evdev_write
                          input_inject_event     write_lock(tasklist_lock);
                         lock(&dev->event_lock);
                        read_lock(tasklist_lock);
<Interrupt>
kbd_bh() / kd_sound_helper()
input_inject_event
lock(&dev->event_lock); // Deadlock risk

The deadlock occurs because:
1. Both kbd_bh and kd_sound_helper run in interrupt context
2. tasklist_lock is interrupt-unsafe
3. When evdev_write holds both dev->event_lock and tasklist_lock,
   interrupt context attempts to acquire dev->event_lock create deadlock
   risks

Convert both kbd_bh and kd_sound_helper to use workqueues. This moves
input_inject_event execution to process context, where it's safe to
acquire locks that may be held by code using interrupt-unsafe locks.

Reported-by: syzbot+79c403850e6816dc39cf@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/all/66f6c8ce.050a0220.46d20.001c.GAE@google.com/T/#u
Fixes: fb09d0ac0772 ("tty: Fix the keyboard led light display problem")

Signed-off-by: Yu Peng <pengyu@kylinos.cn>
---
Hi, tejun
Thank you for pointing that out. I'll make sure to use the proper format in 
future submissions.
---
Changes in v3:
  - use the correct signature format.
Changes in v2:
  - enable_work needs to be used in pairs with disable_work_sync,
    not with cancel_work_sync.
---
 drivers/tty/vt/keyboard.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
index ee1d9c448c7e..d3d9c2fda467 100644
--- a/drivers/tty/vt/keyboard.c
+++ b/drivers/tty/vt/keyboard.c
@@ -131,8 +131,8 @@ static const unsigned char max_vals[] = {
 
 static const int NR_TYPES = ARRAY_SIZE(max_vals);
 
-static void kbd_bh(struct tasklet_struct *unused);
-static DECLARE_TASKLET_DISABLED(keyboard_tasklet, kbd_bh);
+static void kbd_bh(struct work_struct *unused);
+static DECLARE_WORK_DISABLED(keyboard_work, kbd_bh);
 
 static struct input_handler kbd_handler;
 static DEFINE_SPINLOCK(kbd_event_lock);
@@ -264,23 +264,23 @@ static int kd_sound_helper(struct input_handle *handle, void *data)
 	return 0;
 }
 
-static void kd_nosound(struct timer_list *unused)
+static void kd_nosound(struct work_struct *unused)
 {
 	static unsigned int zero;
 
 	input_handler_for_each_handle(&kbd_handler, &zero, kd_sound_helper);
 }
 
-static DEFINE_TIMER(kd_mksound_timer, kd_nosound);
+static DECLARE_DELAYED_WORK(kd_mksound_worker, kd_nosound);
 
 void kd_mksound(unsigned int hz, unsigned int ticks)
 {
-	timer_delete_sync(&kd_mksound_timer);
+	cancel_delayed_work_sync(&kd_mksound_worker);
 
 	input_handler_for_each_handle(&kbd_handler, &hz, kd_sound_helper);
 
 	if (hz && ticks)
-		mod_timer(&kd_mksound_timer, jiffies + ticks);
+		schedule_delayed_work(&kd_mksound_worker, ticks);
 }
 EXPORT_SYMBOL(kd_mksound);
 
@@ -390,7 +390,7 @@ static void put_queue_utf8(struct vc_data *vc, u32 value)
 /* FIXME: review locking for vt.c callers */
 static void set_leds(void)
 {
-	tasklet_schedule(&keyboard_tasklet);
+	schedule_work(&keyboard_work);
 }
 
 /*
@@ -1024,10 +1024,10 @@ static int kbd_led_trigger_activate(struct led_classdev *cdev)
 	struct kbd_led_trigger *trigger =
 		container_of(cdev->trigger, struct kbd_led_trigger, trigger);
 
-	tasklet_disable(&keyboard_tasklet);
+	disable_work_sync(&keyboard_work);
 	if (ledstate != -1U)
 		led_set_brightness(cdev, ledstate & trigger->mask ? LED_FULL : LED_OFF);
-	tasklet_enable(&keyboard_tasklet);
+	enable_work(&keyboard_work);
 
 	return 0;
 }
@@ -1243,7 +1243,7 @@ void vt_kbd_con_stop(unsigned int console)
  * handle the scenario when keyboard handler is not registered yet
  * but we already getting updates from the VT to update led state.
  */
-static void kbd_bh(struct tasklet_struct *unused)
+static void kbd_bh(struct work_struct *unused)
 {
 	unsigned int leds;
 	unsigned long flags;
@@ -1535,7 +1535,7 @@ static void kbd_event(struct input_handle *handle, unsigned int event_type,
 
 	spin_unlock(&kbd_event_lock);
 
-	tasklet_schedule(&keyboard_tasklet);
+	schedule_work(&keyboard_work);
 	do_poke_blanked_console = 1;
 	schedule_console_callback();
 }
@@ -1607,12 +1607,12 @@ static void kbd_disconnect(struct input_handle *handle)
  */
 static void kbd_start(struct input_handle *handle)
 {
-	tasklet_disable(&keyboard_tasklet);
+	disable_work_sync(&keyboard_work);
 
 	if (ledstate != -1U)
 		kbd_update_leds_helper(handle, &ledstate);
 
-	tasklet_enable(&keyboard_tasklet);
+	enable_work(&keyboard_work);
 }
 
 static const struct input_device_id kbd_ids[] = {
@@ -1662,8 +1662,8 @@ int __init kbd_init(void)
 	if (error)
 		return error;
 
-	tasklet_enable(&keyboard_tasklet);
-	tasklet_schedule(&keyboard_tasklet);
+	enable_work(&keyboard_work);
+	schedule_work(&keyboard_work);
 
 	return 0;
 }
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 1/2] workqueue: Add initialization macro for work items that disabled by default
  2025-10-17  7:56       ` [PATCH v3 " Yu Peng
  2025-10-17  7:56         ` [PATCH v3 2/2] tty/vt: Fix possible deadlock in input_inject_event Yu Peng
@ 2025-10-17 15:35         ` Tejun Heo
  1 sibling, 0 replies; 10+ messages in thread
From: Tejun Heo @ 2025-10-17 15:35 UTC (permalink / raw)
  To: Yu Peng
  Cc: changlianzhi, dmitry.torokhov, gregkh, jiangshanlai, jirislaby,
	legion, linux-kernel, linux-serial, lkp, mingo, myrrhperiwinkle,
	oe-lkp, oliver.sang, syzbot+79c403850e6816dc39cf, tglx

On Fri, Oct 17, 2025 at 03:56:54PM +0800, Yu Peng wrote:
> In certain scenarios, workqueue tasks that are disabled by default are
> required. Similar to DECLARE_TASKLET_DISABLED, the DECLARE_WORK_DISABLED
> macro is added to achieve this functionality.
> 
> Signed-off-by: Yu Peng <pengyu@kylinos.cn>

Applied to wq/for-6.19.

Thanks.

-- 
tejun

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2025-10-17 15:35 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-09-28 13:08 [PATCH] tty/vt: Fix possible deadlock in input_inject_event pengyu
2025-09-29  4:54 ` Dmitry Torokhov
2025-10-01 10:42   ` pengyu
2025-09-30  6:31 ` kernel test robot
2025-10-01 10:23   ` [PATCH v2 1/2] workqueue: Add initialization macro for work items that disabled by default pengyu
2025-10-01 10:23     ` [PATCH v2 2/2] tty/vt: Fix possible deadlock in input_inject_event pengyu
2025-10-07 19:18     ` [PATCH v2 1/2] workqueue: Add initialization macro for work items that disabled by default Tejun Heo
2025-10-17  7:56       ` [PATCH v3 " Yu Peng
2025-10-17  7:56         ` [PATCH v3 2/2] tty/vt: Fix possible deadlock in input_inject_event Yu Peng
2025-10-17 15:35         ` [PATCH v3 1/2] workqueue: Add initialization macro for work items that disabled by default Tejun Heo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).