Commit a985343ba906 ("vxlan: refactor verification and
application of configuration") introduced a change in the
behaviour of initial MTU setting: earlier, the MTU for a link
created on top of a given lower device, without an initial MTU
specification, was set to the MTU of the lower device minus
headroom as a result of this path in vxlan_dev_configure():
if (!conf->mtu)
dev->mtu = lowerdev->mtu -
(use_ipv6 ? VXLAN6_HEADROOM : VXLAN_HEADROOM);
which is now gone. Now, the initial MTU, in absence of a
configured value, is simply set by ether_setup() to ETH_DATA_LEN
(1500 bytes).
This breaks userspace expectations in case the MTU of
the lower device is higher than 1500 bytes minus headroom.
Restore the previous behaviour by calculating, for a new link,
the MTU from the lower device, if present, and if no value is
explicitly configured.
Reported-by: Junhan Yan <redacted>
Fixes: a985343ba906 ("vxlan: refactor verification and application of configuration")
Signed-off-by: Stefano Brivio <redacted>
---
I guess this should be queued up for -stable, back to 4.13.
I'm actually introducing the third occurrence of this calculation (there's
another one in vxlan_config_apply(), and one in vxlan_change_mtu()). I would
anyway fix the userspace breakage first, and then plan on getting rid of several
bits of MTU logic duplication, which spans further than this.
drivers/net/vxlan.c | 3 +++
1 file changed, 3 insertions(+)
From: Stephen Hemminger <stephen@networkplumber.org> Date: 2017-12-13 23:19:51
On Wed, 13 Dec 2017 23:37:00 +0100
Stefano Brivio [off-list ref] wrote:
Commit a985343ba906 ("vxlan: refactor verification and
application of configuration") introduced a change in the
behaviour of initial MTU setting: earlier, the MTU for a link
created on top of a given lower device, without an initial MTU
specification, was set to the MTU of the lower device minus
headroom as a result of this path in vxlan_dev_configure():
if (!conf->mtu)
dev->mtu = lowerdev->mtu -
(use_ipv6 ? VXLAN6_HEADROOM : VXLAN_HEADROOM);
which is now gone. Now, the initial MTU, in absence of a
configured value, is simply set by ether_setup() to ETH_DATA_LEN
(1500 bytes).
This breaks userspace expectations in case the MTU of
the lower device is higher than 1500 bytes minus headroom.
Restore the previous behaviour by calculating, for a new link,
the MTU from the lower device, if present, and if no value is
explicitly configured.
Reported-by: Junhan Yan <redacted>
Fixes: a985343ba906 ("vxlan: refactor verification and application of configuration")
Signed-off-by: Stefano Brivio <redacted>
---
Good catch.
Acked-by: Stephen Hemminger <stephen@networkplumber.org>
Commit a985343ba906 ("vxlan: refactor verification and
application of configuration") introduced a change in the
behaviour of initial MTU setting: earlier, the MTU for a link
created on top of a given lower device, without an initial MTU
specification, was set to the MTU of the lower device minus
headroom as a result of this path in vxlan_dev_configure():
if (!conf->mtu)
dev->mtu = lowerdev->mtu -
(use_ipv6 ? VXLAN6_HEADROOM : VXLAN_HEADROOM);
which is now gone. Now, the initial MTU, in absence of a
configured value, is simply set by ether_setup() to ETH_DATA_LEN
(1500 bytes).
This breaks userspace expectations in case the MTU of
the lower device is higher than 1500 bytes minus headroom.
Restore the previous behaviour by calculating, for a new link,
the MTU from the lower device, if present, and if no value is
explicitly configured.
Reported-by: Junhan Yan <redacted>
Fixes: a985343ba906 ("vxlan: refactor verification and application of configuration")
Signed-off-by: Stefano Brivio <redacted>
---
I guess this should be queued up for -stable, back to 4.13.
I'm actually introducing the third occurrence of this calculation (there's
another one in vxlan_config_apply(), and one in vxlan_change_mtu()). I would
anyway fix the userspace breakage first, and then plan on getting rid of several
bits of MTU logic duplication, which spans further than this.
As you note, there is another occurrence of this calculation in
vxlan_config_apply():
[...]
if (lowerdev) {
[...]
max_mtu = lowerdev->mtu - (use_ipv6 ? VXLAN6_HEADROOM :
VXLAN_HEADROOM);
}
if (dev->mtu > max_mtu)
dev->mtu = max_mtu;
[...]
Unless I'm overlooking something, this should already do the same thing and
your patch is redundant.
Regards,
Matthias
On Thu, 14 Dec 2017 00:57:32 +0100
Matthias Schiffer [off-list ref] wrote:
As you note, there is another occurrence of this calculation in
vxlan_config_apply():
[...]
if (lowerdev) {
[...]
max_mtu = lowerdev->mtu - (use_ipv6 ? VXLAN6_HEADROOM :
VXLAN_HEADROOM);
}
if (dev->mtu > max_mtu)
dev->mtu = max_mtu;
[...]
Unless I'm overlooking something, this should already do the same thing and
your patch is redundant.
The code above sets max_mtu, and only if dev->mtu exceeds that, the
latter is then clamped.
What my patch does is to actually set dev->mtu to that value, no matter
what's the previous value set by ether_setup() (only on creation, and
only if lowerdev is there), just like the previous behaviour used to be.
Let's consider these two cases, on the existing code:
1. lowerdev->mtu is 1500:
- ether_setup(), called by vxlan_setup(), sets dev->mtu to 1500
- here max_mtu is 1450
- we enter the second if clause above (dev->mtu > max_mtu)
- at the end of vxlan_config_apply(), dev->mtu will be 1450
which is consistent with the previous behaviour.
2. lowerdev->mtu is 9000:
- ether_setup(), called by vxlan_setup(), sets dev->mtu to 1500
- here max_mtu is 8950
- we do not enter the second if clause above (dev->mtu < max_mtu)
- at the end of vxlan_config_apply(), dev->mtu will still be 1500
which is not consistent with the previous behaviour, where it used to
be 8950 instead.
--
Stefano
On Thu, 14 Dec 2017 00:57:32 +0100
Matthias Schiffer [off-list ref] wrote:
quoted
As you note, there is another occurrence of this calculation in
vxlan_config_apply():
[...]
if (lowerdev) {
[...]
max_mtu = lowerdev->mtu - (use_ipv6 ? VXLAN6_HEADROOM :
VXLAN_HEADROOM);
}
if (dev->mtu > max_mtu)
dev->mtu = max_mtu;
[...]
Unless I'm overlooking something, this should already do the same thing and
your patch is redundant.
The code above sets max_mtu, and only if dev->mtu exceeds that, the
latter is then clamped.
What my patch does is to actually set dev->mtu to that value, no matter
what's the previous value set by ether_setup() (only on creation, and
only if lowerdev is there), just like the previous behaviour used to be.
Let's consider these two cases, on the existing code:
1. lowerdev->mtu is 1500:
- ether_setup(), called by vxlan_setup(), sets dev->mtu to 1500
- here max_mtu is 1450
- we enter the second if clause above (dev->mtu > max_mtu)
- at the end of vxlan_config_apply(), dev->mtu will be 1450
which is consistent with the previous behaviour.
2. lowerdev->mtu is 9000:
- ether_setup(), called by vxlan_setup(), sets dev->mtu to 1500
- here max_mtu is 8950
- we do not enter the second if clause above (dev->mtu < max_mtu)
- at the end of vxlan_config_apply(), dev->mtu will still be 1500
which is not consistent with the previous behaviour, where it used to
be 8950 instead.
Ah, thank you for the explanation, I was missing the context that this was
about higher rather than lower MTUs.
Personally, I would prefer a change like the following, as it does not
introduce another duplication of the MTU calculation (not tested at all):
On Thu, 14 Dec 2017 01:25:40 +0100
Matthias Schiffer [off-list ref] wrote:
On 12/14/2017 01:10 AM, Stefano Brivio wrote:
quoted
On Thu, 14 Dec 2017 00:57:32 +0100
Matthias Schiffer [off-list ref] wrote:
quoted
As you note, there is another occurrence of this calculation in
vxlan_config_apply():
[...]
if (lowerdev) {
[...]
max_mtu = lowerdev->mtu - (use_ipv6 ? VXLAN6_HEADROOM :
VXLAN_HEADROOM);
}
if (dev->mtu > max_mtu)
dev->mtu = max_mtu;
[...]
Unless I'm overlooking something, this should already do the same thing and
your patch is redundant.
The code above sets max_mtu, and only if dev->mtu exceeds that, the
latter is then clamped.
What my patch does is to actually set dev->mtu to that value, no matter
what's the previous value set by ether_setup() (only on creation, and
only if lowerdev is there), just like the previous behaviour used to be.
Let's consider these two cases, on the existing code:
1. lowerdev->mtu is 1500:
- ether_setup(), called by vxlan_setup(), sets dev->mtu to 1500
- here max_mtu is 1450
- we enter the second if clause above (dev->mtu > max_mtu)
- at the end of vxlan_config_apply(), dev->mtu will be 1450
which is consistent with the previous behaviour.
2. lowerdev->mtu is 9000:
- ether_setup(), called by vxlan_setup(), sets dev->mtu to 1500
- here max_mtu is 8950
- we do not enter the second if clause above (dev->mtu < max_mtu)
- at the end of vxlan_config_apply(), dev->mtu will still be 1500
which is not consistent with the previous behaviour, where it used to
be 8950 instead.
Ah, thank you for the explanation, I was missing the context that this was
about higher rather than lower MTUs.
Personally, I would prefer a change like the following, as it does not
introduce another duplication of the MTU calculation (not tested at all):
You would also need to check that lowerdev is present, though.
Otherwise, you're changing the behaviour again, that is, if lowerdev is
not present, we want to keep 1500 and not set ETH_MAX_MTU (65535).
Sure you can change the if condition to reflect that, but IMHO it
becomes quite awkward.
--
Stefano
On Thu, 14 Dec 2017 01:25:40 +0100
Matthias Schiffer [off-list ref] wrote:
quoted
On 12/14/2017 01:10 AM, Stefano Brivio wrote:
quoted
On Thu, 14 Dec 2017 00:57:32 +0100
Matthias Schiffer [off-list ref] wrote:
quoted
As you note, there is another occurrence of this calculation in
vxlan_config_apply():
[...]
if (lowerdev) {
[...]
max_mtu = lowerdev->mtu - (use_ipv6 ? VXLAN6_HEADROOM :
VXLAN_HEADROOM);
}
if (dev->mtu > max_mtu)
dev->mtu = max_mtu;
[...]
Unless I'm overlooking something, this should already do the same thing and
your patch is redundant.
The code above sets max_mtu, and only if dev->mtu exceeds that, the
latter is then clamped.
What my patch does is to actually set dev->mtu to that value, no matter
what's the previous value set by ether_setup() (only on creation, and
only if lowerdev is there), just like the previous behaviour used to be.
Let's consider these two cases, on the existing code:
1. lowerdev->mtu is 1500:
- ether_setup(), called by vxlan_setup(), sets dev->mtu to 1500
- here max_mtu is 1450
- we enter the second if clause above (dev->mtu > max_mtu)
- at the end of vxlan_config_apply(), dev->mtu will be 1450
which is consistent with the previous behaviour.
2. lowerdev->mtu is 9000:
- ether_setup(), called by vxlan_setup(), sets dev->mtu to 1500
- here max_mtu is 8950
- we do not enter the second if clause above (dev->mtu < max_mtu)
- at the end of vxlan_config_apply(), dev->mtu will still be 1500
which is not consistent with the previous behaviour, where it used to
be 8950 instead.
Ah, thank you for the explanation, I was missing the context that this was
about higher rather than lower MTUs.
Personally, I would prefer a change like the following, as it does not
introduce another duplication of the MTU calculation (not tested at all):
You would also need to check that lowerdev is present, though.
Otherwise, you're changing the behaviour again, that is, if lowerdev is
not present, we want to keep 1500 and not set ETH_MAX_MTU (65535).
Sure you can change the if condition to reflect that, but IMHO it
becomes quite awkward.
Hmm, right. So here's a
Reviewed-by: Matthias Schiffer <redacted>
for the minimal change for -stable.
On Thu, 14 Dec 2017 01:25:40 +0100
Matthias Schiffer [off-list ref] wrote:
quoted
On 12/14/2017 01:10 AM, Stefano Brivio wrote:
quoted
On Thu, 14 Dec 2017 00:57:32 +0100
Matthias Schiffer [off-list ref] wrote:
quoted
As you note, there is another occurrence of this calculation in
vxlan_config_apply():
[...]
if (lowerdev) {
[...]
max_mtu = lowerdev->mtu - (use_ipv6 ? VXLAN6_HEADROOM :
VXLAN_HEADROOM);
}
if (dev->mtu > max_mtu)
dev->mtu = max_mtu;
[...]
Unless I'm overlooking something, this should already do the same thing and
your patch is redundant.
The code above sets max_mtu, and only if dev->mtu exceeds that, the
latter is then clamped.
What my patch does is to actually set dev->mtu to that value, no matter
what's the previous value set by ether_setup() (only on creation, and
only if lowerdev is there), just like the previous behaviour used to be.
Let's consider these two cases, on the existing code:
1. lowerdev->mtu is 1500:
- ether_setup(), called by vxlan_setup(), sets dev->mtu to 1500
- here max_mtu is 1450
- we enter the second if clause above (dev->mtu > max_mtu)
- at the end of vxlan_config_apply(), dev->mtu will be 1450
which is consistent with the previous behaviour.
2. lowerdev->mtu is 9000:
- ether_setup(), called by vxlan_setup(), sets dev->mtu to 1500
- here max_mtu is 8950
- we do not enter the second if clause above (dev->mtu < max_mtu)
- at the end of vxlan_config_apply(), dev->mtu will still be 1500
which is not consistent with the previous behaviour, where it used to
be 8950 instead.
Ah, thank you for the explanation, I was missing the context that this was
about higher rather than lower MTUs.
Personally, I would prefer a change like the following, as it does not
introduce another duplication of the MTU calculation (not tested at all):
You would also need to check that lowerdev is present, though.
if we move it up in "if (lowerdev) { ..." branch we will be checking the presence
of "lowerdev" and also not calculating it again. Also I would check max_mtu for
minimum as it might happen to be negative, though unlikely corner case...
Otherwise, you're changing the behaviour again, that is, if lowerdev is
not present, we want to keep 1500 and not set ETH_MAX_MTU (65535).
Sure you can change the if condition to reflect that, but IMHO it
becomes quite awkward.
On Thu, 14 Dec 2017 14:23:36 +0300
Alexey Kodanev [off-list ref] wrote:
On 12/14/2017 03:31 AM, Stefano Brivio wrote:
quoted
On Thu, 14 Dec 2017 01:25:40 +0100
Matthias Schiffer [off-list ref] wrote:
quoted
On 12/14/2017 01:10 AM, Stefano Brivio wrote:
quoted
On Thu, 14 Dec 2017 00:57:32 +0100
Matthias Schiffer [off-list ref] wrote:
quoted
As you note, there is another occurrence of this calculation in
vxlan_config_apply():
[...]
if (lowerdev) {
[...]
max_mtu = lowerdev->mtu - (use_ipv6 ? VXLAN6_HEADROOM :
VXLAN_HEADROOM);
}
if (dev->mtu > max_mtu)
dev->mtu = max_mtu;
[...]
Unless I'm overlooking something, this should already do the same thing and
your patch is redundant.
The code above sets max_mtu, and only if dev->mtu exceeds that, the
latter is then clamped.
What my patch does is to actually set dev->mtu to that value, no matter
what's the previous value set by ether_setup() (only on creation, and
only if lowerdev is there), just like the previous behaviour used to be.
Let's consider these two cases, on the existing code:
1. lowerdev->mtu is 1500:
- ether_setup(), called by vxlan_setup(), sets dev->mtu to 1500
- here max_mtu is 1450
- we enter the second if clause above (dev->mtu > max_mtu)
- at the end of vxlan_config_apply(), dev->mtu will be 1450
which is consistent with the previous behaviour.
2. lowerdev->mtu is 9000:
- ether_setup(), called by vxlan_setup(), sets dev->mtu to 1500
- here max_mtu is 8950
- we do not enter the second if clause above (dev->mtu < max_mtu)
- at the end of vxlan_config_apply(), dev->mtu will still be 1500
which is not consistent with the previous behaviour, where it used to
be 8950 instead.
Ah, thank you for the explanation, I was missing the context that this was
about higher rather than lower MTUs.
Personally, I would prefer a change like the following, as it does not
introduce another duplication of the MTU calculation (not tested at all):
You would also need to check that lowerdev is present, though.
if we move it up in "if (lowerdev) { ..." branch we will be checking the presence
of "lowerdev" and also not calculating it again. Also I would check max_mtu for
minimum as it might happen to be negative, though unlikely corner case...
Indeed it might happen to be negative (only for IPv6, down to -2), good
catch.
For the benefit of others: it took me a few minutes to see how this is
*not* unrelated, because we are introducing a direct assignment of
dev->mtu to set max_mtu, whereas earlier it was just used in
comparisons, so it didn't matter whether it was negative.
I don't really have a strong preference here. On one hand, you're
hiding this a bit from the "device creation" path. On the other hand,
it's a bit more compact. So I'm also fine with this.
Can you perhaps submit a formal patch?
--
Stefano
On Thu, 14 Dec 2017 14:23:36 +0300
Alexey Kodanev [off-list ref] wrote:
quoted
On 12/14/2017 03:31 AM, Stefano Brivio wrote:
...
quoted
if we move it up in "if (lowerdev) { ..." branch we will be checking the presence
of "lowerdev" and also not calculating it again. Also I would check max_mtu for
minimum as it might happen to be negative, though unlikely corner case...
Indeed it might happen to be negative (only for IPv6, down to -2), good
catch.
For the benefit of others: it took me a few minutes to see how this is
*not* unrelated, because we are introducing a direct assignment of
dev->mtu to set max_mtu, whereas earlier it was just used in
comparisons, so it didn't matter whether it was negative.
I don't really have a strong preference here. On one hand, you're
hiding this a bit from the "device creation" path. On the other hand,
it's a bit more compact. So I'm also fine with this.
Can you perhaps submit a formal patch?