Hi KR,
thank for submitting the patches
They have been uploaded to mainline and are under review
https://urldefense.com/v3/__https://github.com/apache/nuttx/pull/16466__;!!O7_YSHcmd9jp3hj_4dEAcyQ!yLV1nobwBFiEr5gCY_xNYvnCqhLJeSSyqnb6uJgKkEsQ6_1NJtMiI_zeeQRnjWuYSysKvDDj5FMrky7Yhq8$
Best regards
Alin
________________________________
Från: kr....@kerogit.eu <kr....@kerogit.eu>
Skickat: den 1 juni 2025 22:42
Till: dev@nuttx.apache.org <dev@nuttx.apache.org>
Ämne: RFC: fix race conditions in drivers/serial/serial.c
While going through the code in drivers/serial/serial. c, I noticed
this comment: The head and tail pointers are 16-bit values. The only
time that the following could be unsafe is if the CPU made two
non-atomic 8-bit accesses to obtain the 16-bit
While going through the code in drivers/serial/serial.c, I noticed
this
comment:
The head and tail pointers are 16-bit values. The only time that
the following could be unsafe is if the CPU made two non-atomic
8-bit accesses to obtain the 16-bit head index.
This is what happens for (at least) AVR architecture. These are 8bit
microcontrollers and as such, they will fetch the 16-bit value in
two
8-bit load instructions; interrupt routine may execute between those
and
change the value being read. This will result in corrupted read (one
byte of the value will be from pre-interrupt state and the other
from
post-interrupt state.)
This patch introduces CONFIG_ARCH_LD_16BIT_NOT_ATOMIC configuration
option, which is automatically selected for architectures known to
be
unable to load 16bit values in one instruction. (It is currently
only
set for AVR. I presume it might be also useful for Z80 but I do not
have
any experience with that architecture so I did no changes there.)
When
this configuration option is set, reads of recv.head and xmit.tail
are
enclosed with enter_critical_section and leave_critical_section
calls
to
prevent interrupt handlers from running, if needed. Not all reads
need
to be protected this way - some are already in existing critical
sections and some happen with the UART-specific interrupt disabled.
If the configuration option is not set, the code simply loads the
value
into a local variable. Subsequent direct uses of the unprotected
volatile variable are replaced with the local variables for both
cases.
There is also a related change that only applies when
CONFIG_SERIAL_IFLOWCONTROL_WATERMARKS is set. Aside from the
protection
selected by CONFIG_ARCH_LD_16BIT_NOT_ATOMIC, this patch also fixes
calculation of the nbuffered value. This calculation is not running
with
interrupts disabled and value of rxbuf->head can change between the
condition and actual computation. Even if the load itself is atomic,
this leads to TOCTOU error.
Impact: this change should not impact architectures that do not
benefit
from this change at all unless CONFIG_SERIAL_IFLOWCONTROL is set. If
CONFIG_SERIAL_IFLOWCONTROL is set, the only change that remains in
effect is the fix of the TOCTOU error.
Testing: Patch was tested with rv-virt:nsh - disassembly of
functions
from drivers/serial/serial.c was compared and did not change after
the
patch was applied - with one exception. The exception was an address
of
a global variable, I assume it was caused by change of g_version
length
(which notes that the tree is dirty) and is therefore
inconsequential.
CONFIG_SERIAL_IFLOWCONTROL was not set in this test.
Configuration with CONFIG_SERIAL_IFLOWCONTROL set was tested by
building
it for AVR DA/DB. Configuration with CONFIG_SERIAL_IFLOWCONTROL
unset
was tested by custom echo application running on AVR128DA28 for a
few
hours.
I also ran CI test: 1044 passed, 10 skipped, 14 deselected, 3
warnings
in 2463.23s (0:41:03)
On a related note, it seems to me that there may be a bug in echo
handling in uart_readv. If dev->tc_lflag & ECHO is true, then
uart_putxmitchar is called. This function adds the received
character
to
transmit buffer and increments xmit.head value. However,
uart_putxmitchar is also called from uart_writev; this function
states
"Only one user can access dev->xmit.head at a time" and takes the
dev->xmit.lock. As far as I can see, uart_readv does not take the
lock
and may interfere with uart_writev. Evaluating and solving this
properly
is currently above my skillset though. (If this is even considered
serious enough to warrant a fix.)
The patch series also fixes a typo in drivers/serial/serial.c
Both patches are attached to this message and also available in a
git
repository nuttx.git at git.kerogit.eu accessible through HTTP/S.
(Trying to prevent bot traffic by not posting the URL in
machine-readable form.) The relevant branch is called
uart_fixes_rfc1.
If the patches are acceptable, I would like to ask someone with
GitHub
account to open a PR (I don't have a GH account.) Any comments or
suggestions are welcome.
