[v1] fuzz: map all BARs and enable PCI devices

[PATCH] fuzz: map all BARs and enable PCI devices

Posted by Alexander Bulekov 3 years, 4 months ago

Prior to this patch, the fuzzer found inputs to map PCI device BARs and
enable the device. While it is nice that the fuzzer can do this, it
added significant overhead, since the fuzzer needs to map all the
BARs (regenerating the memory topology), at the start of each input.
With this patch, we do this once, before fuzzing, mitigating some of
this overhead.

Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
 tests/qtest/fuzz/generic_fuzz.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/tests/qtest/fuzz/generic_fuzz.c b/tests/qtest/fuzz/generic_fuzz.c
index 07ad690683..d95093ee53 100644
--- a/tests/qtest/fuzz/generic_fuzz.c
+++ b/tests/qtest/fuzz/generic_fuzz.c
@@ -16,6 +16,7 @@
 
 #include "hw/core/cpu.h"
 #include "tests/qtest/libqos/libqtest.h"
+#include "tests/qtest/libqos/pci-pc.h"
 #include "fuzz.h"
 #include "fork_fuzz.h"
 #include "exec/address-spaces.h"
@@ -762,6 +763,22 @@ static int locate_fuzz_objects(Object *child, void *opaque)
     return 0;
 }
 
+
+static void pci_enum(gpointer pcidev, gpointer bus)
+{
+    PCIDevice *dev = pcidev;
+    QPCIDevice *qdev;
+
+    qdev = qpci_device_find(bus, dev->devfn);
+    g_assert(qdev != NULL);
+    for (int i = 0; i < 6; i++) {
+        if (dev->io_regions[i].size) {
+            qpci_iomap(qdev, i, NULL);
+        }
+    }
+    qpci_device_enable(qdev);
+}
+
 static void generic_pre_fuzz(QTestState *s)
 {
     GHashTableIter iter;
@@ -810,6 +827,12 @@ static void generic_pre_fuzz(QTestState *s)
         exit(1);
     }
 
+    QPCIBus *pcibus;
+
+    pcibus = qpci_new_pc(s, NULL);
+    g_ptr_array_foreach(fuzzable_pci_devices, pci_enum, pcibus);
+    qpci_free_pc(pcibus);
+
     counter_shm_init();
 }
 
-- 
2.28.0

Re: [PATCH] fuzz: map all BARs and enable PCI devices

Posted by Darren Kenny 3 years, 4 months ago

Hi Alex,

On Wednesday, 2020-12-09 at 15:10:54 -05, Alexander Bulekov wrote:
> Prior to this patch, the fuzzer found inputs to map PCI device BARs and
> enable the device. While it is nice that the fuzzer can do this, it
> added significant overhead, since the fuzzer needs to map all the
> BARs (regenerating the memory topology), at the start of each input.
> With this patch, we do this once, before fuzzing, mitigating some of
> this overhead.
>
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>

In general this looks good, I've a small comment/nit below, but nothing
serious, so:

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>

> ---
>  tests/qtest/fuzz/generic_fuzz.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/tests/qtest/fuzz/generic_fuzz.c b/tests/qtest/fuzz/generic_fuzz.c
> index 07ad690683..d95093ee53 100644
> --- a/tests/qtest/fuzz/generic_fuzz.c
> +++ b/tests/qtest/fuzz/generic_fuzz.c
> @@ -16,6 +16,7 @@
>  
>  #include "hw/core/cpu.h"
>  #include "tests/qtest/libqos/libqtest.h"
> +#include "tests/qtest/libqos/pci-pc.h"
>  #include "fuzz.h"
>  #include "fork_fuzz.h"
>  #include "exec/address-spaces.h"
> @@ -762,6 +763,22 @@ static int locate_fuzz_objects(Object *child, void *opaque)
>      return 0;
>  }
>  
> +
> +static void pci_enum(gpointer pcidev, gpointer bus)
> +{
> +    PCIDevice *dev = pcidev;
> +    QPCIDevice *qdev;
> +
> +    qdev = qpci_device_find(bus, dev->devfn);
> +    g_assert(qdev != NULL);
> +    for (int i = 0; i < 6; i++) {
> +        if (dev->io_regions[i].size) {
> +            qpci_iomap(qdev, i, NULL);
> +        }
> +    }
> +    qpci_device_enable(qdev);
> +}
> +
>  static void generic_pre_fuzz(QTestState *s)
>  {
>      GHashTableIter iter;
> @@ -810,6 +827,12 @@ static void generic_pre_fuzz(QTestState *s)
>          exit(1);
>      }
>  
> +    QPCIBus *pcibus;

