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 04:27:23 UTC
[GitHub] [incubator-nuttx] anchao opened a new pull request #3598: net/ip: bypass UDP input only when the device address is invalid
anchao opened a new pull request #3598:
URL: https://github.com/apache/incubator-nuttx/pull/3598
## Summary
net/ip: bypass UDP input only when the device address is invalid
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.
Reference:
https: //tools.ietf.org/html/rfc1542
Signed-off-by: chao.an <an...@xiaomi.com>
## Impact
DHCP unicast
## Testing
```
NUTTX(DHCPC) -> HOSTAP(DHCPD)
0.0.0.0 -> 255.255.255.255 DISCOVER
192.168.31.1 -> 192.168.31.166 OFFER
0.0.0.0 -> 255.255.255.255 REQUEST
192.168.31.1 -> 192.168.31.166 ACK
```
## linked issue:
https://github.com/apache/incubator-nuttx/pull/3586
https://github.com/apache/incubator-nuttx/pull/3593
--
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 #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 #3598:
URL: https://github.com/apache/incubator-nuttx/pull/3598#issuecomment-828608814
> When the SO_BINDTODEVICE ingress logic is implemented properly (as required by specification), then there is no need for the change of this PR.
Even if this inappropriate PR is merged, the changes I suggest for SO_BINDTODEVICE are required behavior and still must be implemented to order to be compliant with Linux behavior. And once implemented, there is no need for this PR. This PR is the wrong 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] btashton commented on pull request #3598: net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #3598:
URL: https://github.com/apache/incubator-nuttx/pull/3598#issuecomment-825419436
The traffic must meet these requirements to follow this code path though right? I just want to make sure I am not overlooking something.
1) The traffic would have to be directed at the device hardware address.
2) Device interface has to be up.
3) Device IP Address must be 0.0.0.0
--
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 #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 #3598:
URL: https://github.com/apache/incubator-nuttx/pull/3598#issuecomment-828569203
> Yes, I have imagine this solution before, but it is fragile because mulitple socket can set SO_BINDTODEVICE option, a simple flags will break the last user. We need a bound count here at least.
No, only one socket can be bound to the device. At least that is my understanding. All incoming packets must go only to the bound socket, hence only one socket may be bound to a device. I think attempting to bind second socket will result in an error, but I am not sure which error.
We really must avoid explicit filtering logic like that proposed by this PR. It is not good OS design. Imagine what would happen to the network code if we support all special cases with hacks like this. We would end up with a mess.
--
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 #3598: net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3598:
URL: https://github.com/apache/incubator-nuttx/pull/3598#issuecomment-828762219
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] xiaoxiang781216 commented on pull request #3598: net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3598:
URL: https://github.com/apache/incubator-nuttx/pull/3598#issuecomment-828504058
> 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)
>
Yes, I have imagine this solution before, but it is fragile because mulitple socket can set SO_BINDTODEVICE option, a simple flags will break the last user. We need a bound count here at least.
--
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 #3598: net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3598:
URL: https://github.com/apache/incubator-nuttx/pull/3598#issuecomment-828569203
> Yes, I have imagine this solution before, but it is fragile because mulitple socket can set SO_BINDTODEVICE option, a simple flags will break the last user. We need a bound count here at least.
No, only one socket can be bound to the device. At least that is my understanding.
--
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 #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 #3598:
URL: https://github.com/apache/incubator-nuttx/pull/3598#issuecomment-828569203
> Yes, I have imagine this solution before, but it is fragile because mulitple socket can set SO_BINDTODEVICE option, a simple flags will break the last user. We need a bound count here at least.
No, only one socket can be bound to the device. At least that is my understanding. All incoming packets must go only to the bound socket, hence only one socket may be bound to a device. I think attempting to bind second socket will result in an error, but I am not sure which error.
--
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 #3598: net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #3598:
URL: https://github.com/apache/incubator-nuttx/pull/3598#issuecomment-825377029
Hi @btashton @antmerlino @patacongo @xiaoxiang781216 @jerpelea
Please help to review this PR, thanks!
--
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 #3598: net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3598:
URL: https://github.com/apache/incubator-nuttx/pull/3598#issuecomment-828464059
[From #3601
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 commented on a change in pull request #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 #3598:
URL: https://github.com/apache/incubator-nuttx/pull/3598#discussion_r619238232
##########
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] patacongo commented on pull request #3598: net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3598:
URL: https://github.com/apache/incubator-nuttx/pull/3598#issuecomment-826145734
Many of the most important technical comments have ended up on the companion PR #3601. If you are evaluating that, please read my comments/suggestions with regard to:
1. Removing al IPv6 changes. I think it is premature to make these IPv6 changes which do not appear complete or correct to me. DHCPv6 is a completely different creature and blindly copying the IPv4 changes is, I believe, in correct.
2. I recommended a more unifying design based on the existing support of the SO_BINDTODEVICE socket option. That option is intended to handle this case and the IPv4 change is really most correctly an part of the behavior of that socket option. That socket option works on IPv6 just as well without slamming in arbitrary cloned logic.
Again, see #3601 for better description.
--
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 #3598: net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 closed pull request #3598:
URL: https://github.com/apache/incubator-nuttx/pull/3598
--
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 #3598: net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3598:
URL: https://github.com/apache/incubator-nuttx/pull/3598#issuecomment-825485270
Yes, I think so.
--
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 #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 #3598:
URL: https://github.com/apache/incubator-nuttx/pull/3598#issuecomment-828906462
Ok, let's assume that only one user will call SO_BINDTODEVICE at the same time. Let's focus on #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] anchao commented on pull request #3598: net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #3598:
URL: https://github.com/apache/incubator-nuttx/pull/3598#issuecomment-828237529
Link: 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] patacongo edited a comment on pull request #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 #3598:
URL: https://github.com/apache/incubator-nuttx/pull/3598#issuecomment-828464059
[From #3601
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] patacongo edited a comment on pull request #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 #3598:
URL: https://github.com/apache/incubator-nuttx/pull/3598#issuecomment-828464059
[From #3601
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] xiaoxiang781216 commented on pull request #3598: net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3598:
URL: https://github.com/apache/incubator-nuttx/pull/3598#issuecomment-825735724
See my comment here: 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] antmerlino commented on pull request #3598: net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
antmerlino commented on pull request #3598:
URL: https://github.com/apache/incubator-nuttx/pull/3598#issuecomment-825417219
@btashton I just really don't like the prospect of unwanted data getting into the network when conditions align. But that's fine, it's probably overkill.
--
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 #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 #3598:
URL: https://github.com/apache/incubator-nuttx/pull/3598#discussion_r619238232
##########
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] btashton commented on pull request #3598: net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #3598:
URL: https://github.com/apache/incubator-nuttx/pull/3598#issuecomment-825414772
Personally I'm not sure this is really worth the extra knob here, what are you worried about @antmerlino? I don't see the overhead.
--
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 #3598: net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3598:
URL: https://github.com/apache/incubator-nuttx/pull/3598#issuecomment-826145734
Many of the most important technical comments have ended up on the companion PR #3601. If you are evaluating that, please read my comments/suggestions with regard to:
1. Removing al IPv6 changes. I think it is premature to make these IPv6 changes which do not appear complete or correct to me. DHCPv6 is a completely different creature and blindly copying the IPv4 changes is, I believe, in correct.
2. I recommended a more unifying design based on the existing support of the SO_BINDTODEVICE socket option. That option is intended to handle this case and the IPv4 change is really most correctly an part of the behavior of that socket option. That socket option works on IPv6 just as well without slamming in arbitrary cloned logic.
Again, see #3601 for better description.
--
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 #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 #3598:
URL: https://github.com/apache/incubator-nuttx/pull/3598#issuecomment-828464059
[From #3601
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] patacongo edited a comment on pull request #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 #3598:
URL: https://github.com/apache/incubator-nuttx/pull/3598#issuecomment-828569203
> Yes, I have imagine this solution before, but it is fragile because mulitple socket can set SO_BINDTODEVICE option, a simple flags will break the last user. We need a bound count here at least.
No, only one socket can be bound to the device. At least that is my understanding. All incoming packets must go only to the bound socket, hence only one socket may be bound to a device. I think attempting to bind second socket will result in an error, but I am not sure which error.
We really must avoid explicit filtering logic like that proposed by this PR. It is not good OS design.
--
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 #3598: net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3598:
URL: https://github.com/apache/incubator-nuttx/pull/3598#issuecomment-828608814
> When the SO_BINDTODEVICE ingress logic is implemented properly (as required by specification), then there is no need for the change of this PR.
Even if this inappropriate PR is merged, the changes I suggest for SO_BINDTODEVICE are required behavior and still must be implemented. And once implemented, there is no need for this PR. This PR is the wrong 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] xiaoxiang781216 commented on pull request #3598: net/ip: bypass UDP input only when the device address is invalid
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3598:
URL: https://github.com/apache/incubator-nuttx/pull/3598#issuecomment-828906462
Ok, let's assume that only one user will call SO_BINDTODEVICE at the same time.
--
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