public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Zhang, Lubo" <lubo.zhang@intel.com>
To: "Wu, Jiaxin" <jiaxin.wu@intel.com>,
	"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: "Ye, Ting" <ting.ye@intel.com>, "Fu, Siyuan" <siyuan.fu@intel.com>
Subject: Re: [patch] NetworkPkg: Fix driver binding issue in TCP dxe.
Date: Tue, 14 Mar 2017 01:55:15 +0000	[thread overview]
Message-ID: <7619447B08B8F74DA4FF2A813B79803B39B01E7C@shsmsx102.ccr.corp.intel.com> (raw)
In-Reply-To: <895558F6EA4E3B41AC93A00D163B7274162A1BD5@SHSMSX103.ccr.corp.intel.com>

Thanks for your comments, will send another patch to update this.


Best regards
Lubo

-----Original Message-----
From: Wu, Jiaxin 
Sent: Monday, March 13, 2017 3:44 PM
To: Zhang, Lubo <lubo.zhang@intel.com>; edk2-devel@lists.01.org
Cc: Ye, Ting <ting.ye@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>
Subject: RE: [edk2] [patch] NetworkPkg: Fix driver binding issue in TCP dxe.

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


      reply	other threads:[~2017-03-14  1:55 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-03-10 10:02 [patch] NetworkPkg: Fix driver binding issue in TCP dxe Zhang Lubo
2017-03-13  7:44 ` Wu, Jiaxin
2017-03-14  1:55   ` Zhang, Lubo [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7619447B08B8F74DA4FF2A813B79803B39B01E7C@shsmsx102.ccr.corp.intel.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox