From ea449f3f9e96d01de3f16c10f06ded16b15fc9ea Mon Sep 17 00:00:00 2001 From: "Martine S. Lenders" Date: Sat, 22 Jun 2019 01:26:37 +0200 Subject: [PATCH] gnrc_ipv6: remove obsolete and harmful reception code When reworking the reception of IPv6 packets I reset a previously set `ipv6` snip as follows when the IPv6 extension handler returns a packet (see first hunk of this commit): ```C ipv6 = pkt->next->next ``` With `gnrc_ipv6_ext` this makes *somewhat* sense, `pkt->next` was previously equal to `ipv6` and after the function call `pkt->next` is the marked extension header, while `pkt->next->next` is the IPv6 header. However, since `ipv6` is already write-protected i.e. `ipv6->users == 1` (see ll. 665-675), any additional call of `gnrc_pktbuf_start_write()` [won't][start-write-doc] duplicate the packet. In fact, the only `gnrc_pktbuf_start_write()` in `gnrc_ipv6_ext` is used to send the *result* to the subscribers of that extension header type, leaving the original packet unchanged for the caller. As such `ipv6` remains the pointer to the IPv6 header whether we set it in the line above or not. So we actually don't need that line. However, the extension header handling also returns a packet when `gnrc_ipv6_ext` is not compiled in. In that case it is just a dummy define that returns the packet you give provide it which means that this still holds true: `pkt->next == ipv6`. So setting `ipv6` in this case is actually harmful, as `ipv6` now points to the NETIF header [following the IPv6 header][pkt-structure] in the packet and this causes the `user` counter of that NETIF header `hdr` to be decremented if `hdr->users > 1` in the write-protection I removed in hunk 2 of this commit: ```C /* pkt might not be writable yet, if header was given above */ ipv6 = gnrc_pktbuf_start_write(ipv6); if (ipv6 == NULL) { DEBUG("ipv6: unable to get write access to packet: dropping it\n"); gnrc_pktbuf_release(pkt); return; } ``` But as we already established, `ipv6->users` is already 1, so we don't actually need the write protection here either. Since the packet stays unchanged after the `ipv6` snip, we also don't need to re-search for `netif_hdr` after the other two lines are removed. [start-write-doc]: https://doc.riot-os.org/group__net__gnrc__pktbuf.html#ga640418467294ae3d408c109ab27bd617 [pkt-structure]: https://doc.riot-os.org/group__net__gnrc__pkt.html#ga278e783e56a5ee6f1bd7b81077ed82a7 --- sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c b/sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c index c5d2fba31a..3e69b051eb 100644 --- a/sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c +++ b/sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c @@ -724,10 +724,7 @@ static void _receive(gnrc_pktsnip_t *pkt) ipv6_addr_to_str(addr_str, &(hdr->dst), sizeof(addr_str)), first_nh, byteorder_ntohs(hdr->len)); - if ((pkt = gnrc_ipv6_ext_process_hopopt(pkt, &first_nh)) != NULL) { - ipv6 = pkt->next->next; - } - else { + if ((pkt = gnrc_ipv6_ext_process_hopopt(pkt, &first_nh)) == NULL) { DEBUG("ipv6: packet's extension header was errorneous or packet was " "consumed due to it\n"); return; @@ -762,16 +759,7 @@ static void _receive(gnrc_pktsnip_t *pkt) else if (--(hdr->hl) > 0) { /* drop packets that *reach* Hop Limit 0 */ DEBUG("ipv6: forward packet to next hop\n"); - /* pkt might not be writable yet, if header was given above */ - ipv6 = gnrc_pktbuf_start_write(ipv6); - if (ipv6 == NULL) { - DEBUG("ipv6: unable to get write access to packet: dropping it\n"); - gnrc_pktbuf_release(pkt); - return; - } - /* remove L2 headers around IPV6 */ - netif_hdr = gnrc_pktsnip_search_type(pkt, GNRC_NETTYPE_NETIF); if (netif_hdr != NULL) { gnrc_pktbuf_remove_snip(pkt, netif_hdr); }