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 <[email protected]> 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/[email protected]?part=1
