Re: [PATCH net-next 1/5] net: fib_rules: support for match on ip_proto, sport and dport

On Sun, Feb 25, 2018 at 7:04 AM, Nikolay Aleksandrov
[off-list ref] wrote:

On 25/02/18 07:44, Roopa Prabhu wrote:

quoted

From: Roopa Prabhu <redacted>

uapi for ip_proto, sport and dport range match
in fib rules.

Signed-off-by: Roopa Prabhu <redacted>
---
 include/net/fib_rules.h        | 31 +++++++++++++-
 include/uapi/linux/fib_rules.h |  8 ++++
 net/core/fib_rules.c           | 94 +++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 130 insertions(+), 3 deletions(-)

You should probably update validate_rulemsg() as well, these aren't added in the per-proto
policies and nothing validates if the attribute data is actually there. Maybe I'm missing
something obvious, but it looks like many other FRA_ attributes don't have such checks.

yeah, I added the sport checks there initially and later removed it
since I did not see any of the
other FRA_* attributes there. and then ended up adding it in the
respective rule add and del functions where other attributes
 were validated for consistency. I can submit a follow on patch to
move all FRA_* attribute validations to validate_rulemsg().

I can sure start with the new attribute validation in this series in
validate_rulemsg in v2

quoted

diff --git a/include/net/fib_rules.h b/include/net/fib_rules.h
index b3d2162..6d99202 100644
--- a/include/net/fib_rules.h
+++ b/include/net/fib_rules.h

@@ -11,6 +11,11 @@
 #include <net/rtnetlink.h>
 #include <net/fib_notifier.h>

+struct fib_port_range {
+     __u16 start;
+     __u16 end;
+};
+
 struct fib_kuid_range {
      kuid_t start;
      kuid_t end;

@@ -27,7 +32,7 @@ struct fib_rule {
      u8                      action;
      u8                      l3mdev;
      u8                      proto;
-     /* 1 byte hole, try to use */
+     u8                      ip_proto;
      u32                     target;
      __be64                  tun_id;
      struct fib_rule __rcu   *ctarget;

@@ -40,6 +45,8 @@ struct fib_rule {
      char                    iifname[IFNAMSIZ];
      char                    oifname[IFNAMSIZ];
      struct fib_kuid_range   uid_range;
+     struct fib_port_range   sport_range;
+     struct fib_port_range   dport_range;
      struct rcu_head         rcu;
 };

@@ -144,6 +151,28 @@ static inline u32 frh_get_table(struct fib_rule_hdr *frh, struct nlattr **nla)
      return frh->table;
 }

+static inline bool fib_rule_port_inrange(struct fib_port_range *a,
+                                      __be16 port)
+{
+     if (!a->start)
+             return true;

Can start be == 0 ?
IIUC this check is unnecessary because when you're adding the new rule,
you do a check for start > 0 so it shouldn't be possible to be 0.

quoted

+     return ntohs(port) >= a->start &&
+             ntohs(port) <= a->end;
+}
+
+static inline bool fib_rule_port_range_valid(const struct fib_port_range *a)
+{
+     return a->start > 0 && a->end < 0xffff &&
+                     a->start <= a->end;

nit: alignment (also can be on a single line)

quoted

+}
+
+static inline bool fib_rule_port_range_compare(struct fib_port_range *a,
+                                            struct fib_port_range *b)
+{
+     return a->start == b->start &&
+             a->end == b->end;

nit: alignment (also can be on a single line)

quoted

+}
+
 struct fib_rules_ops *fib_rules_register(const struct fib_rules_ops *,
                                       struct net *);
 void fib_rules_unregister(struct fib_rules_ops *);

diff --git a/include/uapi/linux/fib_rules.h b/include/uapi/linux/fib_rules.h
index 77d90ae..232df14 100644
--- a/include/uapi/linux/fib_rules.h
+++ b/include/uapi/linux/fib_rules.h

@@ -35,6 +35,11 @@ struct fib_rule_uid_range {
      __u32           end;
 };

+struct fib_rule_port_range {
+     __u16           start;
+     __u16           end;
+};
+
 enum {
      FRA_UNSPEC,
      FRA_DST,        /* destination address */

@@ -59,6 +64,9 @@ enum {
      FRA_L3MDEV,     /* iif or oif is l3mdev goto its table */
      FRA_UID_RANGE,  /* UID range */
      FRA_PROTOCOL,   /* Originator of the rule */
+     FRA_IP_PROTO,   /* ip proto */
+     FRA_SPORT_RANGE, /* sport */
+     FRA_DPORT_RANGE, /* dport */
      __FRA_MAX
 };

diff --git a/net/core/fib_rules.c b/net/core/fib_rules.c
index a6aea80..5008235 100644
--- a/net/core/fib_rules.c
+++ b/net/core/fib_rules.c

@@ -33,6 +33,10 @@ bool fib_rule_matchall(const struct fib_rule *rule)
      if (!uid_eq(rule->uid_range.start, fib_kuid_range_unset.start) ||
          !uid_eq(rule->uid_range.end, fib_kuid_range_unset.end))
              return false;
+     if (fib_rule_port_range_valid(&rule->sport_range))
+             return false;
+     if (fib_rule_port_range_valid(&rule->dport_range))
+             return false;
      return true;
 }
 EXPORT_SYMBOL_GPL(fib_rule_matchall);

@@ -221,6 +225,12 @@ static int nla_put_uid_range(struct sk_buff *skb, struct fib_kuid_range *range)
      return nla_put(skb, FRA_UID_RANGE, sizeof(out), &out);
 }

