[PATCH] fbcon: Avoid cancel_delayed_work_sync() in atomic context

[PATCH] fbcon: Avoid cancel_delayed_work_sync() in atomic context

[Yao Yiqi]

From: Yao Yiqi <yaoyiqi3@huawei.com>

fbcon_cursor() can be reached from the console printing path with
interrupts disabled, but it might sleep in two paths:

  console_unlock()                  <- IRQ disabled here
    vt_console_print()
      hide_cursor()
        con_cursor()
          fbcon_cursor()
            fbcon_del_cursor_work() <- this call may sleep

  console_unlock()                    <- IRQ disabled here
    vt_console_print()
      lf()
        con_scroll()
          fbcon_scroll()
            fbcon_cursor()
              fbcon_del_cursor_work() <- this call may sleep

When the VT software cursor is enabled (vc_cursor_type & CUR_SW),
fbcon_cursor() calls fbcon_del_cursor_work(), which uses
cancel_delayed_work_sync() and therefore may sleep.

With CONFIG_DEBUG_ATOMIC_SLEEP enabled, there are reports like this:

  BUG: sleeping function called from invalid context at kernel/workqueue.c:4722
  in_atomic(): 1, irqs_disabled(): 1, non_block: 0
  ...
  __cancel_work_sync
  fbcon_del_cursor_work
  fbcon_cursor
  hide_cursor
  vt_console_print
  console_unlock
  ...

Use the non-synchronizing cancel when called with interrupts disabled or
from an interrupt context. We still keep the synchronizing cancel in
sleepable contexts, where it is needed to drain the work before
teardown.

Signed-off-by: Yao Yiqi <yaoyiqi3@huawei.com>
---
 drivers/video/fbdev/core/fbcon.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index 94abbb5ac766..f1dbf973e44b 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -56,6 +56,9 @@
  *  more details.
  */
 
+#include "linux/irqflags.h"
+#include "linux/preempt.h"
+#include "linux/workqueue.h"
 #include <linux/export.h>
 #include <linux/module.h>
 #include <linux/types.h>
@@ -441,7 +444,14 @@ static void fbcon_del_cursor_work(struct fb_info *info)
 {
 	struct fbcon_par *par = info->fbcon_par;
 
-	cancel_delayed_work_sync(&par->cursor_work);
+	/*
+	 * fbcon_cursor() can be reached from the console printing path with 
+	 * interrupts disabled. Only synchronize when sleeping is allowed.
+	 */
+	if (in_interrupt() || irqs_disabled())
+		cancel_delayed_work(&par->cursor_work);
+	else
+		cancel_delayed_work_sync(&par->cursor_work);
 }
 
 unsigned char *fbcon_cursor_glyph(const struct vc_font *font, unsigned int cursor_type)
-- 
2.56.0

Re: [PATCH] fbcon: Avoid cancel_delayed_work_sync() in atomic context

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] fbcon: fbcon_del_cursor_work() still sleeps in atomic context on PREEMPT_RT
- [Medium] fbcon: fb_flashcursor() re-queues cursor_work after cancel_delayed_work()
--

commit 81badb044cb0837eebe19d72924931a878fcf8e9
Author: Yao Yiqi <yaoyiqi3@huawei.com>

fbcon: Avoid cancel_delayed_work_sync() in atomic context

This commit updates fbcon_del_cursor_work() to use cancel_delayed_work()
instead of cancel_delayed_work_sync() when in_interrupt() or irqs_disabled()
is true, avoiding sleeping when fbcon_cursor() is called from the console
printing path with interrupts disabled.

> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index 7720dfc80c165..14611be17cb19 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c

[ ... ]

