Created attachment 290423 [details] [review]
libnm: allow zero prefix length for NMIPRoute

NMIPRoute is used by NMSettingIPConfig, but also
NMIPConfig. In the former case, default routes are (still)
disallowed. But in the NMIPConfig use-case, it can make sense
to expose default routes as NMIPRoute instances.

Relax the restriction to allow this future change.

https://bugzilla.gnome.org/show_bug.cgi?id=739969

Signed-off-by: Thomas Haller <thaller@redhat.com>

A couple things:

1) We need to update libnm-util to handle this too, since if you add a 0-plen route with libnm-core, push it to NM, and then a libnm-util client reads the route, it would see the route as invalid.

2) Plus the daemon needs to elide 0-plen routes when constructing the old 'routes' property for both NMSettingIPxConfig and NMIPxConfig's D-Bus interface.

It's a bit odd to me that you can (with this patch) call nm_setting_ip_config_route_add(<0 plen route) and that will return 'success' but then the setting will fail to verify...

I think instead we should just do the full patch?  That would involve releasing the restrictions entirely in libnm-core/nm-setting-ip-config.c for nm_ip_route_new() and nm_setting_ip_config_add_route(), but updating nm_utils_ip4_routes_from_variant() (and to_variant, and the same for IPv6 helpers) to ignore zero-plen routes silently.  That way apps that use 'routes-data' would get everything, but stuff that uses 'routes' would ignore zero plen routes.

Then in the daemon adjusting the D-Bus converters for ":routes" in NMIP4Config and NMIP6Config in get_property().

(In reply to comment #2)
> A couple things:
> 
> 1) We need to update libnm-util to handle this too, since if you add a 0-plen
> route with libnm-core, push it to NM, and then a libnm-util client reads the
> route, it would see the route as invalid.

There is no need for libnm-util to support that (unless there is a good reason to).

Even if we would fix libnm-util-1.0, we cannot ever expose default routes to NMSettingIPxConfig:routes or NMIPxConfig:routes without potentially breaking clients.



> 2) Plus the daemon needs to elide 0-plen routes when constructing the old
> 'routes' property for both NMSettingIPxConfig and NMIPxConfig's D-Bus
> interface.

This patch only changes that NMIPRoute can now hold default routes. Still, neither NMIPConfig nor NMSettingIPConfig actually add them, allow them, or expose them via DBUS.
At the same time, NMSettingIPConfig sent from clients to deamon now fails verification (which is correct, why are they adding default routes?!).


> It's a bit odd to me that you can (with this patch) call
> nm_setting_ip_config_route_add(<0 plen route) and that will return 'success'
> but then the setting will fail to verify...

In the process of creating a setting, you can easily create invalid ones. verify()/normalize() very much anticipates that.


> I think instead we should just do the full patch?  That would involve releasing
> the restrictions entirely in libnm-core/nm-setting-ip-config.c for
> nm_ip_route_new() and nm_setting_ip_config_add_route(), but updating
> nm_utils_ip4_routes_from_variant() (and to_variant, and the same for IPv6
> helpers) to ignore zero-plen routes silently.  That way apps that use
> 'routes-data' would get everything, but stuff that uses 'routes' would ignore
> zero plen routes.

The point is, that a IPRoute class is also useful to express default routes. This patch allows that. We don't have to change the places that currently don't allow default routes to allow them.

If we want, we can do that later -- when there is an actual case for it.

> Then in the daemon adjusting the D-Bus converters for ":routes" in NMIP4Config
> and NMIP6Config in get_property().

