Re: [PATCH 2/3] vt: order console_blanked between blanking and unblanking

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] vt: redundant barriers for console_blanked under console_lock
- [Medium] vt: smp_store_release placed before protected state updates
--

commit e185177321eb3b6f0a4a6c836f482dfda649b14a
Author: Jaidev Shastri <jaidevshastri@vt.edu>

vt: order console_blanked between blanking and unblanking

This commit adds smp_store_release() and smp_load_acquire() around
console_blanked in do_blank_screen() and do_unblank_screen() to order its
updates against blank_state and the console state.

> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index 3abbd6cf9bd69..7264021672baa 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -4683,7 +4683,8 @@ void do_blank_screen(int entering_gfx)
>  		hide_cursor(vc);
>  		save_screen(vc);
>  		vc->vc_sw->con_blank(vc, VESA_VSYNC_SUSPEND, 1);
> -		console_blanked = fg_console + 1;
> +		/* Pairs with the smp_load_acquire() in do_unblank_screen(). */
> +		smp_store_release(&console_blanked, fg_console + 1);

[Severity: Medium]
Are these barriers necessary for synchronization between do_blank_screen() and
do_unblank_screen()?

Looking at do_blank_screen(), it expects the console lock to be held:

	might_sleep();
	WARN_CONSOLE_UNLOCKED();

do_unblank_screen() also expects the console lock to be held:

	WARN_CONSOLE_UNLOCKED();

Since both functions are fully serialized by the console lock, wouldn't these
barriers be redundant for synchronizing between them?

Additionally, the commit message states that do_blank_screen() stores the value
after blank_state and the console state have been updated. However, the release
store is placed before blank_state and set_origin(vc) are updated here:

		smp_store_release(&console_blanked, fg_console + 1);
		blank_state = blank_off;
		set_origin(vc);

Does this fail to provide the memory ordering guaranteed by the commit message
for concurrent lockless readers?

>  		blank_state = blank_off;
>  		set_origin(vc);
>  		return;

[ ... ]

> @@ -4705,7 +4707,8 @@ void do_blank_screen(int entering_gfx)
>  	/* In case we need to reset origin, blanking hook returns 1 */
>  	i = vc->vc_sw->con_blank(vc, vesa_off_interval ? VESA_VSYNC_SUSPEND :
>  				 (vesa_blank_mode + 1), 0);
> -	console_blanked = fg_console + 1;
> +	/* Pairs with the smp_load_acquire() in do_unblank_screen(). */
> +	smp_store_release(&console_blanked, fg_console + 1);

[Severity: Medium]
Similar to the previous case, the release store is placed before the potential
call to set_origin(vc), leaving the console state update unordered:

	smp_store_release(&console_blanked, fg_console + 1);
	if (i)
		set_origin(vc);

Could a concurrent reader acquire console_blanked but still read stale values
for the console origin?

>  	if (i)
>  		set_origin(vc);
>  

[ ... ]

> @@ -4737,7 +4740,8 @@ void do_unblank_screen(int leaving_gfx)
>  	WARN_CONSOLE_UNLOCKED();
>  
>  	ignore_poke = 0;
> -	if (!console_blanked)
> +	/* Pairs with the smp_store_release() in do_blank_screen(). */
> +	if (!smp_load_acquire(&console_blanked))

[Severity: Medium]
If the intention was to protect true lockless readers such as tioclinux():

	case TIOCL_BLANKEDSCREEN:
		return console_blanked;

is the protection incomplete? console_blanked is still updated with a plain
store later in do_unblank_screen():

	console_blanked = 0;

Would this plain store remain exposed to those lockless readers and leave the
data race intact?

>  		return;
>  	if (!vc_cons_allocated(fg_console)) {
>  		/* impossible */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921-mb-vt-v1-0-71b7990c26c1@vt.edu?part=2