Re: [PATCH net-next] ipv6: reset all.force_forwarding when disabling all.forwarding

[netdev-bot+sashiko]

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

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

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] In net/ipv6/addrconf.c, addrconf_fixup_forwarding() (lines
  908-915) now sends inet6_netconf_notify_devconf(net, RTM_NEWNETCONF,…
- [Medium] ipv6: missing NETCONFA_FORCE_FORWARDING in
  inet6_netconf_fill_devconf()
- [Low] The patch fixes a user-visible bug but has no Fixes: tag.

--

ipv6: reset all.force_forwarding when disabling all.forwarding

When all.forwarding is disabled, this patch also clears
all.force_forwarding, so that writing 1 to all.force_forwarding later
re-enables the interfaces. It also sends a netconf notification for the
change.

> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index d9d13e78bf13b..9007a58039e65 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -905,6 +905,14 @@ static int addrconf_fixup_forwarding(const struct ctl_table *table, int *p, int
>  						     net->ipv6.devconf_dflt);
>  
>  		addrconf_forward_change(net, newf);
> +		/* Disabling all.forwarding also resets all.force_forwarding */
> +		if (newf == 0 && net->ipv6.devconf_all->force_forwarding) {
> +			WRITE_ONCE(net->ipv6.devconf_all->force_forwarding, 0);

[Severity: Low]
Should this patch carry a Fixes: tag for commit f24987ef6959, the commit
that added the force_forwarding sysctl?

That commit added the per-interface reset in addrconf_forward_change():

    if (newf == 0)
        WRITE_ONCE(idev->cnf.force_forwarding, 0);

It did not reset devconf_all->force_forwarding. Because
addrconf_sysctl_force_forwarding() only acts on a write when
old_val != new_val, writing 1 again does nothing.

Without the tag, stable kernels that have force_forwarding may not pick
up this fix.

> +			inet6_netconf_notify_devconf(net, RTM_NEWNETCONF,
> +						     NETCONFA_FORCE_FORWARDING,
> +						     NETCONFA_IFINDEX_ALL,
> +						     net->ipv6.devconf_all);

[Severity: Medium]
Does this notification carry the force_forwarding value at all?

inet6_netconf_notify_devconf() sizes the skb with
inet6_netconf_msgsize_devconf(type) and fills it with
inet6_netconf_fill_devconf(). Neither helper handles
NETCONFA_FORCE_FORWARDING. They only know about NETCONFA_FORWARDING,
NETCONFA_MC_FORWARDING, NETCONFA_PROXY_NEIGH and
NETCONFA_IGNORE_ROUTES_WITH_LINKDOWN.

So when all.forwarding is set to 0 while all.force_forwarding is set,
RTNLGRP_IPV6_NETCONF listeners get an RTM_NEWNETCONF for
NETCONFA_IFINDEX_ALL that contains only NETCONFA_IFINDEX. The size and
fill helpers agree with each other, so WARN_ON(err == -EMSGSIZE) in
inet6_netconf_notify_devconf() never fires. The message without a
value goes out silently.

An RTM_GETNETCONF request that ends up with all set in
inet6_netconf_fill_devconf() also omits force_forwarding. A listener
therefore cannot recover the new state by querying afterwards either.

The commit message says netconf listeners are notified. With this
message they cannot tell what changed or what the new value is.

The gap predates this patch. The existing force_forwarding
notifications in addrconf_sysctl_force_forwarding() and
addrconf_force_forward_change() have the same problem. This patch adds
one more caller that depends on that path.

Could NETCONFA_FORCE_FORWARDING handling be added to
inet6_netconf_msgsize_devconf() and inet6_netconf_fill_devconf(),
either as a preceding patch or in the same series?

[Severity: Medium]
Does this notification actually carry the new force_forwarding value?

inet6_netconf_fill_devconf() only puts NETCONFA_FORWARDING,
NETCONFA_MC_FORWARDING, NETCONFA_PROXY_NEIGH and
NETCONFA_IGNORE_ROUTES_WITH_LINKDOWN. It has no branch for
NETCONFA_FORCE_FORWARDING, so for this type it emits only the ifindex:

net/ipv6/addrconf.c:inet6_netconf_fill_devconf() {
    ...
	if ((all || type == NETCONFA_IGNORE_ROUTES_WITH_LINKDOWN) &&
	    nla_put_s32(skb, NETCONFA_IGNORE_ROUTES_WITH_LINKDOWN,
			READ_ONCE(devconf->ignore_routes_with_linkdown)) < 0)
		goto nla_put_failure;

out:
	nlmsg_end(skb, nlh);
    ...
}

inet6_netconf_msgsize_devconf() reserves no space for it either. The two
helpers agree, so no WARN fires.

As a result, RTNLGRP_IPV6_NETCONF listeners get an RTM_NEWNETCONF for
"all" with no NETCONFA_FORCE_FORWARDING attribute.

The commit message says "Reset all.force_forwarding too and notify netconf
listeners", but listeners cannot tell from this message what changed or
what the new value is.

The missing fill support dates back to f24987ef6959. The existing
force_forwarding notifications in addrconf_sysctl_force_forwarding() and
addrconf_force_forward_change() have the same gap. This patch adds one more
caller that depends on that path.

Could NETCONFA_FORCE_FORWARDING branches be added to both
inet6_netconf_msgsize_devconf() and inet6_netconf_fill_devconf(), either
as a prerequisite or in the same series?

> +		}
>  		if ((!newf) ^ (!old))
>  			inet6_netconf_notify_devconf(net, RTM_NEWNETCONF,
>  						     NETCONFA_FORWARDING,

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109-ipv6_reset_all-v1-1-815dfd508246%40proxmox.com