+static int nla_put_port_range(struct sk_buff *skb, int attrtype,
+                           struct fib_port_range *range)
+{
+     return nla_put(skb, attrtype, sizeof(*range), range);
+}
+
 static int fib_rule_match(struct fib_rule *rule, struct fib_rules_ops *ops,
                        struct flowi *fl, int flags,
                        struct fib_lookup_arg *arg)

@@ -425,6 +435,17 @@ static int rule_exists(struct fib_rules_ops *ops, struct fib_rule_hdr *frh,
                  !uid_eq(r->uid_range.end, rule->uid_range.end))
                      continue;

+             if (r->ip_proto != rule->ip_proto)
+                     continue;
+
+             if (!fib_rule_port_range_compare(&r->sport_range,
+                                              &rule->sport_range))
+                     continue;
+
+             if (!fib_rule_port_range_compare(&r->dport_range,
+                                              &rule->dport_range))
+                     continue;
+
              if (!ops->compare(r, frh, tb))
                      continue;
              return 1;

@@ -432,6 +453,20 @@ static int rule_exists(struct fib_rules_ops *ops, struct fib_rule_hdr *frh,
      return 0;
 }

+static int nla_get_port_range(struct nlattr *pattr,
+                           struct fib_port_range *port_range)
+{
+     const struct fib_port_range *pr = nla_data(pattr);
+
+     if (!fib_rule_port_range_valid(pr))
+             return -EINVAL;
+
+     port_range->start = pr->start;
+     port_range->end = pr->end;
+
+     return 0;
+}
+
 int fib_nl_newrule(struct sk_buff *skb, struct nlmsghdr *nlh,
                 struct netlink_ext_ack *extack)
 {

@@ -569,6 +604,23 @@ int fib_nl_newrule(struct sk_buff *skb, struct nlmsghdr *nlh,
              rule->uid_range = fib_kuid_range_unset;
      }

+     if (tb[FRA_IP_PROTO])
+             rule->ip_proto = nla_get_u8(tb[FRA_IP_PROTO]);
+
+     if (tb[FRA_SPORT_RANGE]) {
+             err = nla_get_port_range(tb[FRA_SPORT_RANGE],
+                                      &rule->sport_range);
+             if (err)
+                     goto errout_free;
+     }
+
+     if (tb[FRA_DPORT_RANGE]) {
+             err = nla_get_port_range(tb[FRA_DPORT_RANGE],
+                                      &rule->dport_range);
+             if (err)
+                     goto errout_free;
+     }
+
      if ((nlh->nlmsg_flags & NLM_F_EXCL) &&
          rule_exists(ops, frh, tb, rule)) {
              err = -EEXIST;

@@ -638,6 +690,8 @@ int fib_nl_delrule(struct sk_buff *skb, struct nlmsghdr *nlh,
      struct fib_rule *rule, *r;
      struct nlattr *tb[FRA_MAX+1];
      struct fib_kuid_range range;
+     struct fib_port_range *sprange = NULL;
+     struct fib_port_range *dprange = NULL;
      int err = -EINVAL;

      if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*frh)))