(In reply to comment #3)
> (In reply to comment #2)
> > A couple things:
> > 
> > 1) We need to update libnm-util to handle this too, since if you add a 0-plen
> > route with libnm-core, push it to NM, and then a libnm-util client reads the
> > route, it would see the route as invalid.
> 
> There is no need for libnm-util to support that (unless there is a good reason
> to).
> 
> Even if we would fix libnm-util-1.0, we cannot ever expose default routes to
> NMSettingIPxConfig:routes or NMIPxConfig:routes without potentially breaking
> clients.

Yeah, that's what I mean I guess.  We cannot expose them in the 'routes' properties.  So agreed.

> > 2) Plus the daemon needs to elide 0-plen routes when constructing the old
> > 'routes' property for both NMSettingIPxConfig and NMIPxConfig's D-Bus
> > interface.
> 
> This patch only changes that NMIPRoute can now hold default routes. Still,
> neither NMIPConfig nor NMSettingIPConfig actually add them, allow them, or
> expose them via DBUS.

Except for nm_setting_ipX_config_add_route(), which will now allow plen=0 to be added to the setting...  which seems odd since they return 'gboolean'.

> At the same time, NMSettingIPConfig sent from clients to deamon now fails
> verification (which is correct, why are they adding default routes?!).
> 
> 
> > It's a bit odd to me that you can (with this patch) call
> > nm_setting_ip_config_route_add(<0 plen route) and that will return 'success'
> > but then the setting will fail to verify...
> 
> In the process of creating a setting, you can easily create invalid ones.
> verify()/normalize() very much anticipates that.

verify() does anticipate that, but it's mostly for the D-Bus side of things where no error checking happens when deserializing the property.  Client-side libraries can provide more immediately error checking and more descriptive errors.

> > I think instead we should just do the full patch?  That would involve releasing
> > the restrictions entirely in libnm-core/nm-setting-ip-config.c for
> > nm_ip_route_new() and nm_setting_ip_config_add_route(), but updating
> > nm_utils_ip4_routes_from_variant() (and to_variant, and the same for IPv6
> > helpers) to ignore zero-plen routes silently.  That way apps that use
> > 'routes-data' would get everything, but stuff that uses 'routes' would ignore
> > zero plen routes.
> 
> The point is, that a IPRoute class is also useful to express default routes.
> This patch allows that. We don't have to change the places that currently don't
> allow default routes to allow them.

Yes, I agree that it is useful.  I'm just saying I think nm_setting_ip_config_add_route() should return FALSE for plen = 0.  I'm a bit worried about the g_assert() in nm_ipX_config_merge_setting() otherwise.

The rest of the patch is OK with me.

> Yes, I agree that it is useful.  I'm just saying I think
> nm_setting_ip_config_add_route() should return FALSE for plen = 0.

The add_() methods currently only return FALSE to mean "the address/route was already in the list", and I'm pretty sure there is code that assumes/depends on that.

