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

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

[Fiona Ebner]

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;
 
     /*
      * @lock: protects access to @access_bitmap, @done_bitmap and
-- 
2.39.2

Re: [PATCH] block/copy-before-write: use uint64_t for timeout in nanoseconds

[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 ...

> 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>

Re: [PATCH] block/copy-before-write: use uint64_t for timeout in nanoseconds

[Fiona Ebner]

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

Re: [PATCH] block/copy-before-write: use uint64_t for timeout in nanoseconds

[Vladimir Sementsov-Ogievskiy]

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>

-- 
Best regards,
Vladimir

Re: [PATCH] block/copy-before-write: use uint64_t for timeout in nanoseconds

[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.

Kevin

Re: [PATCH] block/copy-before-write: use uint64_t for timeout in nanoseconds

[Fiona Ebner]

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

Re: [PATCH] block/copy-before-write: use uint64_t for timeout in nanoseconds

[Kevin Wolf]

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