Skip to content

IPv6.connect: only return once any IP address is configured#432

Merged
MagnusS merged 6 commits into
mirage:masterfrom
hannesm:ipv6-wait
Sep 11, 2020
Merged

IPv6.connect: only return once any IP address is configured#432
MagnusS merged 6 commits into
mirage:masterfrom
hannesm:ipv6-wait

Conversation

@hannesm
Copy link
Copy Markdown
Member

@hannesm hannesm commented Sep 2, 2020

This reflects the behaviour of DHCP in the IPv4 stack: we have to temporarily
start the Netif.listen function to potentially receive neighbour
advertisements (and router advertisements etc.), and once an IP address moves
into the PREFERRED state, we signal completion to our caller.

This avoids busy-waiting polling loops in the tests (and all other user code
that first needs to ensure to be able to send out data).

It is not yet clear to me whether this is a sensible and useful solution (esp.
in a dual stack with IPv4 configured by DHCP setup) for the long term, but in
the medium term it is good enough: a IPv6 stack is only useful for transport
layers (UDP, TCP) if we can send and receive datagrams over it.

This reflects the behaviour of DHCP in the IPv4 stack: we have to temporarily
start the Netif.listen function to potentially receive neighbour
advertisements (and router advertisements etc.), and once an IP address moves
into the PREFERRED state, we signal completion to our caller.

This avoids busy-waiting polling loops in the tests (and all other user code
that first needs to ensure to be able to send out data).

It is not yet clear to me whether this is a sensible and useful solution (esp.
in a dual stack with IPv4 configured by DHCP setup) for the long term, but in
the medium term it is good enough: a IPv6 stack is only useful for transport
layers (UDP, TCP) if we can send and receive datagrams over it.
@hannesm hannesm requested a review from MagnusS September 2, 2020 21:04
@hannesm
Copy link
Copy Markdown
Member Author

hannesm commented Sep 2, 2020

now, we should add a test case where a neighbour advertisement is sent as a reply to the ND, and also check this error condition (it should not busy loop (or Ipv6.connect not returning), but rather blow up with some error message -- or try with a different IP address generated).

Copy link
Copy Markdown
Member

@MagnusS MagnusS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a reasonable solution to me, at least for now -- it avoids the caller having to add workarounds to wait for the IP after a call to connect and makes the ipv6 stack behave more as ipv4

- RFC 4861 (4.3) requires ND to _not_ contain the link-level address if source=unspecified
- RFC 4861 (4.4) requires NA to not have flag S set if multicast or unsolicited unicast advertisements

Ipv6.connect: install a timeout (3 seconds) -- if no IP address is configured after this timeout, fail (Lwt.fail)
@hannesm
Copy link
Copy Markdown
Member Author

hannesm commented Sep 9, 2020

@MagnusS I got DAD to work with a FreeBSD host system on the other side (see the last commit for the changes). If this is fine with you, I'd proceed to merge this (though test cases would be good to have as well).

@hannesm
Copy link
Copy Markdown
Member Author

hannesm commented Sep 9, 2020

is there already testing code which injects a packet into vnetif (and awaits an reply packet)? -- EDIT: I think test_icmpv4.ml is a good base to work out some tests for this issue (DAD).

likely there are more error conditions: what if only some IP addresses could be configured, but not all (i.e. only link-local but no global ones)? what if only globals could be configured, but no link local?

@MagnusS
Copy link
Copy Markdown
Member

MagnusS commented Sep 11, 2020

Changes LGTM and CI is green so merging.

@MagnusS MagnusS merged commit 49235d9 into mirage:master Sep 11, 2020
@hannesm hannesm deleted the ipv6-wait branch September 11, 2020 22:32
hannesm added a commit to hannesm/opam-repository that referenced this pull request Sep 22, 2020
CHANGES:

* Assorted IPv6 improvements (mirage/mirage-tcpip#428 mirage/mirage-tcpip#431 mirage/mirage-tcpip#432 @MagnusS @hannesm)
  - set length in packets to be sent
  - preserve updated ctx from Ndv6.handle
  - fix ICMP checksum computation
  - implement Mirage_stack.V6 signature
  - add connect, mtu, iperf tests
  - fix DAD protocol implementation (and test it)
  - avoid out of bounds accesses of IPv6 packets (check length before accessing)
* Fix 32 bit issues (@MagnusS)
* Implement stack-direct and tcp disconnect: tear down existing connections (mirage/mirage-tcpip#429 @hannesm)
* Treat broadcast address of network as broadcast as well (mirage/mirage-tcpip#430 @hannesm, reported in mirage/mirage-tcpip#427)
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

Successfully merging this pull request may close these issues.

2 participants