* [PATCH v2 0/7] Cleanup and fix @errp dereference
@ 2024-02-23 8:56 Zhao Liu
2024-02-23 8:56 ` [PATCH v2 1/7] hw/cxl/cxl-host: Fix missing ERRP_GUARD() in cxl_fixed_memory_window_config() Zhao Liu
` (7 more replies)
0 siblings, 8 replies; 18+ messages in thread
From: Zhao Liu @ 2024-02-23 8:56 UTC (permalink / raw)
To: Jonathan Cameron, Fan Ni, Laurent Vivier, Alistair Francis,
Edgar E . Iglesias, Peter Maydell, Michael S . Tsirkin,
Marcel Apfelbaum, Alex Williamson, Cédric Le Goater,
Philippe Mathieu-Daudé,
Markus Armbruster, Paolo Bonzini, Michael Tokarev
Cc: qemu-devel, qemu-arm, qemu-trivial, Zhao Liu
From: Zhao Liu <zhao1.liu@intel.com>
Hi all,
This is my v2 series to clean and fix @errp dereference.
Introduction
============
The patches 1-6 fix the cases that deference @errp without
ERRP_GUARD(), and they are based on my previsous v1 [1].
The patch 7 is merged in this series from another single patch [2].
Based on v1, v2 mainly add more description about problematic codes to
make review easier (with Markus' suggestion) and avoid dereferencing
@errp in patch 3's special case.
Future Work
===========
Additionally, I realized that my journey to fix @errp with ERRP_GUARD()
is still not over, because @errp's second restriction said:
* Without ERRP_GUARD(), use of the @errp parameter is restricted:
...
* - It should not be passed to error_prepend() or
* error_append_hint(), because that doesn't work with &error_fatal.
* ERRP_GUARD() lifts these restrictions.
With this restrication, there're still many places need to add missing
ERRP_GUARD().
[1]: https://lore.kernel.org/qemu-devel/20240221094317.994454-1-zhao1.liu@linux.intel.com/
[2]: https://lore.kernel.org/qemu-devel/20240221073948.768828-1-zhao1.liu@linux.intel.com/
Thanks and Best Regards,
Zhao
---
Zhao Liu (7):
hw/cxl/cxl-host: Fix missing ERRP_GUARD() in
cxl_fixed_memory_window_config()
hw/display/macfb: Fix missing ERRP_GUARD() in macfb_nubus_realize()
hw/mem/cxl_type3: Fix missing ERRP_GUARD() in ct3_realize()
hw/misc/xlnx-versal-trng: Check returned bool in
trng_prop_fault_event_set()
hw/pci-bridge/cxl_upstream: Fix missing ERRP_GUARD() in
cxl_usp_realize()
hw/vfio/iommufd: Fix missing ERRP_GUARD() in iommufd_cdev_getfd()
hw/intc: Check @errp to handle the error of
IOAPICCommonClass.realize()
hw/cxl/cxl-host.c | 1 +
hw/display/macfb.c | 1 +
hw/intc/ioapic_common.c | 4 ++++
hw/mem/cxl_type3.c | 1 +
hw/misc/xlnx-versal-trng.c | 3 +--
hw/pci-bridge/cxl_upstream.c | 1 +
hw/vfio/iommufd.c | 1 +
7 files changed, 10 insertions(+), 2 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 1/7] hw/cxl/cxl-host: Fix missing ERRP_GUARD() in cxl_fixed_memory_window_config()
2024-02-23 8:56 [PATCH v2 0/7] Cleanup and fix @errp dereference Zhao Liu
@ 2024-02-23 8:56 ` Zhao Liu
2024-02-26 16:07 ` Jonathan Cameron via
2024-02-28 7:30 ` Markus Armbruster
2024-02-23 8:56 ` [PATCH v2 2/7] hw/display/macfb: Fix missing ERRP_GUARD() in macfb_nubus_realize() Zhao Liu
` (6 subsequent siblings)
7 siblings, 2 replies; 18+ messages in thread
From: Zhao Liu @ 2024-02-23 8:56 UTC (permalink / raw)
To: Jonathan Cameron, Fan Ni, Laurent Vivier, Alistair Francis,
Edgar E . Iglesias, Peter Maydell, Michael S . Tsirkin,
Marcel Apfelbaum, Alex Williamson, Cédric Le Goater,
Philippe Mathieu-Daudé,
Markus Armbruster, Paolo Bonzini, Michael Tokarev
Cc: qemu-devel, qemu-arm, qemu-trivial, Zhao Liu
From: Zhao Liu <zhao1.liu@intel.com>
As the comment in qapi/error, dereferencing @errp requires
ERRP_GUARD():
* = Why, when and how to use ERRP_GUARD() =
*
* Without ERRP_GUARD(), use of the @errp parameter is restricted:
* - It must not be dereferenced, because it may be null.
...
* ERRP_GUARD() lifts these restrictions.
*
* To use ERRP_GUARD(), add it right at the beginning of the function.
* @errp can then be used without worrying about the argument being
* NULL or &error_fatal.
*
* Using it when it's not needed is safe, but please avoid cluttering
* the source with useless code.
But in cxl_fixed_memory_window_config(), @errp is dereferenced in 2
places without ERRP_GUARD():
fw->enc_int_ways = cxl_interleave_ways_enc(fw->num_targets, errp);
if (*errp) {
return;
}
and
fw->enc_int_gran =
cxl_interleave_granularity_enc(object->interleave_granularity,
errp);
if (*errp) {
return;
}
For the above 2 places, we check "*errp", because neither function
returns a suitable error code. And since machine_set_cfmw() - the caller
of cxl_fixed_memory_window_config() - doesn't get the NULL @errp
parameter as the "set" method of object property,
cxl_fixed_memory_window_config() hasn't triggered the bug that
dereferencing the NULL @errp.
To follow the requirement of @errp, add missing ERRP_GUARD() in
cxl_fixed_memory_window_config().
Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
Suggested by credit:
Markus: Referred his explanation about ERRP_GUARD().
---
v2:
* Add the @errp dereference code in commit message to make review
easier. (Markus)
---
hw/cxl/cxl-host.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/cxl/cxl-host.c b/hw/cxl/cxl-host.c
index 2aa776c79c74..c5f5fcfd64d0 100644
--- a/hw/cxl/cxl-host.c
+++ b/hw/cxl/cxl-host.c
@@ -26,6 +26,7 @@ static void cxl_fixed_memory_window_config(CXLState *cxl_state,
CXLFixedMemoryWindowOptions *object,
Error **errp)
{
+ ERRP_GUARD();
g_autofree CXLFixedWindow *fw = g_malloc0(sizeof(*fw));
strList *target;
int i;
--
2.34.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v2 2/7] hw/display/macfb: Fix missing ERRP_GUARD() in macfb_nubus_realize()
2024-02-23 8:56 [PATCH v2 0/7] Cleanup and fix @errp dereference Zhao Liu
2024-02-23 8:56 ` [PATCH v2 1/7] hw/cxl/cxl-host: Fix missing ERRP_GUARD() in cxl_fixed_memory_window_config() Zhao Liu
@ 2024-02-23 8:56 ` Zhao Liu
2024-02-23 8:56 ` [PATCH v2 3/7] hw/mem/cxl_type3: Fix missing ERRP_GUARD() in ct3_realize() Zhao Liu
` (5 subsequent siblings)
7 siblings, 0 replies; 18+ messages in thread
From: Zhao Liu @ 2024-02-23 8:56 UTC (permalink / raw)
To: Jonathan Cameron, Fan Ni, Laurent Vivier, Alistair Francis,
Edgar E . Iglesias, Peter Maydell, Michael S . Tsirkin,
Marcel Apfelbaum, Alex Williamson, Cédric Le Goater,
Philippe Mathieu-Daudé,
Markus Armbruster, Paolo Bonzini, Michael Tokarev
Cc: qemu-devel, qemu-arm, qemu-trivial, Zhao Liu
From: Zhao Liu <zhao1.liu@intel.com>
As the comment in qapi/error, dereferencing @errp requires
ERRP_GUARD():
* = Why, when and how to use ERRP_GUARD() =
*
* Without ERRP_GUARD(), use of the @errp parameter is restricted:
* - It must not be dereferenced, because it may be null.
...
* ERRP_GUARD() lifts these restrictions.
*
* To use ERRP_GUARD(), add it right at the beginning of the function.
* @errp can then be used without worrying about the argument being
* NULL or &error_fatal.
*
* Using it when it's not needed is safe, but please avoid cluttering
* the source with useless code.
But in macfb_nubus_realize(), @errp is dereferenced without
ERRP_GUARD():
ndc->parent_realize(dev, errp);
if (*errp) {
return;
}
Here we check *errp, because the ndc->parent_realize(), as a
DeviceClass.realize() callback, returns void. And since
macfb_nubus_realize(), also as a DeviceClass.realize(), doesn't get the
NULL @errp parameter, it hasn't triggered the bug that dereferencing the
NULL @errp.
To follow the requirement of @errp, add missing ERRP_GUARD() in
macfb_nubus_realize().
Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
Suggested by credit:
Markus: Referred his explanation about ERRP_GUARD().
---
v2:
* Add the @errp dereference code in commit message to make review
easier. (Markus)
---
hw/display/macfb.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/display/macfb.c b/hw/display/macfb.c
index 418e99c8e18e..1ace341a0ff4 100644
--- a/hw/display/macfb.c
+++ b/hw/display/macfb.c
@@ -714,6 +714,7 @@ static void macfb_nubus_set_irq(void *opaque, int n, int level)
static void macfb_nubus_realize(DeviceState *dev, Error **errp)
{
+ ERRP_GUARD();
NubusDevice *nd = NUBUS_DEVICE(dev);
MacfbNubusState *s = NUBUS_MACFB(dev);
MacfbNubusDeviceClass *ndc = NUBUS_MACFB_GET_CLASS(dev);
--
2.34.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v2 3/7] hw/mem/cxl_type3: Fix missing ERRP_GUARD() in ct3_realize()
2024-02-23 8:56 [PATCH v2 0/7] Cleanup and fix @errp dereference Zhao Liu
2024-02-23 8:56 ` [PATCH v2 1/7] hw/cxl/cxl-host: Fix missing ERRP_GUARD() in cxl_fixed_memory_window_config() Zhao Liu
2024-02-23 8:56 ` [PATCH v2 2/7] hw/display/macfb: Fix missing ERRP_GUARD() in macfb_nubus_realize() Zhao Liu
@ 2024-02-23 8:56 ` Zhao Liu
2024-02-26 16:07 ` Jonathan Cameron via
2024-02-23 8:56 ` [PATCH v2 4/7] hw/misc/xlnx-versal-trng: Check returned bool in trng_prop_fault_event_set() Zhao Liu
` (4 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Zhao Liu @ 2024-02-23 8:56 UTC (permalink / raw)
To: Jonathan Cameron, Fan Ni, Laurent Vivier, Alistair Francis,
Edgar E . Iglesias, Peter Maydell, Michael S . Tsirkin,
Marcel Apfelbaum, Alex Williamson, Cédric Le Goater,
Philippe Mathieu-Daudé,
Markus Armbruster, Paolo Bonzini, Michael Tokarev
Cc: qemu-devel, qemu-arm, qemu-trivial, Zhao Liu
From: Zhao Liu <zhao1.liu@intel.com>
As the comment in qapi/error, dereferencing @errp requires
ERRP_GUARD():
* = Why, when and how to use ERRP_GUARD() =
*
* Without ERRP_GUARD(), use of the @errp parameter is restricted:
* - It must not be dereferenced, because it may be null.
...
* ERRP_GUARD() lifts these restrictions.
*
* To use ERRP_GUARD(), add it right at the beginning of the function.
* @errp can then be used without worrying about the argument being
* NULL or &error_fatal.
*
* Using it when it's not needed is safe, but please avoid cluttering
* the source with useless code.
But in ct3_realize(), @errp is dereferenced without ERRP_GUARD():
cxl_doe_cdat_init(cxl_cstate, errp);
if (*errp) {
goto err_free_special_ops;
}
Here we check *errp, because cxl_doe_cdat_init() returns void. And
ct3_realize() - as a PCIDeviceClass.realize() method - doesn't get the
NULL @errp parameter, it hasn't triggered the bug that dereferencing
the NULL @errp.
To follow the requirement of @errp, add missing ERRP_GUARD() in
ct3_realize().
Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
Suggested by credit:
Markus: Referred his explanation about ERRP_GUARD().
---
v2:
* Add the @errp dereference code in commit message to make review
easier. (Markus)
---
hw/mem/cxl_type3.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index e8801805b90f..a3b0761f843b 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -645,6 +645,7 @@ static DOEProtocol doe_cdat_prot[] = {
static void ct3_realize(PCIDevice *pci_dev, Error **errp)
{
+ ERRP_GUARD();
CXLType3Dev *ct3d = CXL_TYPE3(pci_dev);
CXLComponentState *cxl_cstate = &ct3d->cxl_cstate;
ComponentRegisters *regs = &cxl_cstate->crb;
--
2.34.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v2 4/7] hw/misc/xlnx-versal-trng: Check returned bool in trng_prop_fault_event_set()
2024-02-23 8:56 [PATCH v2 0/7] Cleanup and fix @errp dereference Zhao Liu
` (2 preceding siblings ...)
2024-02-23 8:56 ` [PATCH v2 3/7] hw/mem/cxl_type3: Fix missing ERRP_GUARD() in ct3_realize() Zhao Liu
@ 2024-02-23 8:56 ` Zhao Liu
2024-02-23 17:53 ` Philippe Mathieu-Daudé
2024-02-23 8:56 ` [PATCH v2 5/7] hw/pci-bridge/cxl_upstream: Fix missing ERRP_GUARD() in cxl_usp_realize() Zhao Liu
` (3 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Zhao Liu @ 2024-02-23 8:56 UTC (permalink / raw)
To: Jonathan Cameron, Fan Ni, Laurent Vivier, Alistair Francis,
Edgar E . Iglesias, Peter Maydell, Michael S . Tsirkin,
Marcel Apfelbaum, Alex Williamson, Cédric Le Goater,
Philippe Mathieu-Daudé,
Markus Armbruster, Paolo Bonzini, Michael Tokarev
Cc: qemu-devel, qemu-arm, qemu-trivial, Zhao Liu
From: Zhao Liu <zhao1.liu@intel.com>
As the comment in qapi/error, dereferencing @errp requires
ERRP_GUARD():
* = Why, when and how to use ERRP_GUARD() =
*
* Without ERRP_GUARD(), use of the @errp parameter is restricted:
* - It must not be dereferenced, because it may be null.
...
* ERRP_GUARD() lifts these restrictions.
*
* To use ERRP_GUARD(), add it right at the beginning of the function.
* @errp can then be used without worrying about the argument being
* NULL or &error_fatal.
*
* Using it when it's not needed is safe, but please avoid cluttering
* the source with useless code.
But in trng_prop_fault_event_set, @errp is dereferenced without
ERRP_GUARD():
visit_type_uint32(v, name, events, errp);
if (*errp) {
return;
}
Currently, since trng_prop_fault_event_set() doesn't get the NULL @errp
parameter as a "set" method of object property, it hasn't triggered the
bug that dereferencing the NULL @errp.
And since visit_type_uint32() returns bool, check the returned bool
directly instead of dereferencing @errp, then we needn't the add missing
ERRP_GUARD().
Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Suggested by credit:
Markus: Referred his explanation about ERRP_GUARD().
---
v2:
* Add the @errp dereference code in commit message to make review
easier. (Markus)
* Check the returned bool instead of dereferencing @errp. (Markus)
---
hw/misc/xlnx-versal-trng.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/hw/misc/xlnx-versal-trng.c b/hw/misc/xlnx-versal-trng.c
index b8111b8b6626..6495188dc748 100644
--- a/hw/misc/xlnx-versal-trng.c
+++ b/hw/misc/xlnx-versal-trng.c
@@ -644,8 +644,7 @@ static void trng_prop_fault_event_set(Object *obj, Visitor *v,
Property *prop = opaque;
uint32_t *events = object_field_prop_ptr(obj, prop);
- visit_type_uint32(v, name, events, errp);
- if (*errp) {
+ if (!visit_type_uint32(v, name, events, errp)) {
return;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v2 5/7] hw/pci-bridge/cxl_upstream: Fix missing ERRP_GUARD() in cxl_usp_realize()
2024-02-23 8:56 [PATCH v2 0/7] Cleanup and fix @errp dereference Zhao Liu
` (3 preceding siblings ...)
2024-02-23 8:56 ` [PATCH v2 4/7] hw/misc/xlnx-versal-trng: Check returned bool in trng_prop_fault_event_set() Zhao Liu
@ 2024-02-23 8:56 ` Zhao Liu
2024-02-26 16:07 ` Jonathan Cameron via
2024-03-12 10:39 ` Thomas Huth
2024-02-23 8:56 ` [PATCH v2 6/7] hw/vfio/iommufd: Fix missing ERRP_GUARD() in iommufd_cdev_getfd() Zhao Liu
` (2 subsequent siblings)
7 siblings, 2 replies; 18+ messages in thread
From: Zhao Liu @ 2024-02-23 8:56 UTC (permalink / raw)
To: Jonathan Cameron, Fan Ni, Laurent Vivier, Alistair Francis,
Edgar E . Iglesias, Peter Maydell, Michael S . Tsirkin,
Marcel Apfelbaum, Alex Williamson, Cédric Le Goater,
Philippe Mathieu-Daudé,
Markus Armbruster, Paolo Bonzini, Michael Tokarev
Cc: qemu-devel, qemu-arm, qemu-trivial, Zhao Liu
From: Zhao Liu <zhao1.liu@intel.com>
As the comment in qapi/error, dereferencing @errp requires
ERRP_GUARD():
* = Why, when and how to use ERRP_GUARD() =
*
* Without ERRP_GUARD(), use of the @errp parameter is restricted:
* - It must not be dereferenced, because it may be null.
...
* ERRP_GUARD() lifts these restrictions.
*
* To use ERRP_GUARD(), add it right at the beginning of the function.
* @errp can then be used without worrying about the argument being
* NULL or &error_fatal.
*
* Using it when it's not needed is safe, but please avoid cluttering
* the source with useless code.
But in cxl_usp_realize(), @errp is dereferenced without ERRP_GUARD():
cxl_doe_cdat_init(cxl_cstate, errp);
if (*errp) {
goto err_cap;
}
Here we check *errp, because cxl_doe_cdat_init() returns void. And since
cxl_usp_realize() - as a PCIDeviceClass.realize() method - doesn't get
the NULL @errp parameter, it hasn't triggered the bug that dereferencing
the NULL @errp.
To follow the requirement of @errp, add missing ERRP_GUARD() in
cxl_usp_realize().
Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
Suggested by credit:
Markus: Referred his explanation about ERRP_GUARD().
---
v2:
* Add the @errp dereference code in commit message to make review
easier. (Markus)
---
hw/pci-bridge/cxl_upstream.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/pci-bridge/cxl_upstream.c b/hw/pci-bridge/cxl_upstream.c
index e87eb4017713..03d123cca0ef 100644
--- a/hw/pci-bridge/cxl_upstream.c
+++ b/hw/pci-bridge/cxl_upstream.c
@@ -289,6 +289,7 @@ static void free_default_cdat_table(CDATSubHeader **cdat_table, int num,
static void cxl_usp_realize(PCIDevice *d, Error **errp)
{
+ ERRP_GUARD();
PCIEPort *p = PCIE_PORT(d);
CXLUpstreamPort *usp = CXL_USP(d);
CXLComponentState *cxl_cstate = &usp->cxl_cstate;
--
2.34.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v2 6/7] hw/vfio/iommufd: Fix missing ERRP_GUARD() in iommufd_cdev_getfd()
2024-02-23 8:56 [PATCH v2 0/7] Cleanup and fix @errp dereference Zhao Liu
` (4 preceding siblings ...)
2024-02-23 8:56 ` [PATCH v2 5/7] hw/pci-bridge/cxl_upstream: Fix missing ERRP_GUARD() in cxl_usp_realize() Zhao Liu
@ 2024-02-23 8:56 ` Zhao Liu
2024-02-23 8:56 ` [PATCH v2 7/7] hw/intc: Check @errp to handle the error of IOAPICCommonClass.realize() Zhao Liu
2024-03-11 3:51 ` [PATCH v2 0/7] Cleanup and fix @errp dereference Zhao Liu
7 siblings, 0 replies; 18+ messages in thread
From: Zhao Liu @ 2024-02-23 8:56 UTC (permalink / raw)
To: Jonathan Cameron, Fan Ni, Laurent Vivier, Alistair Francis,
Edgar E . Iglesias, Peter Maydell, Michael S . Tsirkin,
Marcel Apfelbaum, Alex Williamson, Cédric Le Goater,
Philippe Mathieu-Daudé,
Markus Armbruster, Paolo Bonzini, Michael Tokarev
Cc: qemu-devel, qemu-arm, qemu-trivial, Zhao Liu
From: Zhao Liu <zhao1.liu@intel.com>
As the comment in qapi/error, dereferencing @errp requires
ERRP_GUARD():
* = Why, when and how to use ERRP_GUARD() =
*
* Without ERRP_GUARD(), use of the @errp parameter is restricted:
* - It must not be dereferenced, because it may be null.
...
* ERRP_GUARD() lifts these restrictions.
*
* To use ERRP_GUARD(), add it right at the beginning of the function.
* @errp can then be used without worrying about the argument being
* NULL or &error_fatal.
*
* Using it when it's not needed is safe, but please avoid cluttering
* the source with useless code.
But in iommufd_cdev_getfd(), @errp is dereferenced without ERRP_GUARD():
if (*errp) {
error_prepend(errp, VFIO_MSG_PREFIX, path);
}
Currently, since vfio_attach_device() - the caller of
iommufd_cdev_getfd() - is always called in DeviceClass.realize() context
and doesn't get the NULL @errp parameter, iommufd_cdev_getfd()
hasn't triggered the bug that dereferencing the NULL @errp.
To follow the requirement of @errp, add missing ERRP_GUARD() in
iommufd_cdev_getfd().
Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
Suggested by credit:
Markus: Referred his explanation about ERRP_GUARD().
---
v2:
* Add the @errp dereference code in commit message to make review
easier. (Markus)
---
hw/vfio/iommufd.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index 9bfddc136089..7baf49e6ee9e 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -116,6 +116,7 @@ static void iommufd_cdev_unbind_and_disconnect(VFIODevice *vbasedev)
static int iommufd_cdev_getfd(const char *sysfs_path, Error **errp)
{
+ ERRP_GUARD();
long int ret = -ENOTTY;
char *path, *vfio_dev_path = NULL, *vfio_path = NULL;
DIR *dir = NULL;
--
2.34.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v2 7/7] hw/intc: Check @errp to handle the error of IOAPICCommonClass.realize()
2024-02-23 8:56 [PATCH v2 0/7] Cleanup and fix @errp dereference Zhao Liu
` (5 preceding siblings ...)
2024-02-23 8:56 ` [PATCH v2 6/7] hw/vfio/iommufd: Fix missing ERRP_GUARD() in iommufd_cdev_getfd() Zhao Liu
@ 2024-02-23 8:56 ` Zhao Liu
2024-02-23 17:52 ` Philippe Mathieu-Daudé
2024-03-11 3:51 ` [PATCH v2 0/7] Cleanup and fix @errp dereference Zhao Liu
7 siblings, 1 reply; 18+ messages in thread
From: Zhao Liu @ 2024-02-23 8:56 UTC (permalink / raw)
To: Jonathan Cameron, Fan Ni, Laurent Vivier, Alistair Francis,
Edgar E . Iglesias, Peter Maydell, Michael S . Tsirkin,
Marcel Apfelbaum, Alex Williamson, Cédric Le Goater,
Philippe Mathieu-Daudé,
Markus Armbruster, Paolo Bonzini, Michael Tokarev
Cc: qemu-devel, qemu-arm, qemu-trivial, Zhao Liu
From: Zhao Liu <zhao1.liu@intel.com>
IOAPICCommonClass implements its own private realize(), and this private
realize() allows error.
Since IOAPICCommonClass.realize() returns void, to check the error,
dereference @errp with ERRP_GUARD().
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
v2:
* Add the missing ERRP_GUARD(). (Markus)
* Move this single patch into @errp fixing series. (Micheal)
---
hw/intc/ioapic_common.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/hw/intc/ioapic_common.c b/hw/intc/ioapic_common.c
index cb9bf6214608..efbe6958c8d7 100644
--- a/hw/intc/ioapic_common.c
+++ b/hw/intc/ioapic_common.c
@@ -152,6 +152,7 @@ static int ioapic_dispatch_post_load(void *opaque, int version_id)
static void ioapic_common_realize(DeviceState *dev, Error **errp)
{
+ ERRP_GUARD();
IOAPICCommonState *s = IOAPIC_COMMON(dev);
IOAPICCommonClass *info;
@@ -162,6 +163,9 @@ static void ioapic_common_realize(DeviceState *dev, Error **errp)
info = IOAPIC_COMMON_GET_CLASS(s);
info->realize(dev, errp);
+ if (*errp) {
+ return;
+ }
sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->io_memory);
ioapic_no++;
--
2.34.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 7/7] hw/intc: Check @errp to handle the error of IOAPICCommonClass.realize()
2024-02-23 8:56 ` [PATCH v2 7/7] hw/intc: Check @errp to handle the error of IOAPICCommonClass.realize() Zhao Liu
@ 2024-02-23 17:52 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-02-23 17:52 UTC (permalink / raw)
To: Zhao Liu, Jonathan Cameron, Fan Ni, Laurent Vivier,
Alistair Francis, Edgar E . Iglesias, Peter Maydell,
Michael S . Tsirkin, Marcel Apfelbaum, Alex Williamson,
Cédric Le Goater, Markus Armbruster, Paolo Bonzini,
Michael Tokarev
Cc: qemu-devel, qemu-arm, qemu-trivial, Zhao Liu
On 23/2/24 09:56, Zhao Liu wrote:
> From: Zhao Liu <zhao1.liu@intel.com>
>
> IOAPICCommonClass implements its own private realize(), and this private
> realize() allows error.
>
> Since IOAPICCommonClass.realize() returns void, to check the error,
> dereference @errp with ERRP_GUARD().
>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
> v2:
> * Add the missing ERRP_GUARD(). (Markus)
> * Move this single patch into @errp fixing series. (Micheal)
> ---
> hw/intc/ioapic_common.c | 4 ++++
> 1 file changed, 4 insertions(+)
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 4/7] hw/misc/xlnx-versal-trng: Check returned bool in trng_prop_fault_event_set()
2024-02-23 8:56 ` [PATCH v2 4/7] hw/misc/xlnx-versal-trng: Check returned bool in trng_prop_fault_event_set() Zhao Liu
@ 2024-02-23 17:53 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-02-23 17:53 UTC (permalink / raw)
To: Zhao Liu, Jonathan Cameron, Fan Ni, Laurent Vivier,
Alistair Francis, Edgar E . Iglesias, Peter Maydell,
Michael S . Tsirkin, Marcel Apfelbaum, Alex Williamson,
Cédric Le Goater, Markus Armbruster, Paolo Bonzini,
Michael Tokarev
Cc: qemu-devel, qemu-arm, qemu-trivial, Zhao Liu
On 23/2/24 09:56, Zhao Liu wrote:
> From: Zhao Liu <zhao1.liu@intel.com>
>
> As the comment in qapi/error, dereferencing @errp requires
> ERRP_GUARD():
>
> * = Why, when and how to use ERRP_GUARD() =
> *
> * Without ERRP_GUARD(), use of the @errp parameter is restricted:
> * - It must not be dereferenced, because it may be null.
> ...
> * ERRP_GUARD() lifts these restrictions.
> *
> * To use ERRP_GUARD(), add it right at the beginning of the function.
> * @errp can then be used without worrying about the argument being
> * NULL or &error_fatal.
> *
> * Using it when it's not needed is safe, but please avoid cluttering
> * the source with useless code.
>
> But in trng_prop_fault_event_set, @errp is dereferenced without
> ERRP_GUARD():
>
> visit_type_uint32(v, name, events, errp);
> if (*errp) {
> return;
> }
>
> Currently, since trng_prop_fault_event_set() doesn't get the NULL @errp
> parameter as a "set" method of object property, it hasn't triggered the
> bug that dereferencing the NULL @errp.
>
> And since visit_type_uint32() returns bool, check the returned bool
> directly instead of dereferencing @errp, then we needn't the add missing
> ERRP_GUARD().
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
> Suggested by credit:
> Markus: Referred his explanation about ERRP_GUARD().
> ---
> v2:
> * Add the @errp dereference code in commit message to make review
> easier. (Markus)
> * Check the returned bool instead of dereferencing @errp. (Markus)
> ---
> hw/misc/xlnx-versal-trng.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/7] hw/cxl/cxl-host: Fix missing ERRP_GUARD() in cxl_fixed_memory_window_config()
2024-02-23 8:56 ` [PATCH v2 1/7] hw/cxl/cxl-host: Fix missing ERRP_GUARD() in cxl_fixed_memory_window_config() Zhao Liu
@ 2024-02-26 16:07 ` Jonathan Cameron via
2024-02-28 7:30 ` Markus Armbruster
1 sibling, 0 replies; 18+ messages in thread
From: Jonathan Cameron via @ 2024-02-26 16:07 UTC (permalink / raw)
To: Zhao Liu
Cc: Fan Ni, Laurent Vivier, Alistair Francis, Edgar E . Iglesias,
Peter Maydell, Michael S . Tsirkin, Marcel Apfelbaum,
Alex Williamson, Cédric Le Goater,
Philippe Mathieu-Daudé,
Markus Armbruster, Paolo Bonzini, Michael Tokarev, qemu-devel,
qemu-arm, qemu-trivial, Zhao Liu
On Fri, 23 Feb 2024 16:56:47 +0800
Zhao Liu <zhao1.liu@linux.intel.com> wrote:
> From: Zhao Liu <zhao1.liu@intel.com>
>
> As the comment in qapi/error, dereferencing @errp requires
> ERRP_GUARD():
>
> * = Why, when and how to use ERRP_GUARD() =
> *
> * Without ERRP_GUARD(), use of the @errp parameter is restricted:
> * - It must not be dereferenced, because it may be null.
> ...
> * ERRP_GUARD() lifts these restrictions.
> *
> * To use ERRP_GUARD(), add it right at the beginning of the function.
> * @errp can then be used without worrying about the argument being
> * NULL or &error_fatal.
> *
> * Using it when it's not needed is safe, but please avoid cluttering
> * the source with useless code.
>
> But in cxl_fixed_memory_window_config(), @errp is dereferenced in 2
> places without ERRP_GUARD():
>
> fw->enc_int_ways = cxl_interleave_ways_enc(fw->num_targets, errp);
> if (*errp) {
> return;
> }
>
> and
>
> fw->enc_int_gran =
> cxl_interleave_granularity_enc(object->interleave_granularity,
> errp);
> if (*errp) {
> return;
> }
>
> For the above 2 places, we check "*errp", because neither function
> returns a suitable error code. And since machine_set_cfmw() - the caller
> of cxl_fixed_memory_window_config() - doesn't get the NULL @errp
> parameter as the "set" method of object property,
> cxl_fixed_memory_window_config() hasn't triggered the bug that
> dereferencing the NULL @errp.
>
> To follow the requirement of @errp, add missing ERRP_GUARD() in
> cxl_fixed_memory_window_config().
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> Suggested by credit:
> Markus: Referred his explanation about ERRP_GUARD().
> ---
> v2:
> * Add the @errp dereference code in commit message to make review
> easier. (Markus)
> ---
> hw/cxl/cxl-host.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/hw/cxl/cxl-host.c b/hw/cxl/cxl-host.c
> index 2aa776c79c74..c5f5fcfd64d0 100644
> --- a/hw/cxl/cxl-host.c
> +++ b/hw/cxl/cxl-host.c
> @@ -26,6 +26,7 @@ static void cxl_fixed_memory_window_config(CXLState *cxl_state,
> CXLFixedMemoryWindowOptions *object,
> Error **errp)
> {
> + ERRP_GUARD();
> g_autofree CXLFixedWindow *fw = g_malloc0(sizeof(*fw));
> strList *target;
> int i;
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 3/7] hw/mem/cxl_type3: Fix missing ERRP_GUARD() in ct3_realize()
2024-02-23 8:56 ` [PATCH v2 3/7] hw/mem/cxl_type3: Fix missing ERRP_GUARD() in ct3_realize() Zhao Liu
@ 2024-02-26 16:07 ` Jonathan Cameron via
0 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron via @ 2024-02-26 16:07 UTC (permalink / raw)
To: Zhao Liu
Cc: Fan Ni, Laurent Vivier, Alistair Francis, Edgar E . Iglesias,
Peter Maydell, Michael S . Tsirkin, Marcel Apfelbaum,
Alex Williamson, Cédric Le Goater,
Philippe Mathieu-Daudé,
Markus Armbruster, Paolo Bonzini, Michael Tokarev, qemu-devel,
qemu-arm, qemu-trivial, Zhao Liu
On Fri, 23 Feb 2024 16:56:49 +0800
Zhao Liu <zhao1.liu@linux.intel.com> wrote:
> From: Zhao Liu <zhao1.liu@intel.com>
>
> As the comment in qapi/error, dereferencing @errp requires
> ERRP_GUARD():
>
> * = Why, when and how to use ERRP_GUARD() =
> *
> * Without ERRP_GUARD(), use of the @errp parameter is restricted:
> * - It must not be dereferenced, because it may be null.
> ...
> * ERRP_GUARD() lifts these restrictions.
> *
> * To use ERRP_GUARD(), add it right at the beginning of the function.
> * @errp can then be used without worrying about the argument being
> * NULL or &error_fatal.
> *
> * Using it when it's not needed is safe, but please avoid cluttering
> * the source with useless code.
>
> But in ct3_realize(), @errp is dereferenced without ERRP_GUARD():
>
> cxl_doe_cdat_init(cxl_cstate, errp);
> if (*errp) {
> goto err_free_special_ops;
> }
>
> Here we check *errp, because cxl_doe_cdat_init() returns void. And
> ct3_realize() - as a PCIDeviceClass.realize() method - doesn't get the
> NULL @errp parameter, it hasn't triggered the bug that dereferencing
> the NULL @errp.
>
> To follow the requirement of @errp, add missing ERRP_GUARD() in
> ct3_realize().
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> Suggested by credit:
> Markus: Referred his explanation about ERRP_GUARD().
> ---
> v2:
> * Add the @errp dereference code in commit message to make review
> easier. (Markus)
> ---
> hw/mem/cxl_type3.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index e8801805b90f..a3b0761f843b 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -645,6 +645,7 @@ static DOEProtocol doe_cdat_prot[] = {
>
> static void ct3_realize(PCIDevice *pci_dev, Error **errp)
> {
> + ERRP_GUARD();
> CXLType3Dev *ct3d = CXL_TYPE3(pci_dev);
> CXLComponentState *cxl_cstate = &ct3d->cxl_cstate;
> ComponentRegisters *regs = &cxl_cstate->crb;
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 5/7] hw/pci-bridge/cxl_upstream: Fix missing ERRP_GUARD() in cxl_usp_realize()
2024-02-23 8:56 ` [PATCH v2 5/7] hw/pci-bridge/cxl_upstream: Fix missing ERRP_GUARD() in cxl_usp_realize() Zhao Liu
@ 2024-02-26 16:07 ` Jonathan Cameron via
2024-03-12 10:39 ` Thomas Huth
1 sibling, 0 replies; 18+ messages in thread
From: Jonathan Cameron via @ 2024-02-26 16:07 UTC (permalink / raw)
To: Zhao Liu
Cc: Fan Ni, Laurent Vivier, Alistair Francis, Edgar E . Iglesias,
Peter Maydell, Michael S . Tsirkin, Marcel Apfelbaum,
Alex Williamson, Cédric Le Goater,
Philippe Mathieu-Daudé,
Markus Armbruster, Paolo Bonzini, Michael Tokarev, qemu-devel,
qemu-arm, qemu-trivial, Zhao Liu
On Fri, 23 Feb 2024 16:56:51 +0800
Zhao Liu <zhao1.liu@linux.intel.com> wrote:
> From: Zhao Liu <zhao1.liu@intel.com>
>
> As the comment in qapi/error, dereferencing @errp requires
> ERRP_GUARD():
>
> * = Why, when and how to use ERRP_GUARD() =
> *
> * Without ERRP_GUARD(), use of the @errp parameter is restricted:
> * - It must not be dereferenced, because it may be null.
> ...
> * ERRP_GUARD() lifts these restrictions.
> *
> * To use ERRP_GUARD(), add it right at the beginning of the function.
> * @errp can then be used without worrying about the argument being
> * NULL or &error_fatal.
> *
> * Using it when it's not needed is safe, but please avoid cluttering
> * the source with useless code.
>
> But in cxl_usp_realize(), @errp is dereferenced without ERRP_GUARD():
>
> cxl_doe_cdat_init(cxl_cstate, errp);
> if (*errp) {
> goto err_cap;
> }
>
> Here we check *errp, because cxl_doe_cdat_init() returns void. And since
> cxl_usp_realize() - as a PCIDeviceClass.realize() method - doesn't get
> the NULL @errp parameter, it hasn't triggered the bug that dereferencing
> the NULL @errp.
>
> To follow the requirement of @errp, add missing ERRP_GUARD() in
> cxl_usp_realize().
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> Suggested by credit:
> Markus: Referred his explanation about ERRP_GUARD().
> ---
> v2:
> * Add the @errp dereference code in commit message to make review
> easier. (Markus)
> ---
> hw/pci-bridge/cxl_upstream.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/hw/pci-bridge/cxl_upstream.c b/hw/pci-bridge/cxl_upstream.c
> index e87eb4017713..03d123cca0ef 100644
> --- a/hw/pci-bridge/cxl_upstream.c
> +++ b/hw/pci-bridge/cxl_upstream.c
> @@ -289,6 +289,7 @@ static void free_default_cdat_table(CDATSubHeader **cdat_table, int num,
>
> static void cxl_usp_realize(PCIDevice *d, Error **errp)
> {
> + ERRP_GUARD();
> PCIEPort *p = PCIE_PORT(d);
> CXLUpstreamPort *usp = CXL_USP(d);
> CXLComponentState *cxl_cstate = &usp->cxl_cstate;
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/7] hw/cxl/cxl-host: Fix missing ERRP_GUARD() in cxl_fixed_memory_window_config()
2024-02-23 8:56 ` [PATCH v2 1/7] hw/cxl/cxl-host: Fix missing ERRP_GUARD() in cxl_fixed_memory_window_config() Zhao Liu
2024-02-26 16:07 ` Jonathan Cameron via
@ 2024-02-28 7:30 ` Markus Armbruster
2024-02-28 16:09 ` Zhao Liu
1 sibling, 1 reply; 18+ messages in thread
From: Markus Armbruster @ 2024-02-28 7:30 UTC (permalink / raw)
To: Zhao Liu
Cc: Jonathan Cameron, Fan Ni, Laurent Vivier, Alistair Francis,
Edgar E . Iglesias, Peter Maydell, Michael S . Tsirkin,
Marcel Apfelbaum, Alex Williamson, Cédric Le Goater,
Philippe Mathieu-Daudé,
Paolo Bonzini, Michael Tokarev, qemu-devel, qemu-arm,
qemu-trivial, Zhao Liu
Zhao Liu <zhao1.liu@linux.intel.com> writes:
> From: Zhao Liu <zhao1.liu@intel.com>
>
> As the comment in qapi/error, dereferencing @errp requires
> ERRP_GUARD():
>
> * = Why, when and how to use ERRP_GUARD() =
> *
> * Without ERRP_GUARD(), use of the @errp parameter is restricted:
> * - It must not be dereferenced, because it may be null.
> ...
> * ERRP_GUARD() lifts these restrictions.
> *
> * To use ERRP_GUARD(), add it right at the beginning of the function.
> * @errp can then be used without worrying about the argument being
> * NULL or &error_fatal.
> *
> * Using it when it's not needed is safe, but please avoid cluttering
> * the source with useless code.
>
> But in cxl_fixed_memory_window_config(), @errp is dereferenced in 2
> places without ERRP_GUARD():
>
> fw->enc_int_ways = cxl_interleave_ways_enc(fw->num_targets, errp);
> if (*errp) {
> return;
> }
>
> and
>
> fw->enc_int_gran =
> cxl_interleave_granularity_enc(object->interleave_granularity,
> errp);
> if (*errp) {
> return;
> }
No need to quote the dereferences in full.
But in cxl_fixed_memory_window_config(), @errp is dereferenced in 2
places without ERRP_GUARD().
In these two places, we check "*errp", because neither function
would do.
Same for the other commit messages.
Hardly worth a respin, though :)
> For the above 2 places, we check "*errp", because neither function
> returns a suitable error code. And since machine_set_cfmw() - the caller
> of cxl_fixed_memory_window_config() - doesn't get the NULL @errp
> parameter as the "set" method of object property,
> cxl_fixed_memory_window_config() hasn't triggered the bug that
> dereferencing the NULL @errp.
>
> To follow the requirement of @errp, add missing ERRP_GUARD() in
> cxl_fixed_memory_window_config().
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> ---
> Suggested by credit:
> Markus: Referred his explanation about ERRP_GUARD().
> ---
> v2:
> * Add the @errp dereference code in commit message to make review
> easier. (Markus)
> ---
> hw/cxl/cxl-host.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/hw/cxl/cxl-host.c b/hw/cxl/cxl-host.c
> index 2aa776c79c74..c5f5fcfd64d0 100644
> --- a/hw/cxl/cxl-host.c
> +++ b/hw/cxl/cxl-host.c
> @@ -26,6 +26,7 @@ static void cxl_fixed_memory_window_config(CXLState *cxl_state,
> CXLFixedMemoryWindowOptions *object,
> Error **errp)
> {
> + ERRP_GUARD();
> g_autofree CXLFixedWindow *fw = g_malloc0(sizeof(*fw));
> strList *target;
> int i;
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/7] hw/cxl/cxl-host: Fix missing ERRP_GUARD() in cxl_fixed_memory_window_config()
2024-02-28 7:30 ` Markus Armbruster
@ 2024-02-28 16:09 ` Zhao Liu
0 siblings, 0 replies; 18+ messages in thread
From: Zhao Liu @ 2024-02-28 16:09 UTC (permalink / raw)
To: Markus Armbruster
Cc: Jonathan Cameron, Fan Ni, Laurent Vivier, Alistair Francis,
Edgar E . Iglesias, Peter Maydell, Michael S . Tsirkin,
Marcel Apfelbaum, Alex Williamson, Cédric Le Goater,
Philippe Mathieu-Daudé,
Paolo Bonzini, Michael Tokarev, qemu-devel, qemu-arm,
qemu-trivial, Zhao Liu
> No need to quote the dereferences in full.
>
> But in cxl_fixed_memory_window_config(), @errp is dereferenced in 2
> places without ERRP_GUARD().
>
> In these two places, we check "*errp", because neither function
>
> would do.
>
> Same for the other commit messages.
>
> Hardly worth a respin, though :)
>
I over-understood your reply. I'll pay attention to that in the follow
up.
Thanks,
Zhao
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 0/7] Cleanup and fix @errp dereference
2024-02-23 8:56 [PATCH v2 0/7] Cleanup and fix @errp dereference Zhao Liu
` (6 preceding siblings ...)
2024-02-23 8:56 ` [PATCH v2 7/7] hw/intc: Check @errp to handle the error of IOAPICCommonClass.realize() Zhao Liu
@ 2024-03-11 3:51 ` Zhao Liu
2024-03-12 16:43 ` Michael S. Tsirkin
7 siblings, 1 reply; 18+ messages in thread
From: Zhao Liu @ 2024-03-11 3:51 UTC (permalink / raw)
To: Michael Tokarev, Philippe Mathieu-Daudé
Cc: Jonathan Cameron, Fan Ni, Laurent Vivier, Alistair Francis,
Edgar E . Iglesias, Peter Maydell, Michael S . Tsirkin,
Marcel Apfelbaum, Alex Williamson, Cédric Le Goater,
Philippe Mathieu-Daudé,
Markus Armbruster, Paolo Bonzini, Michael Tokarev, qemu-devel,
qemu-arm, qemu-trivial, Zhao Liu
Hi Michael & Philippe,
Could this series catch the last train of next releases? ;-)
Thanks,
Zhao
On Fri, Feb 23, 2024 at 04:56:46PM +0800, Zhao Liu wrote:
> Date: Fri, 23 Feb 2024 16:56:46 +0800
> From: Zhao Liu <zhao1.liu@linux.intel.com>
> Subject: [PATCH v2 0/7] Cleanup and fix @errp dereference
> X-Mailer: git-send-email 2.34.1
>
> From: Zhao Liu <zhao1.liu@intel.com>
>
> Hi all,
>
> This is my v2 series to clean and fix @errp dereference.
>
> Introduction
> ============
>
> The patches 1-6 fix the cases that deference @errp without
> ERRP_GUARD(), and they are based on my previsous v1 [1].
>
> The patch 7 is merged in this series from another single patch [2].
>
> Based on v1, v2 mainly add more description about problematic codes to
> make review easier (with Markus' suggestion) and avoid dereferencing
> @errp in patch 3's special case.
>
>
> Future Work
> ===========
>
> Additionally, I realized that my journey to fix @errp with ERRP_GUARD()
> is still not over, because @errp's second restriction said:
>
> * Without ERRP_GUARD(), use of the @errp parameter is restricted:
> ...
> * - It should not be passed to error_prepend() or
> * error_append_hint(), because that doesn't work with &error_fatal.
> * ERRP_GUARD() lifts these restrictions.
>
> With this restrication, there're still many places need to add missing
> ERRP_GUARD().
>
> [1]: https://lore.kernel.org/qemu-devel/20240221094317.994454-1-zhao1.liu@linux.intel.com/
> [2]: https://lore.kernel.org/qemu-devel/20240221073948.768828-1-zhao1.liu@linux.intel.com/
>
> Thanks and Best Regards,
> Zhao
>
> ---
> Zhao Liu (7):
> hw/cxl/cxl-host: Fix missing ERRP_GUARD() in
> cxl_fixed_memory_window_config()
> hw/display/macfb: Fix missing ERRP_GUARD() in macfb_nubus_realize()
> hw/mem/cxl_type3: Fix missing ERRP_GUARD() in ct3_realize()
> hw/misc/xlnx-versal-trng: Check returned bool in
> trng_prop_fault_event_set()
> hw/pci-bridge/cxl_upstream: Fix missing ERRP_GUARD() in
> cxl_usp_realize()
> hw/vfio/iommufd: Fix missing ERRP_GUARD() in iommufd_cdev_getfd()
> hw/intc: Check @errp to handle the error of
> IOAPICCommonClass.realize()
>
> hw/cxl/cxl-host.c | 1 +
> hw/display/macfb.c | 1 +
> hw/intc/ioapic_common.c | 4 ++++
> hw/mem/cxl_type3.c | 1 +
> hw/misc/xlnx-versal-trng.c | 3 +--
> hw/pci-bridge/cxl_upstream.c | 1 +
> hw/vfio/iommufd.c | 1 +
> 7 files changed, 10 insertions(+), 2 deletions(-)
>
> --
> 2.34.1
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 5/7] hw/pci-bridge/cxl_upstream: Fix missing ERRP_GUARD() in cxl_usp_realize()
2024-02-23 8:56 ` [PATCH v2 5/7] hw/pci-bridge/cxl_upstream: Fix missing ERRP_GUARD() in cxl_usp_realize() Zhao Liu
2024-02-26 16:07 ` Jonathan Cameron via
@ 2024-03-12 10:39 ` Thomas Huth
1 sibling, 0 replies; 18+ messages in thread
From: Thomas Huth @ 2024-03-12 10:39 UTC (permalink / raw)
To: Zhao Liu, Jonathan Cameron, Fan Ni, Laurent Vivier,
Alistair Francis, Edgar E . Iglesias, Peter Maydell,
Michael S . Tsirkin, Marcel Apfelbaum, Alex Williamson,
Cédric Le Goater, Philippe Mathieu-Daudé,
Markus Armbruster, Paolo Bonzini, Michael Tokarev
Cc: qemu-devel, qemu-arm, qemu-trivial, Zhao Liu
On 23/02/2024 09.56, Zhao Liu wrote:
> From: Zhao Liu <zhao1.liu@intel.com>
>
> As the comment in qapi/error, dereferencing @errp requires
> ERRP_GUARD():
>
> * = Why, when and how to use ERRP_GUARD() =
> *
> * Without ERRP_GUARD(), use of the @errp parameter is restricted:
> * - It must not be dereferenced, because it may be null.
> ...
> * ERRP_GUARD() lifts these restrictions.
> *
> * To use ERRP_GUARD(), add it right at the beginning of the function.
> * @errp can then be used without worrying about the argument being
> * NULL or &error_fatal.
> *
> * Using it when it's not needed is safe, but please avoid cluttering
> * the source with useless code.
>
> But in cxl_usp_realize(), @errp is dereferenced without ERRP_GUARD():
>
> cxl_doe_cdat_init(cxl_cstate, errp);
> if (*errp) {
> goto err_cap;
> }
>
> Here we check *errp, because cxl_doe_cdat_init() returns void. And since
> cxl_usp_realize() - as a PCIDeviceClass.realize() method - doesn't get
> the NULL @errp parameter, it hasn't triggered the bug that dereferencing
> the NULL @errp.
>
> To follow the requirement of @errp, add missing ERRP_GUARD() in
> cxl_usp_realize().
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> ---
> Suggested by credit:
> Markus: Referred his explanation about ERRP_GUARD().
> ---
> v2:
> * Add the @errp dereference code in commit message to make review
> easier. (Markus)
> ---
> hw/pci-bridge/cxl_upstream.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/hw/pci-bridge/cxl_upstream.c b/hw/pci-bridge/cxl_upstream.c
> index e87eb4017713..03d123cca0ef 100644
> --- a/hw/pci-bridge/cxl_upstream.c
> +++ b/hw/pci-bridge/cxl_upstream.c
> @@ -289,6 +289,7 @@ static void free_default_cdat_table(CDATSubHeader **cdat_table, int num,
>
> static void cxl_usp_realize(PCIDevice *d, Error **errp)
> {
> + ERRP_GUARD();
> PCIEPort *p = PCIE_PORT(d);
> CXLUpstreamPort *usp = CXL_USP(d);
> CXLComponentState *cxl_cstate = &usp->cxl_cstate;
Reviewed-by: Thomas Huth <thuth@redhat.com>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 0/7] Cleanup and fix @errp dereference
2024-03-11 3:51 ` [PATCH v2 0/7] Cleanup and fix @errp dereference Zhao Liu
@ 2024-03-12 16:43 ` Michael S. Tsirkin
0 siblings, 0 replies; 18+ messages in thread
From: Michael S. Tsirkin @ 2024-03-12 16:43 UTC (permalink / raw)
To: Zhao Liu
Cc: Michael Tokarev, Philippe Mathieu-Daudé,
Jonathan Cameron, Fan Ni, Laurent Vivier, Alistair Francis,
Edgar E . Iglesias, Peter Maydell, Marcel Apfelbaum,
Alex Williamson, Cédric Le Goater, Markus Armbruster,
Paolo Bonzini, qemu-devel, qemu-arm, qemu-trivial, Zhao Liu
On Mon, Mar 11, 2024 at 11:51:11AM +0800, Zhao Liu wrote:
> Hi Michael & Philippe,
>
> Could this series catch the last train of next releases? ;-)
>
> Thanks,
> Zhao
yes it will be in subject to last minute testing.
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2024-03-12 16:45 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-23 8:56 [PATCH v2 0/7] Cleanup and fix @errp dereference Zhao Liu
2024-02-23 8:56 ` [PATCH v2 1/7] hw/cxl/cxl-host: Fix missing ERRP_GUARD() in cxl_fixed_memory_window_config() Zhao Liu
2024-02-26 16:07 ` Jonathan Cameron via
2024-02-28 7:30 ` Markus Armbruster
2024-02-28 16:09 ` Zhao Liu
2024-02-23 8:56 ` [PATCH v2 2/7] hw/display/macfb: Fix missing ERRP_GUARD() in macfb_nubus_realize() Zhao Liu
2024-02-23 8:56 ` [PATCH v2 3/7] hw/mem/cxl_type3: Fix missing ERRP_GUARD() in ct3_realize() Zhao Liu
2024-02-26 16:07 ` Jonathan Cameron via
2024-02-23 8:56 ` [PATCH v2 4/7] hw/misc/xlnx-versal-trng: Check returned bool in trng_prop_fault_event_set() Zhao Liu
2024-02-23 17:53 ` Philippe Mathieu-Daudé
2024-02-23 8:56 ` [PATCH v2 5/7] hw/pci-bridge/cxl_upstream: Fix missing ERRP_GUARD() in cxl_usp_realize() Zhao Liu
2024-02-26 16:07 ` Jonathan Cameron via
2024-03-12 10:39 ` Thomas Huth
2024-02-23 8:56 ` [PATCH v2 6/7] hw/vfio/iommufd: Fix missing ERRP_GUARD() in iommufd_cdev_getfd() Zhao Liu
2024-02-23 8:56 ` [PATCH v2 7/7] hw/intc: Check @errp to handle the error of IOAPICCommonClass.realize() Zhao Liu
2024-02-23 17:52 ` Philippe Mathieu-Daudé
2024-03-11 3:51 ` [PATCH v2 0/7] Cleanup and fix @errp dereference Zhao Liu
2024-03-12 16:43 ` Michael S. Tsirkin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.