NIT: I'm not a huge fan of defining variables in the middle of code,
     call me old-fashioned if you will, but I tend to prefer them at the
     top of the function, or block ;)

     It does look good in the diff, but would seem odd in the overall
     code.

Thanks,

Darren.

Re: [PATCH] fuzz: map all BARs and enable PCI devices

Posted by Philippe Mathieu-Daudé 3 years, 4 months ago

On 12/10/20 12:36 PM, Darren Kenny wrote:
> Hi Alex,
> 
> On Wednesday, 2020-12-09 at 15:10:54 -05, Alexander Bulekov wrote:
>> Prior to this patch, the fuzzer found inputs to map PCI device BARs and
>> enable the device. While it is nice that the fuzzer can do this, it
>> added significant overhead, since the fuzzer needs to map all the
>> BARs (regenerating the memory topology), at the start of each input.
>> With this patch, we do this once, before fuzzing, mitigating some of
>> this overhead.
>>
>> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> 
> In general this looks good, I've a small comment/nit below, but nothing
> serious, so:
> 
> Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
> 
>> ---
>>  tests/qtest/fuzz/generic_fuzz.c | 23 +++++++++++++++++++++++
>>  1 file changed, 23 insertions(+)
>>
>> diff --git a/tests/qtest/fuzz/generic_fuzz.c b/tests/qtest/fuzz/generic_fuzz.c
>> index 07ad690683..d95093ee53 100644
>> --- a/tests/qtest/fuzz/generic_fuzz.c
>> +++ b/tests/qtest/fuzz/generic_fuzz.c
>> @@ -16,6 +16,7 @@
>>  
>>  #include "hw/core/cpu.h"
>>  #include "tests/qtest/libqos/libqtest.h"
>> +#include "tests/qtest/libqos/pci-pc.h"
>>  #include "fuzz.h"
>>  #include "fork_fuzz.h"
>>  #include "exec/address-spaces.h"
>> @@ -762,6 +763,22 @@ static int locate_fuzz_objects(Object *child, void *opaque)
>>      return 0;
>>  }
>>  
>> +
>> +static void pci_enum(gpointer pcidev, gpointer bus)
>> +{
>> +    PCIDevice *dev = pcidev;
>> +    QPCIDevice *qdev;
>> +
>> +    qdev = qpci_device_find(bus, dev->devfn);
>> +    g_assert(qdev != NULL);
>> +    for (int i = 0; i < 6; i++) {
>> +        if (dev->io_regions[i].size) {
>> +            qpci_iomap(qdev, i, NULL);
>> +        }
>> +    }
>> +    qpci_device_enable(qdev);
>> +}
>> +
>>  static void generic_pre_fuzz(QTestState *s)
>>  {
>>      GHashTableIter iter;
>> @@ -810,6 +827,12 @@ static void generic_pre_fuzz(QTestState *s)
>>          exit(1);
>>      }
>>  
>> +    QPCIBus *pcibus;
> 
> NIT: I'm not a huge fan of defining variables in the middle of code,
>      call me old-fashioned if you will, but I tend to prefer them at the
>      top of the function, or block ;)

This is barely tolerated in for(;;) loops.

See commit 7be41675f7c ("configure: Force the C standard to gnu99")
and QEMU CODING_STYLE.rst:

 Declarations
 ============

 Mixed declarations (interleaving statements and declarations within
 blocks) are generally not allowed; declarations should be at the
 beginning of blocks.

 Every now and then, an exception is made for declarations inside a
 #ifdef or #ifndef block: if the code looks nicer, such declarations can
 be placed at the top of the block even if there are statements above.
 On the other hand, however, it's often best to move that #ifdef/#ifndef
 block to a separate function altogether.

