| Message ID | 20240429141934.442154-1-f.ebner@proxmox.com |
|---|---|
| State | New |
| Headers | show |
| Series | block/copy-before-write: use uint64_t for timeout in nanoseconds | expand |
Hi Fiona, On 29/4/24 16:19, Fiona Ebner wrote: Not everybody uses an email client that shows the patch content just after the subject (your first lines wasn't making sense at first). Simply duplicating the subject helps to understand: Use uint64_t for timeout in nanoseconds ... > rather than the uint32_t for which the maximum is slightly more than 4 > seconds and larger values would overflow. The QAPI interface allows > specifying the number of seconds, so only values 0 to 4 are safe right > now, other values lead to a much lower timeout than a user expects. > > The block_copy() call where this is used already takes a uint64_t for > the timeout, so no change required there. > > Fixes: 6db7fd1ca9 ("block/copy-before-write: implement cbw-timeout option") > Reported-by: Friedrich Weber <f.weber@proxmox.com> > Signed-off-by: Fiona Ebner <f.ebner@proxmox.com> > --- > block/copy-before-write.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/block/copy-before-write.c b/block/copy-before-write.c > index 8aba27a71d..026fa9840f 100644 > --- a/block/copy-before-write.c > +++ b/block/copy-before-write.c > @@ -43,7 +43,7 @@ typedef struct BDRVCopyBeforeWriteState { > BlockCopyState *bcs; > BdrvChild *target; > OnCbwError on_cbw_error; > - uint32_t cbw_timeout_ns; > + uint64_t cbw_timeout_ns; Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Am 29.04.24 um 16:36 schrieb Philippe Mathieu-Daudé: > Hi Fiona, > > On 29/4/24 16:19, Fiona Ebner wrote: > > Not everybody uses an email client that shows the patch content just > after the subject (your first lines wasn't making sense at first). > > Simply duplicating the subject helps to understand: > > Use uint64_t for timeout in nanoseconds ... > Oh, sorry. I'll try to remember that for the future. Should I re-send as a v2? Best Regards, Fiona
On 29.04.24 17:46, Fiona Ebner wrote: > Am 29.04.24 um 16:36 schrieb Philippe Mathieu-Daudé: >> Hi Fiona, >> >> On 29/4/24 16:19, Fiona Ebner wrote: >> >> Not everybody uses an email client that shows the patch content just >> after the subject (your first lines wasn't making sense at first). >> >> Simply duplicating the subject helps to understand: >> >> Use uint64_t for timeout in nanoseconds ... >> > > Oh, sorry. I'll try to remember that for the future. Should I re-send as > a v2? > Not necessary, I can touch up the message when applying to my block branch. Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Am 29.04.2024 um 16:19 hat Fiona Ebner geschrieben: > rather than the uint32_t for which the maximum is slightly more than 4 > seconds and larger values would overflow. The QAPI interface allows > specifying the number of seconds, so only values 0 to 4 are safe right > now, other values lead to a much lower timeout than a user expects. > > The block_copy() call where this is used already takes a uint64_t for > the timeout, so no change required there. > > Fixes: 6db7fd1ca9 ("block/copy-before-write: implement cbw-timeout option") > Reported-by: Friedrich Weber <f.weber@proxmox.com> > Signed-off-by: Fiona Ebner <f.ebner@proxmox.com> Thanks, applied to the block branch. But I don't think our job is done yet with this. Increasing the limit is good and useful, but even if it's now unlikely to hit with sane values, we should still catch integer overflows in cbw_open() and return an error on too big values instead of silently wrapping around. Kevin
Am 28.05.24 um 18:06 schrieb Kevin Wolf: > Am 29.04.2024 um 16:19 hat Fiona Ebner geschrieben: >> rather than the uint32_t for which the maximum is slightly more than 4 >> seconds and larger values would overflow. The QAPI interface allows >> specifying the number of seconds, so only values 0 to 4 are safe right >> now, other values lead to a much lower timeout than a user expects. >> >> The block_copy() call where this is used already takes a uint64_t for >> the timeout, so no change required there. >> >> Fixes: 6db7fd1ca9 ("block/copy-before-write: implement cbw-timeout option") >> Reported-by: Friedrich Weber <f.weber@proxmox.com> >> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com> > > Thanks, applied to the block branch. > > But I don't think our job is done yet with this. Increasing the limit is > good and useful, but even if it's now unlikely to hit with sane values, > we should still catch integer overflows in cbw_open() and return an > error on too big values instead of silently wrapping around. NANOSECONDS_PER_SECOND is 10^9 and the QAPI type for cbw-timeout is uint32_t, so even with the maximum allowed value, there is no overflow. Should I still add such a check? Best Regards, Fiona
Am 03.06.2024 um 16:45 hat Fiona Ebner geschrieben: > Am 28.05.24 um 18:06 schrieb Kevin Wolf: > > Am 29.04.2024 um 16:19 hat Fiona Ebner geschrieben: > >> rather than the uint32_t for which the maximum is slightly more than 4 > >> seconds and larger values would overflow. The QAPI interface allows > >> specifying the number of seconds, so only values 0 to 4 are safe right > >> now, other values lead to a much lower timeout than a user expects. > >> > >> The block_copy() call where this is used already takes a uint64_t for > >> the timeout, so no change required there. > >> > >> Fixes: 6db7fd1ca9 ("block/copy-before-write: implement cbw-timeout option") > >> Reported-by: Friedrich Weber <f.weber@proxmox.com> > >> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com> > > > > Thanks, applied to the block branch. > > > > But I don't think our job is done yet with this. Increasing the limit is > > good and useful, but even if it's now unlikely to hit with sane values, > > we should still catch integer overflows in cbw_open() and return an > > error on too big values instead of silently wrapping around. > > NANOSECONDS_PER_SECOND is 10^9 and the QAPI type for cbw-timeout is > uint32_t, so even with the maximum allowed value, there is no overflow. > Should I still add such a check? You're right, I missed that cbw_timeout is uint32_t. So uint64_t will be always be enough to hold the result, and the calculation is also done in 64 bits because NANOSECONDS_PER_SECOND is long long. Then we don't need a check. Kevin
diff --git a/block/copy-before-write.c b/block/copy-before-write.c index 8aba27a71d..026fa9840f 100644 --- a/block/copy-before-write.c +++ b/block/copy-before-write.c @@ -43,7 +43,7 @@ typedef struct BDRVCopyBeforeWriteState { BlockCopyState *bcs; BdrvChild *target; OnCbwError on_cbw_error; - uint32_t cbw_timeout_ns; + uint64_t cbw_timeout_ns; /* * @lock: protects access to @access_bitmap, @done_bitmap and
rather than the uint32_t for which the maximum is slightly more than 4 seconds and larger values would overflow. The QAPI interface allows specifying the number of seconds, so only values 0 to 4 are safe right now, other values lead to a much lower timeout than a user expects. The block_copy() call where this is used already takes a uint64_t for the timeout, so no change required there. Fixes: 6db7fd1ca9 ("block/copy-before-write: implement cbw-timeout option") Reported-by: Friedrich Weber <f.weber@proxmox.com> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com> --- block/copy-before-write.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)