Re: [PATCH] tty: fix atomicity violation in n_tty_read

[Theodore Ts'o]

On Sat, Jan 13, 2024 at 12:59:11AM +0800, Gui-Dong Han wrote:
> 
> I apologize for any confusion caused by my reference to Linux 5.17 in
> the patch description. I'm currently working on a project involving
> kernel static analysis to identify atomicity violations, and part of
> this work involves comparison with a previous study that supports up
> to Linux 5.17. Therefore, I initially ran my tool on 5.17 to filter
> potential bugs that are still unaddressed in the upstream. I want to
> clarify that the patch was developed and tested on linux-next. I
> realize now that this may have led to misunderstandings, and I will
> ensure clearer communication in future submissions.
> My experience with Linux kernel contributions is still growing, and I
> acknowledge that my recent submission might have been hasty and lacked
> thorough consideration, especially regarding the critical nature of
> n_tty_read and the potential impacts of the patch, like performance
> concerns. I will take more care in future assessments before
> submitting patches and continue to familiarize myself with the rules
> and practices of the Linux kernel community.

In general, static analysis tools need to be supplemented by an
attempt to understand what the code is trying to do.  This code is
related to the packet mode, which is related to pseudo-tty's --- *not*
the linux serial driver.

>From the man page for tty_ioctl:

       TIOCPKT
              Argument: const int *argp

              Enable (when *argp is nonzero) or disable packet mode.
              Can be applied to the master side of a pseudoterminal
              only (and will return ENOTTY otherwise).  In packet
              mode, each subsequent read(2) will return a packet that
              either contains a single nonzero control byte, or has a
              single byte containing zero ('\0') followed by data
              written on the slave side of the pseudoterminal.  If the
              first byte is not TI‐ OCPKT_DATA (0), it is an OR of one
              or more of the following bits:

              TIOCPKT_FLUSHREAD    The read queue for the terminal is flushed.
              TIOCPKT_FLUSHWRITE   The write queue for the terminal is flushed.
              TIOCPKT_STOP         Output to the terminal is stopped.
              TIOCPKT_START        Output to the terminal is restarted.
              TIOCPKT_DOSTOP       The start and stop characters are ^S/^Q.

              TIOCPKT_NOSTOP       The start and stop characters are not ^S/^Q.

              While  packet  mode is in use, the presence of control status informa‐
              tion to be read from the master side may be detected  by  a  select(2)
              for exceptional conditions or a poll(2) for the POLLPRI event.

              This  mode  is used by rlogin(1) and rlogind(8) to implement a remote-
              echoed, locally ^S/^Q flow-controlled remote login.

The n_tty_read() function is called by the userspace program on the
master side of the pty pair.  This is not, strictly speaking a hot
path; it's not on the interrupt service path of the serial driver, for
example.  So it's unliklely that "fixing" this problem is going to
result an measurable performance impact.

It's also the case that not taking the spinlock before checking the
packet mode is not necessarily going to be disastrous.  Yes, it might
mean that when the user types ^S, sshd might not stop sending
characters to the client right away, and the status report about the
status of the pty gets delayed by a millisecond or two.

So it's actually *not* a big deal.  Now, if you want to make the
argument that it would be nice if these sorts of "false positives" are
suppressed so that it's easier to find real bugs, that's one thing.
But if you're looking at proof that your static checker is actually
fixing Real Bugs (tm), this is probably not the best example.

Cheers,

					- Ted