Hi KR, the patches have been uploaded
Best regards Alin On Thu, Jun 19, 2025 at 1:14 AM <kr....@kerogit.eu> wrote: > Hello, > > finally got to it, 4th revision of the patches is in branch named > uart_fixes_rfc4 and also attached. This revision incorporates > suggestions from GitHub review. > > 1. The order of patches in the series is reversed (not counting the typo > fix patch.) The less intrusive one is now placed before the other. This > way, the smaller fix is not affected by changes that follow, namely by > using the new data type. > > 2. Instead of ifdefs in various places, the patch introduces type > sbuf_size_t in include/nuttx/serial/serial.h . Buffer size, head and > tail variables are changed to this type. > > 3. ARCH_LD_16BIT_NOT_ATOMIC configuration option is renamed to > ARCH_LDST_16BIT_NOT_ATOMIC to denote that store operations are not > atomic as well for the architecture. > > 4. Default serial buffer size is changed from 256 to 252. Additionally, > help of configuration options for USARTn_(RX|TX)BUFSIZE now mentions > that if the architecture is unable to load/store 16-bit values > atomically, the size of buffer is stored in uint8_t. > > 5. New configuration option ARCH_HAVE_SERIAL_8BIT_BUFSIZE is added. This > configuration option triggers change of sbuf_size_t from default int16_t > to architecture-specific uint8_t. It is, however, not selected from AVR > serial driver Kconfig option as the review suggests. Instead, it is > selected by ARCH_LDST_16BIT_NOT_ATOMIC. The reasoning for doing it this > way is that AVR is (likely) not the only architecture suffering from > this race condition - any 8-bit architecture is, unless it does 16-bit > loads/stores atomically. As far as I was able to find, this applies to > Z80 as well. > > Also, serial buffer is likely not the only area that introduces such a > race. If more such cases are found, selecting all configuration options > that enable measures that prevent these race conditions from all > affected architectures would lead to M x N configuration options and all > architectures would need to track new options being added. Selecting all > of them from single point (ie. from ARCH_LDST_16BIT_NOT_ATOMIC) makes > the configuration easier, the architecture only needs to declare what is > is (in)capable of and all related options will be enabled automatically. > > As for the comment from jlaitine - I also find it strange that the > original index is a signed value and I found no reason for it being that > way. And yes, it is left untouched to prevent any changes in other > architectures. > > If there is still something left to improve, let me know. Thanks. > > On 2025-06-11 13:42, alin.jerpe...@sony.com wrote: > > Hi KR, > > > > I added your comments > > > > Best regards > > Alin > > ________________________________ > > Från: kr....@kerogit.eu <kr....@kerogit.eu> > > Skickat: den 11 juni 2025 07:54 > > Till: dev@nuttx.apache.org <dev@nuttx.apache.org> > > Ämne: Re: Sv: Sv: RFC: fix race conditions in drivers/serial/serial.c > > > > Hello, thanks. Here is a response to xiaoxiang781216's review: > could > > we extract a common typedef to avoid #ifdef/#else/#endif spread > the > > code base? Can do - sbuf_size_t seems unused throughout the whole tree > > so I'll add that to serial. h. > > > > > > Hello, > > > > thanks. Here is a response to xiaoxiang781216's review: > > > >> could we extract a common typedef to avoid #ifdef/#else/#endif spread > >> the code base? > > > > Can do - sbuf_size_t seems unused throughout the whole tree so I'll add > > that to serial.h. > > > >> can we change to ARCH_HAVE_8BIT_BUFSIZE and put into serial/Kconfig? > > > > I am not sure if that means to a) change the name from > > ARCH_LD_16BIT_NOT_ATOMIC to ARCH_HAVE_8BIT_BUFSIZE or b) add > > ARCH_HAVE_8BIT_BUFSIZE and have it selected by > > ARCH_LD_16BIT_NOT_ATOMIC? > > Neither seems fully correct though. > > > > The first variant reduces the problem to buffers only but that is too > > narrow. Essentially, any other value that is simultaneously written in > > interrupt code and read in non-interrupt code needs the be treated in > > the same manner as buffer head/tail here. The current name reflects > > that. Also, adding ARCH_LD_32BIT_NOT_ATOMIC may be needed as well > > (possibly not only for AVR but for 16 bit microcontrollers too - if I > > understand it correctly, that applies to Freescale M9S12.) > > > > The second variant implies that all of the arch has 8 bit buffer size > > for all buffers everywhere which is not true and may not be desirable. > > > > How about adding ARCH_HAVE_8BIT_SERIAL_BUFSIZE to serial/Kconfig and > > have it selected by ARCH_LD_16BIT_NOT_ATOMIC? > > > >> could we directly change the default value to 255 > > > > Can be done but that would affect other architectures as well which is > > something I wanted to avoid. Also, using 255 feels dangerous when the > > underlying type is uint8_t. Might lead to bugs where expression like (x > > + 1 > size) never evaluates to true. Not that I see any such code in > > current sources, but if the default value should change for all > > architectures, I would rather use 252 out of abundance of caution. > > > >> move to help of ARCH_LD_16BIT_NOT_ATOMIC > > > > I think it would be better to put something more generic in there. > > Additionally, I'll copy the comment about the buffer size to help for > > USARTn_RX/TXBUFSIZE. > > > >> why not merge to previous patch > > > > Because it fixes different bug (on all architectures.) I will reorder > > the patches to make that more obvious and to have the less intrusive > > one > > be the first in case it turns out that the second one needs to be > > reverted. > > > > On 2025-06-09 15:10, alin.jerpe...@sony.com wrote: > >> Hi KR, > >> > >> the patches have been updated > >> > https://urldefense.com/v3/__https://github.com/apache/nuttx/pull/16466__;!!O7_YSHcmd9jp3hj_4dEAcyQ!1y3RB5EUXoBosi8vAEcjSyRXMCiRnRtrGSfJEO84MiLP3anDf6k6445X2RlUvrB55u8lzti4-TaTVNIzppE$ > >> > >> Best regads > >> Alin > >> > >> ________________________________ > >> Från: kr....@kerogit.eu <kr....@kerogit.eu> > >> Skickat: den 9 juni 2025 12:12 > >> Till: dev@nuttx.apache.org <dev@nuttx.apache.org> > >> Ämne: Re: Sv: RFC: fix race conditions in drivers/serial/serial.c > >> > >> Hello, 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 > >> > >> > >> Hello, > >> > >> 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. > >> > >> Testing was done in the same way as the original version of the > >> series. > >> > >> On 2025-06-05 07:44, kr....@kerogit.eu wrote: > >>> Hello, > >>> > >>> 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. > >>> > >>> 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: > >>>> Hello, > >>>> > >>>> 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.) > >>>> > >>>> Best regards > >>>> > >>>> On 2025-06-02 09:39, alin.jerpe...@sony.com wrote: > >>>>> 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. >
