Bug #5857
closedVLANs on interface with 1500 MTU PPPoE end up with 1508 MTU
100%
Description
Issue described here:
https://github.com/pfsense/pfsense/pull/2598
though that PR won't work to fix since it changes jumbo frame VLAN behavior.
There doesn't appear to be any reason to maintain the behavior of forcing every VLAN to have the same MTU as its parent interface, or any reason to disregard the MTU configured on VLANs. Input validation prevents the problem circumstances there, not allowing VLANs' MTU to be > parent interface MTU.
Updated by David Wood over 8 years ago
I, too, believe that the "All vlans need to use the same mtu value as their parent" comment at the head of interface_vlan_adapt_mtu() is wrong. I share your belief that the restriction is that a VLAN cannot have an MTU exceeding that of the parent interface, Chris.
There is no reason for every VLAN on a parent interface to have the same MTU and interface_vlan_adapt_mtu() has not enforced that restriction for some time despite that comment. You were able to set a explicit VLAN MTU lower than that of the parent interface before I changed this function in the RFC 4638 patch and Renato subsequently refactored the function.
The fix I submitted in #2598 does nothing more than restore the behaviour I introduced in the RFC 4638 patch - a VLAN that does not have an explicitly configured MTU and is not a PPPoE parent interface will have an MTU of no more than 1500. Jumbo frames on a VLAN are still possible - you just have to configure a suitable MTU explicitly. I believe it is unwise for a jumbo MTU to come about implicitly.
A mix of jumbo and non-jumbo MTUs on VLANs with the same parent interface works correctly:- in production on my pfSense 2.2.6 box with RFC 4638 patch (using igb(4) interfaces)
- on a FreeBSD 10.2 box (using bce(4) interfaces) - this configuration is very long standing, probably back to FreeBSD 7.x
- on my pfSense 2.3 test VM (using VirtualBox bridged network interfaces on the FreeBSD 10.2 box, which are vtnet(4) devices)
I therefore see no reason why the fix in #2598 cannot be committed.
However, I have some unease about interface_vlan_adapt_mtu() as it stands, as Renato's refactoring has shown up the possibility of improvement. It is arguably better to adjust the logic so that it more accurately reflects the restriction by:- determining the wanted MTU for the VLAN (explicitly configured value if there is one, otherwise the value wanted for PPPoE if there is one then, if neither of those apply, 1500)
- determining the current MTU of the VLAN
- exiting if the current MTU of the VLAN is the same as the wanted MTU
- determining the current MTU of the parent interface
- lowering the VLAN MTU to the wanted MTU if the wanted MTU is lower than the current MTU of the parent interface
If a more global attempt is being made to tidy up the MTU handling code, the MTU part of interface_configure() should really be looked at. For a start, the repeated "All vlans need to use the same mtu value as their parent" comment should go if it is incorrect, though I believe the code also needs looking at. As part of this process, the interface_vlan_mtu_configured() function might also need attention.
One scenario I didn't even attempt to unravel when writing the RFC 4638 patch was:
- physical interface
- QinQ
- VLAN that is a PPPoE parent interface
I was aware that I was introducing enough additional complexity in the RFC 4638 patch, so I decided not to handle this QinQ case, which I thought was unlikely to exist in any deployed system. I don't have a QinQ capable switch, so couldn't test any changes to the QinQ code.
I found myself wishing on numerous occasions when working on RFC 4638 and IPv6 related issues that interfaces were handled in an entirely method based manner and ideally interfaces were implemented as an abstract class. Until that sort of complete rewrite of the interface code or, more likely, the whole of pfSense is possible, I'll continue to do what I can to help improve the current code.
Updated by Jorge M. Oliveira over 8 years ago
Maybe the logic behind this code could be worked out
https://github.com/pfsense/pfsense/blob/master/src/etc/inc/interfaces.inc#L3134
https://github.com/pfsense/pfsense/blob/master/src/etc/inc/interfaces.inc#L3140
Looks like it is where the +8 increment comes from.
Updated by Jorge M. Oliveira over 8 years ago
Also there is a situation where the value is decremented by 8
https://github.com/pfsense/pfsense/blob/master/src/etc/inc/interfaces.inc#L1878
So everything needs to be dealt with care.
Updated by Luiz Souza over 8 years ago
- Status changed from Confirmed to Feedback
- % Done changed from 0 to 100
The code was reworked and tested with many different settings. All the known issues were fixed.
Updated by Chris Buechler over 8 years ago
- Status changed from Feedback to Resolved
fixed, and not seeing anything that regressed.