Re: [PATCH v3] ipv6: add `force_forwarding` sysctl to enable per-interface forwarding

[Gabriel Goller <g.goller@xxxxxxxxxxx>]

On 02.07.2025 12:05, Nicolas Dichtel wrote:
> Le 02/07/2025 à 09:46, Gabriel Goller a écrit :
> > It is currently impossible to enable ipv6 forwarding on a per-interface
> > basis like in ipv4. To enable forwarding on an ipv6 interface we need to
> > enable it on all interfaces and disable it on the other interfaces using
> > a netfilter rule. This is especially cumbersome if you have lots of
> > interface and only want to enable forwarding on a few. According to the
> > sysctl docs [0] the `net.ipv6.conf.all.forwarding` enables forwarding
> > for all interfaces, while the interface-specific
> > `net.ipv6.conf.<interface>.forwarding` configures the interface
> > Host/Router configuration.
> >
> > Introduce a new sysctl flag `force_forwarding`, which can be set on every
> > interface. The ip6_forwarding function will then check if the global
> > forwarding flag OR the force_forwarding flag is active and forward the
> > packet.
> >
> > To preserver backwards-compatibility reset the flag (on all interfaces)
> > to 0 if the net.ipv6.conf.all.forwarding flag is set to 0.
> >
> > [0]: https://www.kernel.org/doc/Documentation/networking/ip-sysctl.txt
> >
> > Signed-off-by: Gabriel Goller <g.goller@xxxxxxxxxxx>
> > ---
> Please, wait 24 hours before reposting.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/maintainer-netdev.rst#n419

Ah, my bad, thought I posted v2 in the morning as well :(

> [snip]
>
> > @@ -6747,6 +6759,77 @@ static int addrconf_sysctl_disable_policy(const struct ctl_table *ctl, int write
> >  	return ret;
> >  }
> >
> > +/* called with RTNL locked */
> Instead of a comment ...
>
> > +static void addrconf_force_forward_change(struct net *net, __s32 newf)
> > +{
> > +	struct net_device *dev;
> > +	struct inet6_dev *idev;
> > +
> ... put
>
> 	ASSERT_RTNL();

Agree.

> > +	for_each_netdev(net, dev) {
> > +		idev = __in6_dev_get_rtnl_net(dev);
> > +		if (idev) {
> > +			int changed = (!idev->cnf.force_forwarding) ^ (!newf);
> > +
> > +			WRITE_ONCE(idev->cnf.force_forwarding, newf);
> > +			if (changed) {
> > +				inet6_netconf_notify_devconf(dev_net(dev), RTM_NEWNETCONF,
> > +							     NETCONFA_FORCE_FORWARDING,
> > +							     dev->ifindex, &idev->cnf);
> > +			}
> > +		}
> > +	}
> > +}
> > +
> > +static int addrconf_sysctl_force_forwarding(const struct ctl_table *ctl, int write,
> > +					    void *buffer, size_t *lenp, loff_t *ppos)
> > +{
> > +	int *valp = ctl->data;
> > +	int ret;
> > +	int old, new;
> > +
> > +	// get extra params from table
> /* */ for comment
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst#n598

NAK
(https://lore.kernel.org/lkml/CA+55aFyQYJerovMsSoSKS7PessZBr4vNp-3QUUwhqk4A4_jcbg@xxxxxxxxxxxxxx/#r)

But I'll capitalize the first word.

> > +	struct inet6_dev *idev = ctl->extra1;
> > +	struct net *net = ctl->extra2;
> Reverse x-mas tree for the variables declaration
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/maintainer-netdev.rst#n368

Done.

> > +
> > +	// copy table and change extra params to min/max so we can use proc_douintvec_minmax
> > +	struct ctl_table lctl;
> > +
> > +	lctl = *ctl;
> > +	lctl.extra1 = SYSCTL_ZERO;
> > +	lctl.extra2 = SYSCTL_ONE;
> > +
> > +	old = *valp;
> > +	ret = proc_douintvec_minmax(&lctl, write, buffer, lenp, ppos);
> > +	new = *valp;
> I probably missed something. The new value is written in lctl. When is it
> written in ctl?

Ah, sorry there is something missing here.

This is supposed to look like this:

	struct inet6_dev *idev = ctl->extra1;
	struct net *net = ctl->extra2;
	int *valp = ctl->data;
	loff_t pos = *ppos;
	int new = *valp;
	int old = *valp;
	int ret;

	struct ctl_table lctl;

	lctl = *ctl;
	lctl.extra1 = SYSCTL_ZERO;
	lctl.extra2 = SYSCTL_ONE;
	lctl.data = &new;

	ret = proc_douintvec_minmax(&lctl, write, buffer, lenp, ppos);
	
	...

	if (write)
		WRITE_ONCE(*valp, new);
	if (ret)
		*ppos = pos;
	return ret;


> > +
> > +	if (write && old != new) {
> > +		if (!rtnl_net_trylock(net))
> > +			return restart_syscall();
> > +
> > +		if (valp == &net->ipv6.devconf_dflt->force_forwarding) {
> > +			inet6_netconf_notify_devconf(net, RTM_NEWNETCONF,
> > +						     NETCONFA_FORCE_FORWARDING,
> > +						     NETCONFA_IFINDEX_DEFAULT,
> > +						     net->ipv6.devconf_dflt);
> > +		} else if (valp == &net->ipv6.devconf_all->force_forwarding) {
> > +			inet6_netconf_notify_devconf(net, RTM_NEWNETCONF,
> > +						     NETCONFA_FORCE_FORWARDING,
> > +						     NETCONFA_IFINDEX_ALL,
> > +						     net->ipv6.devconf_all);
> > +
> > +			addrconf_force_forward_change(net, new);
> > +		} else {
> > +			inet6_netconf_notify_devconf(net, RTM_NEWNETCONF,
> > +						     NETCONFA_FORCE_FORWARDING,
> > +						     idev->dev->ifindex,
> > +						     &idev->cnf);
> > +		}
> > +		rtnl_net_unlock(net);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> >  static int minus_one = -1;
> >  static const int two_five_five = 255;
> >  static u32 ioam6_if_id_max = U16_MAX;

Thanks for the review!