@@ -667,6 +721,22 @@ int fib_nl_delrule(struct sk_buff *skb, struct nlmsghdr *nlh,
              range = fib_kuid_range_unset;
      }

+     if (tb[FRA_SPORT_RANGE]) {
+             sprange = nla_data(tb[FRA_SPORT_RANGE]);
+             if (!fib_rule_port_range_valid(sprange)) {
+                     err = -EINVAL;
+                     goto errout;
+             }
+     }
+
+     if (tb[FRA_DPORT_RANGE]) {
+             dprange = nla_data(tb[FRA_DPORT_RANGE]);
+             if (!fib_rule_port_range_valid(dprange)) {
+                     err = -EINVAL;
+                     goto errout;
+             }
+     }
+
      list_for_each_entry(rule, &ops->rules_list, list) {
              if (tb[FRA_PROTOCOL] &&
                  (rule->proto != nla_get_u8(tb[FRA_PROTOCOL])))

@@ -712,6 +782,18 @@ int fib_nl_delrule(struct sk_buff *skb, struct nlmsghdr *nlh,
                   !uid_eq(rule->uid_range.end, range.end)))
                      continue;

+             if (tb[FRA_IP_PROTO] &&
+                 (rule->ip_proto != nla_get_u8(tb[FRA_IP_PROTO])))

nit: no need for the extra braces

quoted

+                     continue;
+
+             if (sprange &&
+                 !fib_rule_port_range_compare(&rule->sport_range, sprange))
+                     continue;
+
+             if (dprange &&
+                 !fib_rule_port_range_compare(&rule->dport_range, dprange))
+                     continue;
+
              if (!ops->compare(rule, frh, tb))
                      continue;

@@ -790,7 +872,10 @@ static inline size_t fib_rule_nlmsg_size(struct fib_rules_ops *ops,
                       + nla_total_size(4) /* FRA_FWMASK */
                       + nla_total_size_64bit(8) /* FRA_TUN_ID */
                       + nla_total_size(sizeof(struct fib_kuid_range))
-                      + nla_total_size(1); /* FRA_PROTOCOL */
+                      + nla_total_size(1) /* FRA_PROTOCOL */
+                      + nla_total_size(1) /* FRA_IP_PROTO */
+                      + nla_total_size(sizeof(struct fib_port_range)) /* FRA_SPORT_RANGE */
+                      + nla_total_size(sizeof(struct fib_port_range)); /* FRA_DPORT_RANGE */

      if (ops->nlmsg_payload)
              payload += ops->nlmsg_payload(rule);

@@ -855,7 +940,12 @@ static int fib_nl_fill_rule(struct sk_buff *skb, struct fib_rule *rule,
          (rule->l3mdev &&
           nla_put_u8(skb, FRA_L3MDEV, rule->l3mdev)) ||
          (uid_range_set(&rule->uid_range) &&
-          nla_put_uid_range(skb, &rule->uid_range)))
+          nla_put_uid_range(skb, &rule->uid_range)) ||
+         (fib_rule_port_range_valid(&rule->sport_range) &&
+          nla_put_port_range(skb, FRA_SPORT_RANGE, &rule->sport_range)) ||
+         (fib_rule_port_range_valid(&rule->dport_range) &&
+          nla_put_port_range(skb, FRA_DPORT_RANGE, &rule->dport_range)) ||
+         (rule->ip_proto && nla_put_u8(skb, FRA_IP_PROTO, rule->ip_proto)))
              goto nla_put_failure;

      if (rule->suppress_ifgroup != -1) {