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

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

[Gabriel Goller]

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@proxmox.com>
---

Changelog:
v2 -> v3:
    * remove forwarding=0 setting force_forwarding=0 globally.
    * add min and max (0 and 1) value to sysctl.
v1 -> v2:
    * rename from `do_forwarding` to `force_forwarding`.
    * add global `force_forwarding` flag which will enable
      `force_forwarding` on every interface like the
      `ipv4.all.forwarding` flag.
    * `forwarding`=0 will disable global and per-interface
      `force_forwarding`.
    * export option as NETCONFA_FORCE_FORWARDING.

 Documentation/networking/ip-sysctl.rst |  5 ++
 include/linux/ipv6.h                   |  1 +
 include/uapi/linux/ipv6.h              |  1 +
 include/uapi/linux/netconf.h           |  1 +
 include/uapi/linux/sysctl.h            |  1 +
 net/ipv6/addrconf.c                    | 90 ++++++++++++++++++++++++++
 net/ipv6/ip6_output.c                  |  3 +-
 7 files changed, 101 insertions(+), 1 deletion(-)

diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
index 0f1251cce314..f709aed44cde 100644
--- a/Documentation/networking/ip-sysctl.rst
+++ b/Documentation/networking/ip-sysctl.rst
@@ -2292,6 +2292,11 @@ conf/all/forwarding - BOOLEAN
 proxy_ndp - BOOLEAN
 	Do proxy ndp.
 
+force_forwarding - BOOLEAN
+	Enable forwarding on this interface only -- regardless of the setting on
+	``conf/all/forwarding``. When setting ``conf.all.forwarding`` to 0,
+	the `force_forwarding` flag will be reset on all interfaces.
+
 fwmark_reflect - BOOLEAN
 	Controls the fwmark of kernel-generated IPv6 reply packets that are not
 	associated with a socket for example, TCP RSTs or ICMPv6 echo replies).
