[Patch 0/2] Fix bug in IP driver for IpSec protocol notify

[Patch 0/2] Fix bug in IP driver for IpSec protocol notify

[Fu Siyuan]

Fu Siyuan (2):
  MdeModulePkg/Ip4Dxe: fix a bug in IP4 driver for IpSec protocol
    notify.
  NetworkPkg/Ip6Dxe: fix a bug in IP6 driver for IpSec protocol notify.

 MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Driver.c | 14 +++++++++++---
 MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Input.c  | 10 ++--------
 NetworkPkg/Ip6Dxe/Ip6Driver.c                     | 18 ++++++++++++------
 NetworkPkg/Ip6Dxe/Ip6Input.c                      | 14 ++------------
 4 files changed, 27 insertions(+), 29 deletions(-)

-- 
1.9.5.msysgit.1

[Patch 1/2] MdeModulePkg/Ip4Dxe: fix a bug in IP4 driver for IpSec protocol notify.

[Fu Siyuan]

The IP driver uses EfiCreateProtocolNotifyEvent() to register notify callback
function for IpSec protocol, but it didn't notice that the callback will always
be executed at least once, even the protocol wasn't in handle database.
As a result, the Ip4IpSecProcessPacket() will still always call LocateProtocol()
even the IpSec protocol is not installed, which will impact the network
performance.

Cc: Ye Ting <ting.ye@intel.com>
Cc: Wu Jiaxin <jiaxin.wu@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Fu Siyuan <siyuan.fu@intel.com>
---
 MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Driver.c | 14 +++++++++++---
 MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Input.c  | 10 ++--------
 2 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Driver.c b/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Driver.c
