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

sys/net/gnrc/tcp: fix gnrc_tcp_open() netif handling #20977

Merged
merged 1 commit into from
Nov 13, 2024

Conversation

maribu
Copy link
Member

@maribu maribu commented Nov 11, 2024

Contribution description

gnrc_tcp_open() previously would eventually fail with a timeout without sending any data when no netif was specified and a link-local target address was used. This fixes the behavior:

  • If there is only one netif, we just pick that
  • If there are multiple netifs, fail directly with -EINVAL rather than sending out nothing and waiting for a timeout.

Testing procedure

In one terminal run:

$ make BOARD=native -C tests/net/gnrc_sock_tcp -j && tests/net/gnrc_sock_tcp/bin/native/*.elf -w tap1
[...]
RIOT native interrupts/signals initialized.
RIOT native board initialized.
RIOT native hardware initialization complete.

main(): This is RIOT! (Version: 2025.01-devel-50-ge419c)
RIOT GNRC_TCP test application
> ifconfig
Iface  5  HWaddr: 26:DE:D0:85:BD:B2 
          L2-PDU:1500  MTU:1500  HL:64  Source address length: 6
          Link type: wireless
          inet6 addr: fe80::24de:d0ff:fe85:bdb2  scope: link  VAL
          inet6 group: ff02::1
          inet6 group: ff02::1:ff85:bdb2

> sock_tcp_listen [fe80::24de:d0ff:fe85:bdb2]:1234
sock_tcp_listen: argc=2, argv[0] = sock_tcp_listen, argv[1] = [fe80::24de:d0ff:fe85:bdb2]:1234
sock_tcp_listen: returns 0

In a second (while keeping the TCP server from the first terminal running) run:

$ make BOARD=native -C tests/net/gnrc_sock_tcp -j && tests/net/gnrc_sock_tcp/bin/native/*.elf -w tap0
[...]
RIOT native interrupts/signals initialized.
RIOT native board initialized.
RIOT native hardware initialization complete.

main(): This is RIOT! (Version: 2025.01-devel-50-ge419c)
RIOT GNRC_TCP test application
> sock_tcp_connect [fe80::24de:d0ff:fe85:bdb2]:1234 0
sock_tcp_connect: argc=3, argv[0] = sock_tcp_connect, argv[1] = [fe80::24de:d0ff:fe85:bdb2]:1234, argv[2] = 0
sock_tcp_connect: returns 0

Issues/PRs references

None

@github-actions github-actions bot added Area: network Area: Networking Area: sys Area: System labels Nov 11, 2024
@maribu maribu added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 11, 2024
@riot-ci
Copy link

riot-ci commented Nov 11, 2024

Murdock results

✔️ PASSED

e714707 sys/net/gnrc/tcp: fix gnrc_tcp_open() netif handling

Success Failures Total Runtime
10251 0 10251 15m:37s

Artifacts


/* We did not fine the netif, so rather than wasting more resources
* before timing out without sending anything, return an error */
if (tcb->ll_iface == SOCK_ADDR_ANY_NETIF) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (tcb->ll_iface == SOCK_ADDR_ANY_NETIF) {
if (ipv6_addr_is_link_local(tcb->peer_addr) && tcb->ll_iface == SOCK_ADDR_ANY_NETIF) {

Copy link
Member Author

Choose a reason for hiding this comment

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

I think for link local, we don't even need to check if only one netif is available: Any interface will be equally valid then, so we could always just take the first netif for link local addresses and unspecified netif.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, always chosing the first interface when no interface is specified is way too surprising/confusing.

Just return an error if you'd need to make a choice, like gnrc_udp does.

Copy link
Member Author

Choose a reason for hiding this comment

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

I cannot find where UDP does this. Do you know where the code lives? If that is implemented already, I could split this out info a common function and reuse this. That way, we can ensure that TCP and UDP will behave consistently in this regard.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I just free coded this for now.

sys/net/gnrc/transport_layer/tcp/gnrc_tcp.c Outdated Show resolved Hide resolved
@maribu maribu added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Nov 12, 2024
@maribu maribu changed the title sys/net/gnrc/tcp: improve gnrc_tcp_open() netif handling sys/net/gnrc/tcp: fix gnrc_tcp_open() netif handling Nov 12, 2024
@maribu maribu force-pushed the net/grnc/tcp/select-netif branch 2 times, most recently from 874311e to f36469a Compare November 12, 2024 11:06
gnrc_tcp_open() previously would eventually fail with a timeout without
sending any data when no netif was specified and a link-local target
address was used. This fixes the behavior:

- If there is only one netif, we just pick that
- If there are multiple netifs, fail directly with `-EINVAL` rather than
  sending out nothing and waiting for a timeout.

Co-authored-by: benpicco <benpicco@googlemail.com>
@maribu maribu added this pull request to the merge queue Nov 12, 2024
@maribu maribu removed this pull request from the merge queue due to a manual request Nov 12, 2024
@maribu maribu added this pull request to the merge queue Nov 12, 2024
@maribu maribu removed this pull request from the merge queue due to a manual request Nov 13, 2024
@maribu maribu added this pull request to the merge queue Nov 13, 2024
Merged via the queue into RIOT-OS:master with commit 2623762 Nov 13, 2024
25 checks passed
@maribu maribu deleted the net/grnc/tcp/select-netif branch November 13, 2024 13:30
@maribu
Copy link
Member Author

maribu commented Nov 13, 2024

Thx ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants