1. Patchew
  2. EDK2
  3. View series

[edk2-devel] [PATCH] NetworkPkg/DpcDxe: Update the consequent logic when DpcEntry is NULL

Zhang, Shenglei posted 1 patch 4 years, 6 months ago
Failed in applying to current master (apply log)
NetworkPkg/DpcDxe/Dpc.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
[edk2-devel] [PATCH] NetworkPkg/DpcDxe: Update the consequent logic when DpcEntry is NULL
Posted by Zhang, Shenglei 4 years, 6 months ago 
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;
       }
 
       //
-- 
2.18.0.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48862): https://edk2.groups.io/g/devel/message/48862
Mute This Topic: https://groups.io/mt/34508854/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] NetworkPkg/DpcDxe: Update the consequent logic when DpcEntry is NULL
Posted by Siyuan, Fu 4 years, 6 months ago 
> -----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?

Thanks
Siyuan

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48866): https://edk2.groups.io/g/devel/message/48866
Mute This Topic: https://groups.io/mt/34508854/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] NetworkPkg/DpcDxe: Update the consequent logic when DpcEntry is NULL
Posted by Zhang, Shenglei 4 years, 6 months ago 

> -----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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48869): https://edk2.groups.io/g/devel/message/48869
Mute This Topic: https://groups.io/mt/34508854/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH] NetworkPkg/DxeNetLib: Change the order of conditions in IF statement
Posted by Zhang, Shenglei 4 years, 6 months ago 
The condition, NET_HEADSPACE(&(Nbuf->BlockOp[Index])) < Len, is meaningless
if Index < 0. So 'Index < 0' should be performed first in the if statement.

Cc: Siyuan Fu <siyuan.fu@intel.com>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
---
 NetworkPkg/Library/DxeNetLib/NetBuffer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/NetworkPkg/Library/DxeNetLib/NetBuffer.c b/NetworkPkg/Library/DxeNetLib/NetBuffer.c
index 2408e9a10456..a35e67aa1f6c 100644
--- a/NetworkPkg/Library/DxeNetLib/NetBuffer.c
+++ b/NetworkPkg/Library/DxeNetLib/NetBuffer.c
@@ -1063,7 +1063,7 @@ NetbufAllocSpace (
     } else {
       NetbufGetByte (Nbuf, 0, &Index);
 
-      if ((NET_HEADSPACE(&(Nbuf->BlockOp[Index])) < Len) && (Index > 0)) {
+      if ((Index > 0) && (NET_HEADSPACE(&(Nbuf->BlockOp[Index])) < Len)) {
         Index--;
       }
     }
-- 
2.18.0.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48863): https://edk2.groups.io/g/devel/message/48863
Mute This Topic: https://groups.io/mt/34508855/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] NetworkPkg/DxeNetLib: Change the order of conditions in IF statement
Posted by Siyuan, Fu 4 years, 6 months ago 
Reviewed-by: Siyuan Fu <siyuan.fu@intel.com>

> -----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/DxeNetLib: Change the order of conditions in
> IF statement
> 
> The condition, NET_HEADSPACE(&(Nbuf->BlockOp[Index])) < Len, is
> meaningless
> if Index < 0. So 'Index < 0' should be performed first in the if statement.
> 
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
> ---
>  NetworkPkg/Library/DxeNetLib/NetBuffer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/NetworkPkg/Library/DxeNetLib/NetBuffer.c
> b/NetworkPkg/Library/DxeNetLib/NetBuffer.c
> index 2408e9a10456..a35e67aa1f6c 100644
> --- a/NetworkPkg/Library/DxeNetLib/NetBuffer.c
> +++ b/NetworkPkg/Library/DxeNetLib/NetBuffer.c
> @@ -1063,7 +1063,7 @@ NetbufAllocSpace (
>      } else {
>        NetbufGetByte (Nbuf, 0, &Index);
> 
> -      if ((NET_HEADSPACE(&(Nbuf->BlockOp[Index])) < Len) && (Index > 0)) {
> +      if ((Index > 0) && (NET_HEADSPACE(&(Nbuf->BlockOp[Index])) < Len)) {
>          Index--;
>        }
>      }
> --
> 2.18.0.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48877): https://edk2.groups.io/g/devel/message/48877
Mute This Topic: https://groups.io/mt/34508855/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] NetworkPkg/DxeNetLib: Change the order of conditions in IF statement
Posted by Philippe Mathieu-Daudé 4 years, 6 months ago 
Hi Zhang,

On 10/12/19 9:43 AM, Zhang, Shenglei wrote:
> The condition, NET_HEADSPACE(&(Nbuf->BlockOp[Index])) < Len, is meaningless
> if Index < 0. So 'Index < 0' should be performed first in the if statement.
> 
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
> ---
>   NetworkPkg/Library/DxeNetLib/NetBuffer.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/NetworkPkg/Library/DxeNetLib/NetBuffer.c b/NetworkPkg/Library/DxeNetLib/NetBuffer.c
> index 2408e9a10456..a35e67aa1f6c 100644
> --- a/NetworkPkg/Library/DxeNetLib/NetBuffer.c
> +++ b/NetworkPkg/Library/DxeNetLib/NetBuffer.c
> @@ -1063,7 +1063,7 @@ NetbufAllocSpace (
>       } else {
>         NetbufGetByte (Nbuf, 0, &Index);
>   
> -      if ((NET_HEADSPACE(&(Nbuf->BlockOp[Index])) < Len) && (Index > 0)) {
> +      if ((Index > 0) && (NET_HEADSPACE(&(Nbuf->BlockOp[Index])) < Len)) {

I'm not sure this is correct. Index is unsigned, so it won't be 
negative. NetbufGetByte() can set Index=0. With your change this case is 
not covered anymore.

Maybe your tool is unhappy because the return value of NetbufGetByte() 
isn't checked?

>           Index--;
>         }
>       }
> 

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48886): https://edk2.groups.io/g/devel/message/48886
Mute This Topic: https://groups.io/mt/34508855/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.