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