GNOME Bugzilla – Bug 719905
[Review] Handling nl_objects in platform: netlink cache error: Object exists
Last modified: 2014-02-14 20:58:43 UTC
I see the following two lines in my logfile: <debug> [...] [platform/nm-linux-platform.c:1216] event_notification(): netlink event (type 16) for link: virbr0 (4) <error> [...] [platform/nm-linux-platform.c:1265] event_notification(): netlink cache error: Object exists virbr0 is a bridge. So, the problem is in nm-linux-platform.h, event_notification(). First it gets "cached_object" (finding none) and "kernel_object". Then hack_empty_master_iff_lower_up() modifies the kernel_object. Later it tries to nle = nl_cache_add (cache, kernel_object); which fails (because such a kernel object suddenly exists). Something here does not look right to me. Olatform uses nl_cache. Note, that nl_cache_search() and nl_cache_add() consider most attributes of the nl_object (including the flags that get modified by hack_empty_master_iff_lower_up()). When we cache object, shouldn't we use a cache that only considers attributes that uniquely identify an object AND do not change over the lifetime of an object, like ifindex&family? It seems wrong to me, to add objects to such a cache, because if the objects change, we cannot find them again (except iterating over the entire cache).
Sorry, I did not see it first. nl_cache only considers LINK_ATTR_IFINDEX | LINK_ATTR_FAMILY for equality in the cache. Then I don't see, how the previous error message could come up... ideas?
Be careful about libnl caching. It sometimes proves totally useless, we might want to meet some time and talk about the details and maybe even reconsider the use of the libnl caching facilities.
Please review branch th/bgo719905_platform_caching. There is also a related downstream bug in F20: https://bugzilla.redhat.com/show_bug.cgi?id=1063290
*** Bug 724225 has been marked as a duplicate of this bug. ***
> core/platform: fix caching for link types we normally just say "platform:". (That applies to 4 of the 5 commits actually.) > The problem is, that libnl3 uses both the family and the ifindex of link > objects as identifier for the cache. The ifindex would be > enough to identify the link, however, even worse is that the family of a > link can change (for example when adding a bridge slave). This means, > you cannot find the previously added link after it's family changes. "its family" not "it's" I'm also not sure about "the family of a link can change", since rtnl_link_build_change_request() includes: if (changes->l_family && changes->l_family != orig->l_family) { APPBUG("link change: family is immutable"); return -NLE_IMMUTABLE; } I think the issue may be that the cache can contain both an AF_UNSPEC entry and an AF_BRIDGE entry for the same ifindex, which our code would misinterpret as the family changing. I'm very unsure about this patch. It seems like a gross hack that shouldn't be necessary. We should talk to tgraf, since he reviewed the original patches that added AF_BRIDGE support. (http://lists.infradead.org/pipermail/libnl/2012-November/000730.html et seq). > core/platform: cleanup object_type_from_nl_object() >+ if (!object) { >+ g_return_val_if_fail (FALSE, UNKNOWN_OBJECT_TYPE); >+ return UNKNOWN_OBJECT_TYPE; >+ } Don't do that. Just g_return_val_if_fail(). Compiling with -DG_DISABLE_CHECKS is effectively a request to make the program crash if it does something wrong. >+ /* the returned type string is actually a string constant, so >+ * we can compare pointers. */ is this actually guaranteed by the libnl API? >+ g_return_val_if_fail (FALSE, NULL); >+ return NULL; g_warn_if_reached (); return NULL; (likewise elsewhere) > core: remove busy waiting in nm_device_bring_up() and nm_device_take_down() The logic here seems inarguable, but make sure dcbw sees this patch when he gets back (whether it's already been committed or not) to see if he remembers why that code was there in the first place.
> core/platform: cleanup object_type_from_nl_object() * I would prefer using NULL instead of 0 static const char *type_str_link = 0; ... I also don't like much jumping inside a command with goto. But if it saves code I can live with that.
Pushed an update to th/bgo719905_platform_caching.
>>+ g_return_val_if_fail (FALSE, NULL); >>+ return NULL; > > g_warn_if_reached (); > return NULL; this still applies As for the main commit... I think the code is correct, but I think the commit message is wrong, and I'm starting to think this may all be libnl's fault. (The commit https://github.com/thom311/libnl/commit/fdd1ba220dd7b780400e9d0652cde80e59f63572, which you linked to from the Fedora bug, seems to be going out of its way to change link->l_family to a value which is different from what it "should" be set to according to the kernel.) So, I think you should commit this, but without these two comments which I don't quite understand what you mean by anyway: >+ /* unconditionally set the family to AF_UNSPEC, because if the family >+ * is not set in the link, the identical() function will always return FALSE. */ > What we cannot restore, is unsetting >+ * the family (because the changes mask is libnl internal). */ and without the long explanation in the commit message, because I know that parts of that explanation are wrong, and I suspect other parts of it might be wrong as well. Just say something like "we need to set the link family to AF_UNSPEC for all links before caching them, so they can be matched correctly later" or something, and keep the link to this bug, and commit it. And then we can continue figuring out here what the real root cause of this is, and make further changes to NM or libnl as needed.
(In reply to comment #8) > >>+ g_return_val_if_fail (FALSE, NULL); > >>+ return NULL; > > > > g_warn_if_reached (); > > return NULL; > > this still applies Not really, the "return NULL;" is needed, otherwise the function has no return statement for every code path. > As for the main commit... I think the code is correct, but I think the commit > message is wrong, and I'm starting to think this may all be libnl's fault. (The > commit > https://github.com/thom311/libnl/commit/fdd1ba220dd7b780400e9d0652cde80e59f63572, > which you linked to from the Fedora bug, seems to be going out of its way to > change link->l_family to a value which is different from what it "should" be > set to according to the kernel.) I don't know, should discuss with tgraf. > So, I think you should commit this, but without these two comments which I > don't quite understand what you mean by anyway: > > >+ /* unconditionally set the family to AF_UNSPEC, because if the family > >+ * is not set in the link, the identical() function will always return FALSE. */ > > > > What we cannot restore, is unsetting > >+ * the family (because the changes mask is libnl internal). */ A link object *must* have the index and the family explicitly set, otherwise it will not compare equal to any other object. If you allocate a new link object, rtnl_link_get_family() will return AF_UNSPEC by default, but it is not explicitly set. So, if the caller forget to set the family, we set it to AF_UNSPEC, even if it already appears to be that value. > and without the long explanation in the commit message, because I know that > parts of that explanation are wrong, and I suspect other parts of it might be > wrong as well. Just say something like "we need to set the link family to > AF_UNSPEC for all links before caching them, so they can be matched correctly > later" or something, and keep the link to this bug, and commit it. And then we > can continue figuring out here what the real root cause of this is, and make > further changes to NM or libnl as needed. Sure, this explaination was only a first attempt, for review. I don't yet understand, what the issue really is. But we should come to a conclusion and write it down in detail. So, could you take my first attempt, and fix it where you think it's wrong? (be it in BZ).
Just to confirm... I tested NetworkManager with/without the proposed patches and libnl with/without https://github.com/thom311/libnl/commit/fdd1ba220dd7b780400e9d0652cde80e59f63572 . So, the upping the bridge only fails, if libnl has patch fdd1ba220dd7b780400e9d0652cde80e59f63572 and NetworkManager does not have th/bgo719905_platform_caching (related to https://bugzilla.redhat.com/show_bug.cgi?id=1063290)
(In reply to comment #9) > (In reply to comment #8) > > >>+ g_return_val_if_fail (FALSE, NULL); > > >>+ return NULL; > > > > > > g_warn_if_reached (); > > > return NULL; > > > > this still applies > > Not really, the "return NULL;" is needed, otherwise the function has no return > statement for every code path. Right. I was saying to replace the g_return_val_if_fail(FALSE) with g_warn_if_reached(), and leave the "return NULL;" there. > > >+ /* unconditionally set the family to AF_UNSPEC, because if the family > > >+ * is not set in the link, the identical() function will always return FALSE. */ > > > > > > What we cannot restore, is unsetting > > >+ * the family (because the changes mask is libnl internal). */ > > A link object *must* have the index and the family explicitly set, otherwise it > will not compare equal to any other object. If you allocate a new link object, > rtnl_link_get_family() will return AF_UNSPEC by default, but it is not > explicitly set. So, if the caller forget to set the family, we set it to > AF_UNSPEC, even if it already appears to be that value. OK, so I'd add "explicitly" to the first comment ("is not explicitly set"). And you didn't explain the (partial) second comment ("What we cannot restore..."). > Sure, this explaination was only a first attempt, for review. I don't yet > understand, what the issue really is. But we should come to a conclusion and > write it down in detail. So, could you take my first attempt, and fix it where > you think it's wrong? (be it in BZ). My point was, I think we agree that this fix is the right way to make NM work with libnl 3.2.24, even though we're not totally certain what libnl is doing or why. And given that there are multiple duplicates of this bug in both gnome and rh bugzilla, and it's also blocking my attempts to fix another RHEL bug that involves bridges, I think it would be better to just commit the fix now, and worry about figuring out if it really is our fault or libnl's later. As for specifics of what's wrong with the commit message, well, for one, I'm not sure at this point if the kernel actually exposes separate AF_UNSPEC and AF_BRIDGE objects for a bridge, and libnl then combines them into a single rtnllink object; or if the kernel exposes AF_UNSPEC objects for non-bridges and AF_BRIDGE objects for bridges; or if the kernel only ever exposes AF_UNSPEC objects for both bridges and non-bridges, and libnl is for some reason changing the bridge objects to claim to be AF_BRIDGE. And it's hard to write a commit message explaining the details of the bug without knowing that... So let's just say what we know: in some cases before libnl 3.2.24, and in more (all?) cases after 3.2.24, bridge rtnllink objects in libnl have a family of AF_BRIDGE rather than AF_UNSPEC. And libnl caches require you to specify both ifindex and family when looking up a link, but in most cases in NMPlatform, we have only an ifindex and want to get the corresponding rtnllink, and we don't know whether it's going to be a bridge or not, so we can't specify the correct family. So we cheat and change all link objects to have family AF_UNSPEC before adding them to the cache so we can just look up all links as AF_UNSPEC as well.
(In reply to comment #11) > (In reply to comment #9) > > (In reply to comment #8) > > > >>+ g_return_val_if_fail (FALSE, NULL); > > > >>+ return NULL; > > > > > > > > g_warn_if_reached (); > > > > return NULL; > > > > > > this still applies > > > > Not really, the "return NULL;" is needed, otherwise the function has no return > > statement for every code path. > > Right. I was saying to replace the g_return_val_if_fail(FALSE) with > g_warn_if_reached(), and leave the "return NULL;" there. I deem g_warn_if_reached() useless. If the line of code can be reached, I would add a nm_log_* if that's of any interest. If I am really sure, that this line cannot be reached, I'd add g_assert() -- accepting that NM might crash. If this line should never ever be reached, but I am not so sure, I g_return_* (so that it doesn't crash, just in case). > > > >+ /* unconditionally set the family to AF_UNSPEC, because if the family > > > >+ * is not set in the link, the identical() function will always return FALSE. */ > > > > > > > > What we cannot restore, is unsetting > > > >+ * the family (because the changes mask is libnl internal). */ > > > > A link object *must* have the index and the family explicitly set, otherwise it > > will not compare equal to any other object. If you allocate a new link object, > > rtnl_link_get_family() will return AF_UNSPEC by default, but it is not > > explicitly set. So, if the caller forget to set the family, we set it to > > AF_UNSPEC, even if it already appears to be that value. > > OK, so I'd add "explicitly" to the first comment ("is not explicitly set"). And > you didn't explain the (partial) second comment ("What we cannot restore..."). Reworded both > > Sure, this explaination was only a first attempt, for review. I don't yet > > understand, what the issue really is. But we should come to a conclusion and > > write it down in detail. So, could you take my first attempt, and fix it where > > you think it's wrong? (be it in BZ). > > My point was, I think we agree that this fix is the right way to make NM work > with libnl 3.2.24, even though we're not totally certain what libnl is doing or > why. And given that there are multiple duplicates of this bug in both gnome and > rh bugzilla, and it's also blocking my attempts to fix another RHEL bug that > involves bridges, I think it would be better to just commit the fix now, and > worry about figuring out if it really is our fault or libnl's later. > > > As for specifics of what's wrong with the commit message, well, for one, I'm > not sure at this point if the kernel actually exposes separate AF_UNSPEC and > AF_BRIDGE objects for a bridge, and libnl then combines them into a single > rtnllink object; or if the kernel exposes AF_UNSPEC objects for non-bridges and > AF_BRIDGE objects for bridges; or if the kernel only ever exposes AF_UNSPEC > objects for both bridges and non-bridges, and libnl is for some reason changing > the bridge objects to claim to be AF_BRIDGE. And it's hard to write a commit > message explaining the details of the bug without knowing that... > > So let's just say what we know: in some cases before libnl 3.2.24, and in more > (all?) cases after 3.2.24, bridge rtnllink objects in libnl have a family of > AF_BRIDGE rather than AF_UNSPEC. And libnl caches require you to specify both > ifindex and family when looking up a link, but in most cases in NMPlatform, we > have only an ifindex and want to get the corresponding rtnllink, and we don't > know whether it's going to be a bridge or not, so we can't specify the correct > family. So we cheat and change all link objects to have family AF_UNSPEC before > adding them to the cache so we can just look up all links as AF_UNSPEC as well. Reworded as well Please new review. I also tested it an came around other issues that I tried to fix. Hence, there are new commits.
(In reply to comment #12) > I deem g_warn_if_reached() useless. I deem "g_return_val_if_fail(FALSE)" silly. It doesn't make sense to use a conditional macro when the condition is always FALSe. That's why we have the non-conditional macros. You can do g_return_val_if_reached (NULL); return NULL; if you prefer that. > Please new review. I also tested it an came around other issues that I tried to > fix. Hence, there are new commits. These commits don't seem like they belong on this branch: core: add function nm_utils_ip6_address_clear_host_identifier() platform: clear host identifier before adding a route platform: refactor delete_object() and allow deletion of objects that are not cached platform: do not check for _exists() before deleting addresses and routes (I'm reviewing them below anyway, but I think they should be committed separately.) > core: remove busy waiting in nm_device_bring_up() and nm_device_take_down() dcbw says this needs to be fixed rather than removed, so drop that commit and file a bug for the fact that it's currently broken. (Other than this one, the commits that are actually related to the bridge bug look fine now.) > platform: clear host identifier before adding a route > Adding IPv4 routes, with a non-zero host identifer fails with an > error message. Adding IPv6 addresses, does not return an error, > but it seams to have no effect. "seems" >+ case AF_INET: >+ *((in_addr_t *) dst) = (*((in_addr_t *) network)) & nm_utils_ip4_prefix_to_netmask (plen); Even though it's much simpler in this case, it seems weird to open code this one, but to have a function for the IPv6 case. Maybe you should make the helper function take a family and support both IPv6 and IPv4. (ie, turn nm_utils_ip6_address_clear_host_identifier() into nm_utils_address_clear_host_identifier(), with the prototype of this clear_host_identifier() function). >+ guint32 network_identifier[(sizeof (struct in6_addr) + sizeof (guint32) - 1) / sizeof (guint32)]; Why not use guchar? > platform: refactor delete_object() and allow deletion of objects that are not cached > - allow deletion of object, that we cannot find in the cache you don't really want a comma there > have it currently cached. In this case, fallback to @obj. > > Also try to workaround an issue, that we cannot delete an IPv4 route without > knowing it's scope. and while I'm being nitpicky about the commit message... "fallback" and "workaround" are nouns. You mean "fall back" and "work around". Also, "its" at the end. >+ auto_nl_object struct nl_object *obj_cleanup = obj; appears to be unused >+ * As a workaround, we set the scope to RT_SCOPE_UNIVERSE, so libnl >+ * will not overwrite it. But this only works if we guess correctly. I don't understand... it sounds like you're just arbitrarily replacing one possibly-wrong value with another possibly-wrong value... Also, would it make more sense to put this workaround into the IP4_ROUTE case of delete_object(), rather than in build_rtnl_route()? (So you don't have to pass "FALSE" in every other usage of build_rtnl_route().) > platform: do not check for _exists() before deleting addresses and routes > Before, nm_platform_ip4_address_exists(), et al. look into the cache to see, > whether the address/route already exists and returned an error in this case. "in this case" sounds like you mean it returns an error if the address/route *does* already exist. "returned an error if not". (Also, you don't want the comma after "see".)
(In reply to comment #13) > (In reply to comment #12) > > I deem g_warn_if_reached() useless. > > I deem "g_return_val_if_fail(FALSE)" silly. It doesn't make sense to use a > conditional macro when the condition is always FALSe. That's why we have the > non-conditional macros. > > You can do > > g_return_val_if_reached (NULL); > return NULL; > > if you prefer that. Didn't know. I like that! Done. > > Please new review. I also tested it an came around other issues that I tried to > > fix. Hence, there are new commits. > > These commits don't seem like they belong on this branch: > > core: add function nm_utils_ip6_address_clear_host_identifier() > platform: clear host identifier before adding a route > platform: refactor delete_object() and allow deletion of objects that are > not cached > platform: do not check for _exists() before deleting addresses and routes > > (I'm reviewing them below anyway, but I think they should be committed > separately.) I think, they are related to caching and platform. I'd like to leave the part of this branch, because I hope to merge it soon. > > core: remove busy waiting in nm_device_bring_up() and nm_device_take_down() > > dcbw says this needs to be fixed rather than removed, so drop that commit and > file a bug for the fact that it's currently broken. > > (Other than this one, the commits that are actually related to the bridge bug > look fine now.) Done. See bug 724363 > > platform: clear host identifier before adding a route > > > Adding IPv4 routes, with a non-zero host identifer fails with an > > error message. Adding IPv6 addresses, does not return an error, > > but it seams to have no effect. > > "seems" > > >+ case AF_INET: > >+ *((in_addr_t *) dst) = (*((in_addr_t *) network)) & nm_utils_ip4_prefix_to_netmask (plen); > > Even though it's much simpler in this case, it seems weird to open code this > one, but to have a function for the IPv6 case. > > Maybe you should make the helper function take a family and support both IPv6 > and IPv4. (ie, turn nm_utils_ip6_address_clear_host_identifier() into > nm_utils_address_clear_host_identifier(), with the prototype of this > clear_host_identifier() function). Btw. I renamed the function to nm_utils_ip6_address_clear_host_address(). Added function nm_utils_ip4_address_clear_host_address(). > >+ guint32 network_identifier[(sizeof (struct in6_addr) + sizeof (guint32) - 1) / sizeof (guint32)]; > > Why not use guchar? Because of memory alignment to 4 byte (but I think, on 32bit architecture or larger, everything on stack is alligned to at least 4 byte (not sure though). > > platform: refactor delete_object() and allow deletion of objects that are not cached > > > - allow deletion of object, that we cannot find in the cache > > you don't really want a comma there > > > have it currently cached. In this case, fallback to @obj. > > > > Also try to workaround an issue, that we cannot delete an IPv4 route without > > knowing it's scope. > > and while I'm being nitpicky about the commit message... "fallback" and > "workaround" are nouns. You mean "fall back" and "work around". Also, "its" at > the end. Fixed. > > >+ auto_nl_object struct nl_object *obj_cleanup = obj; > > appears to be unused > > >+ * As a workaround, we set the scope to RT_SCOPE_UNIVERSE, so libnl > >+ * will not overwrite it. But this only works if we guess correctly. > > I don't understand... it sounds like you're just arbitrarily replacing one > possibly-wrong value with another possibly-wrong value... Yes, it's just guessing. But libnl guesses RT_SCOPE_LINK, which is unlikely to be correct, also, because the routes the NetworkManager creates, are RT_SCOPE_UNIVERSE. > Also, would it make more sense to put this workaround into the IP4_ROUTE case > of delete_object(), rather than in build_rtnl_route()? (So you don't have to > pass "FALSE" in every other usage of build_rtnl_route().) Yes, that is nicer. Fixed. > > > > > platform: do not check for _exists() before deleting addresses and routes > > > Before, nm_platform_ip4_address_exists(), et al. look into the cache to see, > > whether the address/route already exists and returned an error in this case. > > "in this case" sounds like you mean it returns an error if the address/route > *does* already exist. "returned an error if not". (Also, you don't want the > comma after "see".) Reworded Repushed!
> Repushed! I don't feel like I need to review it again. Sounds like you fixed everything I cared about.
One more place to do g_return_val_if_reached(): @@ -282,7 +291,8 @@ delete_kernel_object (struct nl_sock *sock, struct nl_object *object) case IP6_ROUTE: return rtnl_route_delete (sock, (struct rtnl_route *) object, 0); default: - g_assert_not_reached (); + g_return_val_if_fail (FALSE, -NLE_INVAL); + return -NLE_INVAL; } }
Beyond that, the branch looks OK to me, and tests out well in some light IPv4 and IPv6 testing, and doing various bridge operations. If the issue in comment 16 is fixed, I think it's OK to push.
Pushed to master as: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=83ff4f819faf90bdd19dd03a338205c91fc42f6e