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

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

> 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.

> 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).

In my opinion, the RxData should be recycled since it has been recorded in Mode->IcmpError.

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