You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/04/23 12:11:06 UTC
[GitHub] [incubator-nuttx] jerpelea opened a new pull request #3601: net/ip: bypass UDP input only when the device address is invalid
jerpelea opened a new pull request #3601:
URL: https://github.com/apache/incubator-nuttx/pull/3601
## Summary
Accept the UDP packet if the devices has not obtained
the IP address to solve the compatibility issue of DHCP
BOOTP working on unicast mode.
Reference:
https://tools.ietf.org/html/rfc1542
## Impact
NET
## Testing
NONE
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #3601: [BP-3598]net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #3601:
URL: https://github.com/apache/incubator-nuttx/pull/3601#issuecomment-828435824
I still do not think this should be merged. The ad hoc heuristics stink and is poor OS design since it mixes DHCPC application knowledge into the OS network. Not modular!. A proper design should be more systematic and not an ugly band-aid stuck in the code.
I suggested using SO_BINDTODEVICE as Linux uses for this purpose. This is clean, compatible and systematic. But no one has had the courtesy to even respond. Rather people are marching ahead with this bad solution.
SO_BINDTODEVICE is already implemented (as the protocol-specific UDP_BINDTODEVICE) but currently only addresses routing. A minor extension is required to support the filter of this PR. This extension is not really optional, but is in fact required by the specification of SO_BINDTODEVICE. The MAN page is a little cryptic, but here is a nice concise description of the required behaviors:
_"SO_BINDTODEVICE forces packets on the socket to only egress the bound interface, regardless of what the IP routing table would normally choose. Similarly only packets which ingress the bound interface will be received on the socket, packets from other interfaces will not be delivered to the socket."_ https://codingrelic.geekhold.com/2009/10/code-snippet-sobindtodevice.html
When the SO_BINDTODEVICE ingress logic is implemented properly (as required by specification), then there is no need for the change of this PR.
I would even be happy to implement the correct solution using the socket option if anyone should any interest in the correct solution. The solution would involve:
1. Add an IFF_BOUNDTODEVICE interface flag indicating that the device is bound to a UDP socket
2. Clear the flag is the option is cleared or the socket is closed.
3. Condition dropping of unrecognized UDP packets on IFF_BOUNDTODEVICE being clear (not bound)
Like Linux, all NuttX applications that want to force acceptance and routing of UDP packets to a socket should set the option. They already do:
$ grep -r BINDTODEVICE netutils
netutils/dhcpc/dhcpc.c:#ifdef CONFIG_NET_UDP_BINDTODEVICE
netutils/dhcpc/dhcpc.c: ret = setsockopt(pdhcpc->sockfd, IPPROTO_UDP, UDP_BINDTODEVICE,
netutils/dhcpc/dhcpc.c: ninfo("setsockopt(BINDTODEVICE) status %d\n", ret);
So no application change is required and only a small, clean, compatible OS change is required.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #3601: [BP-3598]net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #3601:
URL: https://github.com/apache/incubator-nuttx/pull/3601#issuecomment-825848125
> >
> >
> > Happy to run this test tonight with both DHCP v4 and v6 and share the capture if that would be helpful. I think I have everything to do this.
>
> I am not sure who you/what are responding too. One of the Googled references that I cite includes annotated DHCP and DHCPv6 captures.
>
I am saying I can run this test against this PR using DHCPv6 and request unicast.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] jerpelea commented on pull request #3601: [BP-3598]net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
jerpelea commented on pull request #3601:
URL: https://github.com/apache/incubator-nuttx/pull/3601#issuecomment-828212729
@xiaoxiang781216 @anchao can you remove the ipv6 so that we can continue the release
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #3601: [BP-3598]net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3601:
URL: https://github.com/apache/incubator-nuttx/pull/3601#issuecomment-826148816
> I think we should start by removing this special case from the ipv6 logic having looked more at the DHCPv6 implemention. I was not able to get my ipv6 client to quite work correctly last night, but I think the issue is unrelated to this.
If we opt for a protocol neutral solution like SO_BINDTODEVICE (which I believe we should), then the IPv6 case could be handled okay.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #3601: [BP-3598]net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3601:
URL: https://github.com/apache/incubator-nuttx/pull/3601#issuecomment-825845167
>
>
> Happy to run this test tonight with both DHCP v4 and v6 and share the capture if that would be helpful. I think I have everything to do this.
I am not sure who you/what are responding too. One of the Googled references that I cite includes annotated DHCP and DHCPv6 captures.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #3601: [BP-3598]net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #3601:
URL: https://github.com/apache/incubator-nuttx/pull/3601#issuecomment-826150811
> I am not sure what this means? Changes in code base? Configuration differences? Hardware configurations (like multiple network devices)? Anyway, a successful test result does not necessarily prove that a change is correct.
In this case the change is because of support for a new feature in the dhcp client. Before we hard coded the bootp flags to only allow broadcast. Then a change was added to allow for the less noisy unicast option (this is controlled by a Kconfig). At the time I verified the RFC as well as performed local testing to make sure it matched the RFC.
>
> The technical content of the change needs to be explainable based on RFC knowledge not just lucky test results. Successful testing is a necessary but not a sufficient condition for a good change.
Sure, but running an existing client that is deployed can help provide incite to the RFC as well as a reference point. For the IPv6 case I put low effort in here is was mostly just for me to help with my understanding of the problem.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #3601: [BP-3598]net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3601:
URL: https://github.com/apache/incubator-nuttx/pull/3601#issuecomment-826150069
> I was not able to get my ipv6 client to quite work correctly last night, but I think the issue is unrelated to this.
This is also a case where testing does not convince of the correctness. DHCP client was added and verified many years ago. The original bits were part of uIP. It worked fine for many years (maybe a dozen), then Sebastien had routing problems in 2018 and added SO_BINDTODEVICE (aka UDP_BINDTODEVICE).
The worked fine for 2-3 years then the packet filtering logic of these two PRs was needed.
I am not sure what this means? Changes in code base? Configuration differences? Hardware configurations (like multiple network devices)? Anyway, a successful test result does not necessarily prove that a change is correct.
The technical content of the change needs to be explainable based on RFC knowledge not just lucky test results. Successful testing is a necessary but not a sufficient condition for a good change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #3601: [BP-3598]net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #3601:
URL: https://github.com/apache/incubator-nuttx/pull/3601#issuecomment-825874476
Another option that we should consider is the socket option `SO_BINDTODEVICE`. This forces all packets sent and received on an interface to be routed only between the socket and the interface. Linux does this to handle the general UDP broadcast routing problem, but my understanding is that the option was originally developed for exactly the behavior addressed by this PR.
A UDP-only version,` UDP_BINDTODEVICE` was added to the system a few years back to solve a problem very similar to this. You can see that message thread here: _https://groups.google.com/g/nuttx/c/sz_c8N6_vWM/m/t13QUdSVAAAJ_
The kludge that we are trying to force into the system is only part of the issue. Even if the package is received properly, it will not be forward to the listening UDP socket _UNLESS_ you also set `UDP_BINDTODEVICE`. The DHCP client does that for exactly this reason:
```
netutils/dhcpc/dhcpc.c:#ifdef CONFIG_NET_UDP_BINDTODEVICE
netutils/dhcpc/dhcpc.c: ret = setsockopt(pdhcpc->sockfd, IPPROTO_UDP, UDP_BINDTODEVICE,
```
It was be a nicer design if the `UDP_BINDTODEVICE` socket option were the one and only "trigger" to the this behavior. Is this something that you would consider instead of two unrelated but equally necessary changes: This rather kludgey one and the nearly standard socket option.
Using the socket option would probably require a little more information in the device structure, such as the answer to "Is there a UDP socket bound to this device?" If so, that would trigger passing UDP packets through and on to the bound socket.
This feels a lot better to me but perhaps some of you have additional insights? It feels better to me because there is really only one feature: When bound to a device, all UDP packets go to the bound socket. Very nice.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #3601: [BP-3598]net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #3601:
URL: https://github.com/apache/incubator-nuttx/pull/3601#issuecomment-825842439
> I suppose I would agree with that _IF_ someone has studied the DHCPv6 protocol and verified that this is a correct change and not just a mindless, possibly incorrect clone of the IPv4 logic. Otherwise, it is better to wait _UNTIL_ that correct change is understood.
The differences between DHCPv4 and DHCPv6 are substantial. I don't think you can just dump this IPv4 logic into the IPv6 code. I think that just introduces bad logic which we are better off with.
But I am not a DHCPv6 expert and really do not know the answer (I did write the DHCP client/server but that was many years ago). Again, the differences are substantial and I think this kind of slam dunk engineering is probably not the way to go. We do need to at least review the packets a protocols are assure that the unverified IPv6 change is in the ballpark.
https://blog.zivaro.com/accounting-differences-dhcpv6-dhcp
https://packetpushers.net/ipv4-dhcp-vs-ipv6-dhcpv6/
[And many others that you can optain via a serach of DHCP+DHCPv6
Differences like these make me suspicious of the change:
"DHCPv6 uses ICMPv6 Router Advertisement (RA) and IPv6 multicast messages and DHCP uses broadcast IPv4 messages on the LAN." -- This suggests the UDP check may be incorrect???
"DHCPv6 uses link-local IPv6 addresses when communicating between client and relay/server (RFC 6939), and DHCP for IPv4 uses unsolicited broadcasts."
A proper analysis by someone semi-literate in DHCPv6 is really needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #3601: [BP-3598]net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3601:
URL: https://github.com/apache/incubator-nuttx/pull/3601#issuecomment-825874476
Another option that we should consider is the socket option `SO_BINDTODEVICE`. This forces all packets sent and received on an interface to be routed only between the socket and the interface.
A UDP-only version,` UDP_BINDTODEVICE` was added to the system a few years back to solve a problem very similar to this. You can see that message thread here: _https://groups.google.com/g/nuttx/c/sz_c8N6_vWM/m/t13QUdSVAAAJ_
The kludge that we are trying to force into the system is only part of the issue. Even if the package is received properly, it will not be forward to the listening UDP socket _UNLESS_ you also set `UDP_BINDTODEVICE`. The DHCP client does that for exactly this reason:
```
netutils/dhcpc/dhcpc.c:#ifdef CONFIG_NET_UDP_BINDTODEVICE
netutils/dhcpc/dhcpc.c: ret = setsockopt(pdhcpc->sockfd, IPPROTO_UDP, UDP_BINDTODEVICE,
```
It was be a nicer design if the `UDP_BINDTODEVICE` socket option were the one and only "trigger" to the this behavior. Is this something that you would consider instead of two unrelated but equally necessary changes: This rather kludgey one and the nearly standard socket option.
Using the socket option would probably require a little more information in the device structure, such as the answer to "Is there a UDP socket bound to this device?" If so, that would trigger passing UDP packets through and on to the bound socket.
This feels a lot better to me but perhaps some of you have additional insights?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #3601: [BP-3598]net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #3601:
URL: https://github.com/apache/incubator-nuttx/pull/3601#issuecomment-825842439
> I suppose I would agree with that _IF_ someone has studied the DHCPv6 protocol and verified that this is a correct change and not just a mindless, possibly incorrect clone of the IPv4 logic. Otherwise, it is better to wait _UNTIL_ that correct change is understood.
The differences between DHCPv4 and DHCPv6 are substantial. I don't think you can just dump this IPv4 logic into the IPv6 code. I think that just introduces bad logic which we are better off with.
But I am not a DHCPv6 expert and really do not know the answer (I did write the IPv4 DHCP client/server but that was many years ago). Again, the differences are substantial and I think this kind of slam dunk engineering is probably not the way to go. We do need to at least review the packets a protocols are assure that the unverified IPv6 change is in the ballpark.
https://blog.zivaro.com/accounting-differences-dhcpv6-dhcp
https://packetpushers.net/ipv4-dhcp-vs-ipv6-dhcpv6/
[And many others that you can optain via a serach of DHCP+DHCPv6
Differences like these make me suspicious of the change:
"DHCPv6 uses ICMPv6 Router Advertisement (RA) and IPv6 multicast messages and DHCP uses broadcast IPv4 messages on the LAN." -- This suggests the UDP check may be incorrect???
"DHCPv6 uses link-local IPv6 addresses when communicating between client and relay/server (RFC 6939), and DHCP for IPv4 uses unsolicited broadcasts."
A proper analysis by someone semi-literate in DHCPv6 is really needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #3601: [BP-3598]net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #3601:
URL: https://github.com/apache/incubator-nuttx/pull/3601#issuecomment-825874476
Another option that we should consider is the socket option `SO_BINDTODEVICE`. This forces all packets sent and received on an interface to be routed only between the socket and the interface. Linux does this to handle the general UDP broadcast routing problem, but my understanding is that the option was originally developed for exactly the behavior addressed by this PR.
A UDP-only version,` UDP_BINDTODEVICE` was added to the system a few years back to solve a problem very similar to this. You can see that message thread here: _https://groups.google.com/g/nuttx/c/sz_c8N6_vWM/m/t13QUdSVAAAJ_ (not that specific message, but the overall thread).
The kludge that we are trying to force into the system is only part of the issue. Even if the package is received properly, it will not be forward to the listening UDP socket _UNLESS_ you also set `UDP_BINDTODEVICE`. The DHCP client does that for exactly this reason:
```
netutils/dhcpc/dhcpc.c:#ifdef CONFIG_NET_UDP_BINDTODEVICE
netutils/dhcpc/dhcpc.c: ret = setsockopt(pdhcpc->sockfd, IPPROTO_UDP, UDP_BINDTODEVICE,
```
It was be a nicer design if the `UDP_BINDTODEVICE` socket option were the one and only "trigger" to the this behavior. Is this something that you would consider instead of two unrelated but equally necessary changes: This rather kludgey one and the nearly standard socket option.
Using the socket option would probably require a little more information in the device structure, such as the answer to "Is there a UDP socket bound to this device?" If so, that would trigger passing UDP packets through and on to the bound socket.
This feels a lot better to me but perhaps some of you have additional insights? It feels better to me because there is really only one feature: When bound to a device, all UDP packets go to the bound socket. Very nice.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #3601: [BP-3598]net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3601:
URL: https://github.com/apache/incubator-nuttx/pull/3601#issuecomment-827358081
> > I am not sure what this means? Changes in code base? Configuration differences? Hardware configurations (like multiple network devices)? Anyway, a successful test result does not necessarily prove that a change is correct.
>
> In this case the change is because of support for a new feature in the dhcp client. Before we hard coded the bootp flags to only allow broadcast. Then a change was added to allow for the less noisy unicast option (this is controlled by a Kconfig). At the time I verified the RFC as well as performed local testing to make sure it matched the RFC.
>
One more note, we add this feature because there are some routes insist the requester must use the unicast option.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #3601: [BP-3598]net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #3601:
URL: https://github.com/apache/incubator-nuttx/pull/3601#issuecomment-825874476
Another option that we should consider is the socket option `SO_BINDTODEVICE`. This forces all packets sent and received on an interface to be routed only between the socket and the interface. Linux does this to handle the general UDP broadcast routing problem, but my understanding is that the option was originally developed for exactly the behavior addressed by this PR.
A UDP-only version,` UDP_BINDTODEVICE` was added to the system a few years back to solve a problem very similar to this. You can see that message thread here: _https://groups.google.com/g/nuttx/c/sz_c8N6_vWM/m/t13QUdSVAAAJ_ (not that specific message, but the overall thread).
The kludge that we are trying to force into the system is only part of the issue. Even if the package is received properly, it will not be forward to the listening UDP socket _UNLESS_ you also set `UDP_BINDTODEVICE`. The DHCP client does that for exactly this reason:
```
netutils/dhcpc/dhcpc.c:#ifdef CONFIG_NET_UDP_BINDTODEVICE
netutils/dhcpc/dhcpc.c: ret = setsockopt(pdhcpc->sockfd, IPPROTO_UDP, UDP_BINDTODEVICE,
```
It was be a nicer design if the `UDP_BINDTODEVICE` socket option were the one and only "trigger" to the this behavior. Is this something that you would consider instead of two unrelated but equally necessary changes: This rather kludgey one and the nearly standard socket option.
Using the socket option would probably require a little more information in the device structure, such as the answer to "Is there a UDP socket bound to this device?" If so, that would trigger passing UDP packets through and on to the bound socket.
This feels a lot better to me but perhaps some of you have additional insights? It feels better to me because there is really only one feature: When bound to a device, all UDP packets go to the bound socket. Unifying these closely related solutions would be a good thing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #3601: [BP-3598]net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3601:
URL: https://github.com/apache/incubator-nuttx/pull/3601#issuecomment-828449587
> @xiaoxiang781216 @anchao can you remove the ipv6 so that we can continue the release
Please don't rush a potentially bad change through. I think this design is not good and should not be merged. Let's at least have t;hat discussion before forcing this bad change into the repositories, please.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #3601: [BP-3598]net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3601:
URL: https://github.com/apache/incubator-nuttx/pull/3601#discussion_r619244920
##########
File path: net/devif/ipv6_input.c
##########
@@ -433,7 +433,17 @@ int ipv6_input(FAR struct net_driver_s *dev)
}
else
#endif
- if (nxthdr != IP_PROTO_UDP)
+#ifdef NET_UDP_HAVE_STACK
+ if (nxthdr == IP_PROTO_UDP &&
+ net_ipv6addr_cmp(dev->d_ipv6addr, g_ipv6_unspecaddr))
+ {
+ /* Accecpt the UDP packet if the devices has not obtained
+ * the IP address to solve the compatibility issue of DHCP
+ * BOOTP working on unicast mode.
+ */
+ }
+ else
+#endif
Review comment:
What is the rationale for this? I think it is incorrect... at a minimum, the comment is incorrect. DHCP is IPv4 only. Address assignment is normally done using other ICMPv6-based logic (router commands, auto configuration, neighbor discovery.
There is a UDP-based version of DHCPv6, but it is not frequently used. And neither client nor server DHCPv6 is available in NuttX.
I think that any UDP-based hacks in IPv6 should be removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] jerpelea commented on pull request #3601: [BP-3598]net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
jerpelea commented on pull request #3601:
URL: https://github.com/apache/incubator-nuttx/pull/3601#issuecomment-828255526
LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on pull request #3601: [BP-3598]net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #3601:
URL: https://github.com/apache/incubator-nuttx/pull/3601#issuecomment-828237160
> @xiaoxiang781216 @anchao can you remove the ipv6 so that we can continue the release
Hi, @jerpelea , ipv6 part has been removed, please review again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #3601: [BP-3598]net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3601:
URL: https://github.com/apache/incubator-nuttx/pull/3601#issuecomment-828762442
Superceded by PR #3624
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #3601: [BP-3598]net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #3601:
URL: https://github.com/apache/incubator-nuttx/pull/3601#issuecomment-825831336
Happy to run this test tonight with both DHCP v4 and v6 and share the capture if that would be helpful. I think I have everything to do this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #3601: [BP-3598]net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #3601:
URL: https://github.com/apache/incubator-nuttx/pull/3601#issuecomment-825842439
> I suppose I would agree with that _IF_ someone has studied the DHCPv6 protocol and verified that this is a correct change and not just a mindless, possibly incorrect clone of the IPv4 logic. Otherwise, it is better to wait _UNTIL_ that correct change is understood.
The differences between DHCPv4 and DHCPv6 are substantial. I don't think you can just dump this IPv4 logic into the IPv6 code. I think that just introduces bad logic which we are better off with.
But I am not a DHCPv6 expert and really do not know the answer (I did write the DHCP client/server but that was many years ago). Again, the differences are substantial and I think this kind of slam dunk engineering is probably not the way to go. We do need to at least review the packets a protocols are assure that the unverified IPv6 change is in the ballpark.
https://blog.zivaro.com/accounting-differences-dhcpv6-dhcp
https://packetpushers.net/ipv4-dhcp-vs-ipv6-dhcpv6/
[And many others that you can optain via a serach of DHCP+DHCPv6
Differences like these make me suspicious of the change:
"DHCPv6 uses ICMPv6 Router Advertisement (RA) and IPv6 multicast messages and DHCP uses broadcast IPv4 messages on the LAN."
"DHCPv6 uses link-local IPv6 addresses when communicating between client and relay/server (RFC 6939), and DHCP for IPv4 uses unsolicited broadcasts."
A proper analysis by someone semi-literate in DHCPv6 is really needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #3601: [BP-3598]net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3601:
URL: https://github.com/apache/incubator-nuttx/pull/3601#issuecomment-827714753
How about we remove IPv6 part and refine the implemenation in the next release? The change is little big large to check SO_BINDTODEVICE since it's hard to check tcp_conn_s from net_dev_s.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #3601: [BP-3598]net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #3601:
URL: https://github.com/apache/incubator-nuttx/pull/3601#issuecomment-826147191
I think we should start by removing this special case from the ipv6 logic having looked more at the DHCPv6 implemention. I was not able to get my ipv6 client to quite work correctly last night, but I think the issue is unrelated to this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #3601: [BP-3598]net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #3601:
URL: https://github.com/apache/incubator-nuttx/pull/3601#issuecomment-825733420
There are several reasons to apply the same change to IPv6:
1. DHCPv6(https://tools.ietf.org/html/rfc8415) is an official approved standard by IETF
2. More and more route support DHCPv6 by default
3. Kernel shouldn't block userspace implement proprietary DHCPv6 client or server
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #3601: [BP-3598]net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3601:
URL: https://github.com/apache/incubator-nuttx/pull/3601#issuecomment-825842439
> I suppose I would agree with that _IF_ someone has studied the DHCPv6 protocol and verified that this is a correct change and not just a mindless, possibly incorrect clone of the IPv4 logic. Otherwise, it is better to wait _UNTIL_ that correct change is understood.
The differences between DHCPv4 and DHCPv6 are substantial. I don't think you can just dump this IPv4 logic into the IPv6 code. I think that just introduces bad logic which we are better off with.
But I am not a DHCPv6 expert and really do not know the answer (I did write the DHCP client/server but that was many years ago). Again, the differences are substantial and I think this kind of slam dunk engineering is probably not the way to go. We do need to at least review the packets a protocols are assure that the unverified IPv6 change is in the ballpark.
https://blog.zivaro.com/accounting-differences-dhcpv6-dhcp
https://packetpushers.net/ipv4-dhcp-vs-ipv6-dhcpv6/
[And many others that you can optain via a serach of DHCP+DHCPv6
Differences like these make me suspicious of the change:
"DHCPv6 uses ICMPv6 Router Advertisement (RA) and IPv6 multicast messages and DHCP uses broadcast IPv4 messages on the LAN."
"DHCPv6 uses link-local IPv6 addresses when communicating between client and relay/server (RFC 6939), and DHCP for IPv4 uses unsolicited broadcasts."
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #3601: [BP-3598]net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #3601:
URL: https://github.com/apache/incubator-nuttx/pull/3601#issuecomment-825733420
There are several reasons to apply the same change to IPv6:
1. DHCPv6(https://tools.ietf.org/html/rfc8415) is an official approved standard by IETF
2. More and more route support DHCPv6 by default
3. Kernel shouldn't block userspace implement proprietary DHCPv6 client
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #3601: [BP-3598]net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #3601:
URL: https://github.com/apache/incubator-nuttx/pull/3601#issuecomment-828449587
> @xiaoxiang781216 @anchao can you remove the ipv6 so that we can continue the release
Please don't rush a potentially bad change through. I think this design is not good and should not be merged. Let's at least have that discussion before forcing this bad change into the repositories, please.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #3601: [BP-3598]net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3601:
URL: https://github.com/apache/incubator-nuttx/pull/3601#issuecomment-825733420
There are several reasons to apply the same change to IPv6:
1. DHCPv6(https://tools.ietf.org/html/rfc8415) is an official approach standard by IETF
2. More and more route support DHCPv6 by default
3. Kernel shouldn't block userspace implement proprietary DHCPv6 client
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #3601: [BP-3598]net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3601:
URL: https://github.com/apache/incubator-nuttx/pull/3601#issuecomment-825811957
>
>
> There are several reasons to apply the same change to IPv6:
>
> 1. DHCPv6(https://tools.ietf.org/html/rfc8415) is an official approved standard by IETF
>
> 2. More and more route support DHCPv6 by default
>
> 3. Kernel shouldn't block userspace implement proprietary DHCPv6 client or server
I suppose I would agree with that _IF_ someone has studied the DHCPv6 protocol and verified that this is a correct change and not just a mindless, possibly incorrect clone of the IPv4 logic.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 closed pull request #3601: [BP-3598]net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 closed pull request #3601:
URL: https://github.com/apache/incubator-nuttx/pull/3601
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #3601: [BP-3598]net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #3601:
URL: https://github.com/apache/incubator-nuttx/pull/3601#issuecomment-825861907
> > I am saying I can run this test against this PR using DHCPv6 and request unicast.
>
> OK. Sorry to be dense. You cannot use NuttX for that test, however, since it has no DHCPv6 client or server.
>
Sorry I was not clear. I have a out of tree dhcp client that I used previously. It has ipv6 support although I have only used it in ipv4.
I am also open to removing the ipv6 part for now if we want to wait. @xiaoxiang781216 do you have thoughts on that?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #3601: [BP-3598]net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #3601:
URL: https://github.com/apache/incubator-nuttx/pull/3601#issuecomment-825811957
>
>
> There are several reasons to apply the same change to IPv6:
>
> 1. DHCPv6(https://tools.ietf.org/html/rfc8415) is an official approved standard by IETF
>
> 2. More and more route support DHCPv6 by default
>
> 3. Kernel shouldn't block userspace implement proprietary DHCPv6 client or server
I suppose I would agree with that _IF_ someone has studied the DHCPv6 protocol and verified that this is a correct change and not just a mindless, possibly incorrect clone of the IPv4 logic. Otherwise, it is better to wait _UNTIL_ that correct change is understood.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #3601: [BP-3598]net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3601:
URL: https://github.com/apache/incubator-nuttx/pull/3601#issuecomment-828435824
I still do not think this should be merged. The ad hoc heuristics stink and is poor OS design. A proper design should be more systematic and not an ugly band-aid stuck in the code.
I suggested using SO_BINDTODEVICE as Linux uses for this purpose. This is clean, compatible and systematic. But no one has had the courtesy to even respond. Rather people are marching ahead with this bad solution.
I would even be happy to implement the correct solution using the socket option if anyone should any interest in the correct solution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #3601: [BP-3598]net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3601:
URL: https://github.com/apache/incubator-nuttx/pull/3601#discussion_r619244920
##########
File path: net/devif/ipv6_input.c
##########
@@ -433,7 +433,17 @@ int ipv6_input(FAR struct net_driver_s *dev)
}
else
#endif
- if (nxthdr != IP_PROTO_UDP)
+#ifdef NET_UDP_HAVE_STACK
+ if (nxthdr == IP_PROTO_UDP &&
+ net_ipv6addr_cmp(dev->d_ipv6addr, g_ipv6_unspecaddr))
+ {
+ /* Accecpt the UDP packet if the devices has not obtained
+ * the IP address to solve the compatibility issue of DHCP
+ * BOOTP working on unicast mode.
+ */
+ }
+ else
+#endif
Review comment:
What is the rationale for this? I think it is incorrect... at a minimum, the comment is incorrect. DHCP is IPv4 only. Address assignment is normally done using other ICMPv6-based logic (router commands, auto configuration, neighbor discovery.
There is a UDP-based version of DHCPv6, but it is not frequently used. And neither client nor server DHCPv6 is available in NuttX so any DHCPv6 packets must be dropped in any case.
I think that any UDP-based hacks in IPv6 should be removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #3601: [BP-3598]net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #3601:
URL: https://github.com/apache/incubator-nuttx/pull/3601#issuecomment-826147191
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #3601: [BP-3598]net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #3601:
URL: https://github.com/apache/incubator-nuttx/pull/3601#issuecomment-828435824
I still do not think this should be merged. The ad hoc heuristics stink and is poor OS design. A proper design should be more systematic and not an ugly band-aid stuck in the code.
I suggested using SO_BINDTODEVICE as Linux uses for this purpose. This is clean, compatible and systematic. But no one has had the courtesy to even respond. Rather people are marching ahead with this bad solution.
SO_BINDTODEVICE is already implemented (as the protocol-specific UDP_BINDTODEVICE) but currently only addresses routing. A minor extension is required to support the filter of this PR. This extension is not really optional, but is in fact required by the specification of SO_BINDTODEVICE. The MAN page is a little cryptic, but here is a nice concise description of the required behaviors:
_"SO_BINDTODEVICE forces packets on the socket to only egress the bound interface, regardless of what the IP routing table would normally choose. Similarly only packets which ingress the bound interface will be received on the socket, packets from other interfaces will not be delivered to the socket."_ https://codingrelic.geekhold.com/2009/10/code-snippet-sobindtodevice.html
When the SO_BINDTODEVICE ingress logic is implemented properly (as required by specification), then there is no need for the change of this PR.
I would even be happy to implement the correct solution using the socket option if anyone should any interest in the correct solution. The solution would involve:
1. Add an IFF_BOUNDTODEVICE interface flag indicating that the device is bound to a UDP socket
2. Clear the flag is the option is cleared or the socket is closed.
3. Condition dropping of unrecognized UDP packets on IFF_BOUNDTODEVICE being clear (not bound)
Like Linux, all NuttX applications that want to force acceptance and routing of UDP packets to a socket should set the option. They already do:
$ grep -r BINDTODEVICE netutils
netutils/dhcpc/dhcpc.c:#ifdef CONFIG_NET_UDP_BINDTODEVICE
netutils/dhcpc/dhcpc.c: ret = setsockopt(pdhcpc->sockfd, IPPROTO_UDP, UDP_BINDTODEVICE,
netutils/dhcpc/dhcpc.c: ninfo("setsockopt(BINDTODEVICE) status %d\n", ret);
So no application change is required and only a small, clean, compatible OS change is required.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #3601: [BP-3598]net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #3601:
URL: https://github.com/apache/incubator-nuttx/pull/3601#issuecomment-828435824
I still do not think this should be merged. The ad hoc heuristics stink and is poor OS design. A proper design should be more systematic and not an ugly band-aid stuck in the code.
I suggested using SO_BINDTODEVICE as Linux uses for this purpose. This is clean, compatible and systematic. But no one has had the courtesy to even respond. Rather people are marching ahead with this bad solution.
SO_BINDTODEVICE is already implemented (as the protocol-specific UDP_BINDTODEVICE) but currently only addresses routing. A minor extension is required to support the filter of this PR.
I would even be happy to implement the correct solution using the socket option if anyone should any interest in the correct solution. The solution would involve:
1. Add an IFF_BOUNDTODEVICE interface flag indicating that the device is bound to a UDP socket
2. Clear the flag is the option is cleared or the socket is closed.
3. Condition dropping of unrecognized UDP packets on IFF_BOUNDTODEVICE being clear (not bound)
Like Linux, all NuttX applications that want to force acceptance and routing of UDP packets to a socket should set the option. They already do:
$ grep -r BINDTODEVICE netutils
netutils/dhcpc/dhcpc.c:#ifdef CONFIG_NET_UDP_BINDTODEVICE
netutils/dhcpc/dhcpc.c: ret = setsockopt(pdhcpc->sockfd, IPPROTO_UDP, UDP_BINDTODEVICE,
netutils/dhcpc/dhcpc.c: ninfo("setsockopt(BINDTODEVICE) status %d\n", ret);
So no application change is required and only a small, clean, compatible OS change is required.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #3601: [BP-3598]net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #3601:
URL: https://github.com/apache/incubator-nuttx/pull/3601#issuecomment-828435824
I still do not think this should be merged. The ad hoc heuristics stink and is poor OS design. A proper design should be more systematic and not an ugly band-aid stuck in the code.
I suggested using SO_BINDTODEVICE as Linux uses for this purpose. This is clean, compatible and systematic. But no one has had the courtesy to even respond. Rather people are marching ahead with this bad solution.
SO_BINDTODEVICE is already implemented (as the protocol-specific UDP_BINDTODEVICE) but currently only addresses routing. A minor extension is required to support the filter of this PR.
I would even be happy to implement the correct solution using the socket option if anyone should any interest in the correct solution. The solution would involve:
1. Add an IFF_BOUNDTODEVICE interface flag indicating that the device is bound to a UDP socket
2. Clear the flag is the option is cleared or the socket is closed.
3. Condition dropping of unrecognized UDP packets on IFF_BOUNDTODEVICE being clear (not bound)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #3601: [BP-3598]net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3601:
URL: https://github.com/apache/incubator-nuttx/pull/3601#issuecomment-825857346
> I am saying I can run this test against this PR using DHCPv6 and request unicast.
OK. Sorry to be dense. You cannot use NuttX for that test, however, since it has no DHCPv6 client or server.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #3601: [BP-3598]net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3601:
URL: https://github.com/apache/incubator-nuttx/pull/3601#issuecomment-826148816
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org