net: ipconfig: dev_set_mtu call is incompatible with a number of Ethernet drivers

net: ipconfig: dev_set_mtu call is incompatible with a number of Ethernet drivers

[Graeme Smecher]

Hi all,

In a number of ethernet drivers, the MTU can't be changed on a running device. Here's one example (from drivers/net/ethernet/cadence/macb_main.c):

static int macb_change_mtu(struct net_device *dev, int new_mtu)
{
	if (netif_running(dev))
		return -EBUSY;
		dev->mtu = new_mtu;
		return 0;
}

This code is present in a number of other drivers:

- drivers/net/ethernet/renesas/sh_eth.c
- drivers/net/ethernet/cadence/macb_main.c
- drivers/net/ethernet/ni/nixge.c
- drivers/net/ethernet/dlink/sundance.c

In all but nixge, the reasoning is straightforward: device buffers are allocated with a size that depends on MTU, so the device has to be brought down and these buffers re-allocated when the MTU changes. However, the ipconfig code is incompatible with this:

	/* Handle the case where we need non-standard MTU on the boot link (a network
	 * using jumbo frames, for instance).  If we can't set the mtu, don't error
	 * out, we'll try to muddle along.
	 */
	if (ic_dev_mtu != 0) {
		rtnl_lock();
		if ((err = dev_set_mtu(ic_dev->dev, ic_dev_mtu)) < 0)
			pr_err("IP-Config: Unable to set interface mtu to %d (%d)\n",
			       ic_dev_mtu, err);
		rtnl_unlock();
	}

The device is not brought down prior to dev_set_mtu, so we always trigger an -EBUSY with these drivers. The boot log looks like this:

[    6.988410] Sending DHCP requests ., OK
[    8.016248] IP-Config: Got DHCP answer from 192.168.1.1, my address is 192.168.1.217
[    8.023994] IP-Config: Complete:
[    8.027215]      device=eth0, hwaddr=46:b5:da:0d:99:20, ipaddr=192.168.1.217, mask=255.255.255.0, gw=255.255.255.255
[    8.037729]      host=192.168.1.217, domain=threespeedlogic.com, nis-domain=(none)
[    8.045288]      bootserver=192.168.1.1, rootserver=192.168.1.1, rootpath=, mtu=9000
[    8.045296]      nameserver0=192.168.1.1
[    8.057015] IP-Config: Unable to set interface mtu to 9000 (-16)

So - what to do? I can see three defensible arguments:

- The network drivers should allow MTU changes on-the-fly (many do), or
- The ipconfig code could bring the adapter down and up again, or
- This is out-of-scope, and I should be reconfiguring the interface in userspace anyways.

thanks,
Graeme

Re: net: ipconfig: dev_set_mtu call is incompatible with a number of Ethernet drivers

[David Ahern]

On 12/14/23 12:07 PM, Graeme Smecher wrote:
> Hi all,
> 
> In a number of ethernet drivers, the MTU can't be changed on a running
> device. Here's one example (from drivers/net/ethernet/cadence/macb_main.c):
> 

...

> 
> So - what to do? I can see three defensible arguments:
> 
> - The network drivers should allow MTU changes on-the-fly (many do), or
> - The ipconfig code could bring the adapter down and up again, or

looking at the ordering, bringing down the selected device to change the
MTU seems the more reasonable solution.

> - This is out-of-scope, and I should be reconfiguring the interface in
> userspace anyways.
> 

[PATCH] RFC: net: ipconfig: temporarily bring interface down when changing MTU.

[Graeme Smecher]

Several network drivers (sh_eth, macb_main, nixge, sundance) only allow
the MTU to be changed when the interface is down, because their buffer
allocations are performed during ndo_open() and calculated using a
specific MTU.

Kick-tested using QEMU (rtl8139, e1000).

Tested-by: Graeme Smecher <gsmecher@threespeedlogic.com>
---
 net/ipv4/ipconfig.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/ipconfig.c b/net/ipv4/ipconfig.c
index c56b6fe6f0d7..69c2a41393a0 100644
--- a/net/ipv4/ipconfig.c
+++ b/net/ipv4/ipconfig.c
@@ -396,9 +396,21 @@ static int __init ic_setup_if(void)
 	 */
 	if (ic_dev_mtu != 0) {
 		rtnl_lock();
-		if ((err = dev_set_mtu(ic_dev->dev, ic_dev_mtu)) < 0)
-			pr_err("IP-Config: Unable to set interface mtu to %d (%d)\n",
-			       ic_dev_mtu, err);
+		/* Some Ethernet adapters only allow MTU to change when down. */
+		if((err = dev_change_flags(ic_dev->dev, ic_dev->dev->flags | IFF_UP, NULL)))
+			pr_err("IP-Config: About to set MTU, but failed to "
+				 "bring interface %s down! (%d)\n",
+				 ic_dev->dev->name, err);
+		else {
+			if ((err = dev_set_mtu(ic_dev->dev, ic_dev_mtu)) < 0)
+				pr_err("IP-Config: Unable to set interface mtu to %d (%d)\n",
+				       ic_dev_mtu, err);
+
+			if((err = dev_change_flags(ic_dev->dev, ic_dev->dev->flags | IFF_UP, NULL)))
+				pr_err("IP-Config: Trying to set MTU, but unable "
+					 "to bring interface %s back up! (%d)\n",
+					 ic_dev->dev->name, err);
+		}
 		rtnl_unlock();
 	}
 	return 0;