Regards,

Phil.

Re: [PATCH] fuzz: map all BARs and enable PCI devices

Posted by Alexander Bulekov 3 years, 4 months ago

On 201210 1411, Philippe Mathieu-Daudé wrote:
> On 12/10/20 12:36 PM, Darren Kenny wrote:
> > Hi Alex,
> > 
> > On Wednesday, 2020-12-09 at 15:10:54 -05, Alexander Bulekov wrote:
> >> Prior to this patch, the fuzzer found inputs to map PCI device BARs and
> >> enable the device. While it is nice that the fuzzer can do this, it
> >> added significant overhead, since the fuzzer needs to map all the
> >> BARs (regenerating the memory topology), at the start of each input.
> >> With this patch, we do this once, before fuzzing, mitigating some of
> >> this overhead.
> >>
> >> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> > 
> > In general this looks good, I've a small comment/nit below, but nothing
> > serious, so:
> > 
> > Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
> > 
> >> ---
> >>  tests/qtest/fuzz/generic_fuzz.c | 23 +++++++++++++++++++++++
> >>  1 file changed, 23 insertions(+)
> >>
> >> diff --git a/tests/qtest/fuzz/generic_fuzz.c b/tests/qtest/fuzz/generic_fuzz.c
> >> index 07ad690683..d95093ee53 100644
> >> --- a/tests/qtest/fuzz/generic_fuzz.c
> >> +++ b/tests/qtest/fuzz/generic_fuzz.c
> >> @@ -16,6 +16,7 @@
> >>  
> >>  #include "hw/core/cpu.h"
> >>  #include "tests/qtest/libqos/libqtest.h"
> >> +#include "tests/qtest/libqos/pci-pc.h"
> >>  #include "fuzz.h"
> >>  #include "fork_fuzz.h"
> >>  #include "exec/address-spaces.h"
> >> @@ -762,6 +763,22 @@ static int locate_fuzz_objects(Object *child, void *opaque)
> >>      return 0;
> >>  }
> >>  
> >> +
> >> +static void pci_enum(gpointer pcidev, gpointer bus)
> >> +{
> >> +    PCIDevice *dev = pcidev;
> >> +    QPCIDevice *qdev;
> >> +
> >> +    qdev = qpci_device_find(bus, dev->devfn);
> >> +    g_assert(qdev != NULL);
> >> +    for (int i = 0; i < 6; i++) {
> >> +        if (dev->io_regions[i].size) {
> >> +            qpci_iomap(qdev, i, NULL);
> >> +        }
> >> +    }
> >> +    qpci_device_enable(qdev);
> >> +}
> >> +
> >>  static void generic_pre_fuzz(QTestState *s)
> >>  {
> >>      GHashTableIter iter;
> >> @@ -810,6 +827,12 @@ static void generic_pre_fuzz(QTestState *s)
> >>          exit(1);
> >>      }
> >>  
> >> +    QPCIBus *pcibus;
> > 
> > NIT: I'm not a huge fan of defining variables in the middle of code,
> >      call me old-fashioned if you will, but I tend to prefer them at the
> >      top of the function, or block ;)
> 
> This is barely tolerated in for(;;) loops.
> 
> See commit 7be41675f7c ("configure: Force the C standard to gnu99")
> and QEMU CODING_STYLE.rst:
> 
>  Declarations
>  ============
> 
>  Mixed declarations (interleaving statements and declarations within
>  blocks) are generally not allowed; declarations should be at the
>  beginning of blocks.
> 
>  Every now and then, an exception is made for declarations inside a
>  #ifdef or #ifndef block: if the code looks nicer, such declarations can
>  be placed at the top of the block even if there are statements above.
>  On the other hand, however, it's often best to move that #ifdef/#ifndef
>  block to a separate function altogether.
> 
> Regards,
>

Sounds good - I'll send out a v2.
Thanks
-Alex

> Phil.
>