Re: Sv: RFC: fix race conditions in drivers/serial/serial.c

[kr . git]

Hello,




Best regards

On 2025-06-02 09:39, alin.jerpe...@sony.com wrote:

I noticed a review from Allan C. Assis about a spelling error. Updated patches are attached and also available in repository in uart_fixes_rfc2 branch. (Sorry about the extra work, I was even trying to check the word using web search... but I didn't spot that the search autocorrected it so my conclusion was that the spellchecker I also used is just tripping on the plural.)

Hi KR,

thank for submitting the patches

They have been uploaded to mainline and are under review
https://github.com/apache/nuttx/pull/16466


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 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

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

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

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

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.


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

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

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.

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 8-bit load instructions; interrupt routine may execute between those and set for AVR. I presume it might be also useful for Z80 but I do not have enclosed with enter_critical_section and leave_critical_section calls to calculation of the nbuffered value. This calculation is not running with Configuration with CONFIG_SERIAL_IFLOWCONTROL set was tested by building uart_putxmitchar is called. This function adds the received character to and may interfere with uart_writev. Evaluating and solving this properly

0001-drivers-serial-serial-typo-fix-in-comment.patch.gz
Description: application/gzip

0002-drivers-serial-serial-fix-race-conditions.patch.gz
Description: application/gzip