Re: [PATCH 3/3] vt: order the vc_cons[] clear against console_callback()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] tty: vt: unnecessary and flawed lockless synchronization under console_lock
--

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

vt: order the vc_cons[] clear against console_callback()

This commit replaces plain stores and loads of vc_cons[].d with
smp_store_release() and smp_load_acquire() in vc_deallocate() and
console_callback(). It aims to resolve a static memory consistency checker
warning.

> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index 7264021672baa..66aa089bff43a 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -1369,7 +1369,8 @@ struct vc_data *vc_deallocate(unsigned int currcons)
>  		put_pid(vc->vt_pid);
>  		vc_uniscr_set(vc, NULL);
>  		kfree(vc->vc_screenbuf);
> -		vc_cons[currcons].d = NULL;
> +		/* Pairs with the smp_load_acquire() in console_callback(). */
> +		smp_store_release(&vc_cons[currcons].d, NULL);

[Severity: Medium]
Are these memory barriers necessary? Both vc_deallocate() and
console_callback() execute strictly under console_lock. vc_deallocate()
asserts this via WARN_CONSOLE_UNLOCKED(), and console_callback() acquires
it via guard(console_lock)() at the top of the function.

Furthermore, does this unpublish the pointer too late for a lockless reader?
Since it is unpublished after freeing resources like vc_screenbuf, wouldn't
this create a use-after-free window for any hypothetical lockless reader?

>  		if (vc->vc_saved_screen != NULL) {
>  			kfree(vc->vc_saved_screen);
>  			vc->vc_saved_screen = NULL;

[ ... ]

> @@ -3368,7 +3369,8 @@ static void console_callback(struct work_struct *ignored)
>  		poke_blanked_console();
>  	}
>  	if (scrollback_delta) {
> -		struct vc_data *vc = vc_cons[fg_console].d;
> +		/* Pairs with the smp_store_release() in vc_deallocate(). */
> +		struct vc_data *vc = smp_load_acquire(&vc_cons[fg_console].d);

[Severity: Medium]
Since console_lock is held here, is this acquire barrier needed?

Also, doesn't console_callback() still contain plain loads of this same
variable that bypass this acquire barrier? For example, earlier in the
function:

	hide_cursor(vc_cons[fg_console].d);

And later in the function:

	notify_update(vc_cons[fg_console].d);

This appears to leave the lockless synchronization incomplete if it were
actually needed.

>  		clear_selection();
>  		if (vc->vc_mode == KD_TEXT && vc->vc_sw->con_scrolldelta)
>  			vc->vc_sw->con_scrolldelta(vc, scrollback_delta);

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