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
