diff mbox series

[5/6] hw/pci-bridge/cxl_upstream: Fix missing ERRP_GUARD() in cxl_usp_realize()

Message ID 20240221094317.994454-6-zhao1.liu@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series Fix missing ERRP_GUARD() when dereference @errp | expand

Commit Message

Zhao Liu Feb. 21, 2024, 9:43 a.m. UTC 
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.
* - 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.
*
* 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.

Currently, since cxl_usp_realize() - as a PCIDeviceClass.realize()
method - doesn't get the NULL errp parameter, it doesn't trigger the
dereference issue.

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>
---
Suggested by credit:
 Markus: Referred his explanation about ERRP_GUARD().
---
 hw/pci-bridge/cxl_upstream.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Markus Armbruster Feb. 21, 2024, 11:49 a.m. UTC | #1 
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.
> * - 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.
> *
> * 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.
>
> Currently, since cxl_usp_realize() - as a PCIDeviceClass.realize()
> method - doesn't get the NULL errp parameter, it doesn't trigger the
> dereference issue.
>
> 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>
> ---
> Suggested by credit:
>  Markus: Referred his explanation about ERRP_GUARD().
> ---
>  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;

The dereference is

       cxl_doe_cdat_init(cxl_cstate, errp);
       if (*errp) {
           goto err_cap;
       }

As noted in review of PATCH 3, we check *errp, because
cxl_doe_cdat_init() returns void.  Could be improved to return bool,
along with its callees ct3_load_cdat() and ct3_build_cdat(), but that's
a slightly more ambitious cleanup, so

Reviewed-by: Markus Armbruster <armbru@redhat.com>
Zhao Liu Feb. 21, 2024, 3:09 p.m. UTC | #2 
On Wed, Feb 21, 2024 at 12:49:46PM +0100, Markus Armbruster wrote:
> Date: Wed, 21 Feb 2024 12:49:46 +0100
> From: Markus Armbruster <armbru@redhat.com>
> Subject: Re: [PATCH 5/6] hw/pci-bridge/cxl_upstream: Fix missing
>  ERRP_GUARD() in cxl_usp_realize()
> 
> 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.
> > * - 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.
> > *
> > * 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.
> >
> > Currently, since cxl_usp_realize() - as a PCIDeviceClass.realize()
> > method - doesn't get the NULL errp parameter, it doesn't trigger the
> > dereference issue.
> >
> > 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>
> > ---
> > Suggested by credit:
> >  Markus: Referred his explanation about ERRP_GUARD().
> > ---
> >  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;
> 
> The dereference is
> 
>        cxl_doe_cdat_init(cxl_cstate, errp);
>        if (*errp) {
>            goto err_cap;
>        }
> 
> As noted in review of PATCH 3, we check *errp, because
> cxl_doe_cdat_init() returns void.  Could be improved to return bool,
> along with its callees ct3_load_cdat() and ct3_build_cdat(), but that's
> a slightly more ambitious cleanup, so
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>

Thanks! It's a good idea and I can continue to consider such the cleanup
in the follow up.

Will also add the dereference description in commit message to make
review easier.

Regards,
Zhao
Jonathan Cameron Feb. 26, 2024, 3:54 p.m. UTC | #3 
On Wed, 21 Feb 2024 23:09:49 +0800
Zhao Liu <zhao1.liu@linux.intel.com> wrote:

> On Wed, Feb 21, 2024 at 12:49:46PM +0100, Markus Armbruster wrote:
> > Date: Wed, 21 Feb 2024 12:49:46 +0100
> > From: Markus Armbruster <armbru@redhat.com>
> > Subject: Re: [PATCH 5/6] hw/pci-bridge/cxl_upstream: Fix missing
> >  ERRP_GUARD() in cxl_usp_realize()
> > 
> > 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.
> > > * - 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.
> > > *
> > > * 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.
> > >
> > > Currently, since cxl_usp_realize() - as a PCIDeviceClass.realize()
> > > method - doesn't get the NULL errp parameter, it doesn't trigger the
> > > dereference issue.
> > >
> > > 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>
> > > ---
> > > Suggested by credit:
> > >  Markus: Referred his explanation about ERRP_GUARD().
> > > ---
> > >  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;  
> > 
> > The dereference is
> > 
> >        cxl_doe_cdat_init(cxl_cstate, errp);
> >        if (*errp) {
> >            goto err_cap;
> >        }
> > 
> > As noted in review of PATCH 3, we check *errp, because
> > cxl_doe_cdat_init() returns void.  Could be improved to return bool,
> > along with its callees ct3_load_cdat() and ct3_build_cdat(), but that's
> > a slightly more ambitious cleanup, so
> > 
> > Reviewed-by: Markus Armbruster <armbru@redhat.com>
> >  
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>


> 
> Thanks! It's a good idea and I can continue to consider such the cleanup
> in the follow up.
> 
> Will also add the dereference description in commit message to make
> review easier.
> 
> Regards,
> Zhao
>
diff mbox series

Patch

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;