[riot-notifications] [RIOT-OS/RIOT] gnrc_ipv6: remove obsolete and harmful reception code (#11745)

Martine Lenders notifications at github.com
Tue Jun 25 20:24:27 CEST 2019


<!--
The RIOT community cares a lot about code quality.
Therefore, before describing what your contribution is about, we would like
you to make sure that your modifications are compliant with the RIOT
coding conventions, see https://github.com/RIOT-OS/RIOT/wiki/Coding-conventions.
-->

### Contribution description
#### Observation
When subscribing to IPv6 packets on a router for sniffing, the NETIF header is released prematurely, because of a wrong `gnrc_pktbuf_start_write()` call. This test provided reproduces this error case.

#### Explanation
When reworking the reception of IPv6 packets I reset a previously set `ipv6` snip as follows  when the IPv6 extension handler returns a packet:

https://github.com/RIOT-OS/RIOT/blob/56085b10a0dd48fab8c7b5582fef161d6b13d1e1/sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c#L727-L729

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](https://github.com/RIOT-OS/RIOT/blob/56085b10a0dd48fab8c7b5582fef161d6b13d1e1/sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c#L665-L675)), 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:

https://github.com/RIOT-OS/RIOT/blob/56085b10a0dd48fab8c7b5582fef161d6b13d1e1/sys/net/gnrc/network_layer/ipv6/ext/gnrc_ipv6_ext.c#L215-L229

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`.

https://github.com/RIOT-OS/RIOT/blob/56085b10a0dd48fab8c7b5582fef161d6b13d1e1/sys/include/net/gnrc/ipv6/ext.h#L123

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` here:

https://github.com/RIOT-OS/RIOT/blob/56085b10a0dd48fab8c7b5582fef161d6b13d1e1/sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c#L765-L771

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.

https://github.com/RIOT-OS/RIOT/blob/56085b10a0dd48fab8c7b5582fef161d6b13d1e1/sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c#L773-L777

[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

<!--
Put here the description of your contribution:
- describe which part(s) of RIOT is (are) involved
- if it's a bug fix, describe the bug that it solves and how it is solved
- you can also give more information to reviewers about how to test your changes
-->


### Testing procedure
Build and run both `tests/gnrc_ipv6_ext` `tests/gnrc_ipv6_fwd_w_sub` on a board of you preference:

```
export BOARD="<you decide>"
make -C tests/gnrc_ipv6_ext all test
make -C tests/gnrc_ipv6_fwd_w_sub
```

`tests/gnrc_ipv6_ext` should succeed both in this PR and on master. `tests/gnrc_ipv6_fwd_w_sub` will fail on master (considering you cherry-picked the test with 47d633b to it).


<!--
Details steps to test your contribution:
- which test/example to compile for which board and is there a 'test' command
- how to know that it was not working/available in master
- the expected success test output
-->


### Issues/PRs references
None
<!--
Examples: Fixes #1234. See also #5678. Depends on PR #9876.

Please use keywords (e.g., fixes, resolve) with the links to the issues you
resolved, this way they will be automatically closed when your pull request
is merged. See https://help.github.com/articles/closing-issues-using-keywords/.
-->

You can view, comment on, or merge this pull request online at:

  https://github.com/RIOT-OS/RIOT/pull/11745

-- Commit Summary --

  * tests: add test case for pointer confusion
  * gnrc_ipv6: remove obsolete and harmful reception code

-- File Changes --

    M sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c (14)
    A tests/gnrc_ipv6_fwd_w_sub/Makefile (26)
    A tests/gnrc_ipv6_fwd_w_sub/README.md (10)
    A tests/gnrc_ipv6_fwd_w_sub/common.h (48)
    A tests/gnrc_ipv6_fwd_w_sub/main.c (193)
    A tests/gnrc_ipv6_fwd_w_sub/mockup_netif.c (80)
    A tests/gnrc_ipv6_fwd_w_sub/tests/01-run.py (37)

-- Patch Links --

https://github.com/RIOT-OS/RIOT/pull/11745.patch
https://github.com/RIOT-OS/RIOT/pull/11745.diff

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/11745
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190625/8423f942/attachment-0001.html>


More information about the notifications mailing list