Re: MdeModulePkg/UefiPxeBcDxe: Question about IcmpErrorListenHandler in PxeBcImpl.c

[Guo Heyi <heyi.guo@linaro.org>]

Hi Jiaxin,

We tested and the patch worked. We also tested PXE boot and no new issue was observed.

Thanks and regards,

Gary (Heyi Guo)


On Thu, Dec 07, 2017 at 08:20:28PM +0800, Heyi Guo wrote:
> 
> 
> 在 12/7/2017 6:40 PM, Wu, Jiaxin 写道:
> >>You say "IcmpErrorRcvToken is only used to get ICMP error from IP
> >>layer", does that mean only ICMP error packets will go to
> >>IcmpErrorListenHandler?
> >>If it is the case, how do we make it? I can only find a simple
> >>Ip4->Receive is called to receive IP4 packets; how are other types of
> >>IP4 packets filtered out?
> >No, all of the *ICMP* packets with the same station IP address will go to IcmpErrorListenHandler() callback function. Because PXE driver has already configured the current IP protocol only receive the ICMP packets:
> >	ZeroMem (&Private->Ip4ConfigData, sizeof (EFI_IP4_CONFIG_DATA));
> >   	Private->Ip4ConfigData.DefaultProtocol   = EFI_IP_PROTO_ICMP;
> >   	Private->Ip4ConfigData.AcceptIcmpErrors  = TRUE;
> >	Ip4ConfigData.DefaultProtocol   = EFI_IP_PROTO_ICMP;
> >So, it is only used to capture background ICMP packets (Including the ICMP error message) with the same station IP address.
> Many thanks; it explains my question clearly.
> 
> >
> >>If it is not, why don't we need to filter the packets in
> >>IcmpErrorListenHandler? If we recycle the packets in
> >>IcmpErrorListenHandler, will it cause the upper layer of protocols fail
> >>to fetch RxData?
> >IcmpErrorListenHandler() should filter the packets and only handle the ICMP error message. But currently, the code logic is incorrect. I generated one patch as attached one for your reference, can you help to verify whether it works or not? (Ignore my previous suggestion check).
> Sure we can verify it.
> >In my opinion, the RxData should be recycled since it has been recorded in Mode->IcmpError.
> Agree.
> 
> Thanks,
> 
> Gary (Heyi Guo)
> >
> >Thanks,
> >Jiaxin
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >>-----Original Message-----
> >>From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> >>Heyi Guo
> >>Sent: Thursday, December 7, 2017 4:18 PM
> >>To: Wu, Jiaxin <jiaxin.wu@intel.com>; edk2-devel@lists.01.org
> >>Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Dong,
> >>Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
> >>Subject: Re: [edk2] MdeModulePkg/UefiPxeBcDxe: Question about
> >>IcmpErrorListenHandler in PxeBcImpl.c
> >>
> >>Hi Jiaxin,
> >>
> >>Thanks for your reply.
> >>
> >>You say "IcmpErrorRcvToken is only used to get ICMP error from IP
> >>layer", does that mean only ICMP error packets will go to
> >>IcmpErrorListenHandler?
> >>
> >>If it is the case, how do we make it? I can only find a simple
> >>Ip4->Receive is called to receive IP4 packets; how are other types of
> >>IP4 packets filtered out?
> >>
> >>If it is not, why don't we need to filter the packets in
> >>IcmpErrorListenHandler? If we recycle the packets in
> >>IcmpErrorListenHandler, will it cause the upper layer of protocols fail
> >>to fetch RxData?
> >>
> >>Please forgive me if my questions are too stupid :)
> >>
> >>Regards,
> >>
> >>Gary (Heyi Guo)
> >>
> >>
> >>在 12/7/2017 3:48 PM, Wu, Jiaxin 写道:
> >>>Hi Gary,
> >>>
> >>>IcmpErrorRcvToken is only used to get ICMP error from IP layer, and the
> >>data will be copied to Mode->IcmpError. So, I think the RxData should be
> >>recycled.
> >>>Besides, EFI_IP_PROTO_ICMP should be also checked in the call function
> >>but currently it's not:
> >>>    if (!EFI_IP4_EQUAL (&RxData->Header->DestinationAddress, &Mode-
> >>>StationIp.v4)) {
> >>>      //
> >>>      // The dest address is not equal to Station Ip address, discard it.
> >>>      //
> >>>      goto CleanUp;
> >>>    }
> >>>
> >>>    +if (&RxData->Header->Protocol != EFI_IP_PROTO_ICMP) {
> >>>    +//
> >>>    +// The protocol value in the header of the receveid packet should be
> >>EFI_IP_PROTO_ICMP.
> >>>    +//
> >>>    +goto CleanUp;
> >>>    +}
> >>>
> >>>Thanks the report.
> >>>
> >>>Thanks,
> >>>Jiaxin
> >>>
> >>>
> >>>
> >>>>-----Original Message-----
> >>>>From: Guo Heyi [mailto:heyi.guo@linaro.org]
> >>>>Sent: Thursday, December 7, 2017 12:07 PM
> >>>>To: edk2-devel@lists.01.org
> >>>>Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>;
> >>Ni,
> >>>>Ruiyu <ruiyu.ni@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Wu, Jiaxin
> >>>><jiaxin.wu@intel.com>
> >>>>Subject: MdeModulePkg/UefiPxeBcDxe: Question about
> >>>>IcmpErrorListenHandler in PxeBcImpl.c
> >>>>
> >>>>Hi folks,
> >>>>
> >>>>In PxeBcImpl.c, we have IcmpErrorListenHandler which seems to process
> >>>>ICMP errors. But in EfiPxeBcStart function, we can see Private-
> >>>>>IcmpErrorRcvToken.Event is only a common event and Ip4->Receive is
> >>>>called to receive IP4 packets. So will IcmpErrorListenHandler receive all IP4
> >>>>packets belonging to this network interface, or will it only receive ICMP
> >>error
> >>>>packets? If it is the latter situation, how do we make it?
> >>>>
> >>>>The background of this question is that when we flush the network with
> >>>>deprecated ICMP packets (type 15, 16, ...), RxData will not be recycled and
> >>>>the list of UEFI events becomes longer and longer, which finally impacts
> >>>>system performance a lot. If only error ICMP will be received by
> >>>>IcmpErrorListenHandler, we'd like to patch it as below:
> >>>>
> >>>>diff --git
> >>a/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcImpl.c
> >>>>b/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcImpl.c
> >>>>index 6d4f33f..f74b264 100644
> >>>>--- a/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcImpl.c
> >>>>+++ b/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcImpl.c
> >>>>@@ -216,8 +216,6 @@ IcmpErrorListenHandlerDpc (
> >>>>       CopiedPointer += CopiedLen;
> >>>>     }
> >>>>
> >>>>-  goto Resume;
> >>>>-
> >>>>   CleanUp:
> >>>>     gBS->SignalEvent (RxData->RecycleSignal);
> >>>>
> >>>>We tested and it worked, but we are still not sure whether it will impact
> >>>>other code in the network stack.
> >>>>
> >>>>Please let me know your comments.
> >>>>
> >>>>Thanks,
> >>>>
> >>>>Gary (Heyi Guo)
> >>_______________________________________________
> >>edk2-devel mailing list
> >>edk2-devel@lists.01.org
> >>https://lists.01.org/mailman/listinfo/edk2-devel
>