(In reply to comment #5)
> > Yes, I agree that it is useful.  I'm just saying I think
> > nm_setting_ip_config_add_route() should return FALSE for plen = 0.
> 
> The add_() methods currently only return FALSE to mean "the address/route was
> already in the list", and I'm pretty sure there is code that assumes/depends on
> that.

Ok, lets at least fix up the g_assert() in nm_ipX_config_merge_setting() since I think we could hit that if we add default routes to generated settings.  Printing a warning is fine, just not g_assert().

I think this needs a bit more discussion, plus postponing it won't break API or prevent us from doing this in the future...

(In reply to comment #6)
> (In reply to comment #5)
> > > Yes, I agree that it is useful.  I'm just saying I think
> > > nm_setting_ip_config_add_route() should return FALSE for plen = 0.
> > 
> > The add_() methods currently only return FALSE to mean "the address/route was
> > already in the list", and I'm pretty sure there is code that assumes/depends on
> > that.
> 
> Ok, lets at least fix up the g_assert() in nm_ipX_config_merge_setting() since
> I think we could hit that if we add default routes to generated settings. 
> Printing a warning is fine, just not g_assert().

nm_ipX_config_merge_setting() is core-only. We can change it any time.

With this patch, default routes in NMSettingIPConfig are still forbidden (indeed verify() explicitly fails on those). No default routes will be passed to nm_ipX_config_merge_setting() -- as it is now.
The assertion is still very right, and having a default route there would indicate a bug.

I'm not worried about the libnm/ changes.

I am worried about the NMIPConfig changes and the merge_setting() stuff, because of nm_ip4_config_create_setting().  If we relax the *core* (not libnm) restriction later to allow NMIPxConfig to hold default routes (which I think we want to do eventually), we'll now hit that assertion when capturing external configuration.

In fact, nm_ip4_config_capture() already grabs all routes including the default, but default routes are removed in the "Extract gateway from default route" check.  I'm concerned about the fragility; if we remove that seemingly unrelated code in nm_ip4_config_capture() then we'll hit that assertion in merge_setting() later and NM will crash.

I don't mind the patch, I simply thing that the assertion is a bit too harsh and will bite us later since it won't get triggered immediately in normal use-cases.

(In reply to comment #9)
> I'm not worried about the libnm/ changes.
> 
> I am worried about the NMIPConfig changes and the merge_setting() stuff,
> because of nm_ip4_config_create_setting().  If we relax the *core* (not libnm)
> restriction later to allow NMIPxConfig to hold default routes (which I think we
> want to do eventually), we'll now hit that assertion when capturing external
> configuration.

I don't think we ever can add default routes to NMIPxConfig:route or NMSettingIPConfig:route.

That is not the intention (and the patch does not go in that direction).
The patch opens up that NMIPRoute can be used for other properties. Such as (hypothetical)
  NMIPRoute *nm_ip_config_get_default_route(NMIPConfig *config);


> In fact, nm_ip4_config_capture() already grabs all routes including the
> default, but default routes are removed in the "Extract gateway from default
> route" check.  I'm concerned about the fragility; if we remove that seemingly
> unrelated code in nm_ip4_config_capture() then we'll hit that assertion in
> merge_setting() later and NM will crash.
>
> I don't mind the patch, I simply thing that the assertion is a bit too harsh
> and will bite us later since it won't get triggered immediately in normal
> use-cases.

The assertion is in src/ and it asserts that NMSettingIPConfig:route contains no default route. NMIP4Config:route/nm_ip4_config_capture() does not matter for this assertion.

But anyway, any assertion that would ensure that NMIP4Config:route contains no default route must be hold too.

Note also that NMSettingIPConfig:verify() also checks that no default routes present.

(In reply to comment #10)
> > In fact, nm_ip4_config_capture() already grabs all routes including the
> > default, but default routes are removed in the "Extract gateway from default
> > route" check.  I'm concerned about the fragility; if we remove that seemingly
> > unrelated code in nm_ip4_config_capture() then we'll hit that assertion in
> > merge_setting() later and NM will crash.
> >
> > I don't mind the patch, I simply thing that the assertion is a bit too harsh
> > and will bite us later since it won't get triggered immediately in normal
> > use-cases.
> 
> The assertion is in src/ and it asserts that NMSettingIPConfig:route contains
> no default route. NMIP4Config:route/nm_ip4_config_capture() does not matter for
> this assertion.

1) have a device with an assumed connection
2) add a default route to it
3) update_ip_config() is triggered and calls nm_ip4_config_capture()
    - the default route is omitted here but this is the "fragile" part
4) update_ip_config() calls ip4_config_merge_and_apply()
5) ip4_config_merge_and_apply() calls nm_device_set_ip4_config()
6) nm_device_set_ip4_config() calls nm_ip4_config_create_setting() because the device has an assumed connection
7) nm_ip4_config_create_setting() calls nm_ip_route_new_binary()
8) nm_ip_route_new_binary() allows 0-prefix routes to be added

If we ever change nm_ip4_config_capture() to stop removing the default route, then we will also immediately change nm_ip4_config_add_route() because of the g_return_if_fail() in that function.  So now default routes can be stored in NMIP4Config, and can be added to NMSettingIP4Config too because of nm_ip4_config_create_setting() that does not enforce plen>0.  Since nothing in NM verifies the NMSettingIP4Config for generated connections, no assert is triggered yet.

But the g_assert() is still there.