index 792db5c..03ba458 100644
--- a/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Driver.c
+++ b/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Driver.c
@@ -41,12 +41,20 @@ IpSec2InstalledCallback (
   IN VOID       *Context
   )
 {
+  EFI_STATUS    Status;
   //
-  // Close the event so it does not get called again.
+  // Test if protocol was even found.
+  // Notification function will be called at least once.
   //
-  gBS->CloseEvent (Event);
+  Status = gBS->LocateProtocol (&gEfiIpSec2ProtocolGuid, NULL, &mIpSec);
+  if (Status == EFI_SUCCESS && mIpSec != NULL) {
+    //
+    // Close the event so it does not get called again.
+    //
+    gBS->CloseEvent (Event);
 
-  mIpSec2Installed = TRUE;
+    mIpSec2Installed = TRUE;
+  }
 }
 
 /**
diff --git a/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Input.c b/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Input.c
index 09b8f2b..e694323 100644
--- a/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Input.c
+++ b/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Input.c
@@ -1,7 +1,7 @@
 /** @file
   IP4 input process.
   
-Copyright (c) 2005 - 2014, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2005 - 2017, Intel Corporation. All rights reserved.<BR>
 (C) Copyright 2015 Hewlett-Packard Development Company, L.P.<BR>
 
 This program and the accompanying materials
@@ -518,6 +518,7 @@ Ip4IpSecProcessPacket (
   if (!mIpSec2Installed) {
     goto ON_EXIT;
   }
+  ASSERT (mIpSec != NULL);
   
   Packet        = *Netbuf;
   RecycleEvent  = NULL;
@@ -527,13 +528,6 @@ Ip4IpSecProcessPacket (
   FragmentCount = Packet->BlockOpNum;
 
   ZeroMem (&ZeroHead, sizeof (IP4_HEAD));
-  
-  if (mIpSec == NULL) {
-    gBS->LocateProtocol (&gEfiIpSec2ProtocolGuid, NULL, (VOID **) &mIpSec);
-    if (mIpSec == NULL) {
-      goto ON_EXIT;
-    }
-  }
 
   //
   // Check whether the IPsec enable variable is set.
-- 
1.9.5.msysgit.1

[Patch 2/2] NetworkPkg/Ip6Dxe: fix a bug in IP6 driver for IpSec protocol notify.

[Fu Siyuan]

The IP driver uses EfiCreateProtocolNotifyEvent() to register notify callback
function for IpSec protocol, but it didn't notice that the callback will always
be executed at least once, even the protocol wasn't in handle database.
As a result, the Ip6IpSecProcessPacket() will still always call LocateProtocol()
even the IpSec protocol is not installed, which will impact the network
performance.

Cc: Ye Ting <ting.ye@intel.com>
Cc: Wu Jiaxin <jiaxin.wu@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Fu Siyuan <siyuan.fu@intel.com>
---
 NetworkPkg/Ip6Dxe/Ip6Driver.c | 18 ++++++++++++------
 NetworkPkg/Ip6Dxe/Ip6Input.c  | 14 ++------------
 2 files changed, 14 insertions(+), 18 deletions(-)

diff --git a/NetworkPkg/Ip6Dxe/Ip6Driver.c b/NetworkPkg/Ip6Dxe/Ip6Driver.c
index 8a8cc89..43af838 100644
--- a/NetworkPkg/Ip6Dxe/Ip6Driver.c
+++ b/NetworkPkg/Ip6Dxe/Ip6Driver.c
@@ -1,7 +1,7 @@
 /** @file
   The driver binding and service binding protocol for IP6 driver.
 
-  Copyright (c) 2009 - 2016, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2009 - 2017, Intel Corporation. All rights reserved.<BR>
   (C) Copyright 2015 Hewlett-Packard Development Company, L.P.<BR>
 
   This program and the accompanying materials
@@ -41,14 +41,20 @@ IpSec2InstalledCallback (
   IN VOID       *Context
   )
 {
+  EFI_STATUS    Status;
   //
-  // Close the event so it does not get called again.
+  // Test if protocol was even found.
+  // Notification function will be called at least once.
   //
-  gBS->CloseEvent (Event);
-
-  mIpSec2Installed = TRUE;
+  Status = gBS->LocateProtocol (&gEfiIpSec2ProtocolGuid, NULL, &mIpSec);
+  if (Status == EFI_SUCCESS && mIpSec != NULL) {
+    //
+    // Close the event so it does not get called again.
+    //
+    gBS->CloseEvent (Event);
 
-  return;
+    mIpSec2Installed = TRUE;
+  }
 }
 
 /**
diff --git a/NetworkPkg/Ip6Dxe/Ip6Input.c b/NetworkPkg/Ip6Dxe/Ip6Input.c
index e53e087..6aa5555 100644
--- a/NetworkPkg/Ip6Dxe/Ip6Input.c
+++ b/NetworkPkg/Ip6Dxe/Ip6Input.c
@@ -1,7 +1,7 @@
 /** @file
   IP6 internal functions to process the incoming packets.
 
-  Copyright (c) 2009 - 2014, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2009 - 2017, Intel Corporation. All rights reserved.<BR>
   (C) Copyright 2015 Hewlett-Packard Development Company, L.P.<BR>
 
   This program and the accompanying materials
@@ -530,6 +530,7 @@ Ip6IpSecProcessPacket (
   if (!mIpSec2Installed) {
     goto ON_EXIT;
   }
+  ASSERT (mIpSec != NULL);
   
   Packet        = *Netbuf;
   RecycleEvent  = NULL;
@@ -541,17 +542,6 @@ Ip6IpSecProcessPacket (
   FragmentCount = Packet->BlockOpNum;
   ZeroMem (&ZeroHead, sizeof (EFI_IP6_HEADER));
 
-  if (mIpSec == NULL) {
-    gBS->LocateProtocol (&gEfiIpSec2ProtocolGuid, NULL, (VOID **) &mIpSec);
-
-    //
-    // Check whether the ipsec protocol is available.
-    //
-    if (mIpSec == NULL) {
-      goto ON_EXIT;
-    }
-  }
-
   //
   // Check whether the ipsec enable variable is set.
   //
-- 
1.9.5.msysgit.1

Re: [Patch 0/2] Fix bug in IP driver for IpSec protocol notify

[Ye, Ting]

Looks good to me. Series Reviewed-by: Ye Ting <ting.ye@intel.com> 

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Fu Siyuan
Sent: Monday, September 4, 2017 4:07 PM
To: edk2-devel@lists.01.org
Subject: [edk2] [Patch 0/2] Fix bug in IP driver for IpSec protocol notify

Fu Siyuan (2):
  MdeModulePkg/Ip4Dxe: fix a bug in IP4 driver for IpSec protocol
    notify.
  NetworkPkg/Ip6Dxe: fix a bug in IP6 driver for IpSec protocol notify.

 MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Driver.c | 14 +++++++++++---  MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Input.c  | 10 ++--------
 NetworkPkg/Ip6Dxe/Ip6Driver.c                     | 18 ++++++++++++------
 NetworkPkg/Ip6Dxe/Ip6Input.c                      | 14 ++------------
 4 files changed, 27 insertions(+), 29 deletions(-)

--
1.9.5.msysgit.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [Patch 1/2] MdeModulePkg/Ip4Dxe: fix a bug in IP4 driver for IpSec protocol notify.

[Ard Biesheuvel]

Hello Fu,

On 4 September 2017 at 09:07, Fu Siyuan <siyuan.fu@intel.com> wrote:
> The IP driver uses EfiCreateProtocolNotifyEvent() to register notify callback
> function for IpSec protocol, but it didn't notice that the callback will always
> be executed at least once, even the protocol wasn't in handle database.
> As a result, the Ip4IpSecProcessPacket() will still always call LocateProtocol()
> even the IpSec protocol is not installed, which will impact the network
> performance.
>
> Cc: Ye Ting <ting.ye@intel.com>
> Cc: Wu Jiaxin <jiaxin.wu@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Fu Siyuan <siyuan.fu@intel.com>
> ---
>  MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Driver.c | 14 +++++++++++---
>  MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Input.c  | 10 ++--------
>  2 files changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Driver.c b/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Driver.c
> index 792db5c..03ba458 100644
> --- a/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Driver.c
> +++ b/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Driver.c
> @@ -41,12 +41,20 @@ IpSec2InstalledCallback (
>    IN VOID       *Context
>    )
>  {
> +  EFI_STATUS    Status;
>    //
> -  // Close the event so it does not get called again.
> +  // Test if protocol was even found.
> +  // Notification function will be called at least once.
>    //
> -  gBS->CloseEvent (Event);
> +  Status = gBS->LocateProtocol (&gEfiIpSec2ProtocolGuid, NULL, &mIpSec);

While moving this call here from below, you removed the (void **)
cast, which breaks the build on GCC

 In function 'IpSec2InstalledCallback':
49:3: error: passing argument 3 of 'gBS->LocateProtocol' from
incompatible pointer type [-Werror]
   Status = gBS->LocateProtocol (&gEfiIpSec2ProtocolGuid, NULL, &mIpSec);
   ^
49:3: note: expected 'void **' but argument is of type 'struct
EFI_IPSEC2_PROTOCOL **'
cc1: all warnings being treated as errors

Please fix.

Kind regards,
Ard,


> +  if (Status == EFI_SUCCESS && mIpSec != NULL) {
> +    //
> +    // Close the event so it does not get called again.
> +    //
> +    gBS->CloseEvent (Event);
>
> -  mIpSec2Installed = TRUE;
> +    mIpSec2Installed = TRUE;
> +  }
>  }
>
>  /**
> diff --git a/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Input.c b/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Input.c
> index 09b8f2b..e694323 100644
> --- a/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Input.c
> +++ b/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Input.c
> @@ -1,7 +1,7 @@
>  /** @file
>    IP4 input process.
>
> -Copyright (c) 2005 - 2014, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2005 - 2017, Intel Corporation. All rights reserved.<BR>
>  (C) Copyright 2015 Hewlett-Packard Development Company, L.P.<BR>
>
>  This program and the accompanying materials
> @@ -518,6 +518,7 @@ Ip4IpSecProcessPacket (
>    if (!mIpSec2Installed) {
>      goto ON_EXIT;
>    }
> +  ASSERT (mIpSec != NULL);
>
>    Packet        = *Netbuf;
>    RecycleEvent  = NULL;
> @@ -527,13 +528,6 @@ Ip4IpSecProcessPacket (
>    FragmentCount = Packet->BlockOpNum;
>
>    ZeroMem (&ZeroHead, sizeof (IP4_HEAD));
> -
> -  if (mIpSec == NULL) {
> -    gBS->LocateProtocol (&gEfiIpSec2ProtocolGuid, NULL, (VOID **) &mIpSec);
> -    if (mIpSec == NULL) {
> -      goto ON_EXIT;
> -    }
> -  }
>
>    //
>    // Check whether the IPsec enable variable is set.
> --
> 1.9.5.msysgit.1
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel