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

Message ID 20180618150626.16571-1-lynxis@fe80.eu
State Changes Requested
Delegated to: John Crispin
Headers show
Series
  • [OpenWrt-Devel,netifd] iprule: rework interface based rules to handle dynamic interfaces
Related show

Commit Message

Alexander Couzens June 18, 2018, 3:06 p.m. UTC
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(-)

Comments

Hans Dedecker June 18, 2018, 9:18 p.m. UTC | #1
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

Patch
diff mbox series

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);
 }
 
 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) {
+	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