diff --git a/include/linux/ipv6.h b/include/linux/ipv6.h
index 5aeeed22f35b..5380107e466c 100644
--- a/include/linux/ipv6.h
+++ b/include/linux/ipv6.h
@@ -19,6 +19,7 @@ struct ipv6_devconf {
 	__s32		forwarding;
 	__s32		disable_policy;
 	__s32		proxy_ndp;
+	__s32		force_forwarding;
 	__cacheline_group_end(ipv6_devconf_read_txrx);
 
 	__s32		accept_ra;
diff --git a/include/uapi/linux/ipv6.h b/include/uapi/linux/ipv6.h
index cf592d7b630f..d4d3ae774b26 100644
--- a/include/uapi/linux/ipv6.h
+++ b/include/uapi/linux/ipv6.h
@@ -199,6 +199,7 @@ enum {
 	DEVCONF_NDISC_EVICT_NOCARRIER,
 	DEVCONF_ACCEPT_UNTRACKED_NA,
 	DEVCONF_ACCEPT_RA_MIN_LFT,
+	DEVCONF_FORCE_FORWARDING,
 	DEVCONF_MAX
 };
 
diff --git a/include/uapi/linux/netconf.h b/include/uapi/linux/netconf.h
index fac4edd55379..1c8c84d65ae3 100644
--- a/include/uapi/linux/netconf.h
+++ b/include/uapi/linux/netconf.h
@@ -19,6 +19,7 @@ enum {
 	NETCONFA_IGNORE_ROUTES_WITH_LINKDOWN,
 	NETCONFA_INPUT,
 	NETCONFA_BC_FORWARDING,
+	NETCONFA_FORCE_FORWARDING,
 	__NETCONFA_MAX
 };
 #define NETCONFA_MAX	(__NETCONFA_MAX - 1)
diff --git a/include/uapi/linux/sysctl.h b/include/uapi/linux/sysctl.h
index 8981f00204db..63d1464cb71c 100644
--- a/include/uapi/linux/sysctl.h
+++ b/include/uapi/linux/sysctl.h
@@ -573,6 +573,7 @@ enum {
 	NET_IPV6_ACCEPT_RA_FROM_LOCAL=26,
 	NET_IPV6_ACCEPT_RA_RT_INFO_MIN_PLEN=27,
 	NET_IPV6_RA_DEFRTR_METRIC=28,
+	NET_IPV6_FORCE_FORWARDING=29,
 	__NET_IPV6_MAX
 };
 
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index ba2ec7c870cc..cca78f75cf0c 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -239,6 +239,7 @@ static struct ipv6_devconf ipv6_devconf __read_mostly = {
 	.ndisc_evict_nocarrier	= 1,
 	.ra_honor_pio_life	= 0,
 	.ra_honor_pio_pflag	= 0,
+	.force_forwarding	= 0,
 };
 
 static struct ipv6_devconf ipv6_devconf_dflt __read_mostly = {
@@ -303,6 +304,7 @@ static struct ipv6_devconf ipv6_devconf_dflt __read_mostly = {
 	.ndisc_evict_nocarrier	= 1,
 	.ra_honor_pio_life	= 0,
 	.ra_honor_pio_pflag	= 0,
+	.force_forwarding	= 0,
 };
 
 /* Check if link is ready: is it up and is a valid qdisc available */
@@ -857,6 +859,15 @@ static void addrconf_forward_change(struct net *net, __s32 newf)
 		idev = __in6_dev_get_rtnl_net(dev);
 		if (idev) {
 			int changed = (!idev->cnf.forwarding) ^ (!newf);
+			/*
+			 * With the introduction of force_forwarding, we need to be backwards
+			 * compatible, so that means we need to set the force_forwarding flag
+			 * on every interface to 0 if net.ipv6.conf.all.forwarding is set to 0.
+			 * This allows the global forwarding flag to disable forwarding for
+			 * all interfaces.
+			 */
+			if (newf == 0)
+				WRITE_ONCE(idev->cnf.force_forwarding, newf);
 
 			WRITE_ONCE(idev->cnf.forwarding, newf);
 			if (changed)
@@ -5719,6 +5730,7 @@ static void ipv6_store_devconf(const struct ipv6_devconf *cnf,
 	array[DEVCONF_ACCEPT_UNTRACKED_NA] =
 		READ_ONCE(cnf->accept_untracked_na);
 	array[DEVCONF_ACCEPT_RA_MIN_LFT] = READ_ONCE(cnf->accept_ra_min_lft);
+	array[DEVCONF_FORCE_FORWARDING] = READ_ONCE(cnf->force_forwarding);
 }
 
 static inline size_t inet6_ifla6_size(void)
@@ -6747,6 +6759,77 @@ static int addrconf_sysctl_disable_policy(const struct ctl_table *ctl, int write
 	return ret;
 }
 
+/* called with RTNL locked */
+static void addrconf_force_forward_change(struct net *net, __s32 newf)
+{
+	struct net_device *dev;
+	struct inet6_dev *idev;
+
+	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
+	struct inet6_dev *idev = ctl->extra1;
+	struct net *net = ctl->extra2;
+
+	// 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;
+
+	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;
@@ -7217,6 +7300,13 @@ static const struct ctl_table addrconf_sysctl[] = {
 		.extra1		= SYSCTL_ZERO,
 		.extra2		= SYSCTL_TWO,
 	},
+	{
+		.procname	= "force_forwarding",
+		.data		= &ipv6_devconf.force_forwarding,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= addrconf_sysctl_force_forwarding,
+	},
 };
 
 static int __addrconf_sysctl_register(struct net *net, char *dev_name,
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 7bd29a9ff0db..c15ed4197416 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -509,7 +509,8 @@ int ip6_forward(struct sk_buff *skb)
 	u32 mtu;
 
 	idev = __in6_dev_get_safely(dev_get_by_index_rcu(net, IP6CB(skb)->iif));
-	if (READ_ONCE(net->ipv6.devconf_all->forwarding) == 0)
+	if ((idev && READ_ONCE(idev->cnf.force_forwarding) == 0) &&
+	    READ_ONCE(net->ipv6.devconf_all->forwarding) == 0)
 		goto error;
 
 	if (skb->pkt_type != PACKET_HOST)
-- 
2.39.5

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

[Nicolas Dichtel]

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@proxmox.com>
> ---
> 
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


[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();

> +	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

> +	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

> +
> +	// 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?

> +
> +	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;

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

[Jakub Kicinski]

On Wed,  2 Jul 2025 09:46:18 +0200 Gabriel Goller wrote:
> 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.

Should we invert the polarity? It appears that the condition below only
let's this setting _disable_ forwarding. IMO calling it "force" suggests
to the user that it will force it to be enabled.

Nicolas, how do you feel about asking for a selftest here? 
The functionality is fairly trivial from datapath PoV, but feels odd 
to merge uAPI these days without a selftest..

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

[Nicolas Dichtel]

Le 02/07/2025 à 16:34, Jakub Kicinski a écrit :
> On Wed,  2 Jul 2025 09:46:18 +0200 Gabriel Goller wrote:
>> 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.
> 
> Should we invert the polarity? It appears that the condition below only
> let's this setting _disable_ forwarding. IMO calling it "force" suggests
> to the user that it will force it to be enabled.
Not sure to follow you. When force_forwarding is set to 1 the forwarding is
always enabled.

sysctl | all.forwarding | iface.force_forwarding | packet processing from iface
       |      0         |           0            |        no forward
       |      0         |           1            |         forward
       |      1         |           0            |         forward
       |      1         |           1            |         forward

> 
> Nicolas, how do you feel about asking for a selftest here? 
> The functionality is fairly trivial from datapath PoV, but feels odd 
> to merge uAPI these days without a selftest..
No problem, let's do it right.

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

[Jakub Kicinski]

On Wed, 2 Jul 2025 17:14:42 +0200 Nicolas Dichtel wrote:
> > Should we invert the polarity? It appears that the condition below only
> > let's this setting _disable_ forwarding. IMO calling it "force" suggests
> > to the user that it will force it to be enabled.  
> Not sure to follow you. When force_forwarding is set to 1 the forwarding is
> always enabled.
> 
> sysctl | all.forwarding | iface.force_forwarding | packet processing from iface
>        |      0         |           0            |        no forward
>        |      0         |           1            |         forward
>        |      1         |           0            |         forward
>        |      1         |           1            |         forward

Ugh, I can't read comparisons to zero.
Let's switch to more sane logic:

	if (idev && !READ_ONCE(idev->cnf.force_forwarding) &&
	    !READ_ONCE(net->ipv6.devconf_all->forwarding))

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

[Nicolas Dichtel]

Le 02/07/2025 à 18:10, Jakub Kicinski a écrit :
> On Wed, 2 Jul 2025 17:14:42 +0200 Nicolas Dichtel wrote:
>>> Should we invert the polarity? It appears that the condition below only
>>> let's this setting _disable_ forwarding. IMO calling it "force" suggests
>>> to the user that it will force it to be enabled.  
>> Not sure to follow you. When force_forwarding is set to 1 the forwarding is
>> always enabled.
>>
>> sysctl | all.forwarding | iface.force_forwarding | packet processing from iface
>>        |      0         |           0            |        no forward
>>        |      0         |           1            |         forward
>>        |      1         |           0            |         forward
>>        |      1         |           1            |         forward
> 
> Ugh, I can't read comparisons to zero.
> Let's switch to more sane logic:
> 
> 	if (idev && !READ_ONCE(idev->cnf.force_forwarding) &&
> 	    !READ_ONCE(net->ipv6.devconf_all->forwarding))
+1

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

[Randy Dunlap]

On 7/2/25 3:05 AM, 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@proxmox.com>
>> ---


[snip]

>> +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

Hm, lots there from the BK to git transfer in 2005, with a few updates by Mauro, Jakub, and myself.


More recently (2016!), Linus said this:
  https://lore.kernel.org/lkml/CA+55aFyQYJerovMsSoSKS7PessZBr4vNp-3QUUwhqk4A4_jcbg@mail.gmail.com/

which seems to allow for "//" style commenting. But yeah, it hasn't been added to
coding-style.rst.

>> +	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

Shouldn't maintainer-netdev.rst contain something about netdev-style comment blocks?
(not that I'm offering since I think it's ugly)

-- 
~Randy

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

[Nicolas Dichtel]

Le 03/07/2025 à 00:26, Randy Dunlap a écrit :

[snip]

>>> +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
> 
> Hm, lots there from the BK to git transfer in 2005, with a few updates by Mauro, Jakub, and myself.
> 
> 
> More recently (2016!), Linus said this:
>   https://lore.kernel.org/lkml/CA+55aFyQYJerovMsSoSKS7PessZBr4vNp-3QUUwhqk4A4_jcbg@mail.gmail.com/
> 
> which seems to allow for "//" style commenting. But yeah, it hasn't been added to
> coding-style.rst.
I wasn't aware. I always seen '//' rejected.

> 
>>> +	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
> 
> Shouldn't maintainer-netdev.rst contain something about netdev-style comment blocks?
> (not that I'm offering since I think it's ugly)
> 
It has been removed:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=82b8000c28b5

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

[Gabriel Goller]

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@proxmox.com>
>> ---
>>
>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@mail.gmail.com/#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!

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

[Gabriel Goller]

On 02.07.2025 09:10, Jakub Kicinski wrote:
>On Wed, 2 Jul 2025 17:14:42 +0200 Nicolas Dichtel wrote:
>> > Should we invert the polarity? It appears that the condition below only
>> > let's this setting _disable_ forwarding. IMO calling it "force" suggests
>> > to the user that it will force it to be enabled.
>> Not sure to follow you. When force_forwarding is set to 1 the forwarding is
>> always enabled.
>>
>> sysctl | all.forwarding | iface.force_forwarding | packet processing from iface
>>        |      0         |           0            |        no forward
>>        |      0         |           1            |         forward
>>        |      1         |           0            |         forward
>>        |      1         |           1            |         forward
>
>Ugh, I can't read comparisons to zero.
>Let's switch to more sane logic:
>
>	if (idev && !READ_ONCE(idev->cnf.force_forwarding) &&
>	    !READ_ONCE(net->ipv6.devconf_all->forwarding))

Agree!

Thanks for the review.

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

[Nicolas Dichtel]

Le 03/07/2025 à 13:04, Gabriel Goller a écrit :
[snip]
>>> +    // 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@mail.gmail.com/#r)

I will follow the netdev maintainers' guidelines.

If the doc I pointed to is wrong, please update it. It will be easier to find
than a 9-year-old email.


Regards,
Nicolas

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

[Gabriel Goller]

On 03.07.2025 16:04, Nicolas Dichtel wrote:
>Le 03/07/2025 à 13:04, Gabriel Goller a écrit :
>[snip]
>>>> +    // 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@mail.gmail.com/#r)
>
>I will follow the netdev maintainers' guidelines.
>
>If the doc I pointed to is wrong, please update it. It will be easier to find
>than a 9-year-old email.

The netdev maintainers guideline doesn't contain anything about
comments. Also the coding-style doesn't prohibit single-line comments
with the `//` style.

The comments are kinda pointless though, I'll remove them in the next
version.

>Regards,
>Nicolas

Thanks

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

[Randy Dunlap]

On July 2, 2025 11:58:16 PM PDT, Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote:
>Le 03/07/2025 à 00:26, Randy Dunlap a écrit :
>
>[snip]
>
>>>> +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
>> 
>> Hm, lots there from the BK to git transfer in 2005, with a few updates by Mauro, Jakub, and myself.
>> 
>> 
>> More recently (2016!), Linus said this:
>>   https://lore.kernel.org/lkml/CA+55aFyQYJerovMsSoSKS7PessZBr4vNp-3QUUwhqk4A4_jcbg@mail.gmail.com/
>> 
>> which seems to allow for "//" style commenting. But yeah, it hasn't been added to
>> coding-style.rst.
>I wasn't aware. I always seen '//' rejected.
>
>> 
>>>> +	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
>> 
>> Shouldn't maintainer-netdev.rst contain something about netdev-style comment blocks?
>> (not that I'm offering since I think it's ugly)
>> 
>It has been removed:
>https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=82b8000c28b5
>

Oh, thanks.  Sorry I missed that patch. 



~Randy