Bug 1160815

NM notes...

DELAY (eg, forward_delay) is only valid when STP is enabled.  When STP is disabled, NMSettingBridge::forward_delay should be 0.  When STP is enabled, forward_delay must be between 2 and 30 seconds.

NMSettingBridge::verify() does not enforce the correct value ranges for STP-related options when STP is enabled, but it should do so.  So if forward_delay=0 and STP is enabled, the kernel rejects the attempt to set the forward_delay with -ERANGE and the value is left as the default (15 seconds).

Nor does ifcfg-rh warn when STP is enabled but DELAY=0, which it probably should do.

Created attachment 955919 [details]
[PATCH] bridge: Properly check range of STP properties

(In reply to Dan Williams from comment #1)
> NM notes...
> 
> DELAY (eg, forward_delay) is only valid when STP is enabled.  When STP is
> disabled, NMSettingBridge::forward_delay should be 0.

The property defaults to 15. The connection with stp=0 and forward_delay unset would not pass validation.

> When STP is enabled,
> forward_delay must be between 2 and 30 seconds.
> 
> NMSettingBridge::verify() does not enforce the correct value ranges for
> STP-related options when STP is enabled, but it should do so.  So if
> forward_delay=0 and STP is enabled, the kernel rejects the attempt to set
> the forward_delay with -ERANGE and the value is left as the default (15
> seconds).
> 
> Nor does ifcfg-rh warn when STP is enabled but DELAY=0, which it probably
> should do.

Why? It doesn't look like a right place to do that (would it make sense to check this in the keyfile plugin as well then?) and the NMSettingBridge::verify() should cover that anyway.

BR_MIN_AGEING_TIME = 0, so should that check pass !priv->stp, or always TRUE?

Also since min is unsigned, I think Coverity will yell at us for the "if (val < min || val > max) {" check for BR_MIN_AGEING_TIME (which is 0).  Not sure we care about that much though, unless you want to change min/max to gint64.

Also I'd change "zero" to "zero_valid" to make it clearer.

Overall looks good to me.

(In reply to Dan Williams from comment #3)
> BR_MIN_AGEING_TIME = 0, so should that check pass !priv->stp, or always TRUE?

Well, either will work. I'd prefer keeping the STP condition for consistency, but I don't really care.

> Also since min is unsigned, I think Coverity will yell at us for the "if
> (val < min || val > max) {" check for BR_MIN_AGEING_TIME (which is 0).  Not
> sure we care about that much though, unless you want to change min/max to
> gint64.

Hmm, I'd assume Coverity is smarter and don't really know what would look good to it instead. Can I keep it as it is here and address the Coverity issues separately (bug #1173171)?

> Also I'd change "zero" to "zero_valid" to make it clearer.

Done.

> Overall looks good to me.

A forward delay of zero used to work.
I think F20 broke it.
Also, I noticed that I couldn't set it to zero using brctl, so maybe something changed there too

(In reply to Ferry Huberts from comment #5)
> A forward delay of zero used to work.
> I think F20 broke it.
> Also, I noticed that I couldn't set it to zero using brctl, so maybe
> something changed there too

It actually never "worked", it was just ignored by NetworkManager and then defaulted to 15 anyway.

I distinctly remember setting it to 0 and seeing it at 0.
Maybe that was RHEL6

The patch looks good to me (after the changes).

Seems like the fix went out with last f21 update to a later NM Git snapshot.

(In reply to Ferry Huberts from comment #7)
> I distinctly remember setting it to 0 and seeing it at 0.
> Maybe that was RHEL6

Now confirmed it: DELAY=0 works fine on RHEL6

If fact, I believe it to be a mistake that it currently can't be set any lower than 2.