From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id DDC66803FA for ; Wed, 15 Mar 2017 20:51:24 -0700 (PDT) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga104.jf.intel.com with ESMTP; 15 Mar 2017 20:51:24 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.36,170,1486454400"; d="scan'208";a="1108978100" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by orsmga001.jf.intel.com with ESMTP; 15 Mar 2017 20:51:17 -0700 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 15 Mar 2017 20:50:43 -0700 Received: from shsmsx103.ccr.corp.intel.com ([169.254.4.20]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.204]) with mapi id 14.03.0248.002; Thu, 16 Mar 2017 11:50:04 +0800 From: "Wu, Jiaxin" To: "Zhang, Lubo" , "edk2-devel@lists.01.org" CC: "Ye, Ting" , "Fu, Siyuan" Thread-Topic: [PATCH V2] MdeModulePkg: Fix service binding issue in TCP4 and Ip4 dxe. Thread-Index: AQHSnTPFEvFS5jemD0WnnQimnTF4saGWz+mA Date: Thu, 16 Mar 2017 03:50:04 +0000 Message-ID: <895558F6EA4E3B41AC93A00D163B7274162A392C@SHSMSX103.ccr.corp.intel.com> References: <1489544850-35284-1-git-send-email-lubo.zhang@intel.com> In-Reply-To: <1489544850-35284-1-git-send-email-lubo.zhang@intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZWY4ZDA4YjEtYzBlZC00MDAzLTliZjYtNGYxMzI3MDJhOGFjIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX1BVQkxJQyJ9XX1dfSwiU3ViamVjdExhYmVscyI6W10sIlRNQ1ZlcnNpb24iOiIxNS45LjYuNiIsIlRydXN0ZWRMYWJlbEhhc2giOiI5VGpPRmkyaVpBVlwvdmE1eWlpc0RCbkpPbEFWM1FvRDZobTVzWjFlU0Vlaz0ifQ== x-ctpclassification: CTP_PUBLIC x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH V2] MdeModulePkg: Fix service binding issue in TCP4 and Ip4 dxe. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 16 Mar 2017 03:51:25 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable For this patch, my comments as below: 1. The commit log should better cover the changes of Ip4Dxe but currently i= t's not.=20 2. Intel Copyright in SockImpl.h should be 2017, not 2007. Other looks to me. Reviewed-by: Wu Jiaxin Thanks, Jiaxin > -----Original Message----- > From: Zhang, Lubo > Sent: Wednesday, March 15, 2017 10:28 AM > To: edk2-devel@lists.01.org > Cc: Wu, Jiaxin ; Ye, Ting ; Fu, > Siyuan > Subject: [PATCH V2] MdeModulePkg: Fix service binding issue in TCP4 and > Ip4 dxe. >=20 > v2: Handle error case in SockCreateChild and fix typo issue >=20 > 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. >=20 > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Zhang Lubo > Cc: Wu Jiaxin > Cc: Ye Ting > Cc: Fu Siyuan > --- > 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(-) >=20 > 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. >=20 > -Copyright (c) 2005 - 2016, Intel Corporation. All rights reserved.
> +Copyright (c) 2005 - 2017, Intel Corporation. All rights reserved.
> (C) Copyright 2015 Hewlett-Packard Development Company, L.P.
>=20 > This program and the accompanying materials > are licensed and made available under the terms and conditions of the BS= D > License > which accompanies this distribution. The full text of the license may b= e > 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; >=20 > if ((This =3D=3D NULL) || (ChildHandle =3D=3D NULL)) { > return EFI_INVALID_PARAMETER; > } >=20 > @@ -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 =3D=3D IP4_STATE_DESTROY) { > + if (IpInstance->InDestroy) { > gBS->RestoreTPL (OldTpl); > return EFI_SUCCESS; > } >=20 > - State =3D IpInstance->State; > - IpInstance->State =3D IP4_STATE_DESTROY; > + IpInstance->InDestroy =3D TRUE; >=20 > // > // Close the Managed Network protocol. > // > gBS->CloseProtocol ( > @@ -1007,10 +1005,11 @@ Ip4ServiceBindingDestroyChild ( > &gEfiIp4ProtocolGuid, > &IpInstance->Ip4Proto > ); > OldTpl =3D gBS->RaiseTPL (TPL_CALLBACK); > if (EFI_ERROR (Status)) { > + IpInstance->InDestroy =3D FALSE; > goto ON_ERROR; > } >=20 > Status =3D Ip4CleanProtocol (IpInstance); > if (EFI_ERROR (Status)) { > @@ -1031,10 +1030,9 @@ Ip4ServiceBindingDestroyChild ( >=20 > FreePool (IpInstance); > return EFI_SUCCESS; >=20 > ON_ERROR: > - IpInstance->State =3D State; > gBS->RestoreTPL (OldTpl); >=20 > 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 >=20 > -Copyright (c) 2005 - 2016, Intel Corporation. All rights reserved.
> +Copyright (c) 2005 - 2017, Intel Corporation. All rights reserved.
> This program and the accompanying materials > are licensed and made available under the terms and conditions of the BS= D > License > which accompanies this distribution. The full text of the license may b= e > found at > http://opensource.org/licenses/bsd-license.php >=20 > @@ -548,10 +548,11 @@ Ip4InitProtocol ( > ZeroMem (IpInstance, sizeof (IP4_PROTOCOL)); >=20 > IpInstance->Signature =3D IP4_PROTOCOL_SIGNATURE; > CopyMem (&IpInstance->Ip4Proto, &mEfiIp4ProtocolTemplete, sizeof > (IpInstance->Ip4Proto)); > IpInstance->State =3D IP4_STATE_UNCONFIGED; > + IpInstance->InDestroy =3D FALSE; > IpInstance->Service =3D IpSb; >=20 > 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. >=20 > -Copyright (c) 2005 - 2016, Intel Corporation. All rights reserved.
> +Copyright (c) 2005 - 2017, Intel Corporation. All rights reserved.
> (C) Copyright 2015 Hewlett-Packard Development Company, L.P.
>=20 > This program and the accompanying materials > are licensed and made available under the terms and conditions of the BS= D > License > which accompanies this distribution. The full text of the license may b= e > 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 >=20 > // > // 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 { >=20 > EFI_IP4_PROTOCOL Ip4Proto; > EFI_HANDLE Handle; > INTN State; >=20 > + BOOLEAN InDestroy; > + > IP4_SERVICE *Service; > LIST_ENTRY Link; // Link to all the IP protocol f= rom the service >=20 > // > // 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. >=20 > -Copyright (c) 2005 - 2016, Intel Corporation. All rights reserved.
> +Copyright (c) 2005 - 2017, Intel Corporation. All rights reserved.
> This program and the accompanying materials > are licensed and made available under the terms and conditions of the BS= D > License > which accompanies this distribution. The full text of the license may b= e > found at > http://opensource.org/licenses/bsd-license.php
>=20 > @@ -715,20 +715,12 @@ OnError: > VOID > SockDestroy ( > IN OUT SOCKET *Sock > ) > { > - VOID *SockProtocol; > - EFI_GUID *ProtocolGuid; > - EFI_STATUS Status; > - > ASSERT (SockStream =3D=3D Sock->Type); >=20 > - if (Sock->DestroyCallback !=3D 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 ( > ); >=20 > Sock->Parent =3D NULL; > } >=20 > - // > - // Set the protocol guid and driver binding handle > - // in the light of Sock->SockType > - // > - ProtocolGuid =3D &gEfiTcp4ProtocolGuid; > - > - // > - // Retrieve the protocol installed on this sock > - // > - Status =3D 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 ; > } >=20 >=20 > 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. >=20 > -Copyright (c) 2005 - 2006, Intel Corporation. All rights reserved.
> +Copyright (c) 2005 - 2007, Intel Corporation. All rights reserved.
> This program and the accompanying materials > are licensed and made available under the terms and conditions of the BS= D > License > which accompanies this distribution. The full text of the license may b= e > found at > http://opensource.org/licenses/bsd-license.php
>=20 > @@ -14,10 +14,11 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF > ANY KIND, EITHER EXPRESS OR IMPLIED. >=20 > #ifndef _SOCK_IMPL_H_ > #define _SOCK_IMPL_H_ >=20 > #include "Socket.h" > +#include "Tcp4Main.h" >=20 > /** > Signal a event with the given status. >=20 > @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. >=20 > -Copyright (c) 2005 - 2016, Intel Corporation. All rights reserved.
> +Copyright (c) 2005 - 2017, Intel Corporation. All rights reserved.
> This program and the accompanying materials > are licensed and made available under the terms and conditions of the BS= D > License > which accompanies this distribution. The full text of the license may b= e > found at > http://opensource.org/licenses/bsd-license.php
>=20 > @@ -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; >=20 > ASSERT ((Sock !=3D NULL) && (Sock->ProtoHandler !=3D NULL)); >=20 > if (Sock->InDestroy) { > return EFI_SUCCESS; > } >=20 > Sock->InDestroy =3D TRUE; >=20 > + ProtoData =3D (TCP4_PROTO_DATA *) Sock->ProtoReserved; > + Tcb =3D ProtoData->TcpPcb; > + > + ASSERT (Tcb !=3D NULL); > + > Status =3D EfiAcquireLockOrFail (&(Sock->Lock)); > if (EFI_ERROR (Status)) { >=20 > DEBUG ((EFI_D_ERROR, "SockDestroyChild: Get the lock to " > "access socket failed with %r\n", Status)); >=20 > return EFI_ACCESS_DENIED; > } >=20 > // > + // Close the IP protocol. > + // > + gBS->CloseProtocol ( > + Tcb->IpInfo->ChildHandle, > + &gEfiIp4ProtocolGuid, > + ProtoData->TcpService->IpIo->Image, > + Sock->SockHandle > + ); > + > + if (Sock->DestroyCallback !=3D NULL) { > + Sock->DestroyCallback (Sock, Sock->Context); > + } > + > + // > + // Retrieve the protocol installed on this sock > + // > + Status =3D 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 =3D Sock->ProtoHandler (Sock, SOCK_DETACH, NULL); >=20 > if (EFI_ERROR (Status)) { > @@ -207,10 +258,11 @@ SOCKET * > SockCreateChild ( > IN SOCK_INIT_DATA *SockInitData > ) > { > SOCKET *Sock; > + VOID *SockProtocol; > EFI_STATUS Status; >=20 > // > // create a new socket > // > @@ -227,12 +279,11 @@ SockCreateChild ( > if (EFI_ERROR (Status)) { >=20 > DEBUG ((EFI_D_ERROR, "SockCreateChild: Get the lock to " > "access socket failed with %r\n", Status)); >=20 > - 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)) { >=20 > DEBUG ((EFI_D_ERROR, "SockCreateChild: Protocol failed to" > " attach a socket with %r\n", Status)); >=20 > - SockDestroy (Sock); > - Sock =3D NULL; > + goto ERROR; > } >=20 > return Sock; > + > +ERROR: > + > + if (Sock->DestroyCallback !=3D 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; > } >=20 >=20 > /** > 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. >=20 > (C) Copyright 2014 Hewlett-Packard Development Company, L.P.
> -Copyright (c) 2005 - 2014, Intel Corporation. All rights reserved.
> +Copyright (c) 2005 - 2017, Intel Corporation. All rights reserved.
> This program and the accompanying materials > are licensed and made available under the terms and conditions of the BS= D > License > which accompanies this distribution. The full text of the license may b= e > found at > http://opensource.org/licenses/bsd-license.php
>=20 > @@ -330,20 +330,10 @@ Tcp4DetachPcb ( > Tcb =3D ProtoData->TcpPcb; >=20 > ASSERT (Tcb !=3D NULL); >=20 > Tcp4FlushPcb (Tcb); > - > - // > - // Close the IP protocol. > - // > - gBS->CloseProtocol ( > - Tcb->IpInfo->ChildHandle, > - &gEfiIp4ProtocolGuid, > - ProtoData->TcpService->IpIo->Image, > - Sk->SockHandle > - ); >=20 > IpIoRemoveIp (ProtoData->TcpService->IpIo, Tcb->IpInfo); >=20 > FreePool (Tcb); >=20 > -- > 1.9.5.msysgit.1