Re: [PATCH net-next] ipv6: always apply all.force_forwarding to every interface

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new selftest test_all_force_forwarding() in
  tools/testing/selftests/net/ipv6_force_forwarding.sh fails every time…

--

ipv6: always apply all.force_forwarding to every interface

This commit changes addrconf_sysctl_force_forwarding() so that every write
to all.force_forwarding is applied to all interfaces, even when the stored
value does not change. It now takes RTNL for every write and sends netconf
notifications only for actual value changes. It also extends the
ipv6_force_forwarding.sh selftest to cover the new behaviour.

> diff --git a/tools/testing/selftests/net/ipv6_force_forwarding.sh b/tools/testing/selftests/net/ipv6_force_forwarding.sh
> index bf0243366caa7..383d1ec64f1c1 100755
> --- a/tools/testing/selftests/net/ipv6_force_forwarding.sh
> +++ b/tools/testing/selftests/net/ipv6_force_forwarding.sh

[ ... ]

> +test_all_force_forwarding() {
> +    local ret=0
> +
> +    echo "TEST: all.force_forwarding"
> +
> +    ip netns exec $ns2 sysctl -qw net.ipv6.conf.veth21.force_forwarding=0 || return 1
> +    ip netns exec $ns2 sysctl -qw net.ipv6.conf.all.force_forwarding=1 || return 1
> +    check_force_forwarding veth21 1 "all.force_forwarding applied to interfaces" || ret=1

[ ... ]

> +    # Disabling all.forwarding resets force_forwarding, including on all
> +    ip netns exec $ns2 sysctl -qw net.ipv6.conf.all.forwarding=1 || return 1
> +    ip netns exec $ns2 sysctl -qw net.ipv6.conf.all.forwarding=0 || return 1
> +    check_force_forwarding all 0 "all.forwarding=0 resets all.force_forwarding" || ret=1

[Severity: Medium]
Can this check ever pass? Earlier in test_all_force_forwarding() the
script writes all.force_forwarding=1, which stores 1 in
net->ipv6.devconf_all->force_forwarding.

The later all.forwarding=0 write goes through
addrconf_sysctl_forward()->addrconf_fixup_forwarding(). The devconf_all
branch there only updates devconf_dflt->forwarding and then calls
addrconf_forward_change(net, 0).

addrconf_forward_change() clears force_forwarding only in the
per-device configs:

net/ipv6/addrconf.c:addrconf_forward_change() {
    ...
	for_each_netdev(net, dev) {
		idev = __in6_dev_get_rtnl_net(dev);
		if (idev) {
			...
			if (newf == 0 && idev->cnf.force_forwarding) {
				WRITE_ONCE(idev->cnf.force_forwarding, 0);
    ...
}

Nothing resets devconf_all->force_forwarding (or
devconf_dflt->force_forwarding) when forwarding is disabled. So
sysctl -n net.ipv6.conf.all.force_forwarding should still return 1 at
this point. The check would then print "FAIL: all.forwarding=0 resets
all.force_forwarding (got 1, expected 0)", and the script would exit 1
on every run.

ip-sysctl.rst says the flag "will be reset on all interfaces". That
matches the code, not this assertion.

Should this assertion be dropped, or changed to expect 1? Or is the
intent for all.forwarding=0 to also reset conf/all/force_forwarding in
addrconf_fixup_forwarding()?

> +    check_force_forwarding veth21 0 "all.forwarding=0 resets interface force_forwarding" || ret=1
> +
> +    return $ret
> +}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261112-ipv6_always_apply-v1-1-8df846631005%40proxmox.com