Created attachment 232981 [details] Test case demonstrating the bug, and a solution The Xstosbt functions all contain an integer overflow for input values of 2 seconds or greater. The cause is a comparison against the SBT_1S constant, which was probably intended precisely to prevent such an overflow, but it's the wrong constant. Instead of "one second in sbintime units", it should be "one second in the input type's units". A visible symptom of this bug is the ZFS's write throttle. On a very heavily loaded system ZFS will sometimes decide to delay a thread by over 2 seconds. dmu_tx_delay will trigger the overflow in ustosbt, resulting in a delay of over 4000 seconds instead. Very frustrating! The bug was present in FreeBSD 12, too. But there ZFS's logic was different, so the symptom was that any delay of > 2 seconds would turn into a delay of < 2 seconds. Not as noticeable. The bug was introduced in revision 68f57679d66016ba4625f5bf8a99447bbae84fda (SVN r340664). Before that there was a different overflow bug.
Yea, these routines weren't intended for values > 1s originally. My fix was to try cope with that. However, you are correct, it's somewhat wrong. diff --git a/sys/sys/time.h b/sys/sys/time.h index 866a9e788cd0..f3a3bc99a0f2 100644 --- a/sys/sys/time.h +++ b/sys/sys/time.h @@ -207,7 +207,7 @@ nstosbt(int64_t _ns) #ifdef KASSERT KASSERT(_ns >= 0, ("Negative values illegal for nstosbt: %jd", _ns)); #endif - if (_ns >= SBT_1S) { + if (_ns >= 1000000000) { sb = (_ns / 1000000000) * SBT_1S; _ns = _ns % 1000000000; } @@ -231,7 +231,7 @@ ustosbt(int64_t _us) #ifdef KASSERT KASSERT(_us >= 0, ("Negative values illegal for ustosbt: %jd", _us)); #endif - if (_us >= SBT_1S) { + if (_us >= 1000000) { sb = (_us / 1000000) * SBT_1S; _us = _us % 1000000; } @@ -255,7 +255,7 @@ mstosbt(int64_t _ms) #ifdef KASSERT KASSERT(_ms >= 0, ("Negative values illegal for mstosbt: %jd", _ms)); #endif - if (_ms >= SBT_1S) { + if (_ms >= 1000) { sb = (_ms / 1000) * SBT_1S; _ms = _ms % 1000; } Is I think the right fix.
Yep, I think that's the correct fix too. I'm working on an ATF test case, and I'll also look at the inverse functions, sbttous and friends.
I think that the overflow happens in the range 2.147483648ns to 4.294967295ns range for the nstosbt. It will occur in the 2147.483648us 4294.967296us range for ustosbt. etc. The inverse functions are mostly fine. sbtons shifts the value right before multiplying, so won't overflow. sbttous and sbtoms will likely fail for values > 2,147.383648us and 2,147,483.648ms respectively. They could get the same treatment as sbttons, but intervals on the order of 35 minutes are quite rare in the kernel, as are times on the order of a year and a half. Fixing these is a lower priority, but if you have the ATF test case, we can ensure they stay fixed :) I'm happy to commit the fixes since I made the original botch.
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=4c30b9ecd47a2d92565731082a6a4f2bd1e6e051 commit 4c30b9ecd47a2d92565731082a6a4f2bd1e6e051 Author: Warner Losh <imp@FreeBSD.org> AuthorDate: 2022-04-06 03:35:27 +0000 Commit: Warner Losh <imp@FreeBSD.org> CommitDate: 2022-04-06 03:36:59 +0000 fix integer overflow bugs in *stosbt 68f57679d660 Fixed another class of integer overflows, but introduced a boundary condition for 2-4s in ns conversion, 2-~4000s in us conversions and 2-~4,000,000s in ms conversions. This was because we bogusly used SBT_1S for the notion of 1 second, instead of the appropriate power of 10. To fix, just use the appropriate power of 10, which avoids these overflows. This caused some sleeps in ZFS to be on the order of an hour. MFC: 1 day PR: 263073 Sponsored by: Netflix Reviewed by: asomers Differential Revision: https://reviews.freebsd.org/D34790 sys/sys/time.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=c43786cc37641cef02171a3c5be5a588d850e6ab commit c43786cc37641cef02171a3c5be5a588d850e6ab Author: Warner Losh <imp@FreeBSD.org> AuthorDate: 2022-04-06 03:35:27 +0000 Commit: Warner Losh <imp@FreeBSD.org> CommitDate: 2022-04-06 15:58:30 +0000 fix integer overflow bugs in *stosbt 68f57679d660 Fixed another class of integer overflows, but introduced a boundary condition for 2-4s in ns conversion, 2-~4000s in us conversions and 2-~4,000,000s in ms conversions. This was because we bogusly used SBT_1S for the notion of 1 second, instead of the appropriate power of 10. To fix, just use the appropriate power of 10, which avoids these overflows. This caused some sleeps in ZFS to be on the order of an hour. MFC: 1 day PR: 263073 Sponsored by: Netflix Reviewed by: asomers Differential Revision: https://reviews.freebsd.org/D34790 (cherry picked from commit 4c30b9ecd47a2d92565731082a6a4f2bd1e6e051) sys/sys/time.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
A commit in branch releng/13.1 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=11931e7aa83515e5d33f04cf11e8a3913df61d60 commit 11931e7aa83515e5d33f04cf11e8a3913df61d60 Author: Warner Losh <imp@FreeBSD.org> AuthorDate: 2022-04-06 03:35:27 +0000 Commit: Warner Losh <imp@FreeBSD.org> CommitDate: 2022-04-06 17:46:08 +0000 fix integer overflow bugs in *stosbt 68f57679d660 Fixed another class of integer overflows, but introduced a boundary condition for 2-4s in ns conversion, 2-~4000s in us conversions and 2-~4,000,000s in ms conversions. This was because we bogusly used SBT_1S for the notion of 1 second, instead of the appropriate power of 10. To fix, just use the appropriate power of 10, which avoids these overflows. This caused some sleeps in ZFS to be on the order of an hour. Approved by: re@ (gjb) MFC: 1 day PR: 263073 Sponsored by: Netflix Reviewed by: asomers Differential Revision: https://reviews.freebsd.org/D34790 (cherry picked from commit 4c30b9ecd47a2d92565731082a6a4f2bd1e6e051) (cherry picked from commit c43786cc37641cef02171a3c5be5a588d850e6ab) sys/sys/time.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=10f44229dcd93672583ad6b6e1193a9bc9e4f7c7 commit 10f44229dcd93672583ad6b6e1193a9bc9e4f7c7 Author: Alan Somers <asomers@FreeBSD.org> AuthorDate: 2022-04-06 20:03:11 +0000 Commit: Alan Somers <asomers@FreeBSD.org> CommitDate: 2022-05-09 22:38:59 +0000 Fix overflow errors in sbttous and sbttoms Both of these functions would overflow for very large inputs. Add tests for them. Also, add tests for the inverse functions, *stosbt, whose overflow errors were fixed by 4c30b9ecd47. PR: 263073 MFC after: 1 week Sponsored by: Axcient Reviewed by: imp Differential Revision: https://reviews.freebsd.org/D34809 sys/sys/time.h | 12 ++- tests/sys/sys/Makefile | 3 +- tests/sys/sys/time_test.c (new) | 224 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 235 insertions(+), 4 deletions(-)
A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=8f8e64da6be55d3dfebf95f52162eeec90a55143 commit 8f8e64da6be55d3dfebf95f52162eeec90a55143 Author: Alan Somers <asomers@FreeBSD.org> AuthorDate: 2022-04-06 20:03:11 +0000 Commit: Alan Somers <asomers@FreeBSD.org> CommitDate: 2022-05-23 19:11:27 +0000 Fix overflow errors in sbttous and sbttoms Both of these functions would overflow for very large inputs. Add tests for them. Also, add tests for the inverse functions, *stosbt, whose overflow errors were fixed by 4c30b9ecd47. PR: 263073 Sponsored by: Axcient Reviewed by: imp Differential Revision: https://reviews.freebsd.org/D34809 (cherry picked from commit 10f44229dcd93672583ad6b6e1193a9bc9e4f7c7) sys/sys/time.h | 12 ++- tests/sys/sys/Makefile | 3 +- tests/sys/sys/time_test.c (new) | 224 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 235 insertions(+), 4 deletions(-)
A commit in branch stable/12 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=e04d75193865985f64e450931b51c6c2042f913d commit e04d75193865985f64e450931b51c6c2042f913d Author: Alan Somers <asomers@FreeBSD.org> AuthorDate: 2022-04-06 20:03:11 +0000 Commit: Alan Somers <asomers@FreeBSD.org> CommitDate: 2022-08-20 02:24:58 +0000 fix integer overflow bugs in *stosbt 68f57679d660 Fixed another class of integer overflows, but introduced a boundary condition for 2-4s in ns conversion, 2-~4000s in us conversions and 2-~4,000,000s in ms conversions. This was because we bogusly used SBT_1S for the notion of 1 second, instead of the appropriate power of 10. To fix, just use the appropriate power of 10, which avoids these overflows. This caused some sleeps in ZFS to be on the order of an hour. MFC: 1 day PR: 263073 Sponsored by: Netflix Reviewed by: asomers Differential Revision: https://reviews.freebsd.org/D34790 Fix overflow errors in sbttous and sbttoms Both of these functions would overflow for very large inputs. Add tests for them. Also, add tests for the inverse functions, *stosbt, whose overflow errors were fixed by 4c30b9ecd47. PR: 263073 Sponsored by: Axcient Reviewed by: imp Differential Revision: https://reviews.freebsd.org/D34809 (cherry picked from commit 4c30b9ecd47a2d92565731082a6a4f2bd1e6e051) (cherry picked from commit 10f44229dcd93672583ad6b6e1193a9bc9e4f7c7) sys/sys/time.h | 18 ++-- tests/sys/sys/Makefile | 3 +- tests/sys/sys/time_test.c (new) | 224 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 238 insertions(+), 7 deletions(-)