[v1] tests/qtest/fuzz/virtio_net_fuzz.c: fix virtio_net_fuzz_multi

[PATCH] tests/qtest/fuzz/virtio_net_fuzz.c: fix virtio_net_fuzz_multi

Posted by Dmitry Frolov 6 months ago

If QTestState was already CLOSED due to error, calling qtest_clock_step()
afterwards makes no sense and only raises false-crash with message:
"assertion timer != NULL failed".

Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
---
 tests/qtest/fuzz/virtio_net_fuzz.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/qtest/fuzz/virtio_net_fuzz.c b/tests/qtest/fuzz/virtio_net_fuzz.c
index e239875e3b..2f57a8ddd8 100644
--- a/tests/qtest/fuzz/virtio_net_fuzz.c
+++ b/tests/qtest/fuzz/virtio_net_fuzz.c
@@ -81,6 +81,9 @@ static void virtio_net_fuzz_multi(QTestState *s,
         /* Run the main loop */
         qtest_clock_step(s, 100);
         flush_events(s);
+        if (!qtest_probe_child(s)) {
+            return;
+        }
 
         /* Wait on used descriptors */
         if (check_used && !vqa.rx) {
-- 
2.43.0

Re: [PATCH] tests/qtest/fuzz/virtio_net_fuzz.c: fix virtio_net_fuzz_multi

Posted by Michael Tokarev 2 months, 2 weeks ago

23.05.2024 13:28, Dmitry Frolov wrote:
> If QTestState was already CLOSED due to error, calling qtest_clock_step()
> afterwards makes no sense and only raises false-crash with message:
> "assertion timer != NULL failed".

So, can we have any Reviewed-by tags here? :)

Thanks,

/mjt

Re: [PATCH] tests/qtest/fuzz/virtio_net_fuzz.c: fix virtio_net_fuzz_multi

Posted by Alexander Bulekov 5 months, 2 weeks ago

This fixes the almost-immediate timeout issue for me on the
virtio_net_fuzz target, but I'm not sure why this works or if it is
fixing the right problem:

qtest_probe_child is designed to run from a libqtest process which
uses waitpid on the PID of the child (qemu) process (stored in
QTestState->qemu_pid) . With qemu-fuzz we do not have a separate
libqtest and qemu process:

(gdb) p s->qemu_pid
$1 = 0

So we are calling waitpid with pid = 0. From the man-page:
"0 meaning wait for any child process whose process group ID is equal to
that of the calling process at the time of the call to waitpid()."

And we are calling it with WNOHANG. So I would expect that this almost
always returns 0 unless some adjacent thread has changed state
(libfuzzer uses extra threads to manage timeouts).

I'm happy that the fuzzer works again, and am happy to leave a review,
but I would like to first understand what the behavior of
qtest_probe_child here is, since it isn't really designed to work with
the fuzzer.

On 240523 1328, Dmitry Frolov wrote:
> If QTestState was already CLOSED due to error, calling qtest_clock_step()
> afterwards makes no sense and only raises false-crash with message:
> "assertion timer != NULL failed".
> 
> Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
> ---
>  tests/qtest/fuzz/virtio_net_fuzz.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tests/qtest/fuzz/virtio_net_fuzz.c b/tests/qtest/fuzz/virtio_net_fuzz.c
> index e239875e3b..2f57a8ddd8 100644
> --- a/tests/qtest/fuzz/virtio_net_fuzz.c
> +++ b/tests/qtest/fuzz/virtio_net_fuzz.c
> @@ -81,6 +81,9 @@ static void virtio_net_fuzz_multi(QTestState *s,
>          /* Run the main loop */
>          qtest_clock_step(s, 100);
>          flush_events(s);
> +        if (!qtest_probe_child(s)) {
> +            return;
> +        }
>  
>          /* Wait on used descriptors */
>          if (check_used && !vqa.rx) {
> -- 
> 2.43.0
>

Re: *** MAY_BE_SPAM !!! *** Re: [PATCH] tests/qtest/fuzz/virtio_net_fuzz.c: fix virtio_net_fuzz_multi

Posted by Дмитрий Фролов 5 months, 1 week ago

On 13.06.2024 18:54, Alexander Bulekov wrote:
> This fixes the almost-immediate timeout issue for me on the
> virtio_net_fuzz target, but I'm not sure why this works or if it is
> fixing the right problem:
>
> qtest_probe_child is designed to run from a libqtest process which
> uses waitpid on the PID of the child (qemu) process (stored in
> QTestState->qemu_pid) . With qemu-fuzz we do not have a separate
> libqtest and qemu process:
>
> (gdb) p s->qemu_pid
> $1 = 0
>
> So we are calling waitpid with pid = 0. From the man-page:
> "0 meaning wait for any child process whose process group ID is equal to
> that of the calling process at the time of the call to waitpid()."
>
> And we are calling it with WNOHANG. So I would expect that this almost
> always returns 0 unless some adjacent thread has changed state
> (libfuzzer uses extra threads to manage timeouts).
According to 
https://www.redhat.com/en/blog/hardening-qemu-through-continuous-security-testing#:~:text=each%20input%20is%20executed%20within%20a%20forked%20child%20process
"each input is executed within a forked child process".
According to crash reports, an error occurs first (which may be different),
followed by the crash with message "assertion timer != NULL failed". To 
my opinion, waiting for an answer from dead children is the reason of 
crashes.
> I'm happy that the fuzzer works again, and am happy to leave a review,
> but I would like to first understand what the behavior of
> qtest_probe_child here is, since it isn't really designed to work with
> the fuzzer.
>
> On 240523 1328, Dmitry Frolov wrote:
>> If QTestState was already CLOSED due to error, calling qtest_clock_step()
>> afterwards makes no sense and only raises false-crash with message:
>> "assertion timer != NULL failed".
>>
>> Signed-off-by: Dmitry Frolov<frolov@swemel.ru>
>> ---
>>   tests/qtest/fuzz/virtio_net_fuzz.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/tests/qtest/fuzz/virtio_net_fuzz.c b/tests/qtest/fuzz/virtio_net_fuzz.c
>> index e239875e3b..2f57a8ddd8 100644
>> --- a/tests/qtest/fuzz/virtio_net_fuzz.c
>> +++ b/tests/qtest/fuzz/virtio_net_fuzz.c
>> @@ -81,6 +81,9 @@ static void virtio_net_fuzz_multi(QTestState *s,
>>           /* Run the main loop */
>>           qtest_clock_step(s, 100);
>>           flush_events(s);
>> +        if (!qtest_probe_child(s)) {
>> +            return;
>> +        }
>>   
>>           /* Wait on used descriptors */
>>           if (check_used && !vqa.rx) {
>> -- 
>> 2.43.0
>>

Re: [PATCH] tests/qtest/fuzz/virtio_net_fuzz.c: fix virtio_net_fuzz_multi

Posted by Thomas Huth 5 months, 2 weeks ago

On 23/05/2024 12.28, Dmitry Frolov wrote:
> If QTestState was already CLOSED due to error, calling qtest_clock_step()
> afterwards makes no sense and only raises false-crash with message:
> "assertion timer != NULL failed".
> 
> Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
> ---
>   tests/qtest/fuzz/virtio_net_fuzz.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/tests/qtest/fuzz/virtio_net_fuzz.c b/tests/qtest/fuzz/virtio_net_fuzz.c
> index e239875e3b..2f57a8ddd8 100644
> --- a/tests/qtest/fuzz/virtio_net_fuzz.c
> +++ b/tests/qtest/fuzz/virtio_net_fuzz.c
> @@ -81,6 +81,9 @@ static void virtio_net_fuzz_multi(QTestState *s,
>           /* Run the main loop */
>           qtest_clock_step(s, 100);
>           flush_events(s);
> +        if (!qtest_probe_child(s)) {
> +            return;
> +        }

According to your patch description, it rather sounds like the check should 
be done before the qtest_clock_step() ? ... or where does the QTestState get 
closed? During flush_events() ?

  Thomas

Re: [PATCH] tests/qtest/fuzz/virtio_net_fuzz.c: fix virtio_net_fuzz_multi

Posted by Дмитрий Фролов 5 months, 2 weeks ago


On 13.06.2024 13:08, Thomas Huth wrote:
> On 23/05/2024 12.28, Dmitry Frolov wrote:
>> If QTestState was already CLOSED due to error, calling 
>> qtest_clock_step()
>> afterwards makes no sense and only raises false-crash with message:
>> "assertion timer != NULL failed".
>>
>> Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
>> ---
>>   tests/qtest/fuzz/virtio_net_fuzz.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/tests/qtest/fuzz/virtio_net_fuzz.c 
>> b/tests/qtest/fuzz/virtio_net_fuzz.c
>> index e239875e3b..2f57a8ddd8 100644
>> --- a/tests/qtest/fuzz/virtio_net_fuzz.c
>> +++ b/tests/qtest/fuzz/virtio_net_fuzz.c
>> @@ -81,6 +81,9 @@ static void virtio_net_fuzz_multi(QTestState *s,
>>           /* Run the main loop */
>>           qtest_clock_step(s, 100);
>>           flush_events(s);
>> +        if (!qtest_probe_child(s)) {
>> +            return;
>> +        }
>
> According to your patch description, it rather sounds like the check 
> should be done before the qtest_clock_step() ? ... or where does the 
> QTestState get closed? During flush_events() ?
To my understanding, the main loop is executed during flush_events(), 
where an error may occur. This behavior is legit and should not produce 
any crash report.
Without the check, the test continues to wait on used descriptors, and 
finally fails with message: "assertion timer != NULL failed".
Thus, any invalid input data produces a meaningless crash report.
>  Thomas
>

Re: [PATCH] tests/qtest/fuzz/virtio_net_fuzz.c: fix virtio_net_fuzz_multi

Posted by Thomas Huth 5 months, 2 weeks ago

On 13/06/2024 13.59, Дмитрий Фролов wrote:
> 
> 
> On 13.06.2024 13:08, Thomas Huth wrote:
>> On 23/05/2024 12.28, Dmitry Frolov wrote:
>>> If QTestState was already CLOSED due to error, calling qtest_clock_step()
>>> afterwards makes no sense and only raises false-crash with message:
>>> "assertion timer != NULL failed".
>>>
>>> Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
>>> ---
>>>   tests/qtest/fuzz/virtio_net_fuzz.c | 3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>> diff --git a/tests/qtest/fuzz/virtio_net_fuzz.c 
>>> b/tests/qtest/fuzz/virtio_net_fuzz.c
>>> index e239875e3b..2f57a8ddd8 100644
>>> --- a/tests/qtest/fuzz/virtio_net_fuzz.c
>>> +++ b/tests/qtest/fuzz/virtio_net_fuzz.c
>>> @@ -81,6 +81,9 @@ static void virtio_net_fuzz_multi(QTestState *s,
>>>           /* Run the main loop */
>>>           qtest_clock_step(s, 100);
>>>           flush_events(s);
>>> +        if (!qtest_probe_child(s)) {
>>> +            return;
>>> +        }
>>
>> According to your patch description, it rather sounds like the check 
>> should be done before the qtest_clock_step() ? ... or where does the 
>> QTestState get closed? During flush_events() ?
> To my understanding, the main loop is executed during flush_events(), where 
> an error may occur. This behavior is legit and should not produce any crash 
> report.
> Without the check, the test continues to wait on used descriptors, and 
> finally fails with message: "assertion timer != NULL failed".
> Thus, any invalid input data produces a meaningless crash report.

Ok, makes sense now, thanks!

There seems to be another while loop with a flush_events() call later in 
this file, does it maybe need the same treatment, too?

  Thomas

Re: [PATCH] tests/qtest/fuzz/virtio_net_fuzz.c: fix virtio_net_fuzz_multi

Posted by Дмитрий Фролов 5 months, 1 week ago

On 13.06.2024 19:50, Thomas Huth wrote:
> On 13/06/2024 13.59, Дмитрий Фролов wrote:
>>
>>
>> On 13.06.2024 13:08, Thomas Huth wrote:
>>> On 23/05/2024 12.28, Dmitry Frolov wrote:
>>>> If QTestState was already CLOSED due to error, calling 
>>>> qtest_clock_step()
>>>> afterwards makes no sense and only raises false-crash with message:
>>>> "assertion timer != NULL failed".
>>>>
>>>> Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
>>>> ---
>>>>   tests/qtest/fuzz/virtio_net_fuzz.c | 3 +++
>>>>   1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/tests/qtest/fuzz/virtio_net_fuzz.c 
>>>> b/tests/qtest/fuzz/virtio_net_fuzz.c
>>>> index e239875e3b..2f57a8ddd8 100644
>>>> --- a/tests/qtest/fuzz/virtio_net_fuzz.c
>>>> +++ b/tests/qtest/fuzz/virtio_net_fuzz.c
>>>> @@ -81,6 +81,9 @@ static void virtio_net_fuzz_multi(QTestState *s,
>>>>           /* Run the main loop */
>>>>           qtest_clock_step(s, 100);
>>>>           flush_events(s);
>>>> +        if (!qtest_probe_child(s)) {
>>>> +            return;
>>>> +        }
>>>
>>> According to your patch description, it rather sounds like the check 
>>> should be done before the qtest_clock_step() ? ... or where does the 
>>> QTestState get closed? During flush_events() ?
>> To my understanding, the main loop is executed during flush_events(), 
>> where an error may occur. This behavior is legit and should not 
>> produce any crash report.
>> Without the check, the test continues to wait on used descriptors, 
>> and finally fails with message: "assertion timer != NULL failed".
>> Thus, any invalid input data produces a meaningless crash report.
>
> Ok, makes sense now, thanks!
>
> There seems to be another while loop with a flush_events() call later 
> in this file, does it maybe need the same treatment, too?
With this fix, the number of crashes reduced significantly, but I guess, 
you are right...
If another similar crash will occur - I`ll make another patch.
Many thanks!
Dmitry
>  Thomas
>

Re: [PATCH] tests/qtest/fuzz/virtio_net_fuzz.c: fix virtio_net_fuzz_multi

Posted by Дмитрий Фролов 5 months, 2 weeks ago

ping

https://patchew.org/QEMU/20240523102813.396750-2-frolov@swemel.ru/

On 23.05.2024 13:28, Dmitry Frolov wrote:
> If QTestState was already CLOSED due to error, calling qtest_clock_step()
> afterwards makes no sense and only raises false-crash with message:
> "assertion timer != NULL failed".
>
> Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
> ---
>   tests/qtest/fuzz/virtio_net_fuzz.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/tests/qtest/fuzz/virtio_net_fuzz.c b/tests/qtest/fuzz/virtio_net_fuzz.c
> index e239875e3b..2f57a8ddd8 100644
> --- a/tests/qtest/fuzz/virtio_net_fuzz.c
> +++ b/tests/qtest/fuzz/virtio_net_fuzz.c
> @@ -81,6 +81,9 @@ static void virtio_net_fuzz_multi(QTestState *s,
>           /* Run the main loop */
>           qtest_clock_step(s, 100);
>           flush_events(s);
> +        if (!qtest_probe_child(s)) {
> +            return;
> +        }
>   
>           /* Wait on used descriptors */
>           if (check_used && !vqa.rx) {