1. 07 May, 2019 5 commits
    • Thomas Haller's avatar
      device/trivial: add comment about lifetime of "kind" in tc_commit() · f2ae994b
      Thomas Haller authored
      In general, all fields of public NMPlatform* structs must be
      plain/simple. Meaning: copying the struct must be possible without
      caring about cloning/duplicating memory.
      In other words, if there are fields which lifetime is limited,
      then these fields cannot be inside the public part NMPlatform*.
      
      That is why
      
        - "NMPlatformLink.kind", "NMPlatformQdisc.kind", "NMPlatformTfilter.kind"
          are set by platform code to an interned string (g_intern_string())
          that has a static lifetime.
      
        - the "ingress_qos_map" field is inside the ref-counted struct NMPObjectLnkVlan
          and not NMPlatformLnkVlan. This field requires managing the lifetime
          of the array and NMPlatformLnkVlan cannot provide that.
      
      See also for example NMPClass.cmd_obj_copy() which can deep-copy an object.
      But this is only suitable for fields in NMPObject*. The purpose of this
      rule is that you always can safely copy a NMPlatform* struct without
      worrying about the ownership and lifetime of the fields (the field's
      lifetime is unlimited).
      
      This rule and managing of resource lifetime is the main reason for the
      NMPlatform*/NMPObject* split. NMPlatform* structs simply have no mechanism
      for copying/releasing fields, that is why the NMPObject* counterpart exists
      (which is ref-counted and has a copy and destructor function).
      
      This is violated in tc_commit() for the "kind" strings. The lifetime
      of these strings is tied to the setting instance.
      
      We cannot intern the strings (because these are arbitrary strings
      and interned strings are leaked indefinitely). We also cannot g_strdup()
      the strings, because NMPlatform* is not supposed to own strings.
      
      So, just add comments that warn about this ugliness.
      
      The more correct solution would be to move the "kind" fields inside
      NMPObjectQdisc and NMPObjectTfilter, but that is a lot of extra effort.
      f2ae994b
    • Thomas Haller's avatar
      platform: use bool bitfields in NMPlatformActionMirred structure · 36d6aa3b
      Thomas Haller authored
      Arguably, the structure is used inside a union with another (larger)
      struct, hence no memory is saved.
      
      In fact, it may well be slower performance wise to access a boolean bitfield
      than a gboolean (int).
      
      Still, boolean fields in structures should be bool:1 bitfields for
      consistency.
      36d6aa3b
    • Thomas Haller's avatar
      libnm: rename "memory" parameter of fq_codel QDisc to "memory_limit" · 666d5880
      Thomas Haller authored
      Kernel calls the netlink attribute TCA_FQ_CODEL_MEMORY_LIMIT. Likewise,
      iproute2 calls this "memory_limit".
      
      Rename because TC parameters are inherrently tied to the kernel
      implementation and we should use the familiar name.
      666d5880
    • Thomas Haller's avatar
      platform: fix handling of default value for TCA_FQ_CODEL_CE_THRESHOLD · 973db2d4
      Thomas Haller authored
      iproute2 uses the special value ~0u to indicate not to set
      TCA_FQ_CODEL_CE_THRESHOLD in RTM_NEWQDISC. When not explicitly
      setting the value, kernel treats the threshold as disabled.
      
      However note that 0xFFFFFFFFu is not an invalid threshold (as far as
      kernel is concerned). Thus, we should not use that as value to indicate
      that the value is unset. Note that iproute2 uses the special value ~0u
      only internally thereby making it impossible to set the threshold to
      0xFFFFFFFFu). But kernel does not have this limitation.
      
      Maybe the cleanest way would be to add another field to NMPlatformQDisc:
      
          guint32 ce_threshold;
          bool ce_threshold_set:1;
      
      that indicates whether the threshold is enable or not.
      But note that kernel does:
      
          static void codel_params_init(struct codel_params *params)
          {
          ...
                  params->ce_threshold = CODEL_DISABLED_THRESHOLD;
      
          static int fq_codel_change(struct Qdisc *sch, struct nlattr *opt,
                                     struct netlink_ext_ack *extack)
          {
          ...
                  if (tb[TCA_FQ_CODEL_CE_THRESHOLD]) {
                          u64 val = nla_get_u32(tb[TCA_FQ_CODEL_CE_THRESHOLD]);
      
                          q->cparams.ce_threshold = (val * NSEC_PER_USEC) >> CODEL_SHIFT;
                  }
      
          static int fq_codel_dump(struct Qdisc *sch, struct sk_buff *skb)
          {
          ...
                  if (q->cparams.ce_threshold != CODEL_DISABLED_THRESHOLD &&
                      nla_put_u32(skb, TCA_FQ_CODEL_CE_THRESHOLD,
                                  codel_time_to_us(q->cparams.ce_threshold)))
                          goto nla_put_failure;
      
      This means, kernel internally uses the special value 0x83126E97u to indicate
      that the threshold is disabled (WTF). That is because
      
        (((guint64) 0x83126E97u) * NSEC_PER_USEC) >> CODEL_SHIFT == CODEL_DISABLED_THRESHOLD
      
      So in kernel API this value is reserved (and has a special meaning
      to indicate that the threshold is disabled). So, instead of adding a
      ce_threshold_set flag, use the same value that kernel anyway uses.
      973db2d4
    • Thomas Haller's avatar
      platform: fix handling of fq_codel's memory limit default value · 46a90438
      Thomas Haller authored
      The memory-limit is an unsigned integer. It is ugly (if not wrong) to compare unsigned
      values with "-1". When comparing with the default value we must also use an u32 type.
      Instead add a define NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET.
      
      Note that like iproute2 we treat NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET
      to indicate to not set TCA_FQ_CODEL_MEMORY_LIMIT in RTM_NEWQDISC. This
      special value is entirely internal to NetworkManager (or iproute2) and
      kernel will then choose a default memory limit (of 32MB). So setting
      NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET means to leave it to kernel to
      choose a value (which then chooses 32MB).
      
      See kernel's net/sched/sch_fq_codel.c:
      
          static int fq_codel_init(struct Qdisc *sch, struct nlattr *opt,
                                   struct netlink_ext_ack *extack)
          {
          ...
                  q->memory_limit = 32 << 20; /* 32 MBytes */
      
          static int fq_codel_change(struct Qdisc *sch, struct nlattr *opt,
                                     struct netlink_ext_ack *extack)
          ...
                  if (tb[TCA_FQ_CODEL_MEMORY_LIMIT])
                          q->memory_limit = min(1U << 31, nla_get_u32(tb[TCA_FQ_CODEL_MEMORY_LIMIT]));
      
      Note that not having zero as default value is problematic. In fields like
      "NMPlatformIP4Route.table_coerced" and "NMPlatformRoutingRule.suppress_prefixlen_inverse"
      we avoid this problem by storing a coerced value in the structure so that zero is still
      the default. We don't do that here for memory-limit, so the caller must always explicitly
      set the value.
      46a90438
  2. 30 Apr, 2019 2 commits
  3. 18 Apr, 2019 12 commits
  4. 10 Apr, 2019 1 commit
  5. 26 Mar, 2019 1 commit
  6. 13 Mar, 2019 5 commits
    • Thomas Haller's avatar
      9992ac1c
    • Thomas Haller's avatar
      platform: add support for routing-rule objects and cache them in platform · 9934a6a0
      Thomas Haller authored
      Add and implement NMPlatformRoutingRule types and let the platform cache
      handle rules.
      
      Rules are special in two ways:
      
      - they don't have an ifindex. That makes them different from all other
        currently existing NMPlatform* types, which have an "ifindex" field and
        "implement" NMPlatformObjWithIfindex.
      
      - they have an address family, but contrary to addresses and routes, there
        is only one NMPlatformRoutingRule object to handle both address
        families.
      
      Both of these points require some special considerations.
      
      Kernel treats routing-rules quite similar to routes. That is, kernel
      allows to add different rules/routes, as long as they differ in certain
      fields. These "fields" make up the identity of the rules/routes. But
      in practice, it's not defined which fields contribute to the identity
      of these objects. That makes using the netlink API very hard. For
      example, when kernel gains support for a new attribute which
      NetworkManager does not know yet, then users can add two rules/routes
      that look the same to NetworkManager. That can easily result in cache
      inconsistencies.
      
      Another problem is, that older kernel versions may not yet support all
      fields, which NetworkManager (and newer kernels) considers for identity.
      The older kernel will not simply reject netlink messages with these unknown
      keys, instead it will proceed adding the route/rule without it. That means,
      the added route/rule will have a different identity than what NetworkManager
      intended to add.
      9934a6a0
    • Thomas Haller's avatar
      platform: drop unused nm_platform_refresh_all() · 7c5ad2d9
      Thomas Haller authored
      The function is unused. It would require redesign to work with
      future changes, and since it's unused, just drop it.
      
      The long reasoning is:
      
          Currently, a refresh-all is tied to an NMPObjectType. However, with
          NMPObjectRoutingRule (for policy-routing-rules) that will no longer
          be the case.
      
          That is because NMPObjectRoutingRule will be one object type for
          AF_INET and AF_INET6. Contrary to IPv4 addresses and routes, where
          there are two sets of NMPObject types.
      
          The reason is, that it's preferable to treat IPv4 and IPv6 objects
          similarly, that is: as the same type with an address family property.
      
          That also follows netlink, which uses RTM_GET* messages for both
          address families, and the address family is expressed inside the
          message.
      
          But then an API like nm_platform_refresh_all() makes little sense,
          it would require at least an addr_family argument. But since the
          API is unused, just drop it.
      7c5ad2d9
    • Thomas Haller's avatar
      platform: add NMPlatformObjWithIfindex helper structure for handling NMPObject types · ac4a1deb
      Thomas Haller authored
      Until now, all implemented NMPObject types have an ifindex field (from
      links, addresses, routes, qdisc to tfilter).
      
      The NMPObject structure contains a union of all available types, that
      makes it easier to down-case from an NMPObject pointer to the actual
      content.
      
      The "object" field of NMPObject of type NMPlatformObject is the lowest
      common denominator.
      
      We will add NMPlatformRoutingRules (for policy routing rules). That type
      won't have an ifindex field.
      
      Hence, drop the "ifindex" field from NMPlatformObject type. But also add
      a new type NMPlatformObjWithIfindex, that can represent all types that
      have an ifindex.
      ac4a1deb
    • Thomas Haller's avatar
  7. 14 Feb, 2019 3 commits
  8. 12 Feb, 2019 1 commit
  9. 22 Jan, 2019 1 commit
    • Thomas Haller's avatar
      platform: add @replace_peers argument to nm_platform_link_wireguard_change() · 6f8c7b58
      Thomas Haller authored
      The caller may not wish to replace existing peers, but only update/add
      the peers explicitly passed to nm_platform_link_wireguard_change().
      
      I think that is in particular interesting, because for the most part
      NetworkManager will configure the same set of peers over and over again
      (whenever we resolve the DNS name of an IP endpoint of the WireGuard
      peer).
      
      At that point, it seems disruptive to drop all peers and re-add them
      again. Setting @replace_peers to %FALSE allows to only update/add.
      6f8c7b58
  10. 15 Jan, 2019 1 commit
  11. 09 Jan, 2019 1 commit
  12. 27 Dec, 2018 2 commits
    • Thomas Haller's avatar
      platform: return platform-error from link-add function · 691e5d5c
      Thomas Haller authored
      We need more information what failed. Don't only return success/failure,
      but an error number.
      
      Note that we still don't actually return an error number. Only
      the link_add() function is changed to return an nm-error integer.
      691e5d5c
    • 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
  13. 19 Dec, 2018 3 commits
  14. 29 Nov, 2018 1 commit
    • Thomas Haller's avatar
      platform: add nm_platform_link_get_ifi_flags() helper · b445b1f8
      Thomas Haller authored
      Add helper nm_platform_link_get_ifi_flags() to access the
      ifi-flags.
      
      This replaces the internal API _link_get_flags() and makes it public.
      However, the return value also allows to distinguish between errors
      and valid flags.
      
      Also, consider non-visible links. These are links that are in netlink,
      but not visible in udev. The ifi-flags are inherrently netlink specific,
      so it seems wrong to pretend that the link doesn't exist.
      b445b1f8
  15. 12 Nov, 2018 1 commit
    • Thomas Haller's avatar
      build: avoid header conflict for <linux/if.h> and <net/if.h> with "nm-platform.h" · 37e47fbd
      Thomas Haller authored
      In the past, the headers "linux/if.h" and "net/if.h" were incompatible.
      That means, we can either include one or the other, but not both.
      This is fixed in the meantime, however the issue still exists when
      building against older kernel/glibc.
      
      That means, including one of these headers from a header file
      is problematic. In particular if it's a header like "nm-platform.h",
      which itself is dragged in by many other headers.
      
      Avoid that by not including these headers from "platform.h", but instead
      from the source files where needed (or possibly from less popular header
      files).
      
      Currently there is no problem. However, this allows an unknowing user to
      include <net/if.h> at the same time with "nm-platform.h", which is easy
      to get wrong.
      37e47fbd