-- 
2.39.2

Re: [PATCH] RFC: net: ipconfig: temporarily bring interface down when changing MTU.

[Stephen Hemminger]

On Fri, 15 Dec 2023 17:04:31 -0800
Graeme Smecher <gsmecher@threespeedlogic.com> wrote:

> Several network drivers (sh_eth, macb_main, nixge, sundance) only allow
> the MTU to be changed when the interface is down, because their buffer
> allocations are performed during ndo_open() and calculated using a
> specific MTU.
> 
> Kick-tested using QEMU (rtl8139, e1000).
> 
> Tested-by: Graeme Smecher <gsmecher@threespeedlogic.com>
> ---
>  net/ipv4/ipconfig.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/net/ipv4/ipconfig.c b/net/ipv4/ipconfig.c
> index c56b6fe6f0d7..69c2a41393a0 100644
> --- a/net/ipv4/ipconfig.c
> +++ b/net/ipv4/ipconfig.c
> @@ -396,9 +396,21 @@ static int __init ic_setup_if(void)
>  	 */
>  	if (ic_dev_mtu != 0) {
>  		rtnl_lock();
> -		if ((err = dev_set_mtu(ic_dev->dev, ic_dev_mtu)) < 0)
> -			pr_err("IP-Config: Unable to set interface mtu to %d (%d)\n",
> -			       ic_dev_mtu, err);
> +		/* Some Ethernet adapters only allow MTU to change when down. */
Check if interface was already down first.

> +		if((err = dev_change_flags(ic_dev->dev, ic_dev->dev->flags | IFF_UP, NULL)))

Please do not combine function call with err test. Surprised checkpatch doesn't complain about hat.

> +			pr_err("IP-Config: About to set MTU, but failed to "
> +				 "bring interface %s down! (%d)\n",

Don't break lines in error messages.
> +				 ic_dev->dev->name, err);
> +		else {
> +			if ((err = dev_set_mtu(ic_dev->dev, ic_dev_mtu)) < 0)
> +				pr_err("IP-Config: Unable to set interface mtu to %d (%d)\n",
> +				       ic_dev_mtu, err);
> +
> +			if((err = dev_change_flags(ic_dev->dev, ic_dev->dev->flags | IFF_UP, NULL)))
> +				pr_err("IP-Config: Trying to set MTU, but unable "
> +					 "to bring interface %s back up! (%d)\n",
> +					 ic_dev->dev->name, err);
> +		}
>  		rtnl_unlock();
>  	}
>  	return 0;

Re: [PATCH] RFC: net: ipconfig: temporarily bring interface down when changing MTU.

[David Ahern]

