Re: [PATCH 2/2 net-next v2] ipv4: handle devconf post-set actions on netlink updates

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

On 3/31/26 12:36 PM, Paolo Abeni wrote:
> On 3/27/26 1:02 PM, Fernando Fernandez Mancera wrote:
>> When IPv4 device configuration parameters are updated via netlink, the
>> kernel currently only updates the value. This bypasses several
>> post-modification actions that occur when these same parameters are
>> updated via sysctl, such as flushing the routing cache or emitting
>> RTM_NEWNETCONF notifications.
>>
>> This patch addresses the inconsistency by calling the
>> devinet_conf_post_set() helper inside inet_set_link_af(). If a flush is
>> required, we defer it until the netlink attribute parsing loop
>> completes.
> 
> IMHO the above deserve some additional self-test triggering the relevant
> code.
> 

Hi Paolo,

sure, I could include a selftest for this.

>> This ensures consistent behavior and side-effects for devconf changes,
>> regardless of whether they are initiated via sysctl or netlink.
>>
>> Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
>> ---
>> v2: handled forwarding notification and disabling LRO
>> ---
>>   net/ipv4/devinet.c | 29 +++++++++++++++++++++++++++--
>>   1 file changed, 27 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
>> index 8300516fb38f..a35b72662e43 100644
>> --- a/net/ipv4/devinet.c
>> +++ b/net/ipv4/devinet.c
>> @@ -2161,6 +2161,20 @@ static bool devinet_conf_post_set(struct net *net, struct ipv4_devconf *cnf,
>>   					    NETCONFA_IGNORE_ROUTES_WITH_LINKDOWN,
>>   					    ifindex, cnf);
>>   		break;
>> +	case IPV4_DEVCONF_FORWARDING:
>> +		if (new == 1) {
> 
> AI reviews says:
> 
> Does this check miss cases where forwarding is enabled with a value other
> than 1?
> The sysctl path allows enabling IP forwarding with any non-zero value. If
> a user sets IPV4_DEVCONF_FORWARDING to 2 via netlink, forwarding will be
> enabled but this check is bypassed, leaving Large Receive Offload (LRO)
> enabled. Could this cause LRO-coalesced packets to be forwarded without
> proper software segmentation?
> 

If a user sets IPV4_DEVCONF_FORWARDING to 2 via netlink the value would 
be rejected. There is netlink attribute validation and FORWARDING is set 
as boolean with the range [0, 1].

This cannot happen.

>> +			/* it is safe to use container_of() because forwarding case
>> +			 * is only used by the netlink path
>> +			 */
>> +			struct in_device *idev = container_of(cnf, struct in_device, cnf);
>> +
>> +			netif_disable_lro(idev->dev);
> 
> AI review says:
> 
> Is it safe to call netif_disable_lro() here without holding the device
> operations lock?
> While inet_set_link_af() runs under rtnl_lock(), netif_disable_lro()
> modifies device features and calls netdev_update_features(), which
> asserts that the device operations lock is held. Calling this without
> the lock might trigger an assertion or expose the device to data races
> with concurrent feature updates or BPF/XDP attachments.
> Should this use dev_disable_lro() instead, which acquires the required
> lock before disabling LRO?

It is not possible to call this without the holding the lock. The device 
operations lock is held when handling IFLA_AF_SPEC at 
net/core/rtnetlink.c. Calling netif_disable_lro() is the right thing to 
do IMHO. Keep on mind that FORWARDING case here is only triggered by 
netlink path and not sysctl.

Thanks,
Fernando.

> 
> /P
>