diff mbox series

block/copy-before-write: use uint64_t for timeout in nanoseconds

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

Commit Message

Fiona Ebner April 29, 2024, 2:19 p.m. UTC
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(-)

Comments

Philippe Mathieu-Daudé April 29, 2024, 2:36 p.m. UTC | #1
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>
Fiona Ebner April 29, 2024, 2:46 p.m. UTC | #2
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
Vladimir Sementsov-Ogievskiy April 29, 2024, 2:52 p.m. UTC | #3
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>
Kevin Wolf May 28, 2024, 4:06 p.m. UTC | #4
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
Fiona Ebner June 3, 2024, 2:45 p.m. UTC | #5
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
Kevin Wolf June 3, 2024, 4:09 p.m. UTC | #6
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 mbox series

Patch

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