On 12/15/23 7:30 PM, Stephen Hemminger wrote:
>> diff --git a/net/ipv4/ipconfig.c b/net/ipv4/ipconfig.c
>> index c56b6fe6f0d7..69c2a41393a0 100644
>> --- a/net/ipv4/ipconfig.c
>> +++ b/net/ipv4/ipconfig.c
>> @@ -396,9 +396,21 @@ static int __init ic_setup_if(void)
>>  	 */
>>  	if (ic_dev_mtu != 0) {
>>  		rtnl_lock();
>> -		if ((err = dev_set_mtu(ic_dev->dev, ic_dev_mtu)) < 0)
>> -			pr_err("IP-Config: Unable to set interface mtu to %d (%d)\n",
>> -			       ic_dev_mtu, err);
>> +		/* Some Ethernet adapters only allow MTU to change when down. */
> Check if interface was already down first.


it is; see ic_open_devs. ic_close_devs will bring down all interfaces
not configured by ipconfig.

Re: [PATCH] RFC: net: ipconfig: temporarily bring interface down when changing MTU.

[David Ahern]

On 12/15/23 5:04 PM, Graeme Smecher wrote:
\> diff --git a/net/ipv4/ipconfig.c b/net/ipv4/ipconfig.c
> index c56b6fe6f0d7..69c2a41393a0 100644
> --- a/net/ipv4/ipconfig.c
> +++ b/net/ipv4/ipconfig.c
> @@ -396,9 +396,21 @@ static int __init ic_setup_if(void)
>  	 */
>  	if (ic_dev_mtu != 0) {
>  		rtnl_lock();
> -		if ((err = dev_set_mtu(ic_dev->dev, ic_dev_mtu)) < 0)
> -			pr_err("IP-Config: Unable to set interface mtu to %d (%d)\n",
> -			       ic_dev_mtu, err);
> +		/* Some Ethernet adapters only allow MTU to change when down. */
> +		if((err = dev_change_flags(ic_dev->dev, ic_dev->dev->flags | IFF_UP, NULL)))

IFF_UP?

> +			pr_err("IP-Config: About to set MTU, but failed to "
> +				 "bring interface %s down! (%d)\n",
> +				 ic_dev->dev->name, err);
> +		else {
> +			if ((err = dev_set_mtu(ic_dev->dev, ic_dev_mtu)) < 0)

try to set the MTU even if DOWN fails otherwise a regression.


> +				pr_err("IP-Config: Unable to set interface mtu to %d (%d)\n",
> +				       ic_dev_mtu, err);
> +
> +			if((err = dev_change_flags(ic_dev->dev, ic_dev->dev->flags | IFF_UP, NULL)))
> +				pr_err("IP-Config: Trying to set MTU, but unable "
> +					 "to bring interface %s back up! (%d)\n",


This step is not "trying to set MTU"; the last one did:

pr_err("IP-Config: Failed to bring interface %s up after changing MTU
(%d)\n",

> +					 ic_dev->dev->name, err);
> +		}
>  		rtnl_unlock();
>  	}
>  	return 0;

Re: net: ipconfig: dev_set_mtu call is incompatible with a number of Ethernet drivers

[Andrew Lunn]

On Fri, Dec 15, 2023 at 09:49:45AM -0800, David Ahern wrote:
> On 12/14/23 12:07 PM, Graeme Smecher wrote:
> > Hi all,
> > 
> > In a number of ethernet drivers, the MTU can't be changed on a running
> > device. Here's one example (from drivers/net/ethernet/cadence/macb_main.c):
> > 
> 
> ...
> 
> > 
> > So - what to do? I can see three defensible arguments:
> > 
> > - The network drivers should allow MTU changes on-the-fly (many do), or
> > - The ipconfig code could bring the adapter down and up again, or
> 
> looking at the ordering, bringing down the selected device to change the
> MTU seems the more reasonable solution.

But you need to review all the drivers and make sure there are none
which require the interface to be up in order to change the MTU.

So you might actually want to do is first try to change the MTU with
the interface up. If that fails, try it with it down. That should not
cause any regressions.

      Andrew

Re: net: ipconfig: dev_set_mtu call is incompatible with a number of Ethernet drivers

[David Ahern]

On 12/17/23 11:22 AM, Andrew Lunn wrote:
> On Fri, Dec 15, 2023 at 09:49:45AM -0800, David Ahern wrote:
>> On 12/14/23 12:07 PM, Graeme Smecher wrote:
>>> Hi all,
>>>
>>> In a number of ethernet drivers, the MTU can't be changed on a running
>>> device. Here's one example (from drivers/net/ethernet/cadence/macb_main.c):
>>>
>>
>> ...
>>
>>>
>>> So - what to do? I can see three defensible arguments:
>>>
>>> - The network drivers should allow MTU changes on-the-fly (many do), or
>>> - The ipconfig code could bring the adapter down and up again, or
>>
>> looking at the ordering, bringing down the selected device to change the
>> MTU seems the more reasonable solution.
> 
> But you need to review all the drivers and make sure there are none
> which require the interface to be up in order to change the MTU.

Is there really such a driver / H/W? Seems like a bug to me that the
device has to be brought up to configure it. Very counter-intuitive.

> 
> So you might actually want to do is first try to change the MTU with
> the interface up. If that fails, try it with it down. That should not
> cause any regressions.
> 

yea, that would be the least invasive.

Re: [PATCH] RFC: net: ipconfig: temporarily bring interface down when changing MTU.

[Paolo Abeni]

On Fri, 2023-12-15 at 17:04 -0800, Graeme Smecher wrote:
> Several network drivers (sh_eth, macb_main, nixge, sundance) only allow
> the MTU to be changed when the interface is down, because their buffer
> allocations are performed during ndo_open() and calculated using a
> specific MTU.

I think we have to address that in the relevant device drivers. This
could cause significant regressions: changing the MTU may causes the
device to stay down afterwards - if the device fails to re-initialize.

That even for drivers that before failed gracefully.

Instead, at the device level, the driver could allocate in advance the
new ring, and restore the previous one if the allocation fails, always
leaving the device in a consistent status.

Cheers,

Paolo