Re: [PATCH net 0/6] ipv6: fix sysctl error handling and missing notifications

[Fernando Fernandez Mancera <fmancera@suse.de>]

On 6/18/26 6:22 PM, Fernando Fernandez Mancera wrote:
> While working on a different IPv6 patch series I have spotted multiple
> minor bugs around sysctl error handling and notifications. In general,
> they are not serious issues.
> 
> In addition, there is one more issue in forwarding sysctl as it does not
> check for CAP_NET_ADMIN for the namespace. I am keeping that patch out
> of this series and I am aiming it at the net-next tree once it re-opens.
> 
> Fernando Fernandez Mancera (6):
>    ipv6: fix error handling in disable_ipv6 sysctl
>    ipv6: fix error handling in ignore_routes_with_linkdown sysctl
>    ipv6: fix error handling in forwarding sysctl
>    ipv6: fix error handling in disable_policy sysctl
>    ipv6: reset value and position for proxy_ndp sysctl restart
>    ipv6: fix missing notification for ignore_routes_with_linkdown
> 
>   net/ipv6/addrconf.c | 35 +++++++++++++++++++++++++++--------
>   1 file changed, 27 insertions(+), 8 deletions(-)
> 

So I have checked the feedback from sashiko.dev [1]. In general, it is 
real but I have doubts about this one. While it is real, I am not so 
sure we actually care about this scenario. In any case, this is a 
pattern around kernel code and it would require fixes everywhere 
(IPv4/IPv6/other places (?)).

The easier thing to do is what Nicolas suggested about taking the lock 
before parsing the value i.e before calling proc_dointvec().

I also do wonder if I should fix them in individual commits or together 
in the same commit as they have different fixes tag. Or maybe we can aim 
this last part to net-next. Thoughts?

Please, see the feedback attached below.

Thanks!

[1] https://sashiko.dev/#/patchset/20260618162225.4588-1-fmancera%40suse.de

 > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
 > index 8ff015975e279..1cfb223476bd6 100644
 > --- a/net/ipv6/addrconf.c
 > +++ b/net/ipv6/addrconf.c
[ ... ]
 > @@ -6493,8 +6494,12 @@ static int addrconf_sysctl_proxy_ndp(const 
struct ctl_table *ctl, int write,
 >  	if (write && old != new) {
 >  		struct net *net = ctl->extra2;
 >
 > -		if (!rtnl_net_trylock(net))
 > +		if (!rtnl_net_trylock(net)) {
 > +			/* Restore the original values before restarting */
 > +			*valp = old;
 > +			*ppos = pos;
 >  			return restart_syscall();
 > +		}
 >

Does this rollback introduce a race condition that can silently discard
concurrent writes?
The sysctl core does not serialize concurrent writes to the same sysctl
parameter, and proc_dointvec() modifies the global sysctl memory pointed to
by ctl->data directly.
If Thread A and Thread B concurrently write to proxy_ndp, and Thread A fails
to acquire the RTNL lock, setting *valp = old will unconditionally reset the
global memory back to what Thread A observed before its write. This seems
like it would clobber Thread B's concurrent update.
If Thread B successfully acquires the RTNL lock, it will evaluate its local
old != new stack variables as true and emit a RTM_NEWNETCONF netlink
notification. However, the notification will read the corrupted *valp 
memory,
leaving the emitted event and internal sysctl state out of sync.
Could this be avoided by duplicating the ctl_table, pointing the data field
to a local stack variable, calling proc_dointvec() on the local variable,
and only updating the global *valp after successfully acquiring the lock,
similar to how it is handled in addrconf_sysctl_forward()?