Re: [PATCH 1/5] vt: Add cursor-size helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Unprotected concurrent access to the `cur_default` module parameter creates a TOCTOU race that can lead to an infinite loop.
- [Low] The kerneldoc comments inaccurately describe the fallback behavior for unknown cursor types.
--

commit 79b9bf7bbf256d349ee59d8fded428600a818ace
Author: Thomas Zimmermann <tzimmermann@suse.de>

vt: Add cursor-size helpers
    
Cursors in the VT subsystem are blocks within a character cell that are
filled with the foreground color. The new helpers vc_font_cursor_start()
and vc_font_cursor_end() return the scanlines in which the cursor block
starts rsp. ends. This is compatible with VGA hardware

> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index 8f467b22b799e..87c4bc2ef4950 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -264,6 +264,96 @@ unsigned int vc_font_size(const struct vc_font *font)
>  }
>  EXPORT_SYMBOL_GPL(vc_font_size);
>  
> +static unsigned int vc_cursor_start(unsigned int height, unsigned int cursor_size)
> +{
> +retry:
> +	switch (cursor_size) {
[ ... ]
> +	default:
> +		pr_warn_once("Unknown cursor %u\n", cursor_size);
> +		if (cursor_size != CUR_SIZE(cur_default))
> +			cursor_size = CUR_SIZE(cur_default);
> +		else
> +			cursor_size = CUR_UNDERLINE;
> +		goto retry;
> +	}
> +}

[Severity: Medium]
Is there a risk of an infinite loop here due to a data race on cur_default?

Since cur_default is a module parameter that can be modified concurrently,
reading it twice without READ_ONCE() inside vc_cursor_start() allows the
compiler to emit multiple memory reads.

If a concurrent thread modifies the parameter between unknown cursor sizes
(for example, switching between 7 and 8) while this function executes,
could this Time-Of-Check to Time-Of-Use race continuously trigger the goto
retry and cause a soft lockup?

> +/**
> + * vc_font_cursor_start - Calculates the cursor's first scanline within a glyph
> + * @font: The VC font
> + * @cursor_type: The type of cursor pattern
> + *
> + * The parameter @font is an initialized font. The argument in @cursor_type
> + * is one of the CUR_ constants, as stored in struct @vc_data.vc_cursor_type. For
> + * unknown values, the helper draws an underline dash.

[Severity: Low]
Does this kerneldoc accurately describe the fallback behavior?

The documentation for vc_font_cursor_start() promises an unconditional
fallback to an underline dash for unknown values, but the actual code
implementation first falls back to cur_default.

Should the documentation be updated to clarify that it first tries to use
cur_default before falling back to an underline dash?

> + *
> + * Returns:
> + * The index of the cursor's first scanline within the glyph
> + */
> +unsigned int vc_font_cursor_start(const struct vc_font *font, unsigned int cursor_type)
> +{
[ ... ]
> +static unsigned int vc_cursor_end(unsigned int height, unsigned int cursor_size)
> +{
> +retry:
> +	switch (cursor_size) {
[ ... ]
> +	default:
> +		pr_warn_once("Unknown cursor %u\n", cursor_size);
> +		if (cursor_size != CUR_SIZE(cur_default))
> +			cursor_size = CUR_SIZE(cur_default);
> +		else
> +			cursor_size = CUR_UNDERLINE;
> +		goto retry;
> +	}
> +}

[Severity: Medium]
Can this identical fallback logic also result in a soft lockup?

Just like in vc_cursor_start(), the unprotected concurrent access to
cur_default in vc_cursor_end() could potentially trap a thread in an
infinite goto retry loop if the module parameter is modified at the exact
right time.

> +/**
> + * vc_font_cursor_end - Calculates the first scanline after the cursor within a glyph
> + * @font: The VC font
> + * @cursor_type: The type of cursor pattern
> + *
> + * The parameter @font is an initialized font. The argument in @cursor_type
> + * is one of the CUR_ constants, as stored in struct @vc_data.vc_cursor_type. For
> + * unknown values, the helper draws an underline dash.

[Severity: Low]
Is this comment also missing the cur_default fallback behavior?

Similar to vc_font_cursor_start(), the comment for vc_font_cursor_end()
states it draws an underline dash for unknown values, but the code first
relies on cur_default.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911094518.78093-1-tzimmermann@suse.de?part=1