All I am suggesting is to change the g_assert() to a g_return_if_fail() so that NM doesn't crash if we forget about that part later.  I have no problem with keeping it there.  I would simply rather than NM warn and continue, instead of crashing.  That is all.

(In reply to comment #11)
> (In reply to comment #10)
> > > In fact, nm_ip4_config_capture() already grabs all routes including the
> > > default, but default routes are removed in the "Extract gateway from default
> > > route" check.  I'm concerned about the fragility; if we remove that seemingly
> > > unrelated code in nm_ip4_config_capture() then we'll hit that assertion in
> > > merge_setting() later and NM will crash.
> > >
> > > I don't mind the patch, I simply thing that the assertion is a bit too harsh
> > > and will bite us later since it won't get triggered immediately in normal
> > > use-cases.
> > 
> > The assertion is in src/ and it asserts that NMSettingIPConfig:route contains
> > no default route. NMIP4Config:route/nm_ip4_config_capture() does not matter for
> > this assertion.
> 
> 1) have a device with an assumed connection
> 2) add a default route to it

We still don't ever add default routes to NMSettingIPConfig:routes. They are explicitly excluded. Also, I doubt that this will make sense in the future, and it would be quite problematic to change that (backward-compat wise).

Also, I added a check to NMSettingIPConfig:verify() so that such routes would fail verification (and hence assumption would fail).


> 3) update_ip_config() is triggered and calls nm_ip4_config_capture()
>     - the default route is omitted here but this is the "fragile" part

we will not change that. And if we ever will, we have to change several places in the code to accommodate for that. Including removing mentioned assert(). But right now, the assert() is just right, because it guards something that really must be true (and it is true).

> 4) update_ip_config() calls ip4_config_merge_and_apply()
> 5) ip4_config_merge_and_apply() calls nm_device_set_ip4_config()
> 6) nm_device_set_ip4_config() calls nm_ip4_config_create_setting() because the
> device has an assumed connection

nm_ip4_config_create_setting() ignores default routes too (and will continue to do so).

> 7) nm_ip4_config_create_setting() calls nm_ip_route_new_binary()
> 8) nm_ip_route_new_binary() allows 0-prefix routes to be added
> 
> If we ever change nm_ip4_config_capture() to stop removing the default route,
> then we will also immediately change nm_ip4_config_add_route() because of the
> g_return_if_fail() in that function.  So now default routes can be stored in
> NMIP4Config,

This patch does not change that. Default routes still cannot be stored in NMIP4Config:routes.

> and can be added to NMSettingIP4Config too because of
> nm_ip4_config_create_setting() that does not enforce plen>0.  Since nothing in
> NM verifies the NMSettingIP4Config for generated connections, no assert is
> triggered yet.
> 
> But the g_assert() is still there.
> 
> All I am suggesting is to change the g_assert() to a g_return_if_fail() so that
> NM doesn't crash if we forget about that part later.  I have no problem with
> keeping it there.  I would simply rather than NM warn and continue, instead of
> crashing.  That is all.



Seems like we talk around each other :)

Still no code actually makes use of NMIPRoute instances with plen==0. Anything else would be a bug (but AFAIS, no such places exist, otherwise that would need fixing).

And if we ever allow plen=0 NMIPRoute instances somewhere, of course we must remove assertions, but also accommodate the code to support that. But for now, any assertion catching use of such plen=0 NMIPRoute instances is right in place and crashes rightfully in face of a bug.


One might rightfully ask what is then the point of this patch: the point is that we have a class NMIPRoute and it's API currently doesn't allow to express default routes. That is wrong because in the future I can see valid use-cases for that (but without changing current behavior for NMIP4Config:routes and NMSettingIPConfig:routes).
For v1.0 NMIPRoute should not have an API that forbids plen=0. So that later we don't have to change this restriction -- even if the change is actually a "relaxing".

Ok, fine.

merged as:

http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=a2e93f2de4ac375d3acc56ac713727843aa3fb80