[OpenWrt-Devel,netifd] iprule: rework interface based rules to handle dynamic interfaces

Previous netifd would only apply `ip rule`s while config phase.
If the iprule is depending on an interface (iif or oif), the rule
will fail if the interface is not up.

Allow iprules to track interface and their devices by hooking into
into the interface_ip_set_enabled() call.

Fixes: FS#1571
---
 interface-ip.c |   3 ++
 iprule.c       | 104 ++++++++++++++++++++++++++++++++++++++++---------
 iprule.h       |   7 ++++
 3 files changed, 96 insertions(+), 18 deletions(-)

On Mon, Jun 18, 2018 at 5:10 PM Alexander Couzens <lynxis@fe80.eu> wrote:
>
> Previous netifd would only apply `ip rule`s while config phase.
> If the iprule is depending on an interface (iif or oif), the rule
> will fail if the interface is not up.
>
> Allow iprules to track interface and their devices by hooking into
> into the interface_ip_set_enabled() call.
>
> Fixes: FS#1571
> ---
>  interface-ip.c |   3 ++
>  iprule.c       | 104 ++++++++++++++++++++++++++++++++++++++++---------
>  iprule.h       |   7 ++++
>  3 files changed, 96 insertions(+), 18 deletions(-)
>
> diff --git a/interface-ip.c b/interface-ip.c
> index 1e49fe6feac7..1260f6059ecd 100644
> --- a/interface-ip.c
> +++ b/interface-ip.c
> @@ -1451,6 +1451,9 @@ void interface_ip_set_enabled(struct interface_ip_settings *ip, bool enabled)
>                         NULL, 0, 0, ip->iface, "failed_policy", true);
>                 ip->iface->policy_rules_set = enabled;
>         }
> +
> +       /* apply ip rules */
> +       iprule_set_enabled(ip->iface, enabled);
I prefer iprule_init_list installing an interface callback handler via
interface_add_user; based on the IFEV_UP and IFEV_DOWN events the
necessary logic can be triggered inside iprule.c. This would keep the
layering clean inside netifd and avoids the need to export extra
iprule functions
>  }
>
>  void
> diff --git a/iprule.c b/iprule.c
> index 7cf7422f4168..79e6eca9dfb7 100644
> --- a/iprule.c
> +++ b/iprule.c
> @@ -2,6 +2,7 @@
>   * netifd - network interface daemon
>   * Copyright (C) 2012 Felix Fietkau <nbd@openwrt.org>
>   * Copyright (C) 2013 Jo-Philipp Wich <jow@openwrt.org>
> + * Copyright (C) 2018 Alexander Couzens <lynxis@fe80.eu>
>   *
>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License version 2
> @@ -66,6 +67,16 @@ const struct uci_blob_param_list rule_attr_list = {
>         .params = rule_attr,
>  };
>
> +/* interface based rules are dynamic. */
> +static int rule_ready(struct iprule *rule) {
Better to let this function return a bool

Hans
> +       if (rule->flags & IPRULE_OUT && rule->out_dev == NULL)
> +               return 0;
> +
> +       if (rule->flags & IPRULE_IN && rule->in_dev == NULL)
> +               return 0;
> +
> +       return 1;
> +}
>
>  static bool
>  iprule_parse_mark(const char *mark, struct iprule *rule)
> @@ -97,13 +108,65 @@ iprule_parse_mark(const char *mark, struct iprule *rule)
>         return true;
>  }
>
> +/* remove all rules from the system which depend on iface */
> +static void
> +remove_iface_rules(struct interface *iface) {
> +       struct iprule *rule;
> +
> +       vlist_for_each_element(&iprules, rule, node) {
> +               if (!(rule->flags & (IPRULE_IN | IPRULE_OUT)))
> +                       continue;
> +
> +               if (!strcmp(rule->in_iface, iface->name)) {
> +                       if (rule_ready(rule))
> +                               system_del_iprule(rule);
> +                       rule->in_dev[0] = 0;
> +               }
> +
> +               if (!strcmp(rule->out_iface, iface->name)) {
> +                       if (rule_ready(rule))
> +                               system_del_iprule(rule);
> +                       rule->out_dev[0] = 0;
> +               }
> +       }
> +}
> +
> +/* update rules which depend on iface. Add them to the system if they validates afterwards. */
> +static void add_iface_rule(struct interface *iface) {
> +       struct iprule *rule;
> +
> +       vlist_for_each_element(&iprules, rule, node) {
> +               if (rule_ready(rule))
> +                       continue;
> +
> +               if (!strcmp(rule->out_iface, iface->name))
> +                       memcpy(rule->out_dev, iface->l3_dev.dev->ifname, sizeof(rule->out_dev));
> +
> +               if (!strcmp(rule->in_iface, iface->name))
> +                       memcpy(rule->in_dev, iface->l3_dev.dev->ifname, sizeof(rule->in_dev));
> +
> +               if (rule_ready(rule))
> +                       system_add_iprule(rule);
> +       }
> +}
> +
> +/* called by interface_ip_set_enabled */
> +void iprule_set_enabled(struct interface *iface, int enabled)
> +{
> +       if (enabled)
> +               add_iface_rule(iface);
> +       else
> +               remove_iface_rules(iface);
> +}
> +
>  void
>  iprule_add(struct blob_attr *attr, bool v6)
>  {
> -       struct interface *iif = NULL, *oif = NULL;
>         struct blob_attr *tb[__RULE_MAX], *cur;
> -       struct interface *iface;
> +       struct interface *iface, *tmp;
>         struct iprule *rule;
> +       char *iface_name;
>         int af = v6 ? AF_INET6 : AF_INET;
>
>         blobmsg_parse(rule_attr, __RULE_MAX, tb, blobmsg_data(attr), blobmsg_data_len(attr));
> @@ -118,28 +181,30 @@ iprule_add(struct blob_attr *attr, bool v6)
>         if ((cur = tb[RULE_INVERT]) != NULL)
>                 rule->invert = blobmsg_get_bool(cur);
>
> +       /* TODO: check if we need to validate the uci interface name or not */
>         if ((cur = tb[RULE_INTERFACE_IN]) != NULL) {
> -               iif = vlist_find(&interfaces, blobmsg_data(cur), iface, node);
> +               iface_name = calloc(1, strlen(blobmsg_data(cur)) + 1);
> +               rule->in_iface = strcpy(iface_name, blobmsg_data(cur));
> +               rule->flags |= IPRULE_IN;
>
> -               if (!iif || !iif->l3_dev.dev) {
> -                       DPRINTF("Failed to resolve device of network: %s\n", (char *) blobmsg_data(cur));
> -                       goto error;
> +               /* this can be filled later by the set_interface_ip call */
> +               tmp = vlist_find(&interfaces, blobmsg_data(cur), iface, node);
> +               if (tmp && tmp->l3_dev.dev) {
> +                       memcpy(rule->in_dev, tmp->l3_dev.dev->ifname, sizeof(rule->in_dev));
>                 }
> -
> -               memcpy(rule->in_dev, iif->l3_dev.dev->ifname, sizeof(rule->in_dev));
> -               rule->flags |= IPRULE_IN;
>         }
>
> +       /* TODO: check if we need to validate the uci interface name or not */
>         if ((cur = tb[RULE_INTERFACE_OUT]) != NULL) {
> -               oif = vlist_find(&interfaces, blobmsg_data(cur), iface, node);
> +               iface_name = calloc(1, strlen(blobmsg_data(cur)) + 1);
> +               rule->out_iface = strcpy(iface_name, blobmsg_data(cur));
> +               rule->flags |= IPRULE_OUT;
>
> -               if (!oif || !oif->l3_dev.dev) {
> -                       DPRINTF("Failed to resolve device of network: %s\n", (char *) blobmsg_data(cur));
> -                       goto error;
> +               /* this can be filled later by the set_interface_ip call */
> +               tmp = vlist_find(&interfaces, blobmsg_data(cur), iface, node);
> +               if (tmp && tmp->l3_dev.dev) {
> +                       memcpy(rule->out_dev, tmp->l3_dev.dev->ifname, sizeof(rule->out_dev));
>                 }
> -
> -               memcpy(rule->out_dev, oif->l3_dev.dev->ifname, sizeof(rule->out_dev));
> -               rule->flags |= IPRULE_OUT;
>         }
>
>         if ((cur = tb[RULE_SRC]) != NULL) {
> @@ -248,12 +313,15 @@ iprule_update_rule(struct vlist_tree *tree,
>         rule_new = container_of(node_new, struct iprule, node);
>
>         if (node_old) {
> -               system_del_iprule(rule_old);
> +               /* interface based rules must not be present at all times */
> +               if (rule_ready(rule_old))
> +                       system_del_iprule(rule_old);
>                 free(rule_old);
>         }
>
> -       if (node_new)
> +       if (node_new && rule_ready(rule_new)) {
>                 system_add_iprule(rule_new);
> +       }
>  }
>
>  static void __init
> diff --git a/iprule.h b/iprule.h
> index b723bdb05d7d..c399c413efb0 100644
> --- a/iprule.h
> +++ b/iprule.h
> @@ -74,6 +74,11 @@ struct iprule {
>
>         bool invert;
>
> +       /* uci interface name */
> +       const char *in_iface;
> +       const char *out_iface;
> +
> +       /* device name */
>         char in_dev[IFNAMSIZ + 1];
>         char out_dev[IFNAMSIZ + 1];
>
> @@ -102,4 +107,6 @@ void iprule_add(struct blob_attr *attr, bool v6);
>  void iprule_update_start(void);
>  void iprule_update_complete(void);
>
> +void iprule_set_enabled(struct interface *iface, int enabled);
> +
>  #endif
> --
> 2.17.1
>
>
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/listinfo/openwrt-devel