-
Notifications
You must be signed in to change notification settings - Fork 159
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve frame filtering #1100
Improve frame filtering #1100
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks good, clean expressions. Thank you.
source/FreeRTOS_IP.c
Outdated
} | ||
break; | ||
} | ||
|
||
/* Examine the destination MAC from the Ethernet header to see if it matches |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before this comment, I would expect the following conditional break:
default:
if( FreeRTOS_ntohs( pxEthernetHeader->usFrameType ) <= 0x600U )
{
#if ipconfigIS_ENABLED( ipconfigFILTER_OUT_NON_ETHERNET_II_FRAMES )
eReturn = eReleaseBuffer;
#endif
}
else
{
#if ipconfigIS_DISABLED( ipconfigPROCESS_CUSTOM_ETHERNET_FRAMES )
eReturn = eReleaseBuffer;
#endif
}
break;
}
+ if( eReturn == eReleaseBuffer )
+ {
+ /* No use to do more testing. */
+ break;
+ }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you prefer that or using an if statement with breaks? I generally avoid places with multiple layers of breaks like switches in whiles.
source/FreeRTOS_IP.c
Outdated
|
||
/* Examine the destination MAC from the Ethernet header to see if it matches | ||
* that of an end point managed by FreeRTOS+TCP. */ | ||
pxEndPoint = FreeRTOS_MatchingEndpoint( NULL, pucEthernetBuffer ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fairly expensive search for an end-point and wasn't it already done in the network driver?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I believe either we need to assign the endpoint to the network buffer here or just use FreeRTOS_FindEndPointOnMAC
as a preliminary check to match the MAC address along with LLMNR and MDNS MAC addresses if
checks.
It doesn't make much sense to ignore the endpoint result here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well if FreeRTOS_MatchingEndpoint
is called from the NetworkInterface and ipconfigETHERNET_DRIVER_FILTERS_FRAME_TYPES == 0
, then neither should be done. If ipconfigETHERNET_DRIVER_FILTERS_FRAME_TYPES == 1
, the previous call to FreeRTOS_FindEndPointOnMAC
is pretty much wasted work when it is half of the work done in FreeRTOS_MatchingEndpoint
. But you have no way of assigning the pxEndpoint result unless you change the parameter or you are able to use pxPacketBuffer_to_NetworkBuffer
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a bit confused. We cannot "assign the endpoint" in eConsiderFrameForProcessing()
because:
eConsiderFrameForProcessing()
is an optional function. It's presence is controlled byipconfigETHERNET_DRIVER_FILTERS_FRAME_TYPES
so ifipconfigETHERNET_DRIVER_FILTERS_FRAME_TYPES
is enabled, the endoint needs to be assigned elswhere.eConsiderFrameForProcessing()
does not have access to the network buffer, so we cannot assign an endpoint in it.
I do agree that we should use FreeRTOS_FindEndPointOnMAC()
to check if we need to drop the frame, however since eConsiderFrameForProcessing()
doesn not have a pointer to the network interface in used to call:
pxEndPoint = FreeRTOS_FindEndPointOnMAC( &( pxEthernetHeader->xDestinationAddress ), NULL );
which is a bit useless as it can match the MAC of a different network interface.
My original comment was due to this:
The network driver ( or at least the DriverSAM that I'm familiar with ) already did:
pxNextNetworkBufferDescriptor->pxInterface = pxMyInterface;
pxNextNetworkBufferDescriptor->pxEndPoint = FreeRTOS_MatchingEndpoint( pxMyInterface, pxNextNetworkBufferDescriptor->pucEthernetBuffer );
if( pxNextNetworkBufferDescriptor->pxEndPoint == NULL )
{
FreeRTOS_printf( ( "NetworkInterface: can not find a proper endpoint\n" ) );
xRelease = pdTRUE;
}
Therefor, if we ever reached eConsiderFrameForProcessing()
, the frame already has a good endpoint assigned.
As a matter of fact, calling
pxEndPoint = FreeRTOS_MatchingEndpoint( NULL, pucEthernetBuffer );
actually creates a problem of its own: FreeRTOS_MatchingEndpoint()
internally will call pxEasyFit()
with a NULL for an interface pointer. That causes pxEasyFit()
to search on ALL interfaces and potentially return a match on IF_X even though the frame was received on IF_Y
I hope the above is clear, or maybe I'm missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HTRamsey,
FreeRTOS_MatchingEndpoint()
does not guarantee that the frame's destination MAC matches one of our end-point's MAC addresses. FreeRTOS_MatchingEndpoint()
and pxEasyFit()
could select an end-point base on IP-version or destination IP address, so with ipconfigETHERNET_DRIVER_FILTERS_FRAME_TYPES == 0
the ONLY place that ever verifies the destination MAC of a frame against the MAC addresses of our endpoints is when eConsiderFrameForProcessing()
does this:
pxEndPoint = FreeRTOS_FindEndPointOnMAC( &( pxEthernetHeader->xDestinationAddress ), NULL );
if( pxEndPoint != NULL )
{
/* The packet was directed to this node - process it. */
eReturn = eProcessBuffer;
}
I believe the code above is required, but if I were to ever try to improve eConsiderFrameForProcessing()
, I would change it's parameter to pxNetworkBuffer and change the call above to:
pxEndPoint = FreeRTOS_FindEndPointOnMAC( &( pxEthernetHeader->xDestinationAddress ), pxNetworkBuffer );
I agree that calling FreeRTOS_FindEndPointOnMAC()
does work that was already done in FreeRTOS_MatchingEndpoint()
however, FreeRTOS_MatchingEndpoint()
does not store information on whether it found a MAC match. If it could store that info somehow, then we wouldn't have to call FreeRTOS_FindEndPointOnMAC()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed the prioritized methods in pxEasyFit
, so you're right it could just check rMATCH_IP_ADDR
. The other methods in pxEasyFit
are checked in prvAllowIPPacketIPv4
& prvAllowIPPacketIPv6
where it calls FreeRTOS_FindEndPointOnIP_IPv4
, FreeRTOS_FindEndPointOnMAC
, etc. Briefly looking at pxEasyFit
, I thought it checked all of them rather than going in some order.
I think this is where it would be beneficial to implement what I had mentioned here. I don't know how much it affects performance, but there are a few things that are checked multiple times, like endpoints and the IP header, etc.
Also if you had the pxNetworkBuffer
parameter, you could at least check data length too.
Edit: @evpopov I just realized calling eConsiderFrameForProcessing
is inconsistent in the NetworkInterfaces. I don't think it's supposed to be optional as in 'some' Network Interface this is defined:
#if ( ipconfigETHERNET_DRIVER_FILTERS_FRAME_TYPES == 0 )
#define ipCONSIDER_FRAME_FOR_PROCESSING( pucEthernetBuffer ) eProcessBuffer
#else
#define ipCONSIDER_FRAME_FOR_PROCESSING( pucEthernetBuffer ) \
eConsiderFrameForProcessing( ( pucEthernetBuffer ) )
#endif
and in FreeRTOS_IP.c it is the inverse:
#if ipconfigETHERNET_DRIVER_FILTERS_FRAME_TYPES == 0
#define ipCONSIDER_FRAME_FOR_PROCESSING( pucEthernetBuffer ) eConsiderFrameForProcessing( ( pucEthernetBuffer ) )
#else
#define ipCONSIDER_FRAME_FOR_PROCESSING( pucEthernetBuffer ) eProcessBuffer
#endif
So it looks like if it isn't called in the NetworkInterface it is supposed to be called in prvProcessEthernetPacket
. But ATSAME5x does it backwards and maybe some others. I don't know it doesn't seem consistent but I think it isn't intended to be optional.
Ideally all these checks would be handled only once in the network interface in some manner like:
Check Frame -> Find Endpoint -> Check Packet -> Send to IP Task -> Process Packet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wow! I had not seen this. I just searched for ipCONSIDER_FRAME_FOR_PROCESSING
in all the network drivers and you're right. It is defined all over the place.
BTW, from my prespective, the same5x driver defines it exactly as FreeRTOS_IP.c and every other network driver that defines ipCONSIDER_FRAME_FOR_PROCESSING
is backwards.
I don't want to speculate on what those drivers are doing. It would be nice to get some insight from someone with more long-term experience but I think this macro shouldn't be defined in the network drivers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it can be placed before actually queuing the eNetworkRxEvent
outside of the network interface. The less that's required in the network interface, the better in my opinion. That's also why I had previously tried to pull vNetworkInterfaceAllocateRAMToBuffers
outside of the network drivers, because it's basically identical for every driver except for the location to store the packets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HTRamsey wrote:
But you have no way of assigning the pxEndpoint result unless you change the parameter or you are able to use
pxPacketBuffer_to_NetworkBuffer
.
As discussed earlier, it is proposed that eConsiderFrameForProcessing()
gets the packet descriptor as a parameter.
I am not a fan of using pxPacketBuffer_to_NetworkBuffer
unless really necessary.
First of all, thank you for the interesting discussion: @HTRamsey, @evpopov, and @tony-josi-aws . The reception of packets has three important moments:
What happens where?
I know there is still some filtering code in some network interfaces, that code should not be part of the network interface. I would just remove it because the IP-task already has good filtering. @evpopov noticed that The macro and the function have always lead to confusion and mistakes for end-users. I would also like to see a new version of
The function will assume that the following are set:
@evpopov writes:
Yes absolutely, and not necessary. @tony-josi-aws writes:
Yes indeed. And maybe we can also check/reevaluate the use of In FreeRTOS_IP.c, I would rather see this code, in stead of hiding it behind a macro:
The idea behind |
@htibosch personally I would remove Then do steps 1, 2, & 3 that you listed all in a function like Edit: Ideally the buffer is created after some filtering is done, so you don't have to immediately release it if the packet is invalid. Maybe you can pass data pointer, datalength, and interface as the parameters instead of a buffer then only create the buffer if you intend on passing the data to the queue. so something like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks good, clean code. I have two questions left. Thanks.
source/FreeRTOS_IP.c
Outdated
|
||
/* Examine the destination MAC from the Ethernet header to see if it matches | ||
* that of an end point managed by FreeRTOS+TCP. */ | ||
pxEndPoint = FreeRTOS_MatchingEndpoint( NULL, pucEthernetBuffer ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HTRamsey wrote:
But you have no way of assigning the pxEndpoint result unless you change the parameter or you are able to use
pxPacketBuffer_to_NetworkBuffer
.
As discussed earlier, it is proposed that eConsiderFrameForProcessing()
gets the packet descriptor as a parameter.
I am not a fan of using pxPacketBuffer_to_NetworkBuffer
unless really necessary.
7f37651
to
12ded3f
Compare
I don't know how much to do in this PR, or just wait. I fixed the current problems, if we want to move forward with the rest then I would do the following
|
source/FreeRTOS_IP.c
Outdated
if( pucEthernetBuffer == NULL ) | ||
{ | ||
/* The packet buffer was null - release it. */ | ||
eReturn = eReleaseBuffer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: This line is unnecessary as eReturn
is already eReleaseBuffer
here.
break; | ||
#else | ||
/* IPv6 is enabled - Continue filter checks. */ | ||
#endif | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MISRA 2012: else if
constructs should be terminated with non-empty else
case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that else is reached, should we release the buffer? The destination address would be unsupported I guess.
Edit: Shouldn't we check for 0x01005E for IPv4 multicast, similar to how we do for IPv6?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that else is reached, should we release the buffer? The destination address would be unsupported I guess.
Yes, I believe its good to release the buffer.
Shouldn't we check for 0x01005E for IPv4 multicast, similar to how we do for IPv6?
For multicast support, this check should be required. It would be good to use the new multicast MAC address macros in vSetMultiCastIPv4MacAddress
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small side note:
In #1019 I've changed this check to allow all multicast MAC addresses if ipconfigSUPPORT_IP_MULTICAST
is enabled. I might change that in the future, but with multicasts enabled and especially if ipconfigPROCESS_CUSTOM_ETHERNET_FRAMES
is enabled, it really is too early to drop any multicasts at this stage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll change the multicast filtering part after that goes in, for now it just drops ipv6 multicasts if ipv6 is disabled and vice versa for ipv4
@HTRamsey, thanks for the PR, this is a good improvement/cleanup. As the PR doesn't change finding endpoint logic, I believe the rest of the changes can be made as a separate PR. |
/bot run formatting |
87e9c9a
to
b5a21d1
Compare
49a0867
to
28ae29b
Compare
Hi @HTRamsey, Thank you. |
Improve frame filtering
Description
Expand the checks in eConsiderFrameForProcessing to drop invalid frames earlier
Test Steps
Checklist:
Related Issue
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.