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

[kr . git]

Hello,






Testing was done in the same way as the original version of the series.

On 2025-06-05 07:44, kr....@kerogit.eu wrote:

reworked version of the series is in branch uart_fixes_rfc3 and also attached. This version incorporates xiaoxiang781216's suggestion on GitHub to simply make the buffer size stored in uint8_t variable if the architecture cannot handle 16-bit loads atomically. Fix for TOCTOU in watermark calculation was split into a separate patch since it addresses a different issue.

Hello,







Nevertheless, if reads are made atomic, neither of the above is needed.

Thanks for the feedback, will try to do the rework in next few days.

On 2025-06-02 18:58, kr....@kerogit.eu wrote:

based on the feedback in GitHub review, I will redo the patch differently and reduce the size of the circular buffers so the variables fit into uint8_t. To respond to xiaoxiang781216's comment about using atomic_xxx API instead the critical section - I was considering that as well but in the end, I concluded that the critical section is highly unlikely to be compiled in on any architecture that supports SMP. It does set a bad example though, that did not occur to me previously. The atomic_xxx API I decided not to use because when I tried it, it produced buggy code. I will post a patch/question regarding that in a separate thread.

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





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

to be protected this way - some are already in existing critical
sections and some happen with the UART-specific interrupt disabled.


into a local variable. Subsequent direct uses of the unprotected


There is also a related change that only applies when

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.


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

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.


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,

"Only one user can access dev->xmit.head at a time" and takes the

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

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 While going through the code in drivers/serial/serial.c, I noticed this 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 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 If the configuration option is not set, the code simply loads the value volatile variable are replaced with the local variables for both cases. CONFIG_SERIAL_IFLOWCONTROL_WATERMARKS is set. Aside from the protection calculation of the nbuffered value. This calculation is not running with Impact: this change should not impact architectures that do not benefit 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. Configuration with CONFIG_SERIAL_IFLOWCONTROL set was tested by building I also ran CI test: 1044 passed, 10 skipped, 14 deselected, 3 warnings uart_putxmitchar is called. This function adds the received character to uart_putxmitchar is also called from uart_writev; this function states 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 machine-readable form.) The relevant branch is called uart_fixes_rfc1. If the patches are acceptable, I would like to ask someone with GitHub

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

0002-drivers-serial-serial-prevent-race-conditions-on-8-b.patch.gz
Description: application/gzip

0003-drivers-serial-serial-fix-race-condition-in-flow-con.patch.gz
Description: application/gzip