> @@ -441,7 +444,14 @@ static void fbcon_del_cursor_work(struct fb_info *info)
>  {
>  	struct fbcon_par *par = info->fbcon_par;
>  
> -	cancel_delayed_work_sync(&par->cursor_work);
> +	/*
> +	 * fbcon_cursor() can be reached from the console printing path with 
> +	 * interrupts disabled. Only synchronize when sleeping is allowed.
> +	 */
> +	if (in_interrupt() || irqs_disabled())
> +		cancel_delayed_work(&par->cursor_work);
> +	else
> +		cancel_delayed_work_sync(&par->cursor_work);
>  }

[Severity: High]
Can fbcon_del_cursor_work() still call cancel_delayed_work_sync() from an
atomic context when hardware interrupts are enabled?

On CONFIG_PREEMPT_RT, force_legacy_kthread() is true, so
console_emit_next_record() invokes con->write() (vt_console_print()) in task
context without disabling interrupts:

kernel/printk/printk.c:console_emit_next_record() {
    ...
	if (force_legacy_kthread() && !panic_in_progress()) {
		con->write(con, outbuf, pmsg.outbuf_len);
		con->seq = pmsg.seq + 1;
	}
    ...
}

Inside vt_console_print(), spin_trylock(&printing_lock) calls
__rt_spin_trylock(), which acquires printing_lock (an LD_WAIT_CONFIG lock),
enters rcu_read_lock(), and calls migrate_disable() while leaving hardware
interrupts enabled:

drivers/tty/vt/vt.c:vt_console_print() {
    ...
	if (!spin_trylock(&printing_lock))
		return;
    ...
	if (con_is_fg(vc))
		hide_cursor(vc);
    ...
}

When vc->vc_cursor_type & CUR_SW is set, hide_cursor(), lf() -> con_scroll() ->
fbcon_scroll(), and set_cursor() call into fbcon_cursor() ->
fbcon_del_cursor_work(). Because both in_interrupt() and irqs_disabled() are
false, fbcon_del_cursor_work() still calls cancel_delayed_work_sync() ->
__cancel_work_sync() while holding printing_lock and rcu_read_lock().

Similarly, in_interrupt() is marked deprecated in include/linux/preempt.h and
only checks irq_count(), so it does not detect preempt_disable(), spin_lock(),
or rcu_read_lock() critical sections (for example during an oops via
bust_spinlocks(0) -> console_unblank() -> do_unblank_screen() ->
fbcon_cursor()).

Since fbcon_cursor() only stops cursor blinking and does not free par, while
teardown paths such as fbcon_release() and fbcon_deinit() need synchronous
draining, should fbcon_cursor() use a non-synchronizing cancel directly instead
of checking in_interrupt() || irqs_disabled()?

[Severity: Medium]
Also, when fbcon_del_cursor_work() uses cancel_delayed_work() instead of
cancel_delayed_work_sync(), can an in-flight fb_flashcursor() work item
immediately re-arm par->cursor_work after it was canceled?

In fb_flashcursor(), queue_delayed_work() is called outside console_lock and
without checking whether CUR_SW is set or whether the work was canceled:

drivers/video/fbdev/core/fbcon.c:fb_flashcursor() {
    ...
	par->bitops->cursor(vc, info, enable,
			    get_fg_color(vc, info, c),
			    get_bg_color(vc, info, c));
	console_unlock();

	queue_delayed_work(system_power_efficient_wq, &par->cursor_work,
			   par->cur_blink_jiffies);
}

If par->cursor_work was armed (for instance via fbcon_startup(),
fbcon_switch(), fbcon_unblank(), cursor_blink_store(), or when switching to
CUR_SW with CUR_NONE) and fb_flashcursor() is running, vt_console_print() ->
hide_cursor() / set_cursor() -> fbcon_cursor() -> fbcon_del_cursor_work() can
run during or right after fb_flashcursor()'s console_unlock() call.

Because cancel_delayed_work() does not wait for the running fb_flashcursor()
callback to finish, fb_flashcursor() will proceed to queue_delayed_work() and
re-arm par->cursor_work indefinitely.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026145916.414396-1-yyaqia@gmail.com?part=1