Re: [PATCH] NetworkPkg/DpcDxe: Update the consequent logic when DpcEntry is NULL

["Zhang, Shenglei" <shenglei.zhang@intel.com>]

> -----Original Message-----
> From: Fu, Siyuan
> Sent: Saturday, October 12, 2019 3:59 PM
> To: Zhang, Shenglei <shenglei.zhang@intel.com>; devel@edk2.groups.io
> Cc: Wu, Jiaxin <jiaxin.wu@intel.com>
> Subject: RE: [PATCH] NetworkPkg/DpcDxe: Update the consequent logic
> when DpcEntry is NULL
> 
> > -----Original Message-----
> > From: Zhang, Shenglei <shenglei.zhang@intel.com>
> > Sent: 2019年10月12日 15:43
> > To: devel@edk2.groups.io
> > Cc: Fu, Siyuan <siyuan.fu@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>
> > Subject: [PATCH] NetworkPkg/DpcDxe: Update the consequent logic when
> > DpcEntry is NULL
> >
> > If DpcEntry is NULL, it means it failed to be allocated space.
> > ReturnStatus should be EFI_OUT_OF_RESOURCES regardless of the
> > content of mDpcEntryFreeList.
> >
> > Cc: Siyuan Fu <siyuan.fu@intel.com>
> > Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> > Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
> > ---
> >  NetworkPkg/DpcDxe/Dpc.c | 9 +++------
> >  1 file changed, 3 insertions(+), 6 deletions(-)
> >
> > diff --git a/NetworkPkg/DpcDxe/Dpc.c b/NetworkPkg/DpcDxe/Dpc.c
> > index 8a490949dc8c..ebdb978b254a 100644
> > --- a/NetworkPkg/DpcDxe/Dpc.c
> > +++ b/NetworkPkg/DpcDxe/Dpc.c
> > @@ -137,14 +137,11 @@ DpcQueueDpc (
> >        gBS->RaiseTPL (TPL_HIGH_LEVEL);
> >
> >        //
> > -      // If the allocation of a DPC entry fails, and the free list is empty,
> > -      // then return EFI_OUT_OF_RESOURCES.
> > +      // If the allocation of a DPC entry fails, return EFI_OUT_OF_RESOURCES.
> >        //
> >        if (DpcEntry == NULL) {
> > -        if (IsListEmpty (&mDpcEntryFreeList)) {
> > -          ReturnStatus = EFI_OUT_OF_RESOURCES;
> > -          goto Done;
> > -        }
> > +        ReturnStatus = EFI_OUT_OF_RESOURCES;
> > +        goto Done;
> 
> I don't think it's a correct fix. This DpcEntry allocation is inside a for loop which
> tries to allocate 64 new DPC entries, if one of the allocation in the middle of
> this loop failed, the mDpcEntryFreeList will contain useable entries for this
> DpcQueueDpc(), that's why original code doesn't treat it as an error.
> 
> How do you find this issue? By code review or it cause some real problems?
> 
It is my local tool that reported this issue. You are right so I will find another method to
fix this.

Thanks,
Shenglei

> Thanks
> Siyuan
> 
> >        }
> >
> >        //
> > --
> > 2.18.0.windows.1