Thanks for responding guys,
First of all I’d like to apologize, that I cannot currently test the latest commit on “main”. Right now I’m using what is equivalent to 2.4.0 ( b6eac0ca7df8c8935cb840fd9642f6533a0da2e6 ) I’m also using a heavily modified version of the stack that that handles IGMP, has IGMP snooping and also has a quick and dirty port of a mDNS responder. With that being said, I’m doing my best to comment out as much of this custom-ness when testing for this issue. Thanks for the understanding.
I fully agree with the idea the ARP should be preemptive in order to minimize the need for sending queries, however when you combine that with the active maintaining of aging entries, you end up with entries that will never expire even though there is no need for them. Here’s an example. A host pings out DUT and stops but never again attempts to communicate with the DUT. The result is that DUT will never forget the host’s address even though it will never need it again. Therefor it is my personal opinion that aging entries should not be proactively maintained. If “proper” preemptive learning and updating is used, there should be nothing wrong with letting an entry expire. Think about it, if it expires, that means there has not been a single packet between this remote host and the DUT for the default 25 minutes.
Parallel to the above, please look at the following code:
if( ucProtocol != ( uint8_t ) ipPROTOCOL_UDP)
{
if( xCheckRequiresARPResolution( pxNetworkBuffer ) == pdTRUE )
{
eReturn = eWaitingARPResolution;
}
else
{
/* Refresh the age of this cache entry since a packet was received. */
vARPRefreshCacheEntryAge( &( pxIPPacket->xEthernetHeader.xSourceAddress ), pxIPHeader->ulSourceIPAddress );
}
}
The above in combination of xCheckRequiresARPResolution() results in all IGMPv1 and v2 reports for 224.0.0.252 to be entered into the ARP cache given of course ipconfigUSE_LLMNR = 1
I’d also ask what the reasoning is to exclude UDP from the if() above but that is not that important.
Another thing that I identified as problematic is this:
#if ( ipconfigUSE_LLMNR == 1 )
/* A LLMNR request, check for the destination port. */
if( ( usPort == FreeRTOS_ntohs( ipLLMNR_PORT ) ) ||
( pxUDPPacket->xUDPHeader.usSourcePort == FreeRTOS_ntohs( ipLLMNR_PORT ) ) )
{
xReturn = ( BaseType_t ) ulDNSHandlePacket( pxNetworkBuffer );
}
else
#endif /* ipconfigUSE_LLMNR */
#if ( ipconfigUSE_NBNS == 1 )
/* a NetBIOS request, check for the destination port */
if( ( usPort == FreeRTOS_ntohs( ipNBNS_PORT ) ) ||
( pxUDPPacket->xUDPHeader.usSourcePort == FreeRTOS_ntohs( ipNBNS_PORT ) ) )
{
vARPRefreshCacheEntry( &( pxUDPPacket->xEthernetHeader.xSourceAddress ), pxUDPPacket->xIPHeader.ulSourceIPAddress );
xReturn = ( BaseType_t ) ulNBNSHandlePacket( pxNetworkBuffer );
}
else
#endif /* ipconfigUSE_NBNS */
{
xReturn = pdFAIL;
}
The result from the above is that ANY LLMNR ( 224.0.0.252 ) or NBNS ( local net broadcast ) lookup will add the querier’s MAC address to the cache regardless of whether it needs to be there or not. Now couple that with actively maintaining aging entries and that table just keeps growing.
A better ( in my opinion ) approach would be to add entries only if the LLMNR query needs to be responded to or if the NBNS is for our name. I have implemented such solutions but will refrain from posting them here as I’m still evaluating them.
Let me know what you think and sorry for the lengthy post
My main issue was that I noticed my ARP cache getting full and with that comes the possibility of overwriting entries that are actually being used. That’s what got me digging deeper and questioning parts of the code.