[PATCH 0/3 net-next] Misc. minor improvements on IPv4/IPv6 sysctl handling

[PATCH 0/3 net-next] Misc. minor improvements on IPv4/IPv6 sysctl handling

[Fernando Fernandez Mancera]

Minor improvements for sysctl proc handlers of IPv4 and IPv6, they were
found while working on [1] and [2].

[1] https://lore.kernel.org/netdev/20260712013941.4570-1-fmancera@suse.de/

[2] https://lore.kernel.org/netdev/20260622130857.5115-1-fmancera@suse.de/

Fernando Fernandez Mancera (3):
  ipv4: remove unnecessary reset of position pointer
  ipv6: remove unnecessary reset of position pointer
  ipv6: add CAP_NET_ADMIN check for forwarding and force_forwarding
    sysctl

 net/ipv4/devinet.c  |  4 +---
 net/ipv6/addrconf.c | 31 +++++++++++--------------------
 2 files changed, 12 insertions(+), 23 deletions(-)

-- 
2.55.0

[PATCH 1/3 net-next] ipv4: remove unnecessary reset of position pointer

[Fernando Fernandez Mancera]

The position pointer is only advanced if the return value of the proc
handler is positive at new_sync_write(). Therefore no need to manually
reset it when doing error handling.

Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
---
 net/ipv4/devinet.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index a35b72662e43..8b68fe37e6ba 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -2607,10 +2607,9 @@ static int devinet_conf_proc(const struct ctl_table *ctl, int write,
 static int devinet_sysctl_forward(const struct ctl_table *ctl, int write,
 				  void *buffer, size_t *lenp, loff_t *ppos)
 {
+	struct net *net = ctl->extra2;
 	int *valp = ctl->data;
 	int val = *valp;
-	loff_t pos = *ppos;
-	struct net *net = ctl->extra2;
 	int ret;
 
 	if (write && !ns_capable(net->user_ns, CAP_NET_ADMIN))
@@ -2623,7 +2622,6 @@ static int devinet_sysctl_forward(const struct ctl_table *ctl, int write,
 			if (!rtnl_net_trylock(net)) {
 				/* Restore the original values before restarting */
 				*valp = val;
-				*ppos = pos;
 				return restart_syscall();
 			}
 			if (valp == &IPV4_DEVCONF_ALL(net, FORWARDING)) {
-- 
2.55.0

[PATCH 2/3 net-next] ipv6: remove unnecessary reset of position pointer

[Fernando Fernandez Mancera]

The position pointer is only advanced if the return value of the proc
handler is positive at new_sync_write(). Therefore no need to manually
reset it when doing error handling.

Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
---
Note: The checkpatch warnings here are false positive, lctl cannot be
const in this context
---
 net/ipv6/addrconf.c | 24 ++++--------------------
 1 file changed, 4 insertions(+), 20 deletions(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index f1fe9ede1edb..f6fa2715b450 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -6363,10 +6363,9 @@ static void ipv6_ifa_notify(int event, struct inet6_ifaddr *ifp)
 static int addrconf_sysctl_forward(const struct ctl_table *ctl, int write,
 		void *buffer, size_t *lenp, loff_t *ppos)
 {
+	struct ctl_table lctl;
 	int *valp = ctl->data;
 	int val = *valp;
-	loff_t pos = *ppos;
-	struct ctl_table lctl;
 	int ret;
 
 	/*
@@ -6382,8 +6381,6 @@ static int addrconf_sysctl_forward(const struct ctl_table *ctl, int write,
 
 	if (write)
 		ret = addrconf_fixup_forwarding(ctl, valp, val);
-	if (ret)
-		*ppos = pos;
 	return ret;
 }
 
@@ -6462,10 +6459,9 @@ static int addrconf_disable_ipv6(const struct ctl_table *table, int *p, int newf
 static int addrconf_sysctl_disable(const struct ctl_table *ctl, int write,
 		void *buffer, size_t *lenp, loff_t *ppos)
 {
+	struct ctl_table lctl;
 	int *valp = ctl->data;
 	int val = *valp;
-	loff_t pos = *ppos;
-	struct ctl_table lctl;
 	int ret;
 
 	/*
@@ -6481,8 +6477,6 @@ static int addrconf_sysctl_disable(const struct ctl_table *ctl, int write,
 
 	if (write)
 		ret = addrconf_disable_ipv6(ctl, valp, val);
-	if (ret)
-		*ppos = pos;
 	return ret;
 }
 
@@ -6667,10 +6661,9 @@ int addrconf_sysctl_ignore_routes_with_linkdown(const struct ctl_table *ctl,
 						size_t *lenp,
 						loff_t *ppos)
 {
+	struct ctl_table lctl;
 	int *valp = ctl->data;
 	int val = *valp;
-	loff_t pos = *ppos;
-	struct ctl_table lctl;
 	int ret;
 
 	/* ctl->data points to idev->cnf.ignore_routes_when_linkdown
@@ -6685,8 +6678,6 @@ int addrconf_sysctl_ignore_routes_with_linkdown(const struct ctl_table *ctl,
 
 	if (write)
 		ret = addrconf_fixup_linkdown(ctl, valp, val);
-	if (ret)
-		*ppos = pos;
 	return ret;
 }
 
@@ -6767,10 +6758,9 @@ int addrconf_disable_policy(const struct ctl_table *ctl, int *valp, int val)
 static int addrconf_sysctl_disable_policy(const struct ctl_table *ctl, int write,
 				   void *buffer, size_t *lenp, loff_t *ppos)
 {
+	struct ctl_table lctl;
 	int *valp = ctl->data;
 	int val = *valp;
-	loff_t pos = *ppos;
-	struct ctl_table lctl;
 	int ret;
 
 	lctl = *ctl;
@@ -6782,9 +6772,6 @@ static int addrconf_sysctl_disable_policy(const struct ctl_table *ctl, int write
 	if (write && (*valp != val))
 		ret = addrconf_disable_policy(ctl, valp, val);
 
-	if (ret)
-		*ppos = pos;
-
 	return ret;
 }
 
@@ -6816,7 +6803,6 @@ static int addrconf_sysctl_force_forwarding(const struct ctl_table *ctl, int wri
 	int *valp = ctl->data;
 	int new_val = *valp;
 	int old_val = *valp;
-	loff_t pos = *ppos;
 	int ret;
 
 	tmp_ctl.extra1 = SYSCTL_ZERO;
@@ -6852,8 +6838,6 @@ static int addrconf_sysctl_force_forwarding(const struct ctl_table *ctl, int wri
 		rtnl_net_unlock(net);
 	}
 
-	if (ret)
-		*ppos = pos;
 	return ret;
 }
 
-- 
2.55.0

[PATCH 3/3 net-next] ipv6: add CAP_NET_ADMIN check for forwarding and force_forwarding sysctl

[Fernando Fernandez Mancera]

As commit 8292d7f6e871 ("net: ipv4: add capability check for net
administration") did for IPv4, make sure that CAP_NET_ADMIN is required
to modify IPv6 forwarding and force_forwarding sysctl. This keep the
consistency of permission check logic between both protocols.

Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
---
 net/ipv6/addrconf.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index f6fa2715b450..b2a0e6a4189d 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -6363,11 +6363,15 @@ static void ipv6_ifa_notify(int event, struct inet6_ifaddr *ifp)
 static int addrconf_sysctl_forward(const struct ctl_table *ctl, int write,
 		void *buffer, size_t *lenp, loff_t *ppos)
 {
+	struct net *net = ctl->extra2;
 	struct ctl_table lctl;
 	int *valp = ctl->data;
 	int val = *valp;
 	int ret;
 
+	if (write && !ns_capable(net->user_ns, CAP_NET_ADMIN))
+		return -EPERM;
+
 	/*
 	 * ctl->data points to idev->cnf.forwarding, we should
 	 * not modify it until we get the rtnl lock.
@@ -6805,6 +6809,9 @@ static int addrconf_sysctl_force_forwarding(const struct ctl_table *ctl, int wri
 	int old_val = *valp;
 	int ret;
 
+	if (write && !ns_capable(net->user_ns, CAP_NET_ADMIN))
+		return -EPERM;
+
 	tmp_ctl.extra1 = SYSCTL_ZERO;
 	tmp_ctl.extra2 = SYSCTL_ONE;
 	tmp_ctl.data = &new_val;
-- 
2.55.0

Re: [PATCH 1/3 net-next] ipv4: remove unnecessary reset of position pointer

[Ido Schimmel]

On Thu, Jul 16, 2026 at 10:37:11PM +0200, Fernando Fernandez Mancera wrote:
> The position pointer is only advanced if the return value of the proc
> handler is positive at new_sync_write(). Therefore no need to manually
> reset it when doing error handling.
> 
> Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>

Reviewed-by: Ido Schimmel <idosch@nvidia.com>

Re: [PATCH 2/3 net-next] ipv6: remove unnecessary reset of position pointer

[Ido Schimmel]

On Thu, Jul 16, 2026 at 10:37:12PM +0200, Fernando Fernandez Mancera wrote:
> The position pointer is only advanced if the return value of the proc
> handler is positive at new_sync_write(). Therefore no need to manually
> reset it when doing error handling.
> 
> Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>

Reviewed-by: Ido Schimmel <idosch@nvidia.com>

Re: [PATCH 3/3 net-next] ipv6: add CAP_NET_ADMIN check for forwarding and force_forwarding sysctl

[Ido Schimmel]

On Thu, Jul 16, 2026 at 10:37:13PM +0200, Fernando Fernandez Mancera wrote:
> As commit 8292d7f6e871 ("net: ipv4: add capability check for net
> administration") did for IPv4, make sure that CAP_NET_ADMIN is required
> to modify IPv6 forwarding and force_forwarding sysctl. This keep the
> consistency of permission check logic between both protocols.

What's the value beyond protocol consistency given that it creates
intra-protocol inconsistency (none of the other IPv6 sysctl handlers
have this check)? 8292d7f6e871 does not explain why only forwarding was
changed. As-is, I don't see much value in this change. Tested using
[1].

Before:

uid=0(root) gid=0(root) groups=0(root)
v4 fwd: 1
v4 rp_filter: 0
v6 fwd: 0
v6 accept_ra: 0

After:

uid=0(root) gid=0(root) groups=0(root)
v4 fwd: 1
v4 rp_filter: 0
v6 fwd: 1
v6 accept_ra: 0

[1]
#!/bin/bash

id
capsh --drop=cap_net_admin -- -c '
	sysctl -wq net.ipv4.conf.all.forwarding=1 &> /dev/null
	echo "v4 fwd: $?"
	sysctl -wq net.ipv4.conf.all.rp_filter=2 &> /dev/null
	echo "v4 rp_filter: $?"
	sysctl -wq net.ipv6.conf.all.forwarding=1 &> /dev/null
	echo "v6 fwd: $?"
	sysctl -wq net.ipv6.conf.all.accept_ra=2 &> /dev/null
	echo "v6 accept_ra: $?"
'

Re: [PATCH 3/3 net-next] ipv6: add CAP_NET_ADMIN check for forwarding and force_forwarding sysctl

[Fernando Fernandez Mancera]

On 7/21/26 2:24 PM, Ido Schimmel wrote:
> On Thu, Jul 16, 2026 at 10:37:13PM +0200, Fernando Fernandez Mancera wrote:
>> As commit 8292d7f6e871 ("net: ipv4: add capability check for net
>> administration") did for IPv4, make sure that CAP_NET_ADMIN is required
>> to modify IPv6 forwarding and force_forwarding sysctl. This keep the
>> consistency of permission check logic between both protocols.
> 
> What's the value beyond protocol consistency given that it creates
> intra-protocol inconsistency (none of the other IPv6 sysctl handlers
> have this check)? 8292d7f6e871 does not explain why only forwarding was
> changed. As-is, I don't see much value in this change. Tested using
> [1].
> 

Ideally, all IPv6 and IPv4 should require CAP_NET_ADMIN as they are 
indeed configuring networking interface configuration. I thought 
changing all of them all of a sudden would be too much so I went for 
forwarding only.

As to answer why it was done for IPv4 in first place, I guess it was 
done because forwarding is the most popular option and also quite 
relevant in terms of impact in the system behaviour.

If we don't want to merge this as it is, I see 3 ways forward:

1. drop this patch from the series
2. drop the permission check in IPv4 forwarding sysctl
3. extend the check to all sysctls of IPv4 and IPv6

Ideally I would go for 3. if it isn't too much in your opinion. 
Otherwise, likely 1.

Thanks,
Fernando.

> Before:
> 
> uid=0(root) gid=0(root) groups=0(root)
> v4 fwd: 1
> v4 rp_filter: 0
> v6 fwd: 0
> v6 accept_ra: 0
> 
> After:
> 
> uid=0(root) gid=0(root) groups=0(root)
> v4 fwd: 1
> v4 rp_filter: 0
> v6 fwd: 1
> v6 accept_ra: 0
> 
> [1]
> #!/bin/bash
> 
> id
> capsh --drop=cap_net_admin -- -c '
> 	sysctl -wq net.ipv4.conf.all.forwarding=1 &> /dev/null
> 	echo "v4 fwd: $?"
> 	sysctl -wq net.ipv4.conf.all.rp_filter=2 &> /dev/null
> 	echo "v4 rp_filter: $?"
> 	sysctl -wq net.ipv6.conf.all.forwarding=1 &> /dev/null
> 	echo "v6 fwd: $?"
> 	sysctl -wq net.ipv6.conf.all.accept_ra=2 &> /dev/null
> 	echo "v6 accept_ra: $?"
> '
> 

Re: [PATCH 3/3 net-next] ipv6: add CAP_NET_ADMIN check for forwarding and force_forwarding sysctl

[Ido Schimmel]

On Tue, Jul 21, 2026 at 03:46:38PM +0200, Fernando Fernandez Mancera wrote:
> On 7/21/26 2:24 PM, Ido Schimmel wrote:
> > On Thu, Jul 16, 2026 at 10:37:13PM +0200, Fernando Fernandez Mancera wrote:
> > > As commit 8292d7f6e871 ("net: ipv4: add capability check for net
> > > administration") did for IPv4, make sure that CAP_NET_ADMIN is required
> > > to modify IPv6 forwarding and force_forwarding sysctl. This keep the
> > > consistency of permission check logic between both protocols.
> > 
> > What's the value beyond protocol consistency given that it creates
> > intra-protocol inconsistency (none of the other IPv6 sysctl handlers
> > have this check)? 8292d7f6e871 does not explain why only forwarding was
> > changed. As-is, I don't see much value in this change. Tested using
> > [1].
> > 
> 
> Ideally, all IPv6 and IPv4 should require CAP_NET_ADMIN as they are indeed
> configuring networking interface configuration. I thought changing all of
> them all of a sudden would be too much so I went for forwarding only.
> 
> As to answer why it was done for IPv4 in first place, I guess it was done
> because forwarding is the most popular option and also quite relevant in
> terms of impact in the system behaviour.
> 
> If we don't want to merge this as it is, I see 3 ways forward:
> 
> 1. drop this patch from the series
> 2. drop the permission check in IPv4 forwarding sysctl
> 3. extend the check to all sysctls of IPv4 and IPv6
> 
> Ideally I would go for 3. if it isn't too much in your opinion. Otherwise,
> likely 1.

Please drop the patch given that it's merely trying to be consistent
with a change whose scope was never justified. Option 3 is not something
that fits in v2 of "Misc. minor improvements on IPv4/IPv6 sysctl
handling" and requires careful analysis / breakage assessment.