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-boun...@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...@intel.com>; Fu, Siyuan <siyuan...@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.zh...@intel.com>
> Cc: Ye Ting <ting...@intel.com>
> Cc: Fu Siyuan <siyuan...@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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
