[PATCH net v4] ipv4: bump rt_genid when a relevant devconf value changes through netlink

[PATCH net v4] ipv4: bump rt_genid when a relevant devconf value changes through netlink

[Fernando Fernandez Mancera]

When modifying IPv4 devconf values using netlink for some relevant
fields, the rt_cache_flush() call was missing. In addition, disable LRO
if forwarding is enabled on the interface. The flushing and the
disabling of LRO only happens if the value changed. As the value is
converted to int when passing it to ipv4_devconf_set(), it is safe to
cast it earlier to compare it against the current value.

This is needed to avoid possible connectivity issues and ease the
responsibilities of user space tools. Note that this follows the
behavior for the sysctl function ipv4_doint_and_flush() and also
partially devinet_conf_proc() as the missing notification would be
handled in a follow-up patch.

Fixes: 9f0f7272ac95 ("ipv4: AF_INET link address family")
Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
---
v2: use netif_disable_lro() as we already hold netdev_need_ops_lock()
and use net_device struct passed as argument instead of using in_dev->dev
v3: separate it from the series and sent it to net tree instead
v4: handle BC_FORWARDING and ACCEPT_LOCAL too, modify logic to skip
useless calls to ipv4_devconf_set() due to value not modified. Finally,
ammend the commit message. 
---
 net/ipv4/devinet.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index 537bb6c315d2..6dd8fd0a1323 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -2098,6 +2098,8 @@ static int inet_set_link_af(struct net_device *dev, const struct nlattr *nla,
 {
 	struct in_device *in_dev = __in_dev_get_rtnl(dev);
 	struct nlattr *a, *tb[IFLA_INET_MAX+1];
+	struct net *net = dev_net(dev);
+	bool flush_cache = false;
 	int rem;
 
 	if (!in_dev)
@@ -2107,8 +2109,33 @@ static int inet_set_link_af(struct net_device *dev, const struct nlattr *nla,
 		return -EINVAL;
 
 	if (tb[IFLA_INET_CONF]) {
-		nla_for_each_nested(a, tb[IFLA_INET_CONF], rem)
-			ipv4_devconf_set(in_dev, nla_type(a), nla_get_u32(a));
+		nla_for_each_nested(a, tb[IFLA_INET_CONF], rem) {
+			int new_value = (int)nla_get_u32(a);
+
+			if (ipv4_devconf_get(in_dev, nla_type(a)) == new_value)
+				continue;
+
+			ipv4_devconf_set(in_dev, nla_type(a), new_value);
+			switch (nla_type(a)) {
+			case IPV4_DEVCONF_FORWARDING:
+				if (new_value)
+					netif_disable_lro(dev);
+				fallthrough;
+			case IPV4_DEVCONF_NOXFRM:
+			case IPV4_DEVCONF_NOPOLICY:
+			case IPV4_DEVCONF_PROMOTE_SECONDARIES:
+			case IPV4_DEVCONF_ROUTE_LOCALNET:
+			case IPV4_DEVCONF_DROP_UNICAST_IN_L2_MULTICAST:
+			case IPV4_DEVCONF_BC_FORWARDING:
+			case IPV4_DEVCONF_ACCEPT_LOCAL:
+				flush_cache = true;
+				break;
+			default:
+				break;
+			}
+		}
+		if (flush_cache)
+			rt_cache_flush(net);
 	}
 
 	return 0;
-- 
2.53.0

Re: [PATCH net v4] ipv4: bump rt_genid when a relevant devconf value changes through netlink

[David Ahern]

On 3/13/26 8:45 AM, Fernando Fernandez Mancera wrote:
> diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
> index 537bb6c315d2..6dd8fd0a1323 100644
> --- a/net/ipv4/devinet.c
> +++ b/net/ipv4/devinet.c
> @@ -2107,8 +2109,33 @@ static int inet_set_link_af(struct net_device *dev, const struct nlattr *nla,
>  		return -EINVAL;
>  
>  	if (tb[IFLA_INET_CONF]) {
> -		nla_for_each_nested(a, tb[IFLA_INET_CONF], rem)
> -			ipv4_devconf_set(in_dev, nla_type(a), nla_get_u32(a));
> +		nla_for_each_nested(a, tb[IFLA_INET_CONF], rem) {
> +			int new_value = (int)nla_get_u32(a);
> +
> +			if (ipv4_devconf_get(in_dev, nla_type(a)) == new_value)
> +				continue;
> +
> +			ipv4_devconf_set(in_dev, nla_type(a), new_value);
> +			switch (nla_type(a)) {
> +			case IPV4_DEVCONF_FORWARDING:
> +				if (new_value)
> +					netif_disable_lro(dev);
> +				fallthrough;
> +			case IPV4_DEVCONF_NOXFRM:
> +			case IPV4_DEVCONF_NOPOLICY:
> +			case IPV4_DEVCONF_PROMOTE_SECONDARIES:
> +			case IPV4_DEVCONF_ROUTE_LOCALNET:
> +			case IPV4_DEVCONF_DROP_UNICAST_IN_L2_MULTICAST:
> +			case IPV4_DEVCONF_BC_FORWARDING:
> +			case IPV4_DEVCONF_ACCEPT_LOCAL:
> +				flush_cache = true;
> +				break;
> +			default:
> +				break;
> +			}
> +		}
> +		if (flush_cache)
> +			rt_cache_flush(net);
>  	}
>  
>  	return 0;

This replicates logic in devinet_conf_proc for example. Why not refactor
devinet.c to handle changes to values in 1 place only.

Also, you are flushing the cache for more values than sysfs does.

Re: [PATCH net v4] ipv4: bump rt_genid when a relevant devconf value changes through netlink

[Fernando Fernandez Mancera]

On 3/13/26 3:56 PM, David Ahern wrote:
> On 3/13/26 8:45 AM, Fernando Fernandez Mancera wrote:
>> diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
>> index 537bb6c315d2..6dd8fd0a1323 100644
>> --- a/net/ipv4/devinet.c
>> +++ b/net/ipv4/devinet.c
>> @@ -2107,8 +2109,33 @@ static int inet_set_link_af(struct net_device *dev, const struct nlattr *nla,
>>   		return -EINVAL;
>>   
>>   	if (tb[IFLA_INET_CONF]) {
>> -		nla_for_each_nested(a, tb[IFLA_INET_CONF], rem)
>> -			ipv4_devconf_set(in_dev, nla_type(a), nla_get_u32(a));
>> +		nla_for_each_nested(a, tb[IFLA_INET_CONF], rem) {
>> +			int new_value = (int)nla_get_u32(a);
>> +
>> +			if (ipv4_devconf_get(in_dev, nla_type(a)) == new_value)
>> +				continue;
>> +
>> +			ipv4_devconf_set(in_dev, nla_type(a), new_value);
>> +			switch (nla_type(a)) {
>> +			case IPV4_DEVCONF_FORWARDING:
>> +				if (new_value)
>> +					netif_disable_lro(dev);
>> +				fallthrough;
>> +			case IPV4_DEVCONF_NOXFRM:
>> +			case IPV4_DEVCONF_NOPOLICY:
>> +			case IPV4_DEVCONF_PROMOTE_SECONDARIES:
>> +			case IPV4_DEVCONF_ROUTE_LOCALNET:
>> +			case IPV4_DEVCONF_DROP_UNICAST_IN_L2_MULTICAST:
>> +			case IPV4_DEVCONF_BC_FORWARDING:
>> +			case IPV4_DEVCONF_ACCEPT_LOCAL:
>> +				flush_cache = true;
>> +				break;
>> +			default:
>> +				break;
>> +			}
>> +		}
>> +		if (flush_cache)
>> +			rt_cache_flush(net);
>>   	}
>>   
>>   	return 0;
> 

Hi David,

> This replicates logic in devinet_conf_proc for example. Why not refactor
> devinet.c to handle changes to values in 1 place only.

I have been looking into it but seems like too much to do it on the net 
tree. I am preparing a patch for IPv4 unifying the logic as much as 
possible so we can get rid of this mess.

> Also, you are flushing the cache for more values than sysfs does.

I don't think so. The problem is that the handling of sysctl is 
scattered in several places.

IPV4_DEVCONF_FORWARDING is flushed at devinet_sysctl_forward().

IPV4_DEVCONF_NOXFRM, IPV4_DEVCONF_NOPOLICY, 
IPV4_DEVCONF_PROMOTE_SECONDARIES, IPV4_DEVCONF_ROUTE_LOCALNET, and 
IPV4_DEVCONF_DROP_UNICAST_IN_L2_MULTICAST are flushed at 
ipv4_doint_and_flush() used by macro DEVINET_SYSCTL_FLUSHING_ENTRY.

Finally, IPV4_DEVCONF_BC_FORWARDING and IPV4_DEVCONF_ACCEPT_LOCAL are 
flushed at devinet_conf_proc().

Yes, I know, the fact that they are scattered around is quite confusing. 
This could be all unified in a single place but that would be too much 
for net tree IMHO.

Please let me know if I am missing something.

Thanks,
Fernando.

Re: [PATCH net v4] ipv4: bump rt_genid when a relevant devconf value changes through netlink

[Jakub Kicinski]

On Fri, 13 Mar 2026 16:13:20 +0100 Fernando Fernandez Mancera wrote:
> > Also, you are flushing the cache for more values than sysfs does.  
> 
> I don't think so. The problem is that the handling of sysctl is 
> scattered in several places.
> 
> IPV4_DEVCONF_FORWARDING is flushed at devinet_sysctl_forward().
> 
> IPV4_DEVCONF_NOXFRM, IPV4_DEVCONF_NOPOLICY, 
> IPV4_DEVCONF_PROMOTE_SECONDARIES, IPV4_DEVCONF_ROUTE_LOCALNET, and 
> IPV4_DEVCONF_DROP_UNICAST_IN_L2_MULTICAST are flushed at 
> ipv4_doint_and_flush() used by macro DEVINET_SYSCTL_FLUSHING_ENTRY.
> 
> Finally, IPV4_DEVCONF_BC_FORWARDING and IPV4_DEVCONF_ACCEPT_LOCAL are 
> flushed at devinet_conf_proc().
> 
> Yes, I know, the fact that they are scattered around is quite confusing. 
> This could be all unified in a single place but that would be too much 
> for net tree IMHO.
> 
> Please let me know if I am missing something.

I think David means that for some knobs procfs only flushes when value
changes "in one direct", eg

                if (i == IPV4_DEVCONF_ACCEPT_LOCAL - 1 ||                       
                    i == IPV4_DEVCONF_ROUTE_LOCALNET - 1)                       
                        if ((new_value == 0) && (old_value != 0))               
                                rt_cache_flush(net);       

which means only flush on the 1 -> 0 transition.

Re: [PATCH net v4] ipv4: bump rt_genid when a relevant devconf value changes through netlink

[David Ahern]

On 3/14/26 10:06 AM, Jakub Kicinski wrote:
> On Fri, 13 Mar 2026 16:13:20 +0100 Fernando Fernandez Mancera wrote:
>>> Also, you are flushing the cache for more values than sysfs does.  
>>
>> I don't think so. The problem is that the handling of sysctl is 
>> scattered in several places.
>>
>> IPV4_DEVCONF_FORWARDING is flushed at devinet_sysctl_forward().
>>
>> IPV4_DEVCONF_NOXFRM, IPV4_DEVCONF_NOPOLICY, 
>> IPV4_DEVCONF_PROMOTE_SECONDARIES, IPV4_DEVCONF_ROUTE_LOCALNET, and 
>> IPV4_DEVCONF_DROP_UNICAST_IN_L2_MULTICAST are flushed at 
>> ipv4_doint_and_flush() used by macro DEVINET_SYSCTL_FLUSHING_ENTRY.
>>
>> Finally, IPV4_DEVCONF_BC_FORWARDING and IPV4_DEVCONF_ACCEPT_LOCAL are 
>> flushed at devinet_conf_proc().
>>
>> Yes, I know, the fact that they are scattered around is quite confusing. 
>> This could be all unified in a single place but that would be too much 
>> for net tree IMHO.
>>
>> Please let me know if I am missing something.
> 
> I think David means that for some knobs procfs only flushes when value
> changes "in one direct", eg
> 
>                 if (i == IPV4_DEVCONF_ACCEPT_LOCAL - 1 ||                       
>                     i == IPV4_DEVCONF_ROUTE_LOCALNET - 1)                       
>                         if ((new_value == 0) && (old_value != 0))               
>                                 rt_cache_flush(net);       
> 
> which means only flush on the 1 -> 0 transition.

logic subtleties like this example is why I was asking about a proper
refactor.

since this bug has been around for years, why worry about a patch for
-net and the back propagation to stable releases that will follow?

Re: [PATCH net v4] ipv4: bump rt_genid when a relevant devconf value changes through netlink

[Fernando Fernandez Mancera]

On 3/14/26 5:16 PM, David Ahern wrote:
> On 3/14/26 10:06 AM, Jakub Kicinski wrote:
>> On Fri, 13 Mar 2026 16:13:20 +0100 Fernando Fernandez Mancera wrote:
>>>> Also, you are flushing the cache for more values than sysfs does.
>>>
>>> I don't think so. The problem is that the handling of sysctl is
>>> scattered in several places.
>>>
>>> IPV4_DEVCONF_FORWARDING is flushed at devinet_sysctl_forward().
>>>
>>> IPV4_DEVCONF_NOXFRM, IPV4_DEVCONF_NOPOLICY,
>>> IPV4_DEVCONF_PROMOTE_SECONDARIES, IPV4_DEVCONF_ROUTE_LOCALNET, and
>>> IPV4_DEVCONF_DROP_UNICAST_IN_L2_MULTICAST are flushed at
>>> ipv4_doint_and_flush() used by macro DEVINET_SYSCTL_FLUSHING_ENTRY.
>>>
>>> Finally, IPV4_DEVCONF_BC_FORWARDING and IPV4_DEVCONF_ACCEPT_LOCAL are
>>> flushed at devinet_conf_proc().
>>>
>>> Yes, I know, the fact that they are scattered around is quite confusing.
>>> This could be all unified in a single place but that would be too much
>>> for net tree IMHO.
>>>
>>> Please let me know if I am missing something.
>>
>> I think David means that for some knobs procfs only flushes when value
>> changes "in one direct", eg
>>
>>                  if (i == IPV4_DEVCONF_ACCEPT_LOCAL - 1 ||
>>                      i == IPV4_DEVCONF_ROUTE_LOCALNET - 1)
>>                          if ((new_value == 0) && (old_value != 0))
>>                                  rt_cache_flush(net);
>>
>> which means only flush on the 1 -> 0 transition.
> 
> logic subtleties like this example is why I was asking about a proper
> refactor.
> 
> since this bug has been around for years, why worry about a patch for
> -net and the back propagation to stable releases that will follow?
> 

I don't mind re-routing this net-next and put it together with a 
re-work. I will drop the fixes tag in such case. Is that fine for you Jakub?

Thanks,
Fernando.