Re: [PATCH V2] MdeModulePkg: Fix service binding issue in TCP4 and Ip4 dxe.

["Wu, Jiaxin" <jiaxin.wu@intel.com>]

For this patch, my comments as below:

1. The commit log should better cover the changes of Ip4Dxe but currently it's not. 

2. Intel Copyright in SockImpl.h  should be 2017, not 2007.

Other looks to me.

Reviewed-by: Wu Jiaxin <jiaxin.wu@intel.com>

Thanks,
Jiaxin


> -----Original Message-----
> From: Zhang, Lubo
> Sent: Wednesday, March 15, 2017 10:28 AM
> To: edk2-devel@lists.01.org
> Cc: Wu, Jiaxin <jiaxin.wu@intel.com>; Ye, Ting <ting.ye@intel.com>; Fu,
> Siyuan <siyuan.fu@intel.com>
> Subject: [PATCH V2] MdeModulePkg: Fix service binding issue in TCP4 and
> Ip4 dxe.
> 
> v2: Handle error case in SockCreateChild and fix typo issue
> 
> when we destroy the socket Sock and its associated
> protocol control block, we need to first close the
> parent protocol, then remove the protocol from childHandle
> and last to free any data structures that allocated in
> CreateChild. But currently, we free the socket data
> (Socket ConfigureState) before removing the protocol
> form  the childhandle. So if the up layer want to send the
> tcp reset packet in it's driver binding stop function, it will failed.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Zhang Lubo <lubo.zhang@intel.com>
> Cc: Wu Jiaxin <jiaxin.wu@intel.com>
> Cc: Ye Ting <ting.ye@intel.com>
> Cc: Fu Siyuan <siyuan.fu@intel.com>
> ---
>  MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Driver.c  | 10 +--
>  MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Impl.c    |  3 +-
>  MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Impl.h    |  5 +-
>  MdeModulePkg/Universal/Network/Tcp4Dxe/SockImpl.c  | 48 +-----------
>  MdeModulePkg/Universal/Network/Tcp4Dxe/SockImpl.h  |  3 +-
>  .../Universal/Network/Tcp4Dxe/SockInterface.c      | 88
> ++++++++++++++++++++--
>  .../Universal/Network/Tcp4Dxe/Tcp4Dispatcher.c     | 12 +--
>  7 files changed, 95 insertions(+), 74 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Driver.c
> b/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Driver.c
> index 642e453..792db5c 100644
> --- a/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Driver.c
> +++ b/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Driver.c
> @@ -1,9 +1,9 @@
>  /** @file
>    The driver binding and service binding protocol for IP4 driver.
> 
> -Copyright (c) 2005 - 2016, 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
>  are licensed and made available under the terms and conditions of the BSD
> License
>  which accompanies this distribution.  The full text of the license may be
> found at
> @@ -920,11 +920,10 @@ Ip4ServiceBindingDestroyChild (
>    EFI_STATUS                Status;
>    IP4_SERVICE               *IpSb;
>    IP4_PROTOCOL              *IpInstance;
>    EFI_IP4_PROTOCOL          *Ip4;
>    EFI_TPL                   OldTpl;
> -  INTN                      State;
> 
>    if ((This == NULL) || (ChildHandle == NULL)) {
>      return EFI_INVALID_PARAMETER;
>    }
> 
> @@ -958,17 +957,16 @@ Ip4ServiceBindingDestroyChild (
>    // A child can be destroyed more than once. For example,
>    // Ip4DriverBindingStop will destroy all of its children.
>    // when UDP driver is being stopped, it will destroy all
>    // the IP child it opens.
>    //
> -  if (IpInstance->State == IP4_STATE_DESTROY) {
> +  if (IpInstance->InDestroy) {
>      gBS->RestoreTPL (OldTpl);
>      return EFI_SUCCESS;
>    }
> 
> -  State             = IpInstance->State;
> -  IpInstance->State = IP4_STATE_DESTROY;
> +  IpInstance->InDestroy = TRUE;
> 
>    //
>    // Close the Managed Network protocol.
>    //
>    gBS->CloseProtocol (
> @@ -1007,10 +1005,11 @@ Ip4ServiceBindingDestroyChild (
>                    &gEfiIp4ProtocolGuid,
>                    &IpInstance->Ip4Proto
>                    );
>    OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
>    if (EFI_ERROR (Status)) {
> +    IpInstance->InDestroy = FALSE;
>      goto ON_ERROR;
>    }
> 
>    Status = Ip4CleanProtocol (IpInstance);
>    if (EFI_ERROR (Status)) {
> @@ -1031,10 +1030,9 @@ Ip4ServiceBindingDestroyChild (
> 
>    FreePool (IpInstance);
>    return EFI_SUCCESS;
> 
>  ON_ERROR:
> -  IpInstance->State = State;
>    gBS->RestoreTPL (OldTpl);
> 
>    return Status;
>  }
> diff --git a/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Impl.c
> b/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Impl.c
> index 91f1a67..23690ff 100644
> --- a/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Impl.c
> +++ b/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Impl.c
> @@ -1,8 +1,8 @@
>  /** @file
> 
> -Copyright (c) 2005 - 2016, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2005 - 2017, Intel Corporation. All rights reserved.<BR>
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD
> License
>  which accompanies this distribution.  The full text of the license may be
> found at
>  http://opensource.org/licenses/bsd-license.php
> 
> @@ -548,10 +548,11 @@ Ip4InitProtocol (
>    ZeroMem (IpInstance, sizeof (IP4_PROTOCOL));
> 
>    IpInstance->Signature = IP4_PROTOCOL_SIGNATURE;
>    CopyMem (&IpInstance->Ip4Proto, &mEfiIp4ProtocolTemplete, sizeof
> (IpInstance->Ip4Proto));
>    IpInstance->State     = IP4_STATE_UNCONFIGED;
> +  IpInstance->InDestroy   = FALSE;
>    IpInstance->Service   = IpSb;
> 
>    InitializeListHead (&IpInstance->Link);
>    NetMapInit  (&IpInstance->RxTokens);
>    NetMapInit  (&IpInstance->TxTokens);
> diff --git a/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Impl.h
> b/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Impl.h
> index 7a7ad9d..eee38b7 100644
> --- a/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Impl.h
> +++ b/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Impl.h
> @@ -1,9 +1,9 @@
>  /** @file
>    Ip4 internal functions and type defintions.
> 
> -Copyright (c) 2005 - 2016, 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
>  are licensed and made available under the terms and conditions of the BSD
> License
>  which accompanies this distribution.  The full text of the license may be
> found at
> @@ -67,11 +67,10 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF
> ANY KIND, EITHER EXPRESS OR IMPLIED.
>  // is called, it becomes UNCONFIGED again. If (partly) destroyed, it
>  // becomes DESTROY.
>  //
>  #define IP4_STATE_UNCONFIGED    0
>  #define IP4_STATE_CONFIGED      1
> -#define IP4_STATE_DESTROY       2
> 
>  //
>  // The state of IP4 service. It starts from UNSTARTED. It transits
>  // to STARTED if autoconfigure is started. If default address is
>  // configured, it becomes CONFIGED. and if partly destroyed, it goes
> @@ -134,10 +133,12 @@ struct _IP4_PROTOCOL {
> 
>    EFI_IP4_PROTOCOL          Ip4Proto;
>    EFI_HANDLE                Handle;
>    INTN                      State;
> 
> +  BOOLEAN                   InDestroy;
> +
>    IP4_SERVICE               *Service;
>    LIST_ENTRY                Link;       // Link to all the IP protocol from the service
> 
>    //
>    // User's transmit/receive tokens, and received/deliverd packets
> diff --git a/MdeModulePkg/Universal/Network/Tcp4Dxe/SockImpl.c
> b/MdeModulePkg/Universal/Network/Tcp4Dxe/SockImpl.c
> index fc78273..0476077 100644
> --- a/MdeModulePkg/Universal/Network/Tcp4Dxe/SockImpl.c
> +++ b/MdeModulePkg/Universal/Network/Tcp4Dxe/SockImpl.c
> @@ -1,9 +1,9 @@
>  /** @file
>    Implementation of the Socket.
> 
> -Copyright (c) 2005 - 2016, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2005 - 2017, Intel Corporation. All rights reserved.<BR>
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD
> License
>  which accompanies this distribution.  The full text of the license may be
> found at
>  http://opensource.org/licenses/bsd-license.php<BR>
> 
> @@ -715,20 +715,12 @@ OnError:
>  VOID
>  SockDestroy (
>    IN OUT SOCKET *Sock
>    )
>  {
> -  VOID        *SockProtocol;
> -  EFI_GUID    *ProtocolGuid;
> -  EFI_STATUS  Status;
> -
>    ASSERT (SockStream == Sock->Type);
> 
> -  if (Sock->DestroyCallback != NULL) {
> -    Sock->DestroyCallback (Sock, Sock->Context);
> -  }
> -
>    //
>    // Flush the completion token buffered
>    // by sock and rcv, snd buffer
>    //
>    if (!SOCK_IS_UNCONFIGURED (Sock)) {
> @@ -760,48 +752,10 @@ SockDestroy (
>        );
> 
>      Sock->Parent = NULL;
>    }
> 
> -  //
> -  // Set the protocol guid and driver binding handle
> -  // in the light of Sock->SockType
> -  //
> -  ProtocolGuid = &gEfiTcp4ProtocolGuid;
> -
> -  //
> -  // Retrieve the protocol installed on this sock
> -  //
> -  Status = gBS->OpenProtocol (
> -                  Sock->SockHandle,
> -                  ProtocolGuid,
> -                  &SockProtocol,
> -                  Sock->DriverBinding,
> -                  Sock->SockHandle,
> -                  EFI_OPEN_PROTOCOL_GET_PROTOCOL
> -                  );
> -
> -  if (EFI_ERROR (Status)) {
> -
> -    DEBUG ((EFI_D_ERROR, "SockDestroy: Open protocol installed "
> -      "on socket failed with %r\n", Status));
> -
> -    goto FreeSock;
> -  }
> -
> -  //
> -  // Uninstall the protocol installed on this sock
> -  // in the light of Sock->SockType
> -  //
> -  gBS->UninstallMultipleProtocolInterfaces (
> -        Sock->SockHandle,
> -        ProtocolGuid,
> -        SockProtocol,
> -        NULL
> -        );
> -
> -FreeSock:
>    FreePool (Sock);
>    return ;
>  }
> 
> 
> diff --git a/MdeModulePkg/Universal/Network/Tcp4Dxe/SockImpl.h
> b/MdeModulePkg/Universal/Network/Tcp4Dxe/SockImpl.h
> index 4a5a845..d4104a0 100644
> --- a/MdeModulePkg/Universal/Network/Tcp4Dxe/SockImpl.h
> +++ b/MdeModulePkg/Universal/Network/Tcp4Dxe/SockImpl.h
> @@ -1,9 +1,9 @@
>  /** @file
>    Socket implementation header file.
> 
> -Copyright (c) 2005 - 2006, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2005 - 2007, Intel Corporation. All rights reserved.<BR>
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD
> License
>  which accompanies this distribution.  The full text of the license may be
> found at
>  http://opensource.org/licenses/bsd-license.php<BR>
> 
> @@ -14,10 +14,11 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF
> ANY KIND, EITHER EXPRESS OR IMPLIED.
> 
>  #ifndef _SOCK_IMPL_H_
>  #define _SOCK_IMPL_H_
> 
>  #include "Socket.h"
> +#include "Tcp4Main.h"
> 
>  /**
>    Signal a event with the given status.
> 
>    @param Token        The token's event is to be signaled.
> diff --git a/MdeModulePkg/Universal/Network/Tcp4Dxe/SockInterface.c
> b/MdeModulePkg/Universal/Network/Tcp4Dxe/SockInterface.c
> index a8bdef6..f8b535c 100644
> --- a/MdeModulePkg/Universal/Network/Tcp4Dxe/SockInterface.c
> +++ b/MdeModulePkg/Universal/Network/Tcp4Dxe/SockInterface.c
> @@ -1,9 +1,9 @@
>  /** @file
>    Interface function of the Socket.
> 
> -Copyright (c) 2005 - 2016, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2005 - 2017, Intel Corporation. All rights reserved.<BR>
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD
> License
>  which accompanies this distribution.  The full text of the license may be
> found at
>  http://opensource.org/licenses/bsd-license.php<BR>
> 
> @@ -142,30 +142,81 @@ SockBufferToken (
>  EFI_STATUS
>  SockDestroyChild (
>    IN SOCKET *Sock
>    )
>  {
> -  EFI_STATUS  Status;
> +  EFI_STATUS       Status;
> +  TCP4_PROTO_DATA  *ProtoData;
> +  TCP_CB           *Tcb;
> +  VOID             *SockProtocol;
> 
>    ASSERT ((Sock != NULL) && (Sock->ProtoHandler != NULL));
> 
>    if (Sock->InDestroy) {
>      return EFI_SUCCESS;
>    }
> 
>    Sock->InDestroy = TRUE;
> 
> +  ProtoData = (TCP4_PROTO_DATA *) Sock->ProtoReserved;
> +  Tcb       = ProtoData->TcpPcb;
> +
> +  ASSERT (Tcb != NULL);
> +
>    Status = EfiAcquireLockOrFail (&(Sock->Lock));
>    if (EFI_ERROR (Status)) {
> 
>      DEBUG ((EFI_D_ERROR, "SockDestroyChild: Get the lock to "
>        "access socket failed with %r\n", Status));
> 
>      return EFI_ACCESS_DENIED;
>    }
> 
>    //
> +  // Close the IP protocol.
> +  //
> +  gBS->CloseProtocol (
> +         Tcb->IpInfo->ChildHandle,
> +         &gEfiIp4ProtocolGuid,
> +         ProtoData->TcpService->IpIo->Image,
> +         Sock->SockHandle
> +         );
> +
> +  if (Sock->DestroyCallback != NULL) {
> +    Sock->DestroyCallback (Sock, Sock->Context);
> +  }
> +
> +  //
> +  // Retrieve the protocol installed on this sock
> +  //
> +  Status = gBS->OpenProtocol (
> +                  Sock->SockHandle,
> +                  &gEfiTcp4ProtocolGuid,
> +                  &SockProtocol,
> +                  Sock->DriverBinding,
> +                  Sock->SockHandle,
> +                  EFI_OPEN_PROTOCOL_GET_PROTOCOL
> +                  );
> +
> +  if (EFI_ERROR (Status)) {
> +
> +    DEBUG ((EFI_D_ERROR, "SockDestroyChild: Open protocol installed "
> +      "on socket failed with %r\n", Status));
> +  }
> +
> +  //
> +  // Uninstall the protocol installed on this sock
> +  // in the light of Sock->SockType
> +  //
> +  gBS->UninstallMultipleProtocolInterfaces (
> +        Sock->SockHandle,
> +        &gEfiTcp4ProtocolGuid,
> +        SockProtocol,
> +        NULL
> +        );
> +
> +  //
>    // force protocol layer to detach the PCB
>    //
>    Status = Sock->ProtoHandler (Sock, SOCK_DETACH, NULL);
> 
>    if (EFI_ERROR (Status)) {
> @@ -207,10 +258,11 @@ SOCKET *
>  SockCreateChild (
>    IN SOCK_INIT_DATA *SockInitData
>    )
>  {
>    SOCKET      *Sock;
> +  VOID        *SockProtocol;
>    EFI_STATUS  Status;
> 
>    //
>    // create a new socket
>    //
> @@ -227,12 +279,11 @@ SockCreateChild (
>    if (EFI_ERROR (Status)) {
> 
>      DEBUG ((EFI_D_ERROR, "SockCreateChild: Get the lock to "
>        "access socket failed with %r\n", Status));
> 
> -    SockDestroy (Sock);
> -    return NULL;
> +    goto ERROR;
>    }
>    //
>    // inform the protocol layer to attach the socket
>    // with a new protocol control block
>    //
> @@ -241,15 +292,40 @@ SockCreateChild (
>    if (EFI_ERROR (Status)) {
> 
>      DEBUG ((EFI_D_ERROR, "SockCreateChild: Protocol failed to"
>        " attach a socket with %r\n", Status));
> 
> -    SockDestroy (Sock);
> -    Sock = NULL;
> +    goto ERROR;
>    }
> 
>    return Sock;
> +
> +ERROR:
> +
> +  if (Sock->DestroyCallback != NULL) {
> +    Sock->DestroyCallback (Sock, Sock->Context);
> +  }
> +
> +  gBS->OpenProtocol (
> +         Sock->SockHandle,
> +         &gEfiTcp4ProtocolGuid,
> +         &SockProtocol,
> +         Sock->DriverBinding,
> +         Sock->SockHandle,
> +         EFI_OPEN_PROTOCOL_GET_PROTOCOL
> +         );
> +  //
> +  // Uninstall the protocol installed on this sock
> +  //
> +  gBS->UninstallMultipleProtocolInterfaces (
> +        Sock->SockHandle,
> +        &gEfiTcp4ProtocolGuid,
> +        SockProtocol,
> +        NULL
> +        );
> +   SockDestroy (Sock);
> +   return NULL;
>  }
> 
> 
>  /**
>    Configure the specific socket Sock using configuration data ConfigData.
> diff --git a/MdeModulePkg/Universal/Network/Tcp4Dxe/Tcp4Dispatcher.c
> b/MdeModulePkg/Universal/Network/Tcp4Dxe/Tcp4Dispatcher.c
> index 5b327af..702cae8 100644
> --- a/MdeModulePkg/Universal/Network/Tcp4Dxe/Tcp4Dispatcher.c
> +++ b/MdeModulePkg/Universal/Network/Tcp4Dxe/Tcp4Dispatcher.c
> @@ -1,10 +1,10 @@
>  /** @file
>    Tcp request dispatcher implementation.
> 
>  (C) Copyright 2014 Hewlett-Packard Development Company, L.P.<BR>
> -Copyright (c) 2005 - 2014, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2005 - 2017, Intel Corporation. All rights reserved.<BR>
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD
> License
>  which accompanies this distribution.  The full text of the license may be
> found at
>  http://opensource.org/licenses/bsd-license.php<BR>
> 
> @@ -330,20 +330,10 @@ Tcp4DetachPcb (
>    Tcb       = ProtoData->TcpPcb;
> 
>    ASSERT (Tcb != NULL);
> 
>    Tcp4FlushPcb (Tcb);
> -
> -  //
> -  // Close the IP protocol.
> -  //
> -  gBS->CloseProtocol (
> -         Tcb->IpInfo->ChildHandle,
> -         &gEfiIp4ProtocolGuid,
> -         ProtoData->TcpService->IpIo->Image,
> -         Sk->SockHandle
> -         );
> 
>    IpIoRemoveIp (ProtoData->TcpService->IpIo, Tcb->IpInfo);
> 
>    FreePool (Tcb);
> 
> --
> 1.9.5.msysgit.1