Skip to content
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

Typoes and other problems observed when making a spin-off #115

Open
CasperVector opened this issue Sep 2, 2018 · 0 comments
Open

Typoes and other problems observed when making a spin-off #115

CasperVector opened this issue Sep 2, 2018 · 0 comments

Comments

@CasperVector
Copy link

CasperVector commented Sep 2, 2018

Until the recent introduction (DanielAdolfsson/ndppd@8ed35f84) of the autowire functionality (which has its own issues, eg. DanielAdolfsson/ndppd#32) in ndppd, the ability to automatically add necessary entries to the routing table when relaying NDP seemed to be a feature exclusive to odhcpd, which is not widely used outside of OpenWRT. To ease integration in regular Linux distros, I made 6brd, a spin-off version of odhcpd; during its creation, I noticed a few problems with the odhcpd codebase:

  • Typoes: LEASE_ATTR_MAX on this line should be ODHCPD_ATTR_MAX; true on this line should be add.

  • Technically, there is no difference between master and slave interfaces in the NDP relay machanism (although ndproxy_routing can usually be disabled on the master), so the interactions between master and ndp can be greatly simplified (cf. the invocation of 6brd, in comparison with line 229-267 in a reduced form of config.c): just ignore the master option, make the value hybrid an alias for relay, and only enable NDP relaying when there are at least two active interfaces; as the OpenWRT wiki does not mention hybrid as a value of ndp, I find this mostly backwards compatible.

  • While odhcpd does manipulate NDP proxy entries for addresses of the interfaces themselves when handling NETEV_ADDR6_ADD and NETEV_ADDR6_DEL events, it does not add these entries on startup. This seems fine in OpenWRT, perhaps because netifd and odhcpd are started in parallel, so odhcpd will notice when addresses are assigned to the interfaces; however, in application scenarios outside of OpenWRT, the addresses may well be assigned before odhcpd starts. 6brd does not care at all about the interface addresses, and instead recommends the user to use the same address for different interfaces and disable DAD for the address, so that the proposition "all interfaces addresses are accessible from all network segments" is still true; another reason for this recommendation is that some misconfigured Windows machines are known to send incorrect RAs, disrupting IPv6 connectivity on other machines, so static address with no DAD might be desirable. Nevertheless, if full management of the interfaces addresses is really wanted, I guess it would be trivial to add some startup code that calls netlink_get_interface_addrs() in conjunction with setup_addr_for_relaying().

  • odhcpd currently listens to ICMPv6 messages using a layer-2 mechanism, which is incompatible with TUN interfaces; and when this makes odhcpd fail to initialise on an interface, the return value of ndp_setup_interface() is ignored, which could break assumptions like "there are at least two active interfaces". Siimilar problems are observed elsewhere as well in the codebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant