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

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

在 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