diff mbox series

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

Message ID 1519537482-6861-2-git-send-email-roopa@cumulusnetworks.com
State Changes Requested, archived
Delegated to: David Miller
Headers show
Series [net-next,1/5] net: fib_rules: support for match on ip_proto, sport and dport | expand

Commit Message

Roopa Prabhu Feb. 25, 2018, 5:44 a.m. UTC 
From: Roopa Prabhu <roopa@cumulusnetworks.com>

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

Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
---
 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(-)

Comments

Nikolay Aleksandrov Feb. 25, 2018, 3:04 p.m. UTC | #1 
On 25/02/18 07:44, Roopa Prabhu wrote:
> From: Roopa Prabhu <roopa@cumulusnetworks.com>
> 
> uapi for ip_proto, sport and dport range match
> in fib rules.
> 
> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
> ---
>  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.

> 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.

> +	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)

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

> +}
> +
>  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

> +			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) {
>
Nikolay Aleksandrov Feb. 25, 2018, 3:27 p.m. UTC | #2 
On 25/02/18 17:04, Nikolay Aleksandrov wrote:
> On 25/02/18 07:44, Roopa Prabhu wrote:
>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>
>> uapi for ip_proto, sport and dport range match
>> in fib rules.
>>
>> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
>> ---
[snip]
>>  	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.

Nevermind this comment, I spoke too soon and saw the match later. :-)

> 
>> +	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)
> 
>> +}
>> +
>> +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)
> 
>> +}
>> +
>>  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
> 
>> +			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) {
>>
>
Roopa Prabhu Feb. 25, 2018, 5:58 p.m. UTC | #3 
On Sun, Feb 25, 2018 at 7:04 AM, Nikolay Aleksandrov
<nikolay@cumulusnetworks.com> wrote:
> On 25/02/18 07:44, Roopa Prabhu wrote:
>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>
>> uapi for ip_proto, sport and dport range match
>> in fib rules.
>>
>> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
>> ---
>>  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




>
>> 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.
>
>> +     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)
>
>> +}
>> +
>> +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)
>
>> +}
>> +
>>  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
>
>> +                     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) {
>>
>
David Ahern Feb. 26, 2018, 3:08 a.m. UTC | #4 
On 2/24/18 10:44 PM, Roopa Prabhu wrote:
> From: Roopa Prabhu <roopa@cumulusnetworks.com>
> 
> uapi for ip_proto, sport and dport range match
> in fib rules.
> 
> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
> ---
>  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(-)
> 
> 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;

u16 for kernel headers; __u16 is for uapi.

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

Seems like that should be a check that start and end are both not 0.
Given the uses of fib_rule_port_range_valid, perhaps another helper is
needed to make this more readable -- e.g., fib_rule_port_range_set --
which would be used here and fill_rule.


>  	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);

Isn't the data struct from userspace fib_rule_port_range? that begs the
question of whether a separate struct definition is needed for kernel
side. Why not just use the one for both?


> +
> +	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])))
> +			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) {
>
Roopa Prabhu Feb. 26, 2018, 3:38 a.m. UTC | #5 
On Sun, Feb 25, 2018 at 7:08 PM, David Ahern <dsa@cumulusnetworks.com> wrote:
> On 2/24/18 10:44 PM, Roopa Prabhu wrote:
>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>
>> uapi for ip_proto, sport and dport range match
>> in fib rules.
>>
>> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
>> ---
>>  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(-)
>>
>> 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;
>
> u16 for kernel headers; __u16 is for uapi.
>

ack,

>> +};
>> +
>>  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;
>> +     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;
>> +}
>> +
>> +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;
>> +}
>> +
>>  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;
>
> Seems like that should be a check that start and end are both not 0.
> Given the uses of fib_rule_port_range_valid, perhaps another helper is
> needed to make this more readable -- e.g., fib_rule_port_range_set --
> which would be used here and fill_rule.

yeah, was trying to not add two helpers. But, i sure can.


>
>
>>       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);
>
> Isn't the data struct from userspace fib_rule_port_range? that begs the
> question of whether a separate struct definition is needed for kernel
> side. Why not just use the one for both?

yep, will fix. I was following the uid_range code. But, looking back
at it, i think uid range has other reasons to have a separate struct.
will move to one.

>
>
>> +
>> +     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])))
>> +                     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) {
>>
>
diff mbox series

Patch

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