gcoap: Forego IP address comparison in memo finding of multicasts by chrysn · Pull Request #17978 · RIOT-OS/RIOT

chrysn · 2022-04-21T15:19:52Z

Contribution description

gcoap's _find_req_memo compares tokens and IP addresses. That's generally a good thing, but when requests are sent to a multicast address, it is not right (RFC7252 has an explicit exception there).

There may be other areas of multicast support that are lacking, but this is a small enhancement step.

Testing procedure

Have a local CoAP server that responds to multicasts (aiocoap-fileserver will do, even though that might be a bug there, gotta check out whether it's supposed to do that by default).

$ make -C examples/gcoap
> coap get ff01::2 5683 /.well-known/core
</>;ct=40

Previously, the request timed out.

Note that good multicast behavior would be to accept several servers' responses, and (in the command line client) to show the address of the server where the response came from, both is left for further work.

Issues/PRs references

Brought up by ad6sh in https://forum.riot-os.org/t/coap-resource-discovery/3599; the testing instructions are his.

chrysn · 2022-04-21T15:20:54Z

I should also point out that this only detects IPv6 multicast addresses; IPv4 multicast (broadcast) is harder to detect, and out of scope here. (I prefer not to sink time into a protocol that should long have been sunset).

benpicco · 2022-04-21T15:26:02Z

sys/net/application_layer/gcoap/gcoap.c

+                     /* Multicast addresses are not considered in matching responses */
+                     (
+                      memo->remote_ep.family == AF_INET6 &&
+                      ipv6_addr_is_multicast((const ipv6_addr_t *)&memo->remote_ep.addr.ipv6)


Want to move that to a helper function? sock_udp_ep_is_multicast()
(Or just a local one - _ep_is_multicast())

Yes, that'd be a good idea; will update when I get back to it.

benpicco · 2022-05-23T22:01:48Z

This just needs a tiny helper function to avoid cluttering the code like that

benpicco · 2022-08-09T16:24:38Z

How about this patch

diff --git a/sys/net/application_layer/gcoap/gcoap.c b/sys/net/application_layer/gcoap/gcoap.c
index e21f22211f..6c57929ebc 100644
--- a/sys/net/application_layer/gcoap/gcoap.c
+++ b/sys/net/application_layer/gcoap/gcoap.c
@@ -764,6 +764,12 @@ static int _find_resource(gcoap_socket_type_t tl_type,
     return ret;
 }
 
+static bool _memo_ep_is_multicast(const gcoap_request_memo_t *memo)
+{
+    return memo->remote_ep.family == AF_INET6 &&
+           ipv6_addr_is_multicast((const ipv6_addr_t *)&memo->remote_ep.addr.ipv6);
+}
+
 /*
  * Finds the memo for an outstanding request within the _coap_state.open_reqs
  * array. Matches on remote endpoint and token.
@@ -803,15 +809,10 @@ static void _find_req_memo(gcoap_request_memo_t **memo_ptr, coap_pkt_t *src_pdu,
             }
         } else if (coap_get_token_len(memo_pdu) == cmplen) {
             memo_pdu->token = coap_hdr_data_ptr(memo_pdu->hdr);
-            if ((memcmp(src_pdu->token, memo_pdu->token, cmplen) == 0)
-                    && (
-                     sock_udp_ep_equal(&memo->remote_ep, remote) ||
-                     /* Multicast addresses are not considered in matching responses */
-                     (
-                      memo->remote_ep.family == AF_INET6 &&
-                      ipv6_addr_is_multicast((const ipv6_addr_t *)&memo->remote_ep.addr.ipv6)
-                     )
-                     )) {
+            if ((memcmp(src_pdu->token, memo_pdu->token, cmplen) == 0) &&
+                (sock_udp_ep_equal(&memo->remote_ep, remote) ||
+                 /* Multicast addresses are not considered in matching responses */
+                 _memo_ep_is_multicast(memo))) {
                 *memo_ptr = memo;
                 break;
             }

Co-Authored-By: Benjamin Valentin <[email protected]>

chrysn · 2022-08-09T17:10:12Z

Thanks, I lost track of this one. Added your patch, and squashed onto current master to get rid of a merge conflict due to get_token now being an accessor.

18257: drivers/wdt: add periph_wdt_auto_start for early watchdog r=benpicco a=benpicco 19272: gcoap: Do not send responses from multicast addresses r=chrysn a=chrysn ### Contribution description Since #18026, CoAP requests to multicast addresses (eg. `ff02::1`) came back from that exact address, which Linux rightfully just drops. The fix uses the existing multicast check from #17978 (thanks `@benpicco` for making me write this as dedicated function, I just had to generalize it removing one struct layer), and foregoes setting the source address when responding to multicasts. ### Testing procedure * Run the gcoap example * Send a CoAP request to a multicast address RIOT listens to, eg. `./aiocoap-client coap://'[ff02::1%tapbr0]'/.well-known/core --non` Before, this got no response (while you see it arrive on wireshark). After, you get a correct response with two lines of note: ``` WARNING:coap:Sending request to multicast via unicast request method Response arrived from different address; base URI is coap://[fe80::3c63:beff:fe85:ca96%tapbr0]/.well-known/core ``` (The former is aiocoap telling us that we're not using the nonexistent multicast API so it's really more of an anycast, the latter is useful factual information). Co-authored-by: Benjamin Valentin <[email protected]> Co-authored-by: chrysn <[email protected]>

19272: gcoap: Do not send responses from multicast addresses r=benpicco a=chrysn ### Contribution description Since #18026, CoAP requests to multicast addresses (eg. `ff02::1`) came back from that exact address, which Linux rightfully just drops. The fix uses the existing multicast check from #17978 (thanks `@benpicco` for making me write this as dedicated function, I just had to generalize it removing one struct layer), and foregoes setting the source address when responding to multicasts. ### Testing procedure * Run the gcoap example * Send a CoAP request to a multicast address RIOT listens to, eg. `./aiocoap-client coap://'[ff02::1%tapbr0]'/.well-known/core --non` Before, this got no response (while you see it arrive on wireshark). After, you get a correct response with two lines of note: ``` WARNING:coap:Sending request to multicast via unicast request method Response arrived from different address; base URI is coap://[fe80::3c63:beff:fe85:ca96%tapbr0]/.well-known/core ``` (The former is aiocoap telling us that we're not using the nonexistent multicast API so it's really more of an anycast, the latter is useful factual information). Co-authored-by: chrysn <[email protected]>

chrysn requested review from PeterKietzmann, cgundogan, haukepetersen and miri64 as code owners April 21, 2022 15:19

github-actions bot added Area: network Area: Networking Area: sys Area: System labels Apr 21, 2022

benpicco reviewed Apr 21, 2022

View reviewed changes

ad6sh mentioned this pull request Apr 22, 2022

CoAP Resource Discovery using query parameters #17986

Open

gcoap: Forego IP address comparison in memo finding of multicasts

ab6bec6

Co-Authored-By: Benjamin Valentin <[email protected]>

chrysn force-pushed the gcoap-match-response-from-multicast branch from 6b09c84 to ab6bec6 Compare August 9, 2022 17:09

benpicco approved these changes Aug 9, 2022

View reviewed changes

chrysn enabled auto-merge August 9, 2022 17:36

chrysn merged commit 9ca149f into RIOT-OS:master Aug 10, 2022

chrysn deleted the gcoap-match-response-from-multicast branch August 18, 2022 15:49

maribu added this to the Release 2022.10 milestone Oct 14, 2022

chrysn mentioned this pull request Feb 13, 2023

gcoap: Do not send responses from multicast addresses #19272

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gcoap: Forego IP address comparison in memo finding of multicasts#17978

gcoap: Forego IP address comparison in memo finding of multicasts#17978
chrysn merged 1 commit intoRIOT-OS:masterfrom
chrysn-pull-requests:gcoap-match-response-from-multicast

chrysn commented Apr 21, 2022

Uh oh!

chrysn commented Apr 21, 2022

Uh oh!

benpicco Apr 21, 2022 •

edited

Loading

Uh oh!

chrysn Apr 22, 2022

Uh oh!

benpicco commented May 23, 2022

Uh oh!

benpicco commented Aug 9, 2022

Uh oh!

chrysn commented Aug 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

chrysn commented Apr 21, 2022

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

chrysn commented Apr 21, 2022

Uh oh!

benpicco Apr 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrysn Apr 22, 2022

Choose a reason for hiding this comment

Uh oh!

benpicco commented May 23, 2022

Uh oh!

benpicco commented Aug 9, 2022

Uh oh!

chrysn commented Aug 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

benpicco Apr 21, 2022 •

edited

Loading