Re: [patch] NetworkPkg: Fix driver binding issue in TCP dxe.

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

My comments as below:

 1. SockDestroyChild: The sock should be locked to access first, then we can update it (close/uninstall).

 2. SockDestroyChild: Debug info should be "SockDestroyChild:" instead of "SockDestroy:"

 3. One concern in SockCreateChild: 
	Since the " CloseProtocol/Uninstall" operation is removed  from SockDestroy, we need handle it alone while the ERROR may happened in in SockCreateChild. 

Thanks,
Jiaxin


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Zhang Lubo
> Sent: Friday, March 10, 2017 6:02 PM
> To: edk2-devel@lists.01.org
> Cc: Ye, Ting <ting.ye@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>
> Subject: [edk2] [patch] NetworkPkg: Fix driver binding issue in TCP dxe.
> 
> 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: Ye Ting <ting.ye@intel.com>
> Cc: Fu Siyuan <siyuan.fu@intel.com>
> ---
>  NetworkPkg/TcpDxe/SockImpl.c      | 56 +--------------------------------
>  NetworkPkg/TcpDxe/SockImpl.h      |  3 +-
>  NetworkPkg/TcpDxe/SockInterface.c | 66
> +++++++++++++++++++++++++++++++++++++--
>  NetworkPkg/TcpDxe/TcpDispatcher.c | 19 +----------
>  4 files changed, 68 insertions(+), 76 deletions(-)
> 
> diff --git a/NetworkPkg/TcpDxe/SockImpl.c
> b/NetworkPkg/TcpDxe/SockImpl.c
> index 4eb42fb..c5fb176 100644
> --- a/NetworkPkg/TcpDxe/SockImpl.c
> +++ b/NetworkPkg/TcpDxe/SockImpl.c
> @@ -1,9 +1,9 @@
>  /** @file
>    Implementation of the Socket.
> 
> -  Copyright (c) 2009 - 2016, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2009 - 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.
> @@ -826,20 +826,12 @@ OnError:
>  VOID
>  SockDestroy (
>    IN OUT SOCKET *Sock
>    )
>  {
> -  VOID        *SockProtocol;
> -  EFI_GUID    *TcpProtocolGuid;
> -  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)) {
> @@ -870,56 +862,10 @@ SockDestroy (
>        );
> 
>      Sock->Parent = NULL;
>    }
> 
> -  //
> -  // Set the protocol guid and driver binding handle
> -  // in the light of Sock->SockType
> -  //
> -  if (Sock->IpVersion == IP_VERSION_4) {
> -    TcpProtocolGuid = &gEfiTcp4ProtocolGuid;
> -  } else {
> -    TcpProtocolGuid = &gEfiTcp6ProtocolGuid;
> -  }
> -
> -  //
> -  // Retrieve the protocol installed on this sock
> -  //
> -  Status = gBS->OpenProtocol (
> -                  Sock->SockHandle,
> -                  TcpProtocolGuid,
> -                  &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,
> -        TcpProtocolGuid,
> -        SockProtocol,
> -        NULL
> -        );
> -
> -FreeSock:
> -
>    FreePool (Sock);
>  }
> 
>  /**
>    Flush the sndBuffer and rcvBuffer of socket.
> diff --git a/NetworkPkg/TcpDxe/SockImpl.h
> b/NetworkPkg/TcpDxe/SockImpl.h
> index 5a067de..80692b1 100644
> --- a/NetworkPkg/TcpDxe/SockImpl.h
> +++ b/NetworkPkg/TcpDxe/SockImpl.h
> @@ -1,9 +1,9 @@
>  /** @file
>    The function declaration that provided for Socket Interface.
> 
> -  Copyright (c) 2009 - 2016, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2009 - 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.
> @@ -15,10 +15,11 @@
> 
>  #ifndef _SOCK_IMPL_H_
>  #define _SOCK_IMPL_H_
> 
>  #include "Socket.h"
> +#include "TcpMain.h"
> 
>  /**
>    Signal a event with the given status.
> 
>    @param[in] Token        The token's event is to be signaled.
> diff --git a/NetworkPkg/TcpDxe/SockInterface.c
> b/NetworkPkg/TcpDxe/SockInterface.c
> index 21ce643..5269c56 100644
> --- a/NetworkPkg/TcpDxe/SockInterface.c
> +++ b/NetworkPkg/TcpDxe/SockInterface.c
> @@ -1,9 +1,9 @@
>  /** @file
>    Interface function of the Socket.
> 
> -  Copyright (c) 2009 - 2016, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2009 - 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.
> @@ -140,20 +140,82 @@ SockBufferToken (
>  EFI_STATUS
>  SockDestroyChild (
>    IN OUT SOCKET *Sock
>    )
>  {
> -  EFI_STATUS  Status;
> +  EFI_STATUS       Status;
> +  TCP_PROTO_DATA   *ProtoData;
> +  TCP_CB           *Tcb;
> +  EFI_GUID         *IpProtocolGuid;
> +  EFI_GUID         *TcpProtocolGuid;
> +  VOID             *SockProtocol;
> 
>    ASSERT ((Sock != NULL) && (Sock->ProtoHandler != NULL));
> 
>    if (Sock->InDestroy) {
>      return EFI_SUCCESS;
>    }
> 
>    Sock->InDestroy = TRUE;
> 
> +  if (Sock->IpVersion == IP_VERSION_4) {
> +    IpProtocolGuid = &gEfiIp4ProtocolGuid;
> +    TcpProtocolGuid = &gEfiTcp4ProtocolGuid;
> +  } else {
> +    IpProtocolGuid = &gEfiIp6ProtocolGuid;
> +    TcpProtocolGuid = &gEfiTcp6ProtocolGuid;
> +  }
> +  ProtoData = (TCP_PROTO_DATA *) Sock->ProtoReserved;
> +  Tcb       = ProtoData->TcpPcb;
> +
> +  ASSERT (Tcb != NULL);
> +
> +  //
> +  // Close the IP protocol.
> +  //
> +  gBS->CloseProtocol (
> +         Tcb->IpInfo->ChildHandle,
> +         IpProtocolGuid,
> +         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,
> +                  TcpProtocolGuid,
> +                  &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)
> +      );
> +  }
> +
> +  //
> +  // Uninstall the protocol installed on this sock
> +  //
> +  gBS->UninstallMultipleProtocolInterfaces (
> +        Sock->SockHandle,
> +        TcpProtocolGuid,
> +        SockProtocol,
> +        NULL
> +        );
> +
>    Status            = EfiAcquireLockOrFail (&(Sock->Lock));
>    if (EFI_ERROR (Status)) {
> 
>      DEBUG (
>        (EFI_D_ERROR,
> diff --git a/NetworkPkg/TcpDxe/TcpDispatcher.c
> b/NetworkPkg/TcpDxe/TcpDispatcher.c
> index d4bc8ac..9a352b1 100644
> --- a/NetworkPkg/TcpDxe/TcpDispatcher.c
> +++ b/NetworkPkg/TcpDxe/TcpDispatcher.c
> @@ -1,10 +1,10 @@
>  /** @file
>    The implementation of a dispatch routine for processing TCP requests.
> 
>    (C) Copyright 2014 Hewlett-Packard Development Company, L.P.<BR>
> -  Copyright (c) 2009 - 2014, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2009 - 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.
> @@ -421,34 +421,17 @@ TcpDetachPcb (
>    IN OUT SOCKET    *Sk
>    )
>  {
>    TCP_PROTO_DATA   *ProtoData;
>    TCP_CB           *Tcb;
> -  EFI_GUID         *IpProtocolGuid;
> 
> -  if (Sk->IpVersion == IP_VERSION_4) {
> -    IpProtocolGuid = &gEfiIp4ProtocolGuid;
> -  } else {
> -    IpProtocolGuid = &gEfiIp6ProtocolGuid;
> -  }
> -
>    ProtoData = (TCP_PROTO_DATA *) Sk->ProtoReserved;
>    Tcb       = ProtoData->TcpPcb;
> 
>    ASSERT (Tcb != NULL);
> 
>    TcpFlushPcb (Tcb);
> -
> -  //
> -  // Close the IP protocol.
> -  //
> -  gBS->CloseProtocol (
> -         Tcb->IpInfo->ChildHandle,
> -         IpProtocolGuid,
> -         ProtoData->TcpService->IpIo->Image,
> -         Sk->SockHandle
> -         );
> 
>    IpIoRemoveIp (ProtoData->TcpService->IpIo, Tcb->IpInfo);
> 
>    FreePool (Tcb);
> 
> --
> 1.9.5.msysgit.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel