Bugzilla (bugzilla.redhat.com) will be under maintenance for infrastructure upgrades and will not be unavailable on July 31st between 12:30 AM - 05:30 AM UTC. We appreciate your understanding and patience. You can follow status.redhat.com for details.
Bug 1887523 - [bond] The packets_per_slave option should be only valid under balance-rr mode
Summary: [bond] The packets_per_slave option should be only valid under balance-rr mode
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Red Hat Enterprise Linux 8
Classification: Red Hat
Component: NetworkManager
Version: 8.4
Hardware: x86_64
OS: Linux
low
low
Target Milestone: rc
: 8.4
Assignee: Thomas Haller
QA Contact: Vladimir Benes
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2020-10-12 17:34 UTC by Gris Ge
Modified: 2021-05-18 13:30 UTC (History)
10 users (show)

Fixed In Version: NetworkManager-1.30.0-0.2.el8
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2021-05-18 13:29:43 UTC
Type: Bug
Target Upstream Version:


Attachments (Terms of Use)

Description Gris Ge 2020-10-12 17:34:44 UTC
Description of problem:

The `NM.SettingBond.add_option('packets_per_slave', '1')` with mode `802.3ad` should return False as the `packets_per_slave` is only valid under balance-rr mode.

Version-Release number of selected component (if applicable):
NetworkManager-1.26.0-8.el8.x86_64

How reproducible:
100%

Steps to Reproduce:
1. Please check with NM developer for reproducer in nmcli.
2.
3.

Actual results:


Expected results:


Additional info:

Comment 1 Thomas Haller 2020-10-12 19:20:16 UTC
`nm_setting_bond_add_option()` does not perform such kind of validations, nor should it.

For one, that is because you always could also set the GObject property (NM_SETTING_BOND_OPTIONS), and when setting a GObject property, there is no place to validate the input (as `g_object_set()` has no return value to indicate failure).

In general, in libnm it is not the setters that perform validation. Validation should be performed by nm_connection_verify(). That is for example also because often the validity of a setting depends on multiple properties (like `packets_per_slave` only works with certain bond modes). Validating values in the setter would mean that you need to set the options in a certain order. Say you start with a setting "mode=balance-rr, packets_per_slave=1" and you want to get to "mode=802.3ad, ad_actor_sys_prio=5". It means, you either first set "mode=802.3ad" (but then "packets_per_slave=1" would be wrong/inconsistent), or you first set "ad_actor_sys_prio=5" (but then the mode would be wrong).


Actually, add-option does perform some basic form of validation. For example, "packets_per_slave" is validated to be a string. I think that is an actual bug, because it means you can set options via NM_SETTING_BOND_OPTIONS to anything, but there is no corresponding C API to achieve the same. That is a problem, because keyfile reader uses nm_setting_bond_add_option(), and -- in case of an invalid value -- it silently ignores certain values.


Actually, nm_connection_verify() for bond options has a different problem: historically the validation was not strict and accept some settings that would not be valid. This was not fixed so far, because making validation more strict is a breaking change in behavior. Maybe, nm_connection_verify() should be tightend up for bond settings, but it's problematic to do so.


What also should be done is `nm_setting_bond_add_option()` consistently does *not* perform any of the basic validation. Because the basic validation should only happen during nm_connection_verify(), and keyfile reader would better handle reading invalid values -- of course, that would also be a breaking change in behavior *sigh*.


The way bond options are handled by libnm/NMSettingBond is rather ugly, and fixing it would break existing behavior.

Comment 2 Antonio Cardace 2020-10-14 08:51:39 UTC
Gris does the '`packets_per_slave` option get rejected when bond-mode is not 'balance-rr' when using nm_connection_verify()?

Comment 3 Gris Ge 2020-10-14 17:07:57 UTC
The `NM.RemoteConnection.verify()` return True when holding `NM.SettingBond` with `packets_per_slave: 1` and `mode: active-backup`.

Nmstate has verification stage which will warn user about invalid bond options, so I don't expect NM fix it any time soon. Just want to let you know the issue exists.

As the API user of libnm, the document said `add_option(): True if the option was valid and was added to the internal option list, False if it was not.`. And the document also have `validate_option()` which still return `True` in this case.
Please update the document of API also.

Comment 4 Thomas Haller 2020-10-15 09:12:08 UTC
> The `NM.RemoteConnection.verify()` return True when holding `NM.SettingBond` with `packets_per_slave: 1` and `mode: active-backup`.

Yes, that's because 

> > Actually, nm_connection_verify() for bond options has a different problem: historically the validation was not strict and accept some settings that would not be valid. This was not fixed so far, because making validation more strict is a breaking change in behavior. Maybe, nm_connection_verify() should be tightend up for bond settings, but it's problematic to do so.




https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/653

^^ this disables all validation of nm_setting_bond_add_option() and adjust documentation. It does not add stricter validation for nm_connection_verify(). Maybe that should be done in the future, but it's problematic.

Comment 5 Antonio Cardace 2020-10-15 16:22:42 UTC
Gris are you happy with this solution? I know it's not ideal but we might break a lot of (incorrect) connection profiles if we make the verify() stricter.

Unfortunately options which are not compatible with the bond mode get stripped from the connection profile in NM.

Comment 6 Gris Ge 2020-10-18 23:14:38 UTC
I am OK with NM remove all check on bond options and let others do the work.

Comment 7 Antonio Cardace 2020-10-19 09:33:19 UTC
Note that https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/653 only removes the checks when adding an option through 'nm_setting_bond_add_option()', 'nm_connection_verify()' will still do all the previous checks on the bond connection options.

Comment 11 Gris Ge 2020-11-17 03:48:16 UTC
I am OK on `nm_setting_bond_add_option` removing the validation work.

NetworkManager-1.30.0-0.2.el8.x86_64.rpm passed bond test of nmstate CI.

Comment 13 errata-xmlrpc 2021-05-18 13:29:43 UTC
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory (Moderate: NetworkManager and libnma security, bug fix, and enhancement update), and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHSA-2021:1574


Note You need to log in before you can comment on or make changes to this bug.