[PATCH net] net: vlan: allow vlan device MTU change follow real device from smaller to bigger

[PATCH net] net: vlan: allow vlan device MTU change follow real device from smaller to bigger

[Ziyang Xuan]

vlan device MTU can only follow real device change from bigger to smaller
but from smaller to bigger under the premise of vlan device MTU not exceed
the real device MTU.

This issue can be seen using the following commands:

ip link add link eth1 dev eth1.100 type vlan id 100
ip link set eth1 mtu 256
ip link set eth1 mtu 1500
ip link show

Modify to allow vlan device follow real device MTU change from smaller
to bigger when user has not configured vlan device MTU which is not
equal to real device MTU. That also ensure user configuration has higher
priority.

Fixes: 2e477c9bd2bb ("vlan: Propagate physical MTU changes")
Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>
---
 net/8021q/vlan.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
index 788076b002b3..7de4f462525a 100644
--- a/net/8021q/vlan.c
+++ b/net/8021q/vlan.c
@@ -361,6 +361,7 @@ static int __vlan_device_event(struct net_device *dev, unsigned long event)
 static int vlan_device_event(struct notifier_block *unused, unsigned long event,
 			     void *ptr)
 {
+	unsigned int orig_mtu = ((struct netdev_notifier_info_ext *)ptr)->ext.mtu;
 	struct netlink_ext_ack *extack = netdev_notifier_info_to_extack(ptr);
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
 	struct vlan_group *grp;
@@ -419,7 +420,7 @@ static int vlan_device_event(struct notifier_block *unused, unsigned long event,
 
 	case NETDEV_CHANGEMTU:
 		vlan_group_for_each_dev(grp, i, vlandev) {
-			if (vlandev->mtu <= dev->mtu)
+			if (vlandev->mtu <= dev->mtu && vlandev->mtu != orig_mtu)
 				continue;
 
 			dev_set_mtu(vlandev, dev->mtu);
-- 
2.25.1

Re: [PATCH net] net: vlan: allow vlan device MTU change follow real device from smaller to bigger

[Eric Dumazet]

CC Herbert Xu, author of blamed commit.

On Mon, Feb 21, 2022 at 4:28 AM Ziyang Xuan
<william.xuanziyang@huawei.com> wrote:
>
> vlan device MTU can only follow real device change from bigger to smaller
> but from smaller to bigger under the premise of vlan device MTU not exceed
> the real device MTU.
>
> This issue can be seen using the following commands:
>
> ip link add link eth1 dev eth1.100 type vlan id 100
> ip link set eth1 mtu 256
> ip link set eth1 mtu 1500
> ip link show
>
> Modify to allow vlan device follow real device MTU change from smaller
> to bigger when user has not configured vlan device MTU which is not
> equal to real device MTU. That also ensure user configuration has higher
> priority.
>
> Fixes: 2e477c9bd2bb ("vlan: Propagate physical MTU changes")
> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>
> ---
>  net/8021q/vlan.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
> index 788076b002b3..7de4f462525a 100644
> --- a/net/8021q/vlan.c
> +++ b/net/8021q/vlan.c
> @@ -361,6 +361,7 @@ static int __vlan_device_event(struct net_device *dev, unsigned long event)
>  static int vlan_device_event(struct notifier_block *unused, unsigned long event,
>                              void *ptr)
>  {
> +       unsigned int orig_mtu = ((struct netdev_notifier_info_ext *)ptr)->ext.mtu;
>         struct netlink_ext_ack *extack = netdev_notifier_info_to_extack(ptr);
>         struct net_device *dev = netdev_notifier_info_to_dev(ptr);
>         struct vlan_group *grp;
> @@ -419,7 +420,7 @@ static int vlan_device_event(struct notifier_block *unused, unsigned long event,
>
>         case NETDEV_CHANGEMTU:
>                 vlan_group_for_each_dev(grp, i, vlandev) {
> -                       if (vlandev->mtu <= dev->mtu)
> +                       if (vlandev->mtu <= dev->mtu && vlandev->mtu != orig_mtu)
>                                 continue;
>
>                         dev_set_mtu(vlandev, dev->mtu);
> --
> 2.25.1
>

Herbert, do you recall why only a decrease was taken into consideration ?

Thanks.

Re: [PATCH net] net: vlan: allow vlan device MTU change follow real device from smaller to bigger

[Herbert Xu]

On Mon, Feb 21, 2022 at 07:43:18AM -0800, Eric Dumazet wrote:
>
> Herbert, do you recall why only a decrease was taken into consideration ?

Because we shouldn't override administrative settings of the MTU
on the vlan device, unless we have to because of an MTU reduction
on the underlying device.

Yes this is not perfect if the admin never set an MTU to start with
but as we don't have a way of telling whether the admin has or has
not changed the MTU setting, the safest course of action is to do
nothing in that case.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH net] net: vlan: allow vlan device MTU change follow real device from smaller to bigger

[Ziyang Xuan (William)]

> On Mon, Feb 21, 2022 at 07:43:18AM -0800, Eric Dumazet wrote:
>>
>> Herbert, do you recall why only a decrease was taken into consideration ?
> 
> Because we shouldn't override administrative settings of the MTU
> on the vlan device, unless we have to because of an MTU reduction
> on the underlying device.
> 
> Yes this is not perfect if the admin never set an MTU to start with
> but as we don't have a way of telling whether the admin has or has
> not changed the MTU setting, the safest course of action is to do
> nothing in that case.
If the admin has changed the vlan device MTU smaller than the underlying
device MTU firstly, then changed the underlying device MTU smaller than
the vlan device MTU secondly. The admin's configuration has been overridden.
Can we consider that the admin's configuration for the vlan device MTU has
been invalid and disappeared after the second change? I think so.

> 
> Thanks,
> 

Re: [PATCH net] net: vlan: allow vlan device MTU change follow real device from smaller to bigger

[Eric Dumazet]

On Mon, Feb 21, 2022 at 6:06 PM Ziyang Xuan (William)
<william.xuanziyang@huawei.com> wrote:
>
> > On Mon, Feb 21, 2022 at 07:43:18AM -0800, Eric Dumazet wrote:
> >>
> >> Herbert, do you recall why only a decrease was taken into consideration ?
> >
> > Because we shouldn't override administrative settings of the MTU
> > on the vlan device, unless we have to because of an MTU reduction
> > on the underlying device.
> >
> > Yes this is not perfect if the admin never set an MTU to start with
> > but as we don't have a way of telling whether the admin has or has
> > not changed the MTU setting, the safest course of action is to do
> > nothing in that case.
> If the admin has changed the vlan device MTU smaller than the underlying
> device MTU firstly, then changed the underlying device MTU smaller than
> the vlan device MTU secondly. The admin's configuration has been overridden.
> Can we consider that the admin's configuration for the vlan device MTU has
> been invalid and disappeared after the second change? I think so.

The answer is no.

Herbert is saying:

ip link add link eth1 dev eth1.100 type vlan id 100
...
ip link set eth1.100 mtu 800
..
ip link set eth1 mtu 256
ip link set eth1 mtu 1500

-> we do not want eth1.100 mtu being set back to 1500, this might
break applications, depending on old kernel feature.
 Eventually, setting back to 800 seems ok.

If you want this new feature, we need to record in eth1.100 device
that no admin ever changed the mtu,
as Herbert suggested.

Then, it is okay to upgrade the vlan mtu (but still is a behavioral
change that _could_ break some scripts)

Thank you.

Re: [PATCH net] net: vlan: allow vlan device MTU change follow real device from smaller to bigger

[Ziyang Xuan (William)]

> On Mon, Feb 21, 2022 at 6:06 PM Ziyang Xuan (William)
> <william.xuanziyang@huawei.com> wrote:
>>
>>> On Mon, Feb 21, 2022 at 07:43:18AM -0800, Eric Dumazet wrote:
>>>>
>>>> Herbert, do you recall why only a decrease was taken into consideration ?
>>>
>>> Because we shouldn't override administrative settings of the MTU
>>> on the vlan device, unless we have to because of an MTU reduction
>>> on the underlying device.
>>>
>>> Yes this is not perfect if the admin never set an MTU to start with
>>> but as we don't have a way of telling whether the admin has or has
>>> not changed the MTU setting, the safest course of action is to do
>>> nothing in that case.
>> If the admin has changed the vlan device MTU smaller than the underlying
>> device MTU firstly, then changed the underlying device MTU smaller than
>> the vlan device MTU secondly. The admin's configuration has been overridden.
>> Can we consider that the admin's configuration for the vlan device MTU has
>> been invalid and disappeared after the second change? I think so.
> 
> The answer is no.
> 
> Herbert is saying:
> 
> ip link add link eth1 dev eth1.100 type vlan id 100
> ...
> ip link set eth1.100 mtu 800
> ..
> ip link set eth1 mtu 256
> ip link set eth1 mtu 1500
> 
> -> we do not want eth1.100 mtu being set back to 1500, this might
> break applications, depending on old kernel feature.
>  Eventually, setting back to 800 seems ok.

It seem that setting back to 800 more reasonable. We can record user
setting MTU by interface ndo_change_mtu() in struct vlan_dev_priv.

> 
> If you want this new feature, we need to record in eth1.100 device
> that no admin ever changed the mtu,
> as Herbert suggested.
> 
> Then, it is okay to upgrade the vlan mtu (but still is a behavioral
> change that _could_ break some scripts)
> 
> Thank you.
> .
> 

Re: [PATCH net] net: vlan: allow vlan device MTU change follow real device from smaller to bigger

[Ziyang Xuan (William)]

>> On Mon, Feb 21, 2022 at 6:06 PM Ziyang Xuan (William)
>> <william.xuanziyang@huawei.com> wrote:
>>>
>>>> On Mon, Feb 21, 2022 at 07:43:18AM -0800, Eric Dumazet wrote:
>>>>>
>>>>> Herbert, do you recall why only a decrease was taken into consideration ?
>>>>
>>>> Because we shouldn't override administrative settings of the MTU
>>>> on the vlan device, unless we have to because of an MTU reduction
>>>> on the underlying device.
>>>>
>>>> Yes this is not perfect if the admin never set an MTU to start with
>>>> but as we don't have a way of telling whether the admin has or has
>>>> not changed the MTU setting, the safest course of action is to do
>>>> nothing in that case.
>>> If the admin has changed the vlan device MTU smaller than the underlying
>>> device MTU firstly, then changed the underlying device MTU smaller than
>>> the vlan device MTU secondly. The admin's configuration has been overridden.
>>> Can we consider that the admin's configuration for the vlan device MTU has
>>> been invalid and disappeared after the second change? I think so.
>>
>> The answer is no.
>>
>> Herbert is saying:
>>
>> ip link add link eth1 dev eth1.100 type vlan id 100
>> ...
>> ip link set eth1.100 mtu 800
>> ..
>> ip link set eth1 mtu 256
>> ip link set eth1 mtu 1500
>>
>> -> we do not want eth1.100 mtu being set back to 1500, this might
>> break applications, depending on old kernel feature.
>>  Eventually, setting back to 800 seems ok.
> 
> It seem that setting back to 800 more reasonable. We can record user
> setting MTU by interface ndo_change_mtu() in struct vlan_dev_priv.
> 

I attempt to record user setting MTU for vlan device. Use the recorded
MTU to restore when uderlying device change the MTU from smaller to
bigger than recorded vlan device MTU. The modification as following:

diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index 2be4dd7e90a9..b8970e90a279 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -163,6 +163,7 @@ struct netpoll;
  *     @vlan_proto: VLAN encapsulation protocol
  *     @vlan_id: VLAN identifier
  *     @flags: device flags
+ *     @mtu: user setting MTU
  *     @real_dev: underlying netdevice
  *     @dev_tracker: refcount tracker for @real_dev reference
  *     @real_dev_addr: address of underlying netdevice
@@ -179,6 +180,8 @@ struct vlan_dev_priv {
        u16                                     vlan_id;
        u16                                     flags;

+       u32                                     mtu;
+
        struct net_device                       *real_dev;
        netdevice_tracker                       dev_tracker;

diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
index 788076b002b3..492ef88923c2 100644
--- a/net/8021q/vlan.c
+++ b/net/8021q/vlan.c
@@ -365,6 +365,7 @@ static int vlan_device_event(struct notifier_block *unused, unsigned long event,
        struct net_device *dev = netdev_notifier_info_to_dev(ptr);
        struct vlan_group *grp;
        struct vlan_info *vlan_info;
+       unsigned int new_mtu;
        int i, flgs;
        struct net_device *vlandev;
        struct vlan_dev_priv *vlan;
@@ -419,10 +420,10 @@ static int vlan_device_event(struct notifier_block *unused, unsigned long event,

        case NETDEV_CHANGEMTU:
                vlan_group_for_each_dev(grp, i, vlandev) {
-                       if (vlandev->mtu <= dev->mtu)
-                               continue;
+                       vlan = vlan_dev_priv(vlandev);
+                       new_mtu = (!vlan->mtu || dev->mtu < vlan->mtu) ? dev->mtu : vlan->mtu;

-                       dev_set_mtu(vlandev, dev->mtu);
+                       dev_set_mtu(vlandev, new_mtu);
                }
                break;

diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index d1902828a18a..66c2b64d1ece 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -140,7 +140,8 @@ static netdev_tx_t vlan_dev_hard_start_xmit(struct sk_buff *skb,

 static int vlan_dev_change_mtu(struct net_device *dev, int new_mtu)
 {
-       struct net_device *real_dev = vlan_dev_priv(dev)->real_dev;
+       struct vlan_dev_priv *vlan = vlan_dev_priv(dev);
+       struct net_device *real_dev = vlan->real_dev;
        unsigned int max_mtu = real_dev->mtu;

        if (netif_reduces_vlan_mtu(real_dev))
@@ -148,8 +149,15 @@ static int vlan_dev_change_mtu(struct net_device *dev, int new_mtu)
        if (max_mtu < new_mtu)
                return -ERANGE;

-       dev->mtu = new_mtu;
+       /* Identify user MTU change different from the underlying devcie
+        * NETDEV_CHANGEMTU event. Record user setting MTU in mtu member
+        * of struct vlan_dev_priv.
+        */
+       if ((!vlan->mtu && new_mtu != real_dev->mtu) ||
+           (dev->mtu == vlan->mtu && vlan->mtu < real_dev->mtu))
+               vlan->mtu = new_mtu;

+       dev->mtu = new_mtu;
        return 0;
 }


I test it  in various combination scenarios. I found it can not cover one
scenario because user setting can not arrive vlan module codes. For example:

ip link add link eth1 dev eth1.100 type vlan id 100 // eth1.100 MTU 1500
ip link set eth1.100 mtu 1500 // success no error

When new_mtu equal to orig_mtu, user setting operation can not arrive lower
vlan module, vlan module can not perceive, so we can not record the setting.

Before my colleague point that the above setting is not error for user, I
always think that it is invalid setting. But I think his opinion makes sense.

So what do you think about the above setting?

>>
>> If you want this new feature, we need to record in eth1.100 device
>> that no admin ever changed the mtu,
>> as Herbert suggested.
>>
>> Then, it is okay to upgrade the vlan mtu (but still is a behavioral
>> change that _could_ break some scripts)
>>
>> Thank you.
>> .
>>
> .
> 

Re: [PATCH net] net: vlan: allow vlan device MTU change follow real device from smaller to bigger

[Guillaume Nault]

On Mon, Feb 21, 2022 at 06:27:46PM -0800, Eric Dumazet wrote:
> On Mon, Feb 21, 2022 at 6:06 PM Ziyang Xuan (William)
> <william.xuanziyang@huawei.com> wrote:
> >
> > > On Mon, Feb 21, 2022 at 07:43:18AM -0800, Eric Dumazet wrote:
> > >>
> > >> Herbert, do you recall why only a decrease was taken into consideration ?
> > >
> > > Because we shouldn't override administrative settings of the MTU
> > > on the vlan device, unless we have to because of an MTU reduction
> > > on the underlying device.
> > >
> > > Yes this is not perfect if the admin never set an MTU to start with
> > > but as we don't have a way of telling whether the admin has or has
> > > not changed the MTU setting, the safest course of action is to do
> > > nothing in that case.
> > If the admin has changed the vlan device MTU smaller than the underlying
> > device MTU firstly, then changed the underlying device MTU smaller than
> > the vlan device MTU secondly. The admin's configuration has been overridden.
> > Can we consider that the admin's configuration for the vlan device MTU has
> > been invalid and disappeared after the second change? I think so.
> 
> The answer is no.
> 
> Herbert is saying:
> 
> ip link add link eth1 dev eth1.100 type vlan id 100
> ...
> ip link set eth1.100 mtu 800
> ..
> ip link set eth1 mtu 256
> ip link set eth1 mtu 1500
> 
> -> we do not want eth1.100 mtu being set back to 1500, this might
> break applications, depending on old kernel feature.
>  Eventually, setting back to 800 seems ok.
> 
> If you want this new feature, we need to record in eth1.100 device
> that no admin ever changed the mtu,
> as Herbert suggested.
> 
> Then, it is okay to upgrade the vlan mtu (but still is a behavioral
> change that _could_ break some scripts)

What about an explicit option:

  ip link add link eth1 dev eth1.100 type vlan id 100 follow-parent-mtu


Or for something more future proof, an option that can accept several
policies:

  mtu-update <reduce-only,follow,...>

      reduce-only (default):
        update vlan's MTU only if the new MTU is smaller than the
        current one (current behaviour).

      follow:
        always follow the MTU of the parent device.

Then if anyone wants more complex policies:

      follow-if-not-modified:
        follow the MTU of the parent device as long as the VLAN's MTU
        was not manually changed. Otherwise only adjust the VLAN's MTU
        when the parent's one is set to a smaller value.

      follow-if-not-modified-but-not-quite:
        like follow-if-not-modified but revert back to the VLAN's
        last manually modified MTU, if any, whenever possible (that is,
        when the parent device's MTU is set back to a higher value).
        That probably requires the possibility to dump the last
        modified MTU, so the administrator can anticipate the
        consequences of modifying the parent device.

     yet-another-policy (because people have a lot of imagination):
       for example, keep the MTU 4 bytes lower than the parent device,
       to account for VLAN overhead.

Of course feel free to suggest better names and policies :).

This way, we can keep the current behaviour and avoid unexpected
heuristics that are difficult to explain (and even more difficult for
network admins to figure out on their own).

> Thank you.
> 

Re: [PATCH net] net: vlan: allow vlan device MTU change follow real device from smaller to bigger

[Jakub Kicinski]

On Tue, 22 Feb 2022 11:37:33 +0100 Guillaume Nault wrote:
> What about an explicit option:
> 
>   ip link add link eth1 dev eth1.100 type vlan id 100 follow-parent-mtu
> 
> 
> Or for something more future proof, an option that can accept several
> policies:
> 
>   mtu-update <reduce-only,follow,...>
> 
>       reduce-only (default):
>         update vlan's MTU only if the new MTU is smaller than the
>         current one (current behaviour).
> 
>       follow:
>         always follow the MTU of the parent device.
> 
> Then if anyone wants more complex policies:
> 
>       follow-if-not-modified:
>         follow the MTU of the parent device as long as the VLAN's MTU
>         was not manually changed. Otherwise only adjust the VLAN's MTU
>         when the parent's one is set to a smaller value.
> 
>       follow-if-not-modified-but-not-quite:
>         like follow-if-not-modified but revert back to the VLAN's
>         last manually modified MTU, if any, whenever possible (that is,
>         when the parent device's MTU is set back to a higher value).
>         That probably requires the possibility to dump the last
>         modified MTU, so the administrator can anticipate the
>         consequences of modifying the parent device.
> 
>      yet-another-policy (because people have a lot of imagination):
>        for example, keep the MTU 4 bytes lower than the parent device,
>        to account for VLAN overhead.
> 
> Of course feel free to suggest better names and policies :).
> 
> This way, we can keep the current behaviour and avoid unexpected
> heuristics that are difficult to explain (and even more difficult for
> network admins to figure out on their own).

My $0.02 would be that if we want to make changes that require new uAPI
we should do it across uppers.

Re: [PATCH net] net: vlan: allow vlan device MTU change follow real device from smaller to bigger

[Guillaume Nault]

On Tue, Feb 22, 2022 at 03:28:15PM -0800, Jakub Kicinski wrote:
> On Tue, 22 Feb 2022 11:37:33 +0100 Guillaume Nault wrote:
> > What about an explicit option:
> > 
> >   ip link add link eth1 dev eth1.100 type vlan id 100 follow-parent-mtu
> > 
> > 
> > Or for something more future proof, an option that can accept several
> > policies:
> > 
> >   mtu-update <reduce-only,follow,...>
> > 
> >       reduce-only (default):
> >         update vlan's MTU only if the new MTU is smaller than the
> >         current one (current behaviour).
> > 
> >       follow:
> >         always follow the MTU of the parent device.
> > 
> > Then if anyone wants more complex policies:
> > 
> >       follow-if-not-modified:
> >         follow the MTU of the parent device as long as the VLAN's MTU
> >         was not manually changed. Otherwise only adjust the VLAN's MTU
> >         when the parent's one is set to a smaller value.
> > 
> >       follow-if-not-modified-but-not-quite:
> >         like follow-if-not-modified but revert back to the VLAN's
> >         last manually modified MTU, if any, whenever possible (that is,
> >         when the parent device's MTU is set back to a higher value).
> >         That probably requires the possibility to dump the last
> >         modified MTU, so the administrator can anticipate the
> >         consequences of modifying the parent device.
> > 
> >      yet-another-policy (because people have a lot of imagination):
> >        for example, keep the MTU 4 bytes lower than the parent device,
> >        to account for VLAN overhead.
> > 
> > Of course feel free to suggest better names and policies :).
> > 
> > This way, we can keep the current behaviour and avoid unexpected
> > heuristics that are difficult to explain (and even more difficult for
> > network admins to figure out on their own).
> 
> My $0.02 would be that if we want to make changes that require new uAPI
> we should do it across uppers.

Do you mean something like:

  ip link set dev eth0 vlan-mtu-policy <policy-name>

that'd affect all existing (and future) vlans of eth0?

Then I think that for non-ethernet devices, we should reject this
option and skip it when dumping config. But yes, that's another
possibility.

I personnaly don't really mind, as long as we keep a clear behaviour.

What I'd really like to avoid is something like:
  - By default it behaves this way.
  - If you modified the MTU it behaves in another way
  - But if you modified the MTU but later restored the
    original MTU, then you're back to the default behaviour
    (or not?), unless the MTU of the upper device was also
    changed meanwhile, in which case ... to be continued ...
  - BTW, you might not be able to tell how the VLAN's MTU is going to
    behave by simply looking at its configuration, because that also
    depends on past configurations.
  - Well, and if your kernel is older than xxx, then you always get the
    default behaviour.
  - ... and we might modify the heuristics again in the future to
    accomodate with situations or use cases we failed to consider.

Re: [PATCH net] net: vlan: allow vlan device MTU change follow real device from smaller to bigger

[Stephen Hemminger]

On Wed, 23 Feb 2022 12:26:18 +0100
Guillaume Nault <gnault@redhat.com> wrote:

> On Tue, Feb 22, 2022 at 03:28:15PM -0800, Jakub Kicinski wrote:
> > On Tue, 22 Feb 2022 11:37:33 +0100 Guillaume Nault wrote:  
> > > What about an explicit option:
> > > 
> > >   ip link add link eth1 dev eth1.100 type vlan id 100 follow-parent-mtu
> > > 
> > > 
> > > Or for something more future proof, an option that can accept several
> > > policies:
> > > 
> > >   mtu-update <reduce-only,follow,...>
> > > 
> > >       reduce-only (default):
> > >         update vlan's MTU only if the new MTU is smaller than the
> > >         current one (current behaviour).
> > > 
> > >       follow:
> > >         always follow the MTU of the parent device.
> > > 
> > > Then if anyone wants more complex policies:
> > > 
> > >       follow-if-not-modified:
> > >         follow the MTU of the parent device as long as the VLAN's MTU
> > >         was not manually changed. Otherwise only adjust the VLAN's MTU
> > >         when the parent's one is set to a smaller value.
> > > 
> > >       follow-if-not-modified-but-not-quite:
> > >         like follow-if-not-modified but revert back to the VLAN's
> > >         last manually modified MTU, if any, whenever possible (that is,
> > >         when the parent device's MTU is set back to a higher value).
> > >         That probably requires the possibility to dump the last
> > >         modified MTU, so the administrator can anticipate the
> > >         consequences of modifying the parent device.
> > > 
> > >      yet-another-policy (because people have a lot of imagination):
> > >        for example, keep the MTU 4 bytes lower than the parent device,
> > >        to account for VLAN overhead.
> > > 
> > > Of course feel free to suggest better names and policies :).
> > > 
> > > This way, we can keep the current behaviour and avoid unexpected
> > > heuristics that are difficult to explain (and even more difficult for
> > > network admins to figure out on their own).  
> > 
> > My $0.02 would be that if we want to make changes that require new uAPI
> > we should do it across uppers.  
> 
> Do you mean something like:
> 
>   ip link set dev eth0 vlan-mtu-policy <policy-name>
> 
> that'd affect all existing (and future) vlans of eth0?
> 
> Then I think that for non-ethernet devices, we should reject this
> option and skip it when dumping config. But yes, that's another
> possibility.
> 
> I personnaly don't really mind, as long as we keep a clear behaviour.
> 
> What I'd really like to avoid is something like:
>   - By default it behaves this way.
>   - If you modified the MTU it behaves in another way
>   - But if you modified the MTU but later restored the
>     original MTU, then you're back to the default behaviour
>     (or not?), unless the MTU of the upper device was also
>     changed meanwhile, in which case ... to be continued ...
>   - BTW, you might not be able to tell how the VLAN's MTU is going to
>     behave by simply looking at its configuration, because that also
>     depends on past configurations.
>   - Well, and if your kernel is older than xxx, then you always get the
>     default behaviour.
>   - ... and we might modify the heuristics again in the future to
>     accomodate with situations or use cases we failed to consider.
> 

In general these kind of policy choices are done via sysctl knobs.
They aren't done at netlink/ip link level.

Re: [PATCH net] net: vlan: allow vlan device MTU change follow real device from smaller to bigger

[Guillaume Nault]

On Wed, Feb 23, 2022 at 07:17:36AM -0800, Stephen Hemminger wrote:
> On Wed, 23 Feb 2022 12:26:18 +0100
> Guillaume Nault <gnault@redhat.com> wrote:
> 
> > On Tue, Feb 22, 2022 at 03:28:15PM -0800, Jakub Kicinski wrote:
> > > On Tue, 22 Feb 2022 11:37:33 +0100 Guillaume Nault wrote:  
> > > > What about an explicit option:
> > > > 
> > > >   ip link add link eth1 dev eth1.100 type vlan id 100 follow-parent-mtu
> > > > 
> > > > 
> > > > Or for something more future proof, an option that can accept several
> > > > policies:
> > > > 
> > > >   mtu-update <reduce-only,follow,...>
> > > > 
> > > >       reduce-only (default):
> > > >         update vlan's MTU only if the new MTU is smaller than the
> > > >         current one (current behaviour).
> > > > 
> > > >       follow:
> > > >         always follow the MTU of the parent device.
> > > > 
> > > > Then if anyone wants more complex policies:
> > > > 
> > > >       follow-if-not-modified:
> > > >         follow the MTU of the parent device as long as the VLAN's MTU
> > > >         was not manually changed. Otherwise only adjust the VLAN's MTU
> > > >         when the parent's one is set to a smaller value.
> > > > 
> > > >       follow-if-not-modified-but-not-quite:
> > > >         like follow-if-not-modified but revert back to the VLAN's
> > > >         last manually modified MTU, if any, whenever possible (that is,
> > > >         when the parent device's MTU is set back to a higher value).
> > > >         That probably requires the possibility to dump the last
> > > >         modified MTU, so the administrator can anticipate the
> > > >         consequences of modifying the parent device.
> > > > 
> > > >      yet-another-policy (because people have a lot of imagination):
> > > >        for example, keep the MTU 4 bytes lower than the parent device,
> > > >        to account for VLAN overhead.
> > > > 
> > > > Of course feel free to suggest better names and policies :).
> > > > 
> > > > This way, we can keep the current behaviour and avoid unexpected
> > > > heuristics that are difficult to explain (and even more difficult for
> > > > network admins to figure out on their own).  
> > > 
> > > My $0.02 would be that if we want to make changes that require new uAPI
> > > we should do it across uppers.  
> > 
> > Do you mean something like:
> > 
> >   ip link set dev eth0 vlan-mtu-policy <policy-name>
> > 
> > that'd affect all existing (and future) vlans of eth0?
> > 
> > Then I think that for non-ethernet devices, we should reject this
> > option and skip it when dumping config. But yes, that's another
> > possibility.
> > 
> > I personnaly don't really mind, as long as we keep a clear behaviour.
> > 
> > What I'd really like to avoid is something like:
> >   - By default it behaves this way.
> >   - If you modified the MTU it behaves in another way
> >   - But if you modified the MTU but later restored the
> >     original MTU, then you're back to the default behaviour
> >     (or not?), unless the MTU of the upper device was also
> >     changed meanwhile, in which case ... to be continued ...
> >   - BTW, you might not be able to tell how the VLAN's MTU is going to
> >     behave by simply looking at its configuration, because that also
> >     depends on past configurations.
> >   - Well, and if your kernel is older than xxx, then you always get the
> >     default behaviour.
> >   - ... and we might modify the heuristics again in the future to
> >     accomodate with situations or use cases we failed to consider.
> > 
> 
> In general these kind of policy choices are done via sysctl knobs.
> They aren't done at netlink/ip link level.

I don't really mind if the configuration is per vlan, per upper device
or per netns, as long as we keep a clear behaviour by default.

Re: [PATCH net] net: vlan: allow vlan device MTU change follow real device from smaller to bigger

[Jakub Kicinski]

On Wed, 23 Feb 2022 12:26:18 +0100 Guillaume Nault wrote:
> Do you mean something like:
> 
>   ip link set dev eth0 vlan-mtu-policy <policy-name>
> 
> that'd affect all existing (and future) vlans of eth0?

I meant

  ip link set dev vlan0 mtu-policy blah

but also

  ip link set dev bond0 mtu-policy blah

and

  ip link set dev macsec0 mtu-policy blah2
  ip link set dev vxlan0 mtu-policy blah2

etc.

> Then I think that for non-ethernet devices, we should reject this
> option and skip it when dumping config. But yes, that's another
> possibility.
> 
> I personnaly don't really mind, as long as we keep a clear behaviour.
> 
> What I'd really like to avoid is something like:
>   - By default it behaves this way.
>   - If you modified the MTU it behaves in another way
>   - But if you modified the MTU but later restored the
>     original MTU, then you're back to the default behaviour
>     (or not?), unless the MTU of the upper device was also
>     changed meanwhile, in which case ... to be continued ...
>   - BTW, you might not be able to tell how the VLAN's MTU is going to
>     behave by simply looking at its configuration, because that also
>     depends on past configurations.
>   - Well, and if your kernel is older than xxx, then you always get the
>     default behaviour.
>   - ... and we might modify the heuristics again in the future to
>     accomodate with situations or use cases we failed to consider.

To be honest I'm still not clear if this is a real problem.
The patch does not specify what the use case is.

Re: [PATCH net] net: vlan: allow vlan device MTU change follow real device from smaller to bigger

[Guillaume Nault]

On Wed, Feb 23, 2022 at 08:03:42AM -0800, Jakub Kicinski wrote:
> On Wed, 23 Feb 2022 12:26:18 +0100 Guillaume Nault wrote:
> > Do you mean something like:
> > 
> >   ip link set dev eth0 vlan-mtu-policy <policy-name>
> > 
> > that'd affect all existing (and future) vlans of eth0?
> 
> I meant
> 
>   ip link set dev vlan0 mtu-policy blah
> 
> but also
> 
>   ip link set dev bond0 mtu-policy blah
> 
> and
> 
>   ip link set dev macsec0 mtu-policy blah2
>   ip link set dev vxlan0 mtu-policy blah2
> 
> etc.

Unless I'm missing something, that looks very much like what I proposed
(these are all ARPHRD_ETHER devices). It's just a bit unclear whether
"ip link set dev vlan0 mtu-policy blah" applies to vlan0 or to the vlans
that might be stacked on top of it (given your other examples, I assume
it's the later).

> > Then I think that for non-ethernet devices, we should reject this
> > option and skip it when dumping config. But yes, that's another
> > possibility.
> > 
> > I personnaly don't really mind, as long as we keep a clear behaviour.
> > 
> > What I'd really like to avoid is something like:
> >   - By default it behaves this way.
> >   - If you modified the MTU it behaves in another way
> >   - But if you modified the MTU but later restored the
> >     original MTU, then you're back to the default behaviour
> >     (or not?), unless the MTU of the upper device was also
> >     changed meanwhile, in which case ... to be continued ...
> >   - BTW, you might not be able to tell how the VLAN's MTU is going to
> >     behave by simply looking at its configuration, because that also
> >     depends on past configurations.
> >   - Well, and if your kernel is older than xxx, then you always get the
> >     default behaviour.
> >   - ... and we might modify the heuristics again in the future to
> >     accomodate with situations or use cases we failed to consider.
> 
> To be honest I'm still not clear if this is a real problem.
> The patch does not specify what the use case is.

It's probably not a problem as long as we keep sane behaviour by
default. Then we can let admins opt in for something more complex or
loosely defined.

Re: [PATCH net] net: vlan: allow vlan device MTU change follow real device from smaller to bigger

[Jakub Kicinski]

On Wed, 23 Feb 2022 17:58:36 +0100 Guillaume Nault wrote:
> On Wed, Feb 23, 2022 at 08:03:42AM -0800, Jakub Kicinski wrote:
> > I meant
> > 
> >   ip link set dev vlan0 mtu-policy blah
> > 
> > but also
> > 
> >   ip link set dev bond0 mtu-policy blah
> > 
> > and
> > 
> >   ip link set dev macsec0 mtu-policy blah2
> >   ip link set dev vxlan0 mtu-policy blah2
> > 
> > etc.  
> 
> Unless I'm missing something, that looks very much like what I proposed
> (these are all ARPHRD_ETHER devices). It's just a bit unclear whether
> "ip link set dev vlan0 mtu-policy blah" applies to vlan0 or to the vlans
> that might be stacked on top of it (given your other examples, I assume
> it's the later).

No, sorry I thought it would be clear, we need that neuralink ;)
It applies to the device on which it's configured. What I mean
is that bond, macsec, mpls etc have the same "should it follow 
the MTU of the lower device" problem, it's not vlan specific.
Or am I wrong about that?

> > To be honest I'm still not clear if this is a real problem.
> > The patch does not specify what the use case is.  
> 
> It's probably not a problem as long as we keep sane behaviour by
> default. Then we can let admins opt in for something more complex or
> loosely defined.

What I meant was - does anyone actually flip the MTU of their
interfaces back and forth while the system is running. Maybe
people do.

Re: [PATCH net] net: vlan: allow vlan device MTU change follow real device from smaller to bigger

[Guillaume Nault]

On Wed, Feb 23, 2022 at 09:37:49AM -0800, Jakub Kicinski wrote:
> On Wed, 23 Feb 2022 17:58:36 +0100 Guillaume Nault wrote:
> > On Wed, Feb 23, 2022 at 08:03:42AM -0800, Jakub Kicinski wrote:
> > > I meant
> > > 
> > >   ip link set dev vlan0 mtu-policy blah
> > > 
> > > but also
> > > 
> > >   ip link set dev bond0 mtu-policy blah
> > > 
> > > and
> > > 
> > >   ip link set dev macsec0 mtu-policy blah2
> > >   ip link set dev vxlan0 mtu-policy blah2
> > > 
> > > etc.  
> > 
> > Unless I'm missing something, that looks very much like what I proposed
> > (these are all ARPHRD_ETHER devices). It's just a bit unclear whether
> > "ip link set dev vlan0 mtu-policy blah" applies to vlan0 or to the vlans
> > that might be stacked on top of it (given your other examples, I assume
> > it's the later).
> 
> No, sorry I thought it would be clear, we need that neuralink ;)
> It applies to the device on which it's configured. What I mean
> is that bond, macsec, mpls etc have the same "should it follow 
> the MTU of the lower device" problem, it's not vlan specific.
> Or am I wrong about that?

Ok, I get it now, sorry for being slow :). But I wouldn't consider mpls
and vxlan. We have no device type for mpls. For vxlan (and other ip
tunnels) the virtual device isn't directly tied to a physical device.
Also, ip tunnels can resort to fragmentation in case of small MTU on
the output device, so following MTU changes is not a hard requirement
as with vlans.

For other devices, we'd probably have to take into account the fact
that some of them need to have a smaller MTU due to their extra header
(that can be the case for some stacked vlans scenarios).

But honnestly, I don't believe it's worth the extra complexity.

> > > To be honest I'm still not clear if this is a real problem.
> > > The patch does not specify what the use case is.  
> > 
> > It's probably not a problem as long as we keep sane behaviour by
> > default. Then we can let admins opt in for something more complex or
> > loosely defined.
> 
> What I meant was - does anyone actually flip the MTU of their
> interfaces back and forth while the system is running. Maybe
> people do.

In my experience people often try to upgrade their MTU, which is prone
to failure because all nodes on the ethernet segment need to be
upgraded at once (and people like unmanageably big ethernet segments).
So reverting to the previous configuration is often needed.
Another reason for back and forth modifications is fat fingers: change
the MTU of a device, realise that was the wrong one, restore settings
and reapply on the correct device.

More importantly, one path people take to upgrade their MTU is to
ensure that all their traffic is vlan encapsulated, then higher the MTU
of the ethernet device, and finally higher the MTU of each vlan on a
case by case basis. In such scenarios, you certainly _don't_ want the
vlans to follow the MTU of their parent device, no matter if their MTU
is the default one, if it's equal to the current MTU of the eth
interface, if it was ever modified since the creation of the device,
or any other situation heuristics might use.

Re: [PATCH net] net: vlan: allow vlan device MTU change follow real device from smaller to bigger

[Stephen Hemminger]

On Wed, 23 Feb 2022 08:03:42 -0800
Jakub Kicinski <kuba@kernel.org> wrote:

> On Wed, 23 Feb 2022 12:26:18 +0100 Guillaume Nault wrote:
> > Do you mean something like:
> > 
> >   ip link set dev eth0 vlan-mtu-policy <policy-name>
> > 
> > that'd affect all existing (and future) vlans of eth0?  
> 
> I meant
> 
>   ip link set dev vlan0 mtu-policy blah
> 
> but also
> 
>   ip link set dev bond0 mtu-policy blah
> 
> and
> 
>   ip link set dev macsec0 mtu-policy blah2
>   ip link set dev vxlan0 mtu-policy blah2

Sorry, putting this in ip link is not the right place.
It belongs in sysctl (if at all); not convinced this is worth doing.