radvd-project / radvd

radvd | Official repository: https://github.com/radvd-project/radvd
https://radvd.litech.org/
Other
203 stars 106 forks source link

Fix IP6 forwarding test #204

Closed jetomit closed 1 year ago

jetomit commented 1 year ago

Commit 17f1828 accidentally inverted the check for IP6 forwarding on Linux – check_ip6_forwarding returns 0 when forwarding is enabled.

robbat2 commented 1 year ago

Thanks; I tried to write out the truth table for it, would appreciate somebody checking my calculations.

I really think the check_ip6_forwarding return code is something we should improve the clarity on - if it had been called ip6_forwarding_disabled it would have been clearer, the code looks terrible as well.

Old:
radvert.nd_ra_router_lifetime = cease_adv ? 0 : (!check_ip6_forwarding() ? htons(ra_header_info->AdvDefaultLifetime) : 0)
cease_adv   check_ip6_forwarding    !check_ip6_forwarding   radvert.nd_ra_router_lifetime
1           0                       1                       0
1           1                       0                       0
cease_adv   check_ip6_forwarding    !check_ip6_forwarding   radvert.nd_ra_router_lifetime
0           0                       1                       htons(ra_header_info->AdvDefaultLifetime)
0           1                       0                       0

New:
_cease_adv = iface->state_info.cease_adv || !check_ip6_forwarding();
radvert.nd_ra_router_lifetime = _cease_adv ? 0 : htons(ra_header_info->AdvDefaultLifetime);

cease_adv   check_ip6_forwarding    !check_ip6_forwarding   _cease_adv  radvert.nd_ra_router_lifetime
1           0                       *1                      1           0
1           1                       *0                      1           0

cease_adv   check_ip6_forwarding    !check_ip6_forwarding   _cease_adv  radvert.nd_ra_router_lifetime
0           0                       1                       1           0
0           1                       0                       0           htons(ra_header_info->AdvDefaultLifetime)

Fixed:
_cease_adv = iface->state_info.cease_adv || check_ip6_forwarding();
radvert.nd_ra_router_lifetime = _cease_adv ? 0 : htons(ra_header_info->AdvDefaultLifetime);

cease_adv   check_ip6_forwarding    _cease_adv  radvert.nd_ra_router_lifetime
1           0                       1           0
1           1                       1           0

cease_adv   check_ip6_forwarding    _cease_adv  radvert.nd_ra_router_lifetime
0           0                       0           htons(ra_header_info->AdvDefaultLifetime)
0           1                       1           0
jetomit commented 1 year ago

Thanks; I tried to write out the truth table for it, would appreciate somebody checking my calculations.

FWIW I did the same, with the same result.

I really think the check_ip6_forwarding return code is something we should improve the clarity on - if it had been called ip6_forwarding_disabled it would have been clearer, the code looks terrible as well.

I thought about inverting the return value, but renaming the function as you suggest is much clearer. It might make sense to also modify the check_ip6_iface_forwarding function to match – currently it returns 0 when forwarding is disabled for the interface.