diff mbox series

[3/6] hw/mem/cxl_type3: Fix missing ERRP_GUARD() in ct3_realize()

Message ID 20240221094317.994454-4-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 ct3_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
ct3_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/mem/cxl_type3.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Zhao Liu Feb. 21, 2024, 3:11 p.m. UTC | #1 
On Wed, Feb 21, 2024 at 12:35:47PM +0100, Markus Armbruster wrote:
> Date: Wed, 21 Feb 2024 12:35:47 +0100
> From: Markus Armbruster <armbru@redhat.com>
> Subject: Re: [PATCH 3/6] hw/mem/cxl_type3: Fix missing ERRP_GUARD() in
>  ct3_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 ct3_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
> > ct3_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/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;
> 
> The dereference is
> 
>        cxl_doe_cdat_init(cxl_cstate, errp);
>        if (*errp) {
>            goto err_free_special_ops;
>        }
> 
> 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! 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:53 p.m. UTC | #2 
On Wed, 21 Feb 2024 23:11:12 +0800
Zhao Liu <zhao1.liu@linux.intel.com> wrote:

> On Wed, Feb 21, 2024 at 12:35:47PM +0100, Markus Armbruster wrote:
> > Date: Wed, 21 Feb 2024 12:35:47 +0100
> > From: Markus Armbruster <armbru@redhat.com>
> > Subject: Re: [PATCH 3/6] hw/mem/cxl_type3: Fix missing ERRP_GUARD() in
> >  ct3_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 ct3_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
> > > ct3_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/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;  
> > 
> > The dereference is
> > 
> >        cxl_doe_cdat_init(cxl_cstate, errp);
> >        if (*errp) {
> >            goto err_free_special_ops;
> >        }
> > 
> > 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! 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.
> 
Agreed cleanup would be good, but this is fine if you don't want to
do that now.

Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> Regards,
> Zhao
> 
>
diff mbox series

Patch

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;