[PATCH v2] tty: n_tty: annotate lockless read of ldata->icanon in input_available_p()

[PATCH v2] tty: n_tty: annotate lockless read of ldata->icanon in input_available_p()

[Ziyu Zhang]

n_tty_poll() calls input_available_p() without holding termios_rwsem to
check input readiness for select()/poll(). input_available_p() reads
ldata->icanon, which can be concurrently written by n_tty_set_termios()
under down_write(termios_rwsem).

This is a benign race: poll/select readiness is best-effort, and the
actual n_tty_read() path re-checks icanon under down_read(termios_rwsem).
A stale icanon value in poll only causes a transiently incorrect
readiness result, which is permitted by POSIX poll/select semantics.

Since icanon is a bitfield, READ_ONCE()/WRITE_ONCE() cannot be used.
Annotate the read with data_race() and evaluate it as a boolean to document
the intentional lockless access, bypass __auto_type compiler errors, and
suppress data race detector warnings.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202603162328.vY9JOJWL-lkp@intel.com/
Signed-off-by: Ziyu Zhang <ziyuzhang201@gmail.com>
---
 drivers/tty/n_tty.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index e6a0f5b40..0a0d8d70c 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -1909,7 +1909,8 @@ static inline int input_available_p(const struct tty_struct *tty, int poll)
 	const struct n_tty_data *ldata = tty->disc_data;
 	int amt = poll && !TIME_CHAR(tty) && MIN_CHAR(tty) ? MIN_CHAR(tty) : 1;
 
-	if (ldata->icanon && !L_EXTPROC(tty))
+	/* data_race: benign race, poll readiness is best-effort */
+	if (data_race(!!ldata->icanon) && !L_EXTPROC(tty))
 		return ldata->canon_head != ldata->read_tail;
 	else
 		return ldata->commit_head - ldata->read_tail >= amt;
-- 
2.39.5 (Apple Git-154)

Re: [PATCH v2] tty: n_tty: annotate lockless read of ldata->icanon in input_available_p()

[Greg Kroah-Hartman]

On Tue, Mar 17, 2026 at 04:19:11PM +0800, Ziyu Zhang wrote:
> n_tty_poll() calls input_available_p() without holding termios_rwsem to
> check input readiness for select()/poll(). input_available_p() reads
> ldata->icanon, which can be concurrently written by n_tty_set_termios()
> under down_write(termios_rwsem).
> 
> This is a benign race: poll/select readiness is best-effort, and the
> actual n_tty_read() path re-checks icanon under down_read(termios_rwsem).
> A stale icanon value in poll only causes a transiently incorrect
> readiness result, which is permitted by POSIX poll/select semantics.
> 
> Since icanon is a bitfield, READ_ONCE()/WRITE_ONCE() cannot be used.
> Annotate the read with data_race() and evaluate it as a boolean to document
> the intentional lockless access, bypass __auto_type compiler errors, and
> suppress data race detector warnings.
> 
> Reported-by: kernel test robot <lkp@intel.com>

No this robot did not report this issue :(

> Closes: https://lore.kernel.org/oe-kbuild-all/202603162328.vY9JOJWL-lkp@intel.com/
> Signed-off-by: Ziyu Zhang <ziyuzhang201@gmail.com>
> ---
>  drivers/tty/n_tty.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> index e6a0f5b40..0a0d8d70c 100644
> --- a/drivers/tty/n_tty.c
> +++ b/drivers/tty/n_tty.c
> @@ -1909,7 +1909,8 @@ static inline int input_available_p(const struct tty_struct *tty, int poll)
>  	const struct n_tty_data *ldata = tty->disc_data;
>  	int amt = poll && !TIME_CHAR(tty) && MIN_CHAR(tty) ? MIN_CHAR(tty) : 1;
>  
> -	if (ldata->icanon && !L_EXTPROC(tty))
> +	/* data_race: benign race, poll readiness is best-effort */
> +	if (data_race(!!ldata->icanon) && !L_EXTPROC(tty))
>  		return ldata->canon_head != ldata->read_tail;

Given that the previous patch was not even tested, how was this found
and most importantly, tested?  What error was reported that this now
fixes?

What changed to cause this to now be needed?

thanks,

greg k-h

Re: [PATCH v2] tty: n_tty: annotate lockless read of ldata->icanon in input_available_p()

[ziyu zhang]

> No this robot did not report this issue :(

Sorry, that was my mistake. It is my first time submitting a patch. I
misunderstood what the robot meant.
I will definitely drop this tag and be more careful with submission
guidelines next time.

> Given that the previous patch was not even tested, how was this found
> and most importantly, tested?

My apologies for sending the v1 patch without properly compile-testing
it first.
It is a data race found by our testing tool. The detailed report is
showed below.

Kernel panic: ============ DATARACE ============
VarName 11902078599461647536, BlockLineNumber 36, IrLineNumber 3, is write 1
Function: found_watchpoint kernel/kccwf/wp_checker.c:126 [inline]
Function: watchpoints_monitor+0x1ac2/0x22a0 kernel/kccwf/wp_checker.c:217
Function: kccwf_rec_mem_access+0x275e/0x2b70
Function: n_tty_set_termios+0x5d1/0x37a0 drivers/tty/n_tty.c:1793
Function: tty_set_termios+0x112d/0x1b80 drivers/tty/tty_ioctl.c:348
Function: set_termios+0xc1b/0xca0 drivers/tty/tty_ioctl.c:512
Function: tty_mode_ioctl+0x5e5/0x9d0
Function: n_tty_ioctl_helper+0xe5/0x8f0 drivers/tty/tty_ioctl.c:982
Function: n_tty_ioctl+0x253/0x730 drivers/tty/n_tty.c:2509
Function: tty_ioctl+0x1cfb/0x3070 drivers/tty/tty_io.c:2801
Function: vfs_ioctl fs/ioctl.c:51 [inline]
Function: __do_sys_ioctl fs/ioctl.c:598 [inline]
Function: __se_sys_ioctl+0xfc/0x170 fs/ioctl.c:584
Function: do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
Function: do_syscall_64+0xd2/0x200 arch/x86/entry/syscall_64.c:94
Function: entry_SYSCALL_64_after_hwframe+0x77/0x7f
Function: 0x0
============OTHER_INFO============
VarName 16560329611981062523, BlockLineNumber 32, IrLineNumber 1,
watchpoint index 38236
Function: set_report_info+0xbd/0x2c0 kernel/kccwf/report.c:57
Function: setup_watchpoint kernel/kccwf/wp_checker.c:158 [inline]
Function: watchpoints_monitor+0xe0e/0x22a0 kernel/kccwf/wp_checker.c:239
Function: kccwf_rec_mem_access+0x275e/0x2b70
Function: input_available_p drivers/tty/n_tty.c:1923 [inline]
Function: n_tty_poll+0x3f5/0x16b0 drivers/tty/n_tty.c:2452
Function: tty_poll+0x224/0x4a0 drivers/tty/tty_io.c:2199
Function: vfs_poll include/linux/poll.h:82 [inline]
Function: select_poll_one fs/select.c:480 [inline]
Function: do_select+0xce7/0x13d0 fs/select.c:536
Function: core_sys_select+0x3e8/0x5d0 fs/select.c:677
Function: do_pselect fs/select.c:759 [inline]
Function: __do_sys_pselect6 fs/select.c:802 [inline]
Function: __se_sys_pselect6+0x1d8/0x240 fs/select.c:793
Function: do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
Function: do_syscall_64+0xd2/0x200 arch/x86/entry/syscall_64.c:94
Function: entry_SYSCALL_64_after_hwframe+0x77/0x7f
=================END==============

> What error was reported that this now
> fixes?

Error: cannot pass bit-field as __auto_type initializer in C.
The detailed error is showed below.

drivers/tty/n_tty.c:1913:6: error: cannot pass bit-field as
__auto_type initializer in C
    1913 |         if (data_race(ldata->icanon) && !L_EXTPROC(tty))
         |             ^
   include/linux/compiler.h:194:13: note: expanded from macro 'data_race'
     194 |         auto __v = (expr);
            \
         |                    ^
   1 error generated.

vim +1913 drivers/tty/n_tty.c

  1906
  1907  static inline int input_available_p(const struct tty_struct
*tty, int poll)
  1908  {
  1909          const struct n_tty_data *ldata = tty->disc_data;
  1910          int amt = poll && !TIME_CHAR(tty) && MIN_CHAR(tty) ?
MIN_CHAR(tty) : 1;
  1911
  1912          /* data_race: benign race, poll readiness is best-effort */
> 1913          if (data_race(ldata->icanon) && !L_EXTPROC(tty))
  1914                  return ldata->canon_head != ldata->read_tail;
  1915          else
  1916                  return ldata->commit_head - ldata->read_tail >= amt;
  1917  }
  1918

> What changed to cause this to now be needed?

In my initial v1 patch, I carelessly wrapped the bit-field directly in
the data_race() macro without locally compile-testing it.
The data_race(expr) macro internally utilizes __auto_type (or typeof).
And the C standard strictly prohibits passing a bit-field directly as
an initializer for __auto_type.
Adding !! forces the bit-field to be evaluated as a standard
boolean/integer expression, which bypasses this compiler restriction.
I sincerely apologize for the oversight in my initial patch and any
extra work it may have caused.
Thank you for your time and patience. I will submit a v3 patch if necessary.

Best regards,
Ziyu Zhang

Re: [PATCH v2] tty: n_tty: annotate lockless read of ldata->icanon in input_available_p()

[Greg Kroah-Hartman]

On Tue, Mar 17, 2026 at 07:45:44PM +0800, ziyu zhang wrote:
> > Given that the previous patch was not even tested, how was this found
> > and most importantly, tested?
> 
> My apologies for sending the v1 patch without properly compile-testing
> it first.
> It is a data race found by our testing tool. The detailed report is
> showed below.

As per our documentation, you MUST document what type of tool you are
using when you find stuff like this, so we can push back and say "hey,
your tool is broken!"  :)

And as you never actually tested this, I'm kind of thinking that your
tool is broken...

> Kernel panic: ============ DATARACE ============
> VarName 11902078599461647536, BlockLineNumber 36, IrLineNumber 3, is write 1
> Function: found_watchpoint kernel/kccwf/wp_checker.c:126 [inline]
> Function: watchpoints_monitor+0x1ac2/0x22a0 kernel/kccwf/wp_checker.c:217
> Function: kccwf_rec_mem_access+0x275e/0x2b70
> Function: n_tty_set_termios+0x5d1/0x37a0 drivers/tty/n_tty.c:1793
> Function: tty_set_termios+0x112d/0x1b80 drivers/tty/tty_ioctl.c:348
> Function: set_termios+0xc1b/0xca0 drivers/tty/tty_ioctl.c:512
> Function: tty_mode_ioctl+0x5e5/0x9d0
> Function: n_tty_ioctl_helper+0xe5/0x8f0 drivers/tty/tty_ioctl.c:982
> Function: n_tty_ioctl+0x253/0x730 drivers/tty/n_tty.c:2509
> Function: tty_ioctl+0x1cfb/0x3070 drivers/tty/tty_io.c:2801
> Function: vfs_ioctl fs/ioctl.c:51 [inline]
> Function: __do_sys_ioctl fs/ioctl.c:598 [inline]
> Function: __se_sys_ioctl+0xfc/0x170 fs/ioctl.c:584
> Function: do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
> Function: do_syscall_64+0xd2/0x200 arch/x86/entry/syscall_64.c:94
> Function: entry_SYSCALL_64_after_hwframe+0x77/0x7f
> Function: 0x0

What is panicing here?

> ============OTHER_INFO============
> VarName 16560329611981062523, BlockLineNumber 32, IrLineNumber 1,
> watchpoint index 38236
> Function: set_report_info+0xbd/0x2c0 kernel/kccwf/report.c:57
> Function: setup_watchpoint kernel/kccwf/wp_checker.c:158 [inline]
> Function: watchpoints_monitor+0xe0e/0x22a0 kernel/kccwf/wp_checker.c:239
> Function: kccwf_rec_mem_access+0x275e/0x2b70
> Function: input_available_p drivers/tty/n_tty.c:1923 [inline]
> Function: n_tty_poll+0x3f5/0x16b0 drivers/tty/n_tty.c:2452
> Function: tty_poll+0x224/0x4a0 drivers/tty/tty_io.c:2199
> Function: vfs_poll include/linux/poll.h:82 [inline]
> Function: select_poll_one fs/select.c:480 [inline]
> Function: do_select+0xce7/0x13d0 fs/select.c:536
> Function: core_sys_select+0x3e8/0x5d0 fs/select.c:677
> Function: do_pselect fs/select.c:759 [inline]
> Function: __do_sys_pselect6 fs/select.c:802 [inline]
> Function: __se_sys_pselect6+0x1d8/0x240 fs/select.c:793
> Function: do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
> Function: do_syscall_64+0xd2/0x200 arch/x86/entry/syscall_64.c:94
> Function: entry_SYSCALL_64_after_hwframe+0x77/0x7f
> =================END==============
> 
> > What error was reported that this now
> > fixes?
> 
> Error: cannot pass bit-field as __auto_type initializer in C.
> The detailed error is showed below.
> 
> drivers/tty/n_tty.c:1913:6: error: cannot pass bit-field as
> __auto_type initializer in C
>     1913 |         if (data_race(ldata->icanon) && !L_EXTPROC(tty))
>          |             ^
>    include/linux/compiler.h:194:13: note: expanded from macro 'data_race'
>      194 |         auto __v = (expr);
>             \
>          |                    ^
>    1 error generated.

That is the error that the first version of your patch generated when
you actually built it, which proves you never actually tested it!

Always test your changes, ESPECIALLY for core portions of the kernel
that everyone relies on.

> > What changed to cause this to now be needed?
> 
> In my initial v1 patch, I carelessly wrapped the bit-field directly in
> the data_race() macro without locally compile-testing it.
> The data_race(expr) macro internally utilizes __auto_type (or typeof).
> And the C standard strictly prohibits passing a bit-field directly as
> an initializer for __auto_type.
> Adding !! forces the bit-field to be evaluated as a standard
> boolean/integer expression, which bypasses this compiler restriction.
> I sincerely apologize for the oversight in my initial patch and any
> extra work it may have caused.
> Thank you for your time and patience. I will submit a v3 patch if necessary.

Your v1 patch was wrong, and not tested, and I'm asking what changed to
require any of this?  What is wrong with the original code, and what
changed to cause it to suddenly be wrong as this is probably one of the
oldest parts of the kernel still in active use today.

thanks,

greg k-h