Skip to content

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

Merged
maribu merged 1 commit intoRIOT-OS:masterfrom
maribu:net/grnc/tcp/select-netif
Nov 13, 2024
Merged

sys/net/gnrc/tcp: fix gnrc_tcp_open() netif handling#20977
maribu merged 1 commit intoRIOT-OS:masterfrom
maribu:net/grnc/tcp/select-netif

Conversation

@maribu
Copy link
Copy Markdown
Member

@maribu maribu commented Nov 11, 2024 •
edited
Loading

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
Copy Markdown

riot-ci commented Nov 11, 2024 •
edited
Loading

Murdock results

✔️ PASSED

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

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

Artifacts

benpicco
benpicco reviewed Nov 11, 2024
View reviewed changes
sys/net/gnrc/transport_layer/tcp/gnrc_tcp.c Outdated

/* 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
Copy Markdown
Contributor

@benpicco benpicco Nov 11, 2024

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
Copy Markdown
Member Author

@maribu maribu Nov 12, 2024

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
Copy Markdown
Contributor

@benpicco benpicco Nov 12, 2024

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
Copy Markdown
Member Author

@maribu maribu Nov 12, 2024

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
Copy Markdown
Member Author

@maribu maribu Nov 12, 2024

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.

@maribu maribu force-pushed the net/grnc/tcp/select-netif branch from 68acbd0 to a0de15a Compare November 12, 2024 05:52
@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
benpicco
benpicco reviewed Nov 12, 2024
View reviewed changes
@maribu @benpicco
sys/net/gnrc/tcp: fix gnrc_tcp_open() netif handling
e714707 
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 <[email protected]>
@maribu maribu force-pushed the net/grnc/tcp/select-netif branch from f36469a to e714707 Compare November 12, 2024 14:44
@maribu maribu enabled auto-merge November 12, 2024 14:45
@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
@maribu maribu deleted the net/grnc/tcp/select-netif branch November 13, 2024 13:30
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Nov 13, 2024

Thx ❤️

@MrKevinWeiss MrKevinWeiss added this to the Release 2025.01 milestone Jan 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benpicco benpicco benpicco approved these changes

@miri64 miri64 Awaiting requested review from miri64 miri64 is a code owner

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

@brummer-simon brummer-simon Awaiting requested review from brummer-simon

Assignees

No one assigned

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

Milestone

Release 2025.01

Development

Successfully merging this pull request may close these issues.

4 participants

@maribu @riot-ci @benpicco @MrKevinWeiss