Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Preparing library to use loopback device #1020
Preparing library to use loopback device #1020
Changes from 12 commits
1803d90
91badc9
a57b099
e5a8f60
e9f9126
87c517d
2422335
9104b2e
f7d2e35
83f34fd
5669fd8
855d6d5
085741c
35a61c1
c706b49
364bc05
5b24042
a0b59ff
3c97367
7cdbef2
c29f055
5240680
31da177
c2cd991
3b9a259
d695740
d7ee3c2
c05141f
a73a8c1
20f16c2
1150510
16352be
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 logic is not very clear.Isn't it enough to check only for destination address.
In my understanding, We should only process packets for which the destination address is loopback. Source address can only be loopback if there is a bridging/routing supported in the device and as of now our stack does not support bridging/routing. And from an external device only the destination address can be loopback. Please suggest if I am missing anything 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.
@shubnil wrote:
The check is done in
xBadIPv4Loopback()
, which is called fromprvProcessUDPPacket()
, which will be called for any incoming UDP packet.It will return
pdTRUE
in case either the source or the destination address is a loopback address.Before we had a loopback interface, loopback addresses had to be dropped:
which would be the same as :
I think that we should have tested for both
ulDestinationIPAddress
andulSourceIPAddress
because 127.x.x.x was not yet implemented.Now we have added a loopback device, and so 127.x.x.x addresses must be allowed, with the exception of a packet leaving or entering the host. Loopback packets may only travel internally. So that is why:
when either the destination or the source address is a loopback address, the packet must be dropped.
All other packets may be processed.
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 would like understand a little more on Packets will loopback as Source address. What will be the use case for this? Is this for the packets originated from the loopback interface? In that case the packet should not go out of the system. Please suggest if this understanding is correct.
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.
Line no 65 and 243 both have a constant IPv6 address, however one is just const and other is static const. We should ideally have same for both.
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.
Good point.
xIPv6UnspecifiedAddress[]
is quite new and it is put halfway the file among the functions. The unspecified address is the same as the ANY addressFreeRTOS_in6addr_any[]
, which already exists for a long time, and which is global. Let's use that in stead.const struct xIPv6_Address FreeRTOS_in6addr_any = { 0 }; const struct xIPv6_Address FreeRTOS_in6addr_loopback = { { 0U, 0U, 0U, 0U, 0U, 0U, 0U, 0U, 0U, 0U, 0U, 0U, 0U, 0U, 0U, 1U } }; - static const struct xIPv6_Address xIPv6UnspecifiedAddress = { { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 } };
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 logic is not very clear.Isn't it enough to check only for destination address.
In my understanding, We should only process packets for which the destination address is loopback. Source address can only be loopback if there is a bridging/routing supported in the device and as of now our stack does not support bridging/routing. And from an external device only the destination address can be loopback. Please suggest if I am missing anything 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.
I already answered to this question here above.
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.
Codewise it makes more sense to have xBadIPv6Loopback and xBadIPv4Loopback checks at the same level. Here, xBadIPv4Loopback check is happening at FreeRTOS_IP.c but xBadIPv6Loopback check is in IPv6 specfic file. Can we re-arrange the code.
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 answered this and rearranged the code like this:
prvAllowIPPacketIPv4()
callsxBadIPv4Loopback()
prvAllowIPPacketIPv6()
callsxBadIPv6Loopback()
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.
Yeah i think this needs to be removed in the case the endpoint is already present on the list.
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.
But i think we should also add pxEndPoint->pxNext = NULL; in line 262 in case there is no endpoint in the list
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.
Also can we move commit as a separate PR?
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 is 10 points for you, Monika, thanks! Although we assume that endpoints have been cleared, we can not count on it.