1. 13 Mar, 2019 2 commits
    • Thomas Haller's avatar
      platform: add NMPRulesManager for syncing routing rules · b8398b9e
      Thomas Haller authored
      Routing rules are unlike addresses or routes not tied to an interface.
      NetworkManager thinks in terms of connection profiles. That works well
      for addresses and routes, as one profile configures addresses and routes
      for one device. For example, when activating a profile on a device, the
      configuration does not interfere with the addresses/routes of other
      devices. That is not the case for routing rules, which are global, netns-wide
      entities.
      
      When one connection profile specifies rules, then this per-device configuration
      must be merged with the global configuration. And when a device disconnects later,
      the rules must be removed.
      
      Add a new NMPRulesManager API to track/untrack routing rules. Devices can
      register/add there the routing rules they require. And the sync method will
      apply the configuration. This is be implemented on top of NMPlatform's
      caching API.
      b8398b9e
    • Thomas Haller's avatar
  2. 08 Feb, 2019 1 commit
  3. 27 Dec, 2018 1 commit
    • Thomas Haller's avatar
      platform: merge NMPlatformError with nm-error · d18f4032
      Thomas Haller authored
      Platform had it's own scheme for reporting errors: NMPlatformError.
      Before, NMPlatformError indicated success via zero, negative integer
      values are numbers from <errno.h>, and positive integer values are
      platform specific codes. This changes now according to nm-error:
      success is still zero. Negative values indicate a failure, where the
      numeric value is either from <errno.h> or one of our error codes.
      The meaning of positive values depends on the functions. Most functions
      can only report an error reason (negative) and success (zero). For such
      functions, positive values should never be returned (but the caller
      should anticipate them).
      For some functions, positive values could mean additional information
      (but still success). That depends.
      
      This is also what systemd does, except that systemd only returns
      (negative) integers from <errno.h>, while we merge our own error codes
      into the range of <errno.h>.
      
      The advantage is to get rid of one way how to signal errors. The other
      advantage is, that these error codes are compatible with all other
      nm-errno values. For example, previously negative values indicated error
      codes from <errno.h>, but it did not entail error codes from netlink.
      d18f4032
  4. 05 Mar, 2018 1 commit
  5. 11 Dec, 2017 3 commits
  6. 13 Nov, 2017 2 commits
  7. 07 Sep, 2017 1 commit
  8. 24 Aug, 2017 2 commits
    • Thomas Haller's avatar
      platform: return failure reason from route-add and return only netlink response · 774c8a81
      Thomas Haller authored
      Let nm_platform_ip_route_add() and friends return an NMPlatformError
      failure reason.
      
      Also, do_add_addrroute() did not return the response from kernel.
      Instead, it determined success/failure based on the presence of the
      object in the cache. That is racy and does not allow to give a failure
      reason from kernel.
      
      Instead, determine success solely based on the netlink reply from
      kernel. The received errno shall be authorative, there is no need
      to second guess the response.
      
      There is a problem that netlink is not a reliable protocol. In case
      of receive buffer overflow, the response is lost and we don't know
      whether the command succeeded (it likely did). It's unclear how to fix
      that, but for now just return "unspecified" error. We probably avoid
      that already by having a huge buffer size.
      
      Also, downgrade the error message to <warn> level. <error> is really
      for bugs only.
      774c8a81
    • Thomas Haller's avatar
      platform: add nm_platform_ip_route_get() for route-lookup · 33a2a7c3
      Thomas Haller authored
      Inspired from iproute2. As such, don't use libnl3's "struct nl_msg", but
      add _nl_addattr_l() and use a stack-allocated "struct nlmsghdr". With
      this, we are closer to the raw netlink API. It really is simple enough.
      
      The complicated part of the patch is that we re-use the existing netlink
      socket for events. Hence, we must process the socket via our common
      event_handler_recvmsgs(). That also means, that we get the netlink
      response a few layers down the stack and have to return the result
      via DelayedActionWaitForNlResponseData.
      33a2a7c3
  9. 12 Aug, 2017 1 commit
    • Thomas Haller's avatar
      platform: fix cache to use kernel's notion for equality of routes · cdd8c657
      Thomas Haller authored
      Until now, NetworkManager's platform cache for routes used the quadruple
      network/plen,metric,ifindex for equaliy. That is not kernel's
      understanding of how routes behave. For example, with `ip route append`
      you can add two IPv4 routes that only differ by their gateway. To
      the previous form of platform cache, these two routes would wrongly
      look identical, as the cache could not contain both routes. This also
      easily leads to cache-inconsistencies.
      
      Now that we have NM_PLATFORM_IP_ROUTE_CMP_TYPE_ID, fix the route's
      compare operator to match kernel's.
      
      Well, not entirely. Kernel understands more properties for routes then
      NetworkManager. Some of these properties may also be part of the ID according
      to kernel. To NetworkManager such routes would still look identical as
      they only differ in a property that is not understood. This can still
      cause cache-inconsistencies. The only fix here is to add support for
      all these properties in NetworkManager as well. However, it's less serious,
      because with this commit we support several of the more important properties.
      See also the related bug rh#1337855 for kernel.
      
      Another difficulty is that `ip route replace` and `ip route change`
      changes an existing route. The replaced route has the same
      NM_PLATFORM_IP_ROUTE_CMP_TYPE_WEAK_ID, but differ in the actual
      NM_PLATFORM_IP_ROUTE_CMP_TYPE_ID:
      
          # ip -d -4 route show dev v
          # ip monitor route &
          # ip route add 192.168.5.0/24 dev v
          192.168.5.0/24 dev v scope link
          # ip route change 192.168.5.0/24 dev v scope 10
          192.168.5.0/24 dev v scope 10
          # ip -d -4 route show dev v
          unicast 192.168.5.0/24 proto boot scope 10
      
      Note that we only got one RTM_NEWROUTE message, although from NMPCache's
      point of view, a new route (with a particular ID) was added and another
      route (with a different ID) was deleted. The cumbersome workaround is,
      to keep an ordered list of the routes, and figure out which route was
      replaced in response to an RTM_NEWROUTE. In absence of bugs, this should
      work fine. However, as we only rely on events, we might wrongly
      introduce a cache-inconsistancy as well. See the related bug rh#1337860.
      
      Also drop nm_platform_ip4_route_get() and the like. The ID of routes
      is complex, so it makes little sense to look up a route directly.
      cdd8c657
  10. 03 Aug, 2017 2 commits
    • Thomas Haller's avatar
      platform: extend API for adding routes · d373855e
      Thomas Haller authored
      Via the flags of the RTM_NEWROUTE netlink message, kernel and iproute2
      support various variants to add a route.
      
       - ip route add
       - ip route change
       - ip route replace
       - ip route prepend
       - ip route append
       - ip route test
      
      Previously, our nm_platform_ip4_route_add() function was basically
      `ip route replace`. In the future, we should rather user `ip route
      append` instead.
      
      Anyway, expose the netlink message flags in the API. This allows to
      use the various forms, and makes it also more apparent to the user that
      they even exist.
      d373855e
    • Thomas Haller's avatar
      platform: add compare functions for routes with different compare semantics · 372f14a6
      Thomas Haller authored
      Routes are complicated.
      
      `ip route add` and `ip route append` behaves differently with respect to
      determine whether an existing route is idential or not.
      
      Extend the cmp() and hash() functions to have a compare type, that
      covers the different semantics.
      372f14a6
  11. 25 Jul, 2017 2 commits
    • Thomas Haller's avatar
      platform: pass full route object to platform delete function · 2861c591
      Thomas Haller authored
      Contrary to addresses, routes have no ID. When deleting a route,
      you cannot just specify certain properties like network/plen,metric.
      
      Well, actually you can specify only certain properties, but then kernel
      will treat unspecified properties as wildcard and delete the first matching
      route. That is not something we want, because we need to be in control which
      exact route shall be deleted.
      
      Also, rtm_tos *must* match. Even if we like the wildcard behavior,
      we would need to pass TOS to nm_platform_ip4_route_delete() to be
      able to delete routes with non-zero TOS. So, while certain properties
      may be omitted, some must not. See how test_ip4_route_options() was
      broken.
      
      For NetworkManager it only makes ever sense to call delete on a route,
      if the route is already fully known. Which means, we only delete routes
      that we have already in the platform cache (otherwise, how would we know
      that there is something to delete). Because of that, no longer have separate
      IPv4 and IPv6 functions. Instead, have nm_platform_ip_route_delete() which
      accepts a full NMPObject from the platform cache.
      
      The code in core doesn't jet make use of this new functionality. It will
      in the future.
      
      At least, it fixes deleting routes with differing TOS.
      2861c591
    • Thomas Haller's avatar
      platform: fix return value for do_delete_object() · 5b09f715
      Thomas Haller authored
      The return value for the delete methods checks whether the object
      is actually deleted. That is questionable behavior, because if the netlink
      request succeeds, there is little point in checking with the platform cache.
      As it is, it is racy.
      
      Anyway, the previous value was totally wrong.
      
      But it also uncovers another platform bug, which currently breaks
      route tests. Will be fixed next.
      5b09f715
  12. 05 Jul, 2017 2 commits
  13. 31 May, 2017 2 commits
  14. 30 May, 2017 1 commit
    • Francesco Giudici's avatar
      platform/tests: fix test_ip6_route_options · 21a941d4
      Francesco Giudici authored
      when adding a route with RTA_PREFSRC some kernel versions will reject
      the request if the specified source address is still tentative: be sure
      that the just added addresses are no more tentative before adding the
      routes.
      21a941d4
  15. 27 May, 2017 3 commits
  16. 09 Mar, 2017 2 commits
  17. 08 Mar, 2017 1 commit
  18. 06 Mar, 2017 3 commits
  19. 21 Nov, 2016 1 commit
    • Thomas Haller's avatar
      build: don't add subdirectories to include search path but require qualified include · 44ecb415
      Thomas Haller authored
      Keep the include paths clean and separate. We use directories to group source
      files together. That makes sense (I guess), but then we should use this
      grouping also when including files. Thus require to #include files with their
      path relative to "src/".
      
      Also, we build various artifacts from the "src/" tree. Instead of having
      individual CFLAGS for each artifact in Makefile.am, the CFLAGS should be
      unified. Previously, the CFLAGS for each artifact differ and are inconsistent
      in which paths they add to the search path. Fix the inconsistency by just
      don't add the paths at all.
      44ecb415
  20. 22 Oct, 2016 1 commit
  21. 21 Oct, 2016 1 commit
  22. 28 Apr, 2016 2 commits
    • Thomas Haller's avatar
      platform: extend NMIPConfigSource to preserve the rtm_protocol field · 4c2410bc
      Thomas Haller authored
      For addresses (NMPlatformIPAddress) the @addr_source field is ignored
      on a platform level. That is, all addresses inside the platform cache
      have this value set to NM_IP_CONFIG_SOURCE_KERNEL. Maybe, for that reason,
      the source should not be a part of the NMPlatformIPAddress structure, but
      it is convenient for users to piggy back the source inside the platform
      address structure.
      
      For routes, the source is stored in NMPlatformIPRoute's @rt_source
      field. When adding a route to kernel, we set the @rtm_protocol of the
      route depending on the source. However, we want to map different source
      values to the same protocol value.
      
      On the other hand, when kernel sends us a route that gets put inside
      the cache, we must preserve the protocol value and must not map
      different protocol values to the same source.
      The reason is, that a user can add two routes that only differ by
      @rtm_protocol. In that sense, the @rtm_protocol fields is part of the
      unique ID of a kernel route, and thus different values must map to
      different sources.
      
      Fix this, by extending the range of NMIPConfigSource to contain
      a range of protocol fields.
      4c2410bc
    • Thomas Haller's avatar
      core/trivial: rename "source" field of addresses and routes · 6bf02235
      Thomas Haller authored
      The "source" field of NMPlatformIPRoute (now "rt_source") maps to the
      protocol field of the route. The source of NMPlatformIPAddress (now
      "addr_source") has no direct equivalent in the kernel.
      
      As their use is different, they should have different names. Also,
      the name "source" is used all over the place. Hence give the fields
      a more distinct name.
      6bf02235
  23. 11 Apr, 2016 3 commits