fib_rules: add minimum prefix length

PATCH In review Stefan Tomanek 5 participants 33 messages Jul 23, 2013 – Aug 1, 2013

Subsystems (3)

Networking Network drivers IPv4/IPv6

lore

Share & tools

lore

Stefan Tomanek

author

2013-07-23 22:21 UTC #0 lore raw

PATCH 1/1 fib_rules: add minimum prefix length

This change adds a minimum prefix length to the structures of routing rules. If a rule is added with a minimum prefix length >0, only routes meeting this threshold will be considered. Any other (more general) routing table entries will be ignored.

When configuring a system with multiple network uplinks and default routes, it is often convinient to reference the main routing table multiple times - but omitting the default route. Using this patch and a modified "ip" utility, this can be achieved by using the following command sequence:

  $ ip route add table secuplink default via 10.42.23.1

  $ ip rule add pref 100            table main prefixlength 1
  $ ip rule add pref 150 fwmark 0xA table secuplink

With this setup, packets marked 0xA will be processed by the additional routing table "secuplink", but only if no suitable route in the main routing table can be found. By using a minimal prefixlength of 1, the default route (/0) of the table "main" is hidden to packets processed by rule 100; packets traveling to destinations with more specific routing entries are processed as usual.

Signed-off-by Stefan Tomanek <[email protected]>

---
 include/net/fib_rules.h        |    1 +
 include/uapi/linux/fib_rules.h |    2 +-
 net/core/fib_rules.c           |    5 +++++
 net/ipv4/fib_rules.c           |   11 ++++++++++-
 4 files changed, 17 insertions(+), 2 deletions(-)

include/net/fib_rules.h +1 -0

index e361f48..b098a9d 100644
@@ -18,6 +18,7 @@ struct fib_rule {
 	u32			pref;
 	u32			flags;
 	u32			table;
+	u8			table_prefixlen_min;
 	u8			action;
 	u32			target;
 	struct fib_rule __rcu	*ctarget;

include/uapi/linux/fib_rules.h +1 -1

index 51da65b..59cd31b 100644
@@ -45,7 +45,7 @@ enum {
 	FRA_FLOW,	/* flow/class id */
 	FRA_UNUSED6,
 	FRA_UNUSED7,
-	FRA_UNUSED8,
+	FRA_TABLE_PREFIXLEN_MIN,
 	FRA_TABLE,	/* Extended table id */
 	FRA_FWMASK,	/* mask for netfilter mark */
 	FRA_OIFNAME,

net/core/fib_rules.c +5 -0

index 2173544..8a15621 100644
@@ -337,6 +337,8 @@ static int fib_nl_newrule(struct sk_buff *skb, struct nlmsghdr* nlh)
 	rule->action = frh->action;
 	rule->flags = frh->flags;
 	rule->table = frh_get_table(frh, tb);
+	if (tb[FRA_TABLE_PREFIXLEN_MIN])
+		rule->table_prefixlen_min = nla_get_u8(tb[FRA_TABLE_PREFIXLEN_MIN]);
 
 	if (!tb[FRA_PRIORITY] && ops->default_pref)
 		rule->pref = ops->default_pref(ops);

@@ -523,6 +525,7 @@ static inline size_t fib_rule_nlmsg_size(struct fib_rules_ops *ops,
 			 + nla_total_size(IFNAMSIZ) /* FRA_OIFNAME */
 			 + nla_total_size(4) /* FRA_PRIORITY */
 			 + nla_total_size(4) /* FRA_TABLE */
+			 + nla_total_size(1) /* FRA_TABLE_PREFIXLEN_MIN */
 			 + nla_total_size(4) /* FRA_FWMARK */
 			 + nla_total_size(4); /* FRA_FWMASK */

@@ -548,6 +551,8 @@ static int fib_nl_fill_rule(struct sk_buff *skb, struct fib_rule *rule,
 	frh->table = rule->table;
 	if (nla_put_u32(skb, FRA_TABLE, rule->table))
 		goto nla_put_failure;
+	if (nla_put_u8(skb, FRA_TABLE_PREFIXLEN_MIN, rule->table_prefixlen_min))
+		goto nla_put_failure;
 	frh->res1 = 0;
 	frh->res2 = 0;
 	frh->action = rule->action;

net/ipv4/fib_rules.c +10 -1

index 26aa65d..94e9051 100644
@@ -95,8 +95,17 @@ static int fib4_rule_action(struct fib_rule *rule, struct flowi *flp,
 		goto errout;
 
 	err = fib_table_lookup(tbl, &flp->u.ip4, (struct fib_result *) arg->result, arg->flags);
-	if (err > 0)
+	if (err > 0) {
 		err = -EAGAIN;
+		goto errout;
+	}
+	/* do not accept result if the route does not meet the required prefix length */
+	if (arg->result) {
+		if (((struct fib_result *)arg->result)->prefixlen < rule->table_prefixlen_min) {
+			err = -EAGAIN;
+			goto errout;
+		}
+	}
 errout:
 	return err;
 }

Signature · 1 line

1.7.10.4

Stephen Hemminger

2013-07-23 22:38 UTC #1 lore raw

include/uapi/linux/fib_rules.h enum { · line 51

Quoted 13 lines · On Wed, 24 Jul 2013 00:02:21 +0200 Stefan Tomanek…

include/uapi/linux/fib_rules.h +1 -1

index 51da65b..59cd31b 100644
@@ -45,7 +45,7 @@ enum {
 	FRA_FLOW,	/* flow/class id */
 	FRA_UNUSED6,
 	FRA_UNUSED7,
-	FRA_UNUSED8,
+	FRA_TABLE_PREFIXLEN_MIN,
 	FRA_TABLE,	/* Extended table id */
 	FRA_FWMASK,	/* mask for netfilter mark */
 	FRA_OIFNAME,

Not sure if reusing an entry or adding a new value is better to retain compatibility. Also don't you have to update FRA_GENERIC_POLICY?

Stefan Tomanek

author

2013-07-23 22:52 UTC #2 lore raw

Dies schrieb Stephen Hemminger ([email protected]):

Quoted 6 lines

include/uapi/linux/fib_rules.h +0 -0

@@ -45,7 +45,7 @@ enum {
 	FRA_FLOW,	/* flow/class id */
 	FRA_UNUSED6,
 	FRA_UNUSED7,

Quoted 8 lines

include/uapi/linux/fib_rules.h enum { · line 51
Quoted 5 lines
include/uapi/linux/fib_rules.h +1 -1
-	FRA_UNUSED8,
+	FRA_TABLE_PREFIXLEN_MIN,
 	FRA_TABLE,	/* Extended table id */
 	FRA_FWMASK,	/* mask for netfilter mark */
 	FRA_OIFNAME,
Not sure if reusing an entry or adding a new value is better to retain compatibility.

Well, neither am I; my first changesets did in fact act a new value, but I then switched to reusing an existing one. I am open to suggestions from people having more experience with the existing code base.

Quoted 1 line

Also don't you have to update FRA_GENERIC_POLICY?

Well, I'm not sure - as I said, I was glad that I was able to add this functionality (which I was craving for quite some time now) to the kernel, so please feel free to point out (and possibly iron out) any issues with it.

Thank you very much for your response
Stefan Tomanek

Hannes Frederic Sowa

2013-07-24 02:14 UTC #3 lore raw

In reply to Stefan Tomanek

Hi!

Quoted 9 lines · Stefan Tomanek

This change adds a minimum prefix length to the structures of routing rules. If a rule is added with a minimum prefix length >0, only routes meeting this threshold will be considered. Any other (more general) routing table entries will be ignored.
When configuring a system with multiple network uplinks and default routes, it is often convinient to reference the main routing table multiple times - but omitting the default route. Using this patch and a modified "ip" utility, this can be achieved by using the following command sequence:

Yeah, it is sometimes pretty hideous to set up, especially if one uses ppp stuff and such. But I am unsure if this change does actually improve that considerable. Static setups should be easily doable right now and for ppp/vpn stuff, I fear, it would still lack a bit of flexibility.

Off-topic:
I had the idea of supporting per process routing tables to deal with
that: ip route exec table foo pppd ...
So that every change of pppd and childs would only affect table foo (the details could be hairy but currently not enough time to find out).

Quoted 13 lines

net/ipv4/fib_rules.c +3 -1

index 26aa65d..94e9051 100644
@@ -95,8 +95,17 @@ static int fib4_rule_action(struct fib_rule *rule, struct flowi *flp,
 		goto errout;
 
 	err = fib_table_lookup(tbl, &flp->u.ip4, (struct fib_result *) arg->result, arg->flags);
-	if (err > 0)
+	if (err > 0) {
 		err = -EAGAIN;
+		goto errout;
+	}

net/ipv4/fib_rules.c static int fib4_rule_action(struct fib_rule *rule, struct flowi *flp, · line 111

Quoted 10 lines

net/ipv4/fib_rules.c +7 -0

+	/* do not accept result if the route does not meet the required prefix length */
+	if (arg->result) {
+		if (((struct fib_result *)arg->result)->prefixlen < rule->table_prefixlen_min) {
+			err = -EAGAIN;
+			goto errout;
+		}
+	}
 errout:
 	return err;
 }

I would try to factor the prefixlen_min check out into a e.g. fib4_rule_constrain function for which a new field in fib_rules_ops needs to be created as callback. Also it would be nice to have IPv6 support, too. ;)

Greetings,

Hannes

Stefan Tomanek

author

2013-07-24 07:58 UTC #4 lore raw

Dies schrieb Hannes Frederic Sowa ([email protected]):

Quoted 4 lines

Yeah, it is sometimes pretty hideous to set up, especially if one uses ppp stuff and such. But I am unsure if this change does actually improve that considerable. Static setups should be easily doable right now and for ppp/vpn stuff, I fear, it would still lack a bit of flexibility.

Well, it does work for me. I am using a dynamic PPP uplink and an OpenVPN tunnel where some marked traffic is guided through.

To achieve this, I always had to configure pppd with "nodefaultroute" just to add the default route it manually in a separate table, complicating the configuration process.

With my patches added, I can just reference a "masked" version of the main routing table at first, ignoring the default route placed there by pppd:

echo "vpn" >> /etc/iproute2/rt_tables
ip route add table vpn default via tun0
ip rule add pref 100 lookup main prefixlength 0
ip rule add pref 200 fwmark 0xA lookup vpn

             |
             V
[ table main prefixlength >0 ]
             |
             V
       <fwmark 0xA?>   ->  [ table vpn ]
             |                   |
	     |    ,--------------´
	     V    V
        [ table main ]

That way, there is no need to reconfigure pppd, dhclient etc. If a specific route of the main table matches, it will we used. If the main table just points to the default route (prefixlengt == 0), it will be ignored and the packet travels to the next rule. In the end, the complete main table might still be consulted, including the previously shunned default route.

Works great and requires little to no hacking around distribution specific network scripts.

Quoted 4 lines

I would try to factor the prefixlen_min check out into a e.g. fib4_rule_constrain function for which a new field in fib_rules_ops needs to be created as callback. Also it would be nice to have IPv6 support, too. ;)

Why not, sure. Working solutions today, better solutions tomorrow :-)

Stefan Tomanek

author

2013-07-25 16:29 UTC #5 lore raw

In reply to Hannes Frederic Sowa

Dies schrieb Hannes Frederic Sowa ([email protected]):

Quoted 4 lines

I would try to factor the prefixlen_min check out into a e.g. fib4_rule_constrain function for which a new field in fib_rules_ops needs to be created as callback. Also it would be nice to have IPv6 support, too. ;)

I was working on my patchset again and considered your suggestion; however I'm not sure whether factoring out the constraints into a separate function is actually that useful, since they are only called from one specific location for each protocol; can you think of another useful application?

I however did like the idea of adding IPv6 support, so I did - I'll post the new patch later on.

Thanks for your feedback.

Stefan Tomanek

author

2013-07-25 17:43 UTC #6 lore raw

PATCH 1/1 fib_rules/fib6rules: add minimum prefix length

  $ ip route add table secuplink default via 10.42.23.1

  $ ip rule add pref 100            table main prefixlength 1
  $ ip rule add pref 150 fwmark 0xA table secuplink

Signed-off-by Stefan Tomanek <[email protected]>

---
 include/net/fib_rules.h        |    2 ++
 include/uapi/linux/fib_rules.h |    2 +-
 net/core/fib_rules.c           |    5 +++++
 net/ipv4/fib_rules.c           |   11 ++++++++++-
 net/ipv6/fib6_rules.c          |    8 ++++++++
 5 files changed, 26 insertions(+), 2 deletions(-)

include/net/fib_rules.h +2 -0

index e361f48..70f020f 100644
@@ -18,6 +18,7 @@ struct fib_rule {
 	u32			pref;
 	u32			flags;
 	u32			table;
+	u8			table_prefixlen_min;
 	u8			action;
 	u32			target;
 	struct fib_rule __rcu	*ctarget;

@@ -80,6 +81,7 @@ struct fib_rules_ops {
 	[FRA_FWMARK]	= { .type = NLA_U32 }, \
 	[FRA_FWMASK]	= { .type = NLA_U32 }, \
 	[FRA_TABLE]     = { .type = NLA_U32 }, \
+	[FRA_TABLE_PREFIXLEN_MIN] = { .type = NLA_U8 }, \
 	[FRA_GOTO]	= { .type = NLA_U32 }
 
 static inline void fib_rule_get(struct fib_rule *rule)

include/uapi/linux/fib_rules.h +1 -1

index 51da65b..59cd31b 100644
@@ -45,7 +45,7 @@ enum {
 	FRA_FLOW,	/* flow/class id */
 	FRA_UNUSED6,
 	FRA_UNUSED7,
-	FRA_UNUSED8,
+	FRA_TABLE_PREFIXLEN_MIN,
 	FRA_TABLE,	/* Extended table id */
 	FRA_FWMASK,	/* mask for netfilter mark */
 	FRA_OIFNAME,

net/core/fib_rules.c +5 -0

index 2173544..8a15621 100644
@@ -337,6 +337,8 @@ static int fib_nl_newrule(struct sk_buff *skb, struct nlmsghdr* nlh)
 	rule->action = frh->action;
 	rule->flags = frh->flags;
 	rule->table = frh_get_table(frh, tb);
+	if (tb[FRA_TABLE_PREFIXLEN_MIN])
+		rule->table_prefixlen_min = nla_get_u8(tb[FRA_TABLE_PREFIXLEN_MIN]);
 
 	if (!tb[FRA_PRIORITY] && ops->default_pref)
 		rule->pref = ops->default_pref(ops);

@@ -523,6 +525,7 @@ static inline size_t fib_rule_nlmsg_size(struct fib_rules_ops *ops,
 			 + nla_total_size(IFNAMSIZ) /* FRA_OIFNAME */
 			 + nla_total_size(4) /* FRA_PRIORITY */
 			 + nla_total_size(4) /* FRA_TABLE */
+			 + nla_total_size(1) /* FRA_TABLE_PREFIXLEN_MIN */
 			 + nla_total_size(4) /* FRA_FWMARK */
 			 + nla_total_size(4); /* FRA_FWMASK */

@@ -548,6 +551,8 @@ static int fib_nl_fill_rule(struct sk_buff *skb, struct fib_rule *rule,
 	frh->table = rule->table;
 	if (nla_put_u32(skb, FRA_TABLE, rule->table))
 		goto nla_put_failure;
+	if (nla_put_u8(skb, FRA_TABLE_PREFIXLEN_MIN, rule->table_prefixlen_min))
+		goto nla_put_failure;
 	frh->res1 = 0;
 	frh->res2 = 0;
 	frh->action = rule->action;

net/ipv4/fib_rules.c +10 -1

index 26aa65d..94e9051 100644
@@ -95,8 +95,17 @@ static int fib4_rule_action(struct fib_rule *rule, struct flowi *flp,
 		goto errout;
 
 	err = fib_table_lookup(tbl, &flp->u.ip4, (struct fib_result *) arg->result, arg->flags);
-	if (err > 0)
+	if (err > 0) {
 		err = -EAGAIN;
+		goto errout;
+	}
+	/* do not accept result if the route does not meet the required prefix length */
+	if (arg->result) {
+		if (((struct fib_result *)arg->result)->prefixlen < rule->table_prefixlen_min) {
+			err = -EAGAIN;
+			goto errout;
+		}
+	}
 errout:
 	return err;
 }

net/ipv6/fib6_rules.c +8 -0

index 2e1a432..d2fa795 100644
@@ -79,6 +79,14 @@ static int fib6_rule_action(struct fib_rule *rule, struct flowi *flp,
 		struct fib6_rule *r = (struct fib6_rule *)rule;
 
 		/*
+		 * do not accept result if the route does
+		 * not meet the required prefix length
+		 */
+		if (rt->rt6i_dst.plen < rule->table_prefixlen_min) {
+			goto again;
+		}
+
+		/*
 		 * If we need to find a source address for this traffic,
 		 * we check the result if it meets requirement of the rule.
 		 */

Signature · 1 line

1.7.10.4

Hannes Frederic Sowa

2013-07-25 18:17 UTC #7 lore raw

In reply to Stefan Tomanek

Quoted 11 lines · Stefan Tomanek

Dies schrieb Hannes Frederic Sowa ([email protected]):
Quoted 4 lines
I would try to factor the prefixlen_min check out into a e.g. fib4_rule_constrain function for which a new field in fib_rules_ops needs to be created as callback. Also it would be nice to have IPv6 support, too. ;)
I was working on my patchset again and considered your suggestion; however I'm not sure whether factoring out the constraints into a separate function is actually that useful, since they are only called from one specific location for each protocol; can you think of another useful application?

I don't have a strong opinion on that but I do find that it does better fit into the design of fib_rules and will be easier to extend in future IMHO.

Quoted 2 lines

I however did like the idea of adding IPv6 support, so I did - I'll post the new patch later on.

Cool, thanks.

Greetings,

Hannes

Stefan Tomanek

author

2013-07-25 18:28 UTC #8 lore raw

Dies schrieb Hannes Frederic Sowa ([email protected]):

Quoted 4 lines

Quoted 2 lines
I however did like the idea of adding IPv6 support, so I did - I'll post the new patch later on.
Cool, thanks.

I've just completed the patch and sent it to the list, and I also placed the changes in my github account, just in case anyone wants to take a quick peek:
https://github.com/wertarbyte/linux/compare/prefixlength

Andrew Collins

2013-07-25 21:47 UTC #9 lore raw

Quoted 15 lines · Stefan Tomanek

Dies schrieb Hannes Frederic Sowa ([email protected]):
Quoted 4 lines
Quoted 2 lines
I however did like the idea of adding IPv6 support, so I did - I'll post the new patch later on.
Cool, thanks.
I've just completed the patch and sent it to the list, and I also placed the changes in my github account, just in case anyone wants to take a quick peek:
https://github.com/wertarbyte/linux/compare/prefixlength
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 1 line

+ if (arg->result) { ...

This NULL check seems unnecessary (fib_table_lookup doesn't check).

I like the idea, but I agree with Hannes that it'd be nice to split this out into a fib constraint callback.

Stefan Tomanek

author

2013-07-25 22:11 UTC #10 lore raw

Dies schrieb Andrew Collins ([email protected]):

Quoted 7 lines

Quoted 5 lines
I've just completed the patch and sent it to the list, and I also placed the changes in my github account, just in case anyone wants to take a quick peek:
https://github.com/wertarbyte/linux/compare/prefixlength
+ if (arg->result) { ...
This NULL check seems unnecessary (fib_table_lookup doesn't check).

I wasn't sure if it was possible for fib_table_lookup to put a NULL pointer there, so I wanted to make sure following it with ->prefixlen was possible.

Quoted 2 lines

I like the idea, but I agree with Hannes that it'd be nice to split this out into a fib constraint callback.

I'm still not convinced that creating such a callback that will never be called from outside fib{4,}_rule_action() is worth the effort, but my insight into the code is still limited; if it's the right thing to do [tm], I won't stand in the way :-)

Stefan Tomanek

author

2013-07-26 10:46 UTC #11 lore raw

PATCH 1/1 fib_rules: add .suppress operation

This change adds a new operation to the fib_rules_ops struct; it allows the suppression of routing decisions if certain criteria are not met by its results.

The first implemented constraint is a minimum prefix length added to the structures of routing rules. If a rule is added with a minimum prefix length

Quoted 1 line

0, only routes meeting this threshold will be considered. Any other (more

general) routing table entries will be ignored.

  $ ip route add table secuplink default via 10.42.23.1

  $ ip rule add pref 100            table main prefixlength 1
  $ ip rule add pref 150 fwmark 0xA table secuplink

Signed-off-by Stefan Tomanek <[email protected]>

---
 include/net/fib_rules.h        |    4 ++++
 include/uapi/linux/fib_rules.h |    2 +-
 net/core/fib_rules.c           |    8 ++++++++
 net/ipv4/fib_rules.c           |    9 +++++++++
 net/ipv6/fib6_rules.c          |   12 ++++++++++++
 5 files changed, 34 insertions(+), 1 deletion(-)

include/net/fib_rules.h +4 -0

index e361f48..bb15664 100644
@@ -18,6 +18,7 @@ struct fib_rule {
 	u32			pref;
 	u32			flags;
 	u32			table;
+	u8			table_prefixlen_min;
 	u8			action;
 	u32			target;
 	struct fib_rule __rcu	*ctarget;

@@ -46,6 +47,8 @@ struct fib_rules_ops {
 	int			(*action)(struct fib_rule *,
 					  struct flowi *, int,
 					  struct fib_lookup_arg *);
+	int			(*suppress)(struct fib_rule *,
+					    struct fib_lookup_arg *);
 	int			(*match)(struct fib_rule *,
 					 struct flowi *, int);
 	int			(*configure)(struct fib_rule *,

@@ -80,6 +83,7 @@ struct fib_rules_ops {
 	[FRA_FWMARK]	= { .type = NLA_U32 }, \
 	[FRA_FWMASK]	= { .type = NLA_U32 }, \
 	[FRA_TABLE]     = { .type = NLA_U32 }, \
+	[FRA_TABLE_PREFIXLEN_MIN] = { .type = NLA_U8 }, \
 	[FRA_GOTO]	= { .type = NLA_U32 }
 
 static inline void fib_rule_get(struct fib_rule *rule)

include/uapi/linux/fib_rules.h +1 -1

index 51da65b..59cd31b 100644
@@ -45,7 +45,7 @@ enum {
 	FRA_FLOW,	/* flow/class id */
 	FRA_UNUSED6,
 	FRA_UNUSED7,
-	FRA_UNUSED8,
+	FRA_TABLE_PREFIXLEN_MIN,
 	FRA_TABLE,	/* Extended table id */
 	FRA_FWMASK,	/* mask for netfilter mark */
 	FRA_OIFNAME,

net/core/fib_rules.c +8 -0

index 2173544..1e0e1f4 100644
@@ -227,6 +227,9 @@ jumped:
 			err = ops->action(rule, fl, flags, arg);
 
 		if (err != -EAGAIN) {
+			if (ops->suppress && ops->suppress(rule, arg)) {
+				continue;
+			}
 			if ((arg->flags & FIB_LOOKUP_NOREF) ||
 			    likely(atomic_inc_not_zero(&rule->refcnt))) {
 				arg->rule = rule;

@@ -337,6 +340,8 @@ static int fib_nl_newrule(struct sk_buff *skb, struct nlmsghdr* nlh)
 	rule->action = frh->action;
 	rule->flags = frh->flags;
 	rule->table = frh_get_table(frh, tb);
+	if (tb[FRA_TABLE_PREFIXLEN_MIN])
+		rule->table_prefixlen_min = nla_get_u8(tb[FRA_TABLE_PREFIXLEN_MIN]);
 
 	if (!tb[FRA_PRIORITY] && ops->default_pref)
 		rule->pref = ops->default_pref(ops);

@@ -523,6 +528,7 @@ static inline size_t fib_rule_nlmsg_size(struct fib_rules_ops *ops,
 			 + nla_total_size(IFNAMSIZ) /* FRA_OIFNAME */
 			 + nla_total_size(4) /* FRA_PRIORITY */
 			 + nla_total_size(4) /* FRA_TABLE */
+			 + nla_total_size(1) /* FRA_TABLE_PREFIXLEN_MIN */
 			 + nla_total_size(4) /* FRA_FWMARK */
 			 + nla_total_size(4); /* FRA_FWMASK */

@@ -548,6 +554,8 @@ static int fib_nl_fill_rule(struct sk_buff *skb, struct fib_rule *rule,
 	frh->table = rule->table;
 	if (nla_put_u32(skb, FRA_TABLE, rule->table))
 		goto nla_put_failure;
+	if (nla_put_u8(skb, FRA_TABLE_PREFIXLEN_MIN, rule->table_prefixlen_min))
+		goto nla_put_failure;
 	frh->res1 = 0;
 	frh->res2 = 0;
 	frh->action = rule->action;

net/ipv4/fib_rules.c +9 -0

index 26aa65d..cb5fcc1 100644
@@ -101,6 +101,14 @@ errout:
 	return err;
 }
 
+static int fib4_rule_suppress(struct fib_rule *rule, struct fib_lookup_arg *arg) {
+	/* do not accept result if the route does not meet the required prefix length */
+	struct fib_result *result = (struct fib_result *) arg->result;
+	if (result->prefixlen < rule->table_prefixlen_min) {
+		return 1;
+	}
+	return 0;
+}
 
 static int fib4_rule_match(struct fib_rule *rule, struct flowi *fl, int flags)
 {

@@ -267,6 +275,7 @@ static const struct fib_rules_ops __net_initconst fib4_rules_ops_template = {
 	.rule_size	= sizeof(struct fib4_rule),
 	.addr_size	= sizeof(u32),
 	.action		= fib4_rule_action,
+	.suppress	= fib4_rule_suppress,
 	.match		= fib4_rule_match,
 	.configure	= fib4_rule_configure,
 	.delete		= fib4_rule_delete,

net/ipv6/fib6_rules.c +12 -0

index 2e1a432..dcba659 100644
@@ -111,6 +111,17 @@ out:
 	return rt == NULL ? -EAGAIN : 0;
 }
 
+static int fib6_rule_suppress(struct fib_rule *rule, struct fib_lookup_arg *arg) {
+	struct rt6_info *rt = (struct rt6_info *) arg->result;
+	/*
+	 * do not accept result if the route does
+	 * not meet the required prefix length
+	 */
+	if (rt->rt6i_dst.plen < rule->table_prefixlen_min) {
+		return 1;
+	}
+	return 0;
+}
 
 static int fib6_rule_match(struct fib_rule *rule, struct flowi *fl, int flags)
 {

@@ -244,6 +255,7 @@ static const struct fib_rules_ops __net_initconst fib6_rules_ops_template = {
 	.addr_size		= sizeof(struct in6_addr),
 	.action			= fib6_rule_action,
 	.match			= fib6_rule_match,
+	.suppress		= fib6_rule_suppress,
 	.configure		= fib6_rule_configure,
 	.compare		= fib6_rule_compare,
 	.fill			= fib6_rule_fill,

Signature · 1 line

1.7.10.4

Hannes Frederic Sowa

2013-07-26 17:05 UTC #12 lore raw

Re: [PATCH] fib_rules: add .suppress operation

Quoted 11 lines · Stefan Tomanek

(quoted diff) +4 -0

 		if (err != -EAGAIN) {
+			if (ops->suppress && ops->suppress(rule, arg)) {
+				continue;
+			}
 			if ((arg->flags & FIB_LOOKUP_NOREF) ||
 			    likely(atomic_inc_not_zero(&rule->refcnt))) {
 				arg->rule = rule;
 
[...]
 
+static int fib6_rule_suppress(struct fib_rule *rule, struct fib_lookup_arg *arg) {

Review

Quoted 10 lines

(quoted diff) +10 -0

+	struct rt6_info *rt = (struct rt6_info *) arg->result;
+	/*
+	 * do not accept result if the route does
+	 * not meet the required prefix length
+	 */
+	if (rt->rt6i_dst.plen < rule->table_prefixlen_min) {
+		return 1;
+	}
+	return 0;
+}

Urks, fib6_rule_action is broken. The switch (rule->action) does update the rt entry but does not signal the correct error code to stop iterating the rules in case it finds a blackhole, prohibit etc. action (it always signals -EAGAIN).

A change in this logic could have impact to your patch as I currently don't know how the null handling of arg->result will turn out. IPv6 does not preinitialize arg->result as IPv4 does.

I am looking for a solution.

Thanks,

Hannes

Hannes Frederic Sowa

2013-07-27 06:08 UTC #13 lore raw

Re: [PATCH] fib_rules: add .suppress operation

Quoted 5 lines · Hannes Frederic Sowa

Review
Quoted 4 lines · Stefan Tomanek
(quoted diff) +3 -0
 		if (err != -EAGAIN) {
+			if (ops->suppress && ops->suppress(rule, arg)) {
+				continue;
+			}

We need to make sure route lookup succeeded:

			if (!err && ops->suppress && ops->suppress(rule, arg))

Quoted 8 lines

(quoted diff) +8 -0

+static int fib6_rule_suppress(struct fib_rule *rule, struct fib_lookup_arg *arg) {
+	struct rt6_info *rt = (struct rt6_info *) arg->result;
+	/*
+	 * do not accept result if the route does
+	 * not meet the required prefix length
+	 */
+	if (rt->rt6i_dst.plen < rule->table_prefixlen_min) {
+		return 1;

Quoted 8 lines

Review
Quoted 3 lines
(quoted diff) +3 -0
+	}
+	return 0;
+}
Urks, fib6_rule_action is broken. The switch (rule->action) does update the rt entry but does not signal the correct error code to stop iterating the rules in case it finds a blackhole, prohibit etc. action (it always signals -EAGAIN).

I have to take back that fib6_rule_action is broken, it just showes some unwanted side effects in this constalation.

Quoted 5 lines

A change in this logic could have impact to your patch as I currently don't know how the null handling of arg->result will turn out. IPv6 does not preinitialize arg->result as IPv4 does.
I am looking for a solution.

This compile-only-tested patch would make the error reporting consistent with its ipv4 counterpart. arg->result should only be NULL iff the function returns -EAGAIN. So we are clean here.

(It seems this patch also fixes a potential nullptr dereference.)

net/ipv6/fib6_rules.c +11 -3

index 2e1a432..4c8bac7 100644
@@ -55,26 +55,33 @@ static int fib6_rule_action(struct fib_rule *rule, struct flowi *flp,
 	struct fib6_table *table;
 	struct net *net = rule->fr_net;
 	pol_lookup_t lookup = arg->lookup_ptr;
+	int err = 0;
 
 	switch (rule->action) {
 	case FR_ACT_TO_TBL:
 		break;
 	case FR_ACT_UNREACHABLE:
+		err = -ENETUNREACH;
 		rt = net->ipv6.ip6_null_entry;
 		goto discard_pkt;
 	default:
 	case FR_ACT_BLACKHOLE:
+		err = -EINVAL;
 		rt = net->ipv6.ip6_blk_hole_entry;
 		goto discard_pkt;
 	case FR_ACT_PROHIBIT:
+		err = -EACCES;
 		rt = net->ipv6.ip6_prohibit_entry;
 		goto discard_pkt;
 	}
 
 	table = fib6_get_table(net, rule->table);
-	if (table)
-		rt = lookup(net, table, flp6, flags);
+	if (!table) {
+		err = -EAGAIN;
+		goto out;
+	}
 
+	rt = lookup(net, table, flp6, flags);
 	if (rt != net->ipv6.ip6_null_entry) {
 		struct fib6_rule *r = (struct fib6_rule *)rule;

@@ -101,6 +108,7 @@ static int fib6_rule_action(struct fib_rule *rule, struct flowi *flp,
 	}
 again:
 	ip6_rt_put(rt);
+	err = -EAGAIN;
 	rt = NULL;
 	goto out;

@@ -108,7 +116,7 @@ discard_pkt:
 	dst_hold(&rt->dst);
 out:
 	arg->result = rt;
-	return rt == NULL ? -EAGAIN : 0;
+	return err;
 }

Hannes Frederic Sowa

2013-07-27 06:26 UTC #14 lore raw

Re: [PATCH] fib_rules: add .suppress operation

In reply to Stefan Tomanek

Oh, I overlooked something.

Quoted 7 lines · Stefan Tomanek

net/ipv6/fib6_rules.c +1 -0

@@ -111,6 +111,17 @@ out:
 	return rt == NULL ? -EAGAIN : 0;
 }
 
+static int fib6_rule_suppress(struct fib_rule *rule, struct fib_lookup_arg *arg) {

net/ipv6/fib6_rules.c out: · line 124

Quoted 10 lines

net/ipv6/fib6_rules.c +10 -0

+	struct rt6_info *rt = (struct rt6_info *) arg->result;
+	/*
+	 * do not accept result if the route does
+	 * not meet the required prefix length
+	 */
+	if (rt->rt6i_dst.plen < rule->table_prefixlen_min) {
+		return 1;
+	}
+	return 0;
+}

In case of IPv6 (we don't clone the result but merely return a reference to the rt6_info), we also need to decrement the reference count if we suppress the route. A ip6_rt_put should do the trick.

Greetings,

Hannes

Hannes Frederic Sowa

2013-07-27 07:07 UTC #15 lore raw

Re: [PATCH] fib_rules: add .suppress operation

In reply to Stefan Tomanek

Hm, IPv4 actually seems to have the same problem:

net/ipv4/fib_rules.c errout: · line 111

Quoted 8 lines · Stefan Tomanek

net/ipv4/fib_rules.c +8 -0

+static int fib4_rule_suppress(struct fib_rule *rule, struct fib_lookup_arg *arg) {
+	/* do not accept result if the route does not meet the required prefix length */
+	struct fib_result *result = (struct fib_result *) arg->result;
+	if (result->prefixlen < rule->table_prefixlen_min) {
+		return 1;
+	}
+	return 0;
+}

In case of suppressing the route, we need to decrement the reference counter of fib_info:

if (!(fib_flags & FIB_LOOKUP_NOREF))

	fib_info_put(result->fi);

Also:

Quoted 12 lines

index e361f48..bb15664 100644

include/net/fib_rules.h +1 -0

@@ -18,6 +18,7 @@ struct fib_rule {
        u32                     pref;
        u32                     flags;
        u32                     table;
+       u8                      table_prefixlen_min;
        u8                      action;
        u32                     target;
        struct fib_rule __rcu   *ctarget;

include/net/fib_rules.h struct fib_rules_ops { · line 51

Quoted 5 lines

include/net/fib_rules.h +2 -0

@@ -46,6 +47,8 @@ struct fib_rules_ops {   
        int                     (*action)(struct fib_rule *,
                                          struct flowi *, int,
                                          struct fib_lookup_arg *);
+       int                     (*suppress)(struct fib_rule *,
+                                           struct fib_lookup_arg *);

bool and true/false would be nicer.

include/net/fib_rules.h struct fib_rules_ops { · line 54

Quoted 4 lines

include/net/fib_rules.h +0 -0

        int                     (*match)(struct fib_rule *,
                                         struct flowi *, int);
        int                     (*configure)(struct fib_rule *,

Greetings,

Hannes

Stefan Tomanek

author

2013-07-27 10:21 UTC #16 lore raw

Re: [PATCH] fib_rules: add .suppress operation

Thanks for your input, I've put together all the proposed patches in my github branch:

https://github.com/wertarbyte/linux/compare/fib_rule_suppress

Any more ideas, before I squash it all together?

Hannes Frederic Sowa

2013-07-27 15:10 UTC #17 lore raw

Re: [PATCH] fib_rules: add .suppress operation

Quoted 6 lines · Stefan Tomanek

Thanks for your input, I've put together all the proposed patches in my github branch:
https://github.com/wertarbyte/linux/compare/fib_rule_suppress
Any more ideas, before I squash it all together?

This should fix the compile error (was a c&p error from me in the previous mail).

- if (!(fib_flags & FIB_LOOKUP_NOREF))
+ if (!(arg->flags & FIB_LOOKUP_NOREF))

This should get a considerable amount of stress testing now. ;)

Greetings,

Hannes

Stefan Tomanek

author

2013-07-28 10:36 UTC #18 lore raw

Re: [PATCH] fib_rules: add .suppress operation

Dies schrieb Hannes Frederic Sowa ([email protected]):

Quoted 7 lines

This should fix the compile error (was a c&p error from me in the previous mail).
(quoted diff) +1 -1
-               if (!(fib_flags & FIB_LOOKUP_NOREF))
+               if (!(arg->flags & FIB_LOOKUP_NOREF))
This should get a considerable amount of stress testing now. ;)

I amended that change and made a forced push, so now the branch should be up to date and in working condition:
https://github.com/wertarbyte/linux/compare/fib_rule_suppress

Stefan Tomanek

author

2013-07-30 06:53 UTC #19 lore raw

PATCH v2 1/2 fib6_rules: make error handling consistent with IPv4

In reply to Hannes Frederic Sowa

(by Hannes Frederic Sowa <[email protected]>)

This compile-only-tested patch would make the error reporting consistent with its ipv4 counterpart. arg->result should only be NULL iff the function returns -EAGAIN. So we are clean here.

(It seems this patch also fixes a potential nullptr dereference.)

Signed-off-by Stefan Tomanek <[email protected]>

---
 net/ipv6/fib6_rules.c |   14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

net/ipv6/fib6_rules.c +11 -3

index 2e1a432..4c8bac7 100644
@@ -55,26 +55,33 @@ static int fib6_rule_action(struct fib_rule *rule, struct flowi *flp,
 	struct fib6_table *table;
 	struct net *net = rule->fr_net;
 	pol_lookup_t lookup = arg->lookup_ptr;
+	int err = 0;
 
 	switch (rule->action) {
 	case FR_ACT_TO_TBL:
 		break;
 	case FR_ACT_UNREACHABLE:
+		err = -ENETUNREACH;
 		rt = net->ipv6.ip6_null_entry;
 		goto discard_pkt;
 	default:
 	case FR_ACT_BLACKHOLE:
+		err = -EINVAL;
 		rt = net->ipv6.ip6_blk_hole_entry;
 		goto discard_pkt;
 	case FR_ACT_PROHIBIT:
+		err = -EACCES;
 		rt = net->ipv6.ip6_prohibit_entry;
 		goto discard_pkt;
 	}
 
 	table = fib6_get_table(net, rule->table);
-	if (table)
-		rt = lookup(net, table, flp6, flags);
+	if (!table) {
+		err = -EAGAIN;
+		goto out;
+	}
 
+	rt = lookup(net, table, flp6, flags);
 	if (rt != net->ipv6.ip6_null_entry) {
 		struct fib6_rule *r = (struct fib6_rule *)rule;

@@ -101,6 +108,7 @@ static int fib6_rule_action(struct fib_rule *rule, struct flowi *flp,
 	}
 again:
 	ip6_rt_put(rt);
+	err = -EAGAIN;
 	rt = NULL;
 	goto out;

@@ -108,7 +116,7 @@ discard_pkt:
 	dst_hold(&rt->dst);
 out:
 	arg->result = rt;
-	return rt == NULL ? -EAGAIN : 0;
+	return err;
 }

Signature · 1 line

1.7.10.4

Stefan Tomanek

author

2013-07-30 06:53 UTC #20 lore raw

PATCH v2 2/2 fib_rules: add .suppress operation

In reply to Hannes Frederic Sowa

This change adds a new operation to the fib_rules_ops struct; it allows the suppression of routing decisions if certain criteria are not met by its results.

The first implemented constraint is a minimum prefix length added to the structures of routing rules. If a rule is added with a minimum prefix length

Quoted 1 line

0, only routes meeting this threshold will be considered. Any other (more

general) routing table entries will be ignored.

  $ ip route add table secuplink default via 10.42.23.1

  $ ip rule add pref 100            table main prefixlength 1
  $ ip rule add pref 150 fwmark 0xA table secuplink

Signed-off-by Stefan Tomanek <[email protected]>

---
 include/net/fib_rules.h        |    4 ++++
 include/uapi/linux/fib_rules.h |    2 +-
 net/core/fib_rules.c           |    9 +++++++++
 net/ipv4/fib_rules.c           |   12 ++++++++++++
 net/ipv6/fib6_rules.c          |   13 +++++++++++++
 5 files changed, 39 insertions(+), 1 deletion(-)

include/net/fib_rules.h +4 -0

index e361f48..2f286dc 100644
@@ -18,6 +18,7 @@ struct fib_rule {
 	u32			pref;
 	u32			flags;
 	u32			table;
+	u8			table_prefixlen_min;
 	u8			action;
 	u32			target;
 	struct fib_rule __rcu	*ctarget;

@@ -46,6 +47,8 @@ struct fib_rules_ops {
 	int			(*action)(struct fib_rule *,
 					  struct flowi *, int,
 					  struct fib_lookup_arg *);
+	bool			(*suppress)(struct fib_rule *,
+					    struct fib_lookup_arg *);
 	int			(*match)(struct fib_rule *,
 					 struct flowi *, int);
 	int			(*configure)(struct fib_rule *,

@@ -80,6 +83,7 @@ struct fib_rules_ops {
 	[FRA_FWMARK]	= { .type = NLA_U32 }, \
 	[FRA_FWMASK]	= { .type = NLA_U32 }, \
 	[FRA_TABLE]     = { .type = NLA_U32 }, \
+	[FRA_TABLE_PREFIXLEN_MIN] = { .type = NLA_U8 }, \
 	[FRA_GOTO]	= { .type = NLA_U32 }
 
 static inline void fib_rule_get(struct fib_rule *rule)

include/uapi/linux/fib_rules.h +1 -1

index 51da65b..59cd31b 100644
@@ -45,7 +45,7 @@ enum {
 	FRA_FLOW,	/* flow/class id */
 	FRA_UNUSED6,
 	FRA_UNUSED7,
-	FRA_UNUSED8,
+	FRA_TABLE_PREFIXLEN_MIN,
 	FRA_TABLE,	/* Extended table id */
 	FRA_FWMASK,	/* mask for netfilter mark */
 	FRA_OIFNAME,

net/core/fib_rules.c +9 -0

index 2173544..809efd5 100644
@@ -226,6 +226,10 @@ jumped:
 		else
 			err = ops->action(rule, fl, flags, arg);
 
+		if (!err && ops->suppress && ops->suppress(rule, arg)) {
+			continue;
+		}
+
 		if (err != -EAGAIN) {
 			if ((arg->flags & FIB_LOOKUP_NOREF) ||
 			    likely(atomic_inc_not_zero(&rule->refcnt))) {

@@ -337,6 +341,8 @@ static int fib_nl_newrule(struct sk_buff *skb, struct nlmsghdr* nlh)
 	rule->action = frh->action;
 	rule->flags = frh->flags;
 	rule->table = frh_get_table(frh, tb);
+	if (tb[FRA_TABLE_PREFIXLEN_MIN])
+		rule->table_prefixlen_min = nla_get_u8(tb[FRA_TABLE_PREFIXLEN_MIN]);
 
 	if (!tb[FRA_PRIORITY] && ops->default_pref)
 		rule->pref = ops->default_pref(ops);

@@ -523,6 +529,7 @@ static inline size_t fib_rule_nlmsg_size(struct fib_rules_ops *ops,
 			 + nla_total_size(IFNAMSIZ) /* FRA_OIFNAME */
 			 + nla_total_size(4) /* FRA_PRIORITY */
 			 + nla_total_size(4) /* FRA_TABLE */
+			 + nla_total_size(1) /* FRA_TABLE_PREFIXLEN_MIN */
 			 + nla_total_size(4) /* FRA_FWMARK */
 			 + nla_total_size(4); /* FRA_FWMASK */

@@ -548,6 +555,8 @@ static int fib_nl_fill_rule(struct sk_buff *skb, struct fib_rule *rule,
 	frh->table = rule->table;
 	if (nla_put_u32(skb, FRA_TABLE, rule->table))
 		goto nla_put_failure;
+	if (nla_put_u8(skb, FRA_TABLE_PREFIXLEN_MIN, rule->table_prefixlen_min))
+		goto nla_put_failure;
 	frh->res1 = 0;
 	frh->res2 = 0;
 	frh->action = rule->action;

net/ipv4/fib_rules.c +12 -0

index 26aa65d..f93beb6 100644
@@ -101,6 +101,17 @@ errout:
 	return err;
 }
 
+static bool fib4_rule_suppress(struct fib_rule *rule, struct fib_lookup_arg *arg) {
+	/* do not accept result if the route does not meet the required prefix length */
+	struct fib_result *result = (struct fib_result *) arg->result;
+	if (result->prefixlen < rule->table_prefixlen_min) {
+		if (!(arg->flags & FIB_LOOKUP_NOREF)) {
+			fib_info_put(result->fi);
+		}
+		return true;
+	}
+	return false;
+}
 
 static int fib4_rule_match(struct fib_rule *rule, struct flowi *fl, int flags)
 {

@@ -267,6 +278,7 @@ static const struct fib_rules_ops __net_initconst fib4_rules_ops_template = {
 	.rule_size	= sizeof(struct fib4_rule),
 	.addr_size	= sizeof(u32),
 	.action		= fib4_rule_action,
+	.suppress	= fib4_rule_suppress,
 	.match		= fib4_rule_match,
 	.configure	= fib4_rule_configure,
 	.delete		= fib4_rule_delete,

net/ipv6/fib6_rules.c +13 -0

index 4c8bac7..63372ab 100644
@@ -119,6 +119,18 @@ out:
 	return err;
 }
 
+static bool fib6_rule_suppress(struct fib_rule *rule, struct fib_lookup_arg *arg) {
+	struct rt6_info *rt = (struct rt6_info *) arg->result;
+	/*
+	 * do not accept result if the route does
+	 * not meet the required prefix length
+	 */
+	if (rt->rt6i_dst.plen < rule->table_prefixlen_min) {
+		ip6_rt_put(rt);
+		return true;
+	}
+	return false;
+}
 
 static int fib6_rule_match(struct fib_rule *rule, struct flowi *fl, int flags)
 {

@@ -252,6 +264,7 @@ static const struct fib_rules_ops __net_initconst fib6_rules_ops_template = {
 	.addr_size		= sizeof(struct in6_addr),
 	.action			= fib6_rule_action,
 	.match			= fib6_rule_match,
+	.suppress		= fib6_rule_suppress,
 	.configure		= fib6_rule_configure,
 	.compare		= fib6_rule_compare,
 	.fill			= fib6_rule_fill,

Signature · 1 line

1.7.10.4

David Miller

maintainer

2013-07-30 07:03 UTC #21 lore raw

Re: [PATCH v2 2/2] fib_rules: add .suppress operation

From: Stefan Tomanek <[email protected]>
Date: Tue, 30 Jul 2013 08:53:17 +0200

Quoted 5 lines

net/ipv4/fib_rules.c +1 -0

@@ -101,6 +101,17 @@ errout:
 	return err;
 }
 
+static bool fib4_rule_suppress(struct fib_rule *rule, struct fib_lookup_arg *arg) {

...

net/ipv6/fib6_rules.c out: · line 127

Quoted 10 lines

net/ipv6/fib6_rules.c +6 -0

@@ -119,6 +119,18 @@ out:
 	return err;
 }
 
+static bool fib6_rule_suppress(struct fib_rule *rule, struct fib_lookup_arg *arg) {
+	struct rt6_info *rt = (struct rt6_info *) arg->result;
+	/*
+	 * do not accept result if the route does
+	 * not meet the required prefix length
+	 */

Please format this stuff correctly.

Functions are declared like this:

static type NAME(TYPE ARG1, TYPE ARG2)
{
}

You don't put the openning curly brace at the end of the line with the closing parenthesis of the argument list.

Next, comments are to be formatted:

	/* Like
	 * this.
	 */

Thanks.

Stefan Tomanek

author

2013-07-30 07:23 UTC #22 lore raw

Re: [PATCH v2 2/2] fib_rules: add .suppress operation

Dies schrieb David Miller ([email protected]):

Quoted 15 lines

Quoted 1 line
+static bool fib4_rule_suppress(struct fib_rule *rule, struct fib_lookup_arg *arg) {
...
net/ipv6/fib6_rules.c out: · line 127
Quoted 10 lines
net/ipv6/fib6_rules.c +6 -0
@@ -119,6 +119,18 @@ out:
 	return err;
 }
 
+static bool fib6_rule_suppress(struct fib_rule *rule, struct fib_lookup_arg *arg) {
+	struct rt6_info *rt = (struct rt6_info *) arg->result;
+	/*
+	 * do not accept result if the route does
+	 * not meet the required prefix length
+	 */
Please format this stuff correctly.

Quoted 5 lines

Functions are declared like this:
static type NAME(TYPE ARG1, TYPE ARG2)
{
}

Noted and fixed.

Quoted 8 lines

You don't put the openning curly brace at the end of the line with the closing parenthesis of the argument list.
Next, comments are to be formatted:
	/* Like
	 * this.
	 */

Well, to be fair, most comments in the existing code base actually use the style I employed (in fact, I was copying it), so it seems like a blurry line at least.

/*
* If we need to find a source address for this traffic,
* we check the result if it meets requirement of the rule. */
-----8<----
/*
* If FIB_RULE_FIND_SADDR is set and we do not have a
* source address for the traffic, we defer check for
* source address.
*/
-----8<----
/* The lock is not required here, the list in unreacheable
* at the moment this function is called */
-----8<----

                        /* compatibility: if the mark value is non-zero all bits
                         * are compared unless a mask is explicitly specified.
                         */

-----8<----
/*
* There are unresolved goto rules in the list, check if
* any of them are pointing to this new rule. */
-----8<----
/*
* Check if this rule is a target to any of them. If so,
* disable them. As this operation is eventually very
* expensive, it is only performed if goto rules have
* actually been added.
*/
-----8<----

David Miller

maintainer

2013-07-30 07:34 UTC #23 lore raw

Re: [PATCH v2 2/2] fib_rules: add .suppress operation

From: Stefan Tomanek <[email protected]>
Date: Tue, 30 Jul 2013 09:23:05 +0200

Quoted 2 lines

Well, to be fair, most comments in the existing code base actually use the style I employed (in fact, I was copying it), so it seems like a blurry line at least.

Existing mistakes are not excuses for duplicating them :-)

They are a good excuse to send a patch fixing them, however.

Stefan Tomanek

author

2013-07-30 07:46 UTC #24 lore raw

PATCH v3 1/1 fib_rules: add .suppress operation

In reply to Hannes Frederic Sowa

This change adds a new operation to the fib_rules_ops struct; it allows the suppression of routing decisions if certain criteria are not met by its results.

The first implemented constraint is a minimum prefix length added to the structures of routing rules. If a rule is added with a minimum prefix length

Quoted 1 line

0, only routes meeting this threshold will be considered. Any other (more

general) routing table entries will be ignored.

  $ ip route add table secuplink default via 10.42.23.1

  $ ip rule add pref 100            table main prefixlength 1
  $ ip rule add pref 150 fwmark 0xA table secuplink

Signed-off-by Stefan Tomanek <[email protected]>

---
 include/net/fib_rules.h        |    4 ++++
 include/uapi/linux/fib_rules.h |    2 +-
 net/core/fib_rules.c           |    9 +++++++++
 net/ipv4/fib_rules.c           |   15 +++++++++++++++
 net/ipv6/fib6_rules.c          |   13 +++++++++++++
 5 files changed, 42 insertions(+), 1 deletion(-)

include/net/fib_rules.h +4 -0

index e361f48..2f286dc 100644
@@ -18,6 +18,7 @@ struct fib_rule {
 	u32			pref;
 	u32			flags;
 	u32			table;
+	u8			table_prefixlen_min;
 	u8			action;
 	u32			target;
 	struct fib_rule __rcu	*ctarget;

@@ -46,6 +47,8 @@ struct fib_rules_ops {
 	int			(*action)(struct fib_rule *,
 					  struct flowi *, int,
 					  struct fib_lookup_arg *);
+	bool			(*suppress)(struct fib_rule *,
+					    struct fib_lookup_arg *);
 	int			(*match)(struct fib_rule *,
 					 struct flowi *, int);
 	int			(*configure)(struct fib_rule *,

@@ -80,6 +83,7 @@ struct fib_rules_ops {
 	[FRA_FWMARK]	= { .type = NLA_U32 }, \
 	[FRA_FWMASK]	= { .type = NLA_U32 }, \
 	[FRA_TABLE]     = { .type = NLA_U32 }, \
+	[FRA_TABLE_PREFIXLEN_MIN] = { .type = NLA_U8 }, \
 	[FRA_GOTO]	= { .type = NLA_U32 }
 
 static inline void fib_rule_get(struct fib_rule *rule)

include/uapi/linux/fib_rules.h +1 -1

index 51da65b..59cd31b 100644
@@ -45,7 +45,7 @@ enum {
 	FRA_FLOW,	/* flow/class id */
 	FRA_UNUSED6,
 	FRA_UNUSED7,
-	FRA_UNUSED8,
+	FRA_TABLE_PREFIXLEN_MIN,
 	FRA_TABLE,	/* Extended table id */
 	FRA_FWMASK,	/* mask for netfilter mark */
 	FRA_OIFNAME,

net/core/fib_rules.c +9 -0

index 2173544..809efd5 100644
@@ -226,6 +226,10 @@ jumped:
 		else
 			err = ops->action(rule, fl, flags, arg);
 
+		if (!err && ops->suppress && ops->suppress(rule, arg)) {
+			continue;
+		}
+
 		if (err != -EAGAIN) {
 			if ((arg->flags & FIB_LOOKUP_NOREF) ||
 			    likely(atomic_inc_not_zero(&rule->refcnt))) {

@@ -337,6 +341,8 @@ static int fib_nl_newrule(struct sk_buff *skb, struct nlmsghdr* nlh)
 	rule->action = frh->action;
 	rule->flags = frh->flags;
 	rule->table = frh_get_table(frh, tb);
+	if (tb[FRA_TABLE_PREFIXLEN_MIN])
+		rule->table_prefixlen_min = nla_get_u8(tb[FRA_TABLE_PREFIXLEN_MIN]);
 
 	if (!tb[FRA_PRIORITY] && ops->default_pref)
 		rule->pref = ops->default_pref(ops);

@@ -523,6 +529,7 @@ static inline size_t fib_rule_nlmsg_size(struct fib_rules_ops *ops,
 			 + nla_total_size(IFNAMSIZ) /* FRA_OIFNAME */
 			 + nla_total_size(4) /* FRA_PRIORITY */
 			 + nla_total_size(4) /* FRA_TABLE */
+			 + nla_total_size(1) /* FRA_TABLE_PREFIXLEN_MIN */
 			 + nla_total_size(4) /* FRA_FWMARK */
 			 + nla_total_size(4); /* FRA_FWMASK */

@@ -548,6 +555,8 @@ static int fib_nl_fill_rule(struct sk_buff *skb, struct fib_rule *rule,
 	frh->table = rule->table;
 	if (nla_put_u32(skb, FRA_TABLE, rule->table))
 		goto nla_put_failure;
+	if (nla_put_u8(skb, FRA_TABLE_PREFIXLEN_MIN, rule->table_prefixlen_min))
+		goto nla_put_failure;
 	frh->res1 = 0;
 	frh->res2 = 0;
 	frh->action = rule->action;

net/ipv4/fib_rules.c +15 -0

index 26aa65d..0e3df33 100644
@@ -101,6 +101,20 @@ errout:
 	return err;
 }
 
+static bool fib4_rule_suppress(struct fib_rule *rule, struct fib_lookup_arg *arg)
+{
+	/* do not accept result if the route does
+	 * not meet the required prefix length
+	 */
+	struct fib_result *result = (struct fib_result *) arg->result;
+	if (result->prefixlen < rule->table_prefixlen_min) {
+		if (!(arg->flags & FIB_LOOKUP_NOREF)) {
+			fib_info_put(result->fi);
+		}
+		return true;
+	}
+	return false;
+}
 
 static int fib4_rule_match(struct fib_rule *rule, struct flowi *fl, int flags)
 {

@@ -267,6 +281,7 @@ static const struct fib_rules_ops __net_initconst fib4_rules_ops_template = {
 	.rule_size	= sizeof(struct fib4_rule),
 	.addr_size	= sizeof(u32),
 	.action		= fib4_rule_action,
+	.suppress	= fib4_rule_suppress,
 	.match		= fib4_rule_match,
 	.configure	= fib4_rule_configure,
 	.delete		= fib4_rule_delete,

net/ipv6/fib6_rules.c +13 -0

index 4c8bac7..554a4fb 100644
@@ -119,6 +119,18 @@ out:
 	return err;
 }
 
+static bool fib6_rule_suppress(struct fib_rule *rule, struct fib_lookup_arg *arg)
+{
+	struct rt6_info *rt = (struct rt6_info *) arg->result;
+	/* do not accept result if the route does
+	 * not meet the required prefix length
+	 */
+	if (rt->rt6i_dst.plen < rule->table_prefixlen_min) {
+		ip6_rt_put(rt);
+		return true;
+	}
+	return false;
+}
 
 static int fib6_rule_match(struct fib_rule *rule, struct flowi *fl, int flags)
 {

@@ -252,6 +264,7 @@ static const struct fib_rules_ops __net_initconst fib6_rules_ops_template = {
 	.addr_size		= sizeof(struct in6_addr),
 	.action			= fib6_rule_action,
 	.match			= fib6_rule_match,
+	.suppress		= fib6_rule_suppress,
 	.configure		= fib6_rule_configure,
 	.compare		= fib6_rule_compare,
 	.fill			= fib6_rule_fill,

Signature · 1 line

1.7.10.4

David Miller

maintainer

2013-07-31 22:13 UTC #25 lore raw

Re: [PATCH v3] fib_rules: add .suppress operation

From: Stefan Tomanek <[email protected]>
Date: Tue, 30 Jul 2013 09:46:36 +0200

Quoted 19 lines

This change adds a new operation to the fib_rules_ops struct; it allows the suppression of routing decisions if certain criteria are not met by its results.
The first implemented constraint is a minimum prefix length added to the structures of routing rules. If a rule is added with a minimum prefix length
Quoted 1 line
0, only routes meeting this threshold will be considered. Any other (more
general) routing table entries will be ignored.
When configuring a system with multiple network uplinks and default routes, it is often convinient to reference the main routing table multiple times - but omitting the default route. Using this patch and a modified "ip" utility, this can be achieved by using the following command sequence:
  $ ip route add table secuplink default via 10.42.23.1
  $ ip rule add pref 100            table main prefixlength 1
  $ ip rule add pref 150 fwmark 0xA table secuplink

Quoted 7 lines

With this setup, packets marked 0xA will be processed by the additional routing table "secuplink", but only if no suitable route in the main routing table can be found. By using a minimal prefixlength of 1, the default route (/0) of the table "main" is hidden to packets processed by rule 100; packets traveling to destinations with more specific routing entries are processed as usual.
Signed-off-by Stefan Tomanek <[email protected]>

I just want to mention that the more quirky crap we put into the FIB rules layer, the harder it will every be to make a scalable data structure for FIB rule handling.

Right now it's basically a linear walk of rules, with processing at each level.

And every single ipv4 route lookup is going to go through this entire process.

Anyways, there are coding style problems in your change which you'll need to address:

net/core/fib_rules.c jumped: · line 231

Quoted 3 lines

net/core/fib_rules.c +3 -0

+		if (!err && ops->suppress && ops->suppress(rule, arg)) {
+			continue;
+		}

Since statement basic blocks should not use curly braces.

net/ipv4/fib_rules.c errout: · line 113

Quoted 3 lines

net/ipv4/fib_rules.c +3 -0

+		if (!(arg->flags & FIB_LOOKUP_NOREF)) {
+			fib_info_put(result->fi);
+		}

Likewise.

Stefan Tomanek

author

2013-08-01 00:24 UTC #26 lore raw

Re: [PATCH v3] fib_rules: add .suppress operation

Dies schrieb David Miller ([email protected]):

Quoted 6 lines

I just want to mention that the more quirky crap we put into the FIB rules layer, the harder it will every be to make a scalable data structure for FIB rule handling.
Right now it's basically a linear walk of rules, with processing at each level.

And it still is: but instead of just having pre-conditions whether to consult a table, the patch introduces post-conditions that can reject a routing decision retrieved from it.

Quoted 2 lines

Anyways, there are coding style problems in your change which you'll need to address:

Fixed in latest patch, thanks for the hints.

David Miller

maintainer

2013-08-01 00:26 UTC #27 lore raw

Re: [PATCH v3] fib_rules: add .suppress operation

From: Stefan Tomanek <[email protected]>
Date: Thu, 1 Aug 2013 02:24:07 +0200

Quoted 6 lines

Dies schrieb David Miller ([email protected]):
Quoted 4 lines
I just want to mention that the more quirky crap we put into the FIB rules layer, the harder it will every be to make a scalable data structure for FIB rule handling.

Quoted 6 lines

Quoted 2 lines
Right now it's basically a linear walk of rules, with processing at each level.
And it still is: but instead of just having pre-conditions whether to consult a table, the patch introduces post-conditions that can reject a routing decision retrieved from it.

This doesn't change my argument at all. The fact remains that the more complex conditions we add to the fib rule lookup, the harder it will be to optimize fib rule lookups into an O(1) or O(log n) operation.

Stefan Tomanek

author

2013-08-01 00:17 UTC #28 lore raw

PATCH v4 1/1 fib_rules: add .suppress operation

In reply to Hannes Frederic Sowa

This change adds a new operation to the fib_rules_ops struct; it allows the suppression of routing decisions if certain criteria are not met by its results.

The first implemented constraint is a minimum prefix length added to the structures of routing rules. If a rule is added with a minimum prefix length

Quoted 1 line

0, only routes meeting this threshold will be considered. Any other (more

general) routing table entries will be ignored.

  $ ip route add table secuplink default via 10.42.23.1

  $ ip rule add pref 100            table main prefixlength 1
  $ ip rule add pref 150 fwmark 0xA table secuplink

Signed-off-by Stefan Tomanek <[email protected]>

---
 include/net/fib_rules.h        |    4 ++++
 include/uapi/linux/fib_rules.h |    2 +-
 net/core/fib_rules.c           |    8 ++++++++
 net/ipv4/fib_rules.c           |   14 ++++++++++++++
 net/ipv6/fib6_rules.c          |   13 +++++++++++++
 5 files changed, 40 insertions(+), 1 deletion(-)

include/net/fib_rules.h +4 -0

index e361f48..2f286dc 100644
@@ -18,6 +18,7 @@ struct fib_rule {
 	u32			pref;
 	u32			flags;
 	u32			table;
+	u8			table_prefixlen_min;
 	u8			action;
 	u32			target;
 	struct fib_rule __rcu	*ctarget;

@@ -46,6 +47,8 @@ struct fib_rules_ops {
 	int			(*action)(struct fib_rule *,
 					  struct flowi *, int,
 					  struct fib_lookup_arg *);
+	bool			(*suppress)(struct fib_rule *,
+					    struct fib_lookup_arg *);
 	int			(*match)(struct fib_rule *,
 					 struct flowi *, int);
 	int			(*configure)(struct fib_rule *,

@@ -80,6 +83,7 @@ struct fib_rules_ops {
 	[FRA_FWMARK]	= { .type = NLA_U32 }, \
 	[FRA_FWMASK]	= { .type = NLA_U32 }, \
 	[FRA_TABLE]     = { .type = NLA_U32 }, \
+	[FRA_TABLE_PREFIXLEN_MIN] = { .type = NLA_U8 }, \
 	[FRA_GOTO]	= { .type = NLA_U32 }
 
 static inline void fib_rule_get(struct fib_rule *rule)

include/uapi/linux/fib_rules.h +1 -1

index 51da65b..59cd31b 100644
@@ -45,7 +45,7 @@ enum {
 	FRA_FLOW,	/* flow/class id */
 	FRA_UNUSED6,
 	FRA_UNUSED7,
-	FRA_UNUSED8,
+	FRA_TABLE_PREFIXLEN_MIN,
 	FRA_TABLE,	/* Extended table id */
 	FRA_FWMASK,	/* mask for netfilter mark */
 	FRA_OIFNAME,

net/core/fib_rules.c +8 -0

index 2173544..2ef5040 100644
@@ -226,6 +226,9 @@ jumped:
 		else
 			err = ops->action(rule, fl, flags, arg);
 
+		if (!err && ops->suppress && ops->suppress(rule, arg))
+			continue;
+
 		if (err != -EAGAIN) {
 			if ((arg->flags & FIB_LOOKUP_NOREF) ||
 			    likely(atomic_inc_not_zero(&rule->refcnt))) {

@@ -337,6 +340,8 @@ static int fib_nl_newrule(struct sk_buff *skb, struct nlmsghdr* nlh)
 	rule->action = frh->action;
 	rule->flags = frh->flags;
 	rule->table = frh_get_table(frh, tb);
+	if (tb[FRA_TABLE_PREFIXLEN_MIN])
+		rule->table_prefixlen_min = nla_get_u8(tb[FRA_TABLE_PREFIXLEN_MIN]);
 
 	if (!tb[FRA_PRIORITY] && ops->default_pref)
 		rule->pref = ops->default_pref(ops);

@@ -523,6 +528,7 @@ static inline size_t fib_rule_nlmsg_size(struct fib_rules_ops *ops,
 			 + nla_total_size(IFNAMSIZ) /* FRA_OIFNAME */
 			 + nla_total_size(4) /* FRA_PRIORITY */
 			 + nla_total_size(4) /* FRA_TABLE */
+			 + nla_total_size(1) /* FRA_TABLE_PREFIXLEN_MIN */
 			 + nla_total_size(4) /* FRA_FWMARK */
 			 + nla_total_size(4); /* FRA_FWMASK */

@@ -548,6 +554,8 @@ static int fib_nl_fill_rule(struct sk_buff *skb, struct fib_rule *rule,
 	frh->table = rule->table;
 	if (nla_put_u32(skb, FRA_TABLE, rule->table))
 		goto nla_put_failure;
+	if (nla_put_u8(skb, FRA_TABLE_PREFIXLEN_MIN, rule->table_prefixlen_min))
+		goto nla_put_failure;
 	frh->res1 = 0;
 	frh->res2 = 0;
 	frh->action = rule->action;

net/ipv4/fib_rules.c +14 -0

index 26aa65d..9f29066 100644
@@ -101,6 +101,19 @@ errout:
 	return err;
 }
 
+static bool fib4_rule_suppress(struct fib_rule *rule, struct fib_lookup_arg *arg)
+{
+	/* do not accept result if the route does
+	 * not meet the required prefix length
+	 */
+	struct fib_result *result = (struct fib_result *) arg->result;
+	if (result->prefixlen < rule->table_prefixlen_min) {
+		if (!(arg->flags & FIB_LOOKUP_NOREF))
+			fib_info_put(result->fi);
+		return true;
+	}
+	return false;
+}
 
 static int fib4_rule_match(struct fib_rule *rule, struct flowi *fl, int flags)
 {

@@ -267,6 +280,7 @@ static const struct fib_rules_ops __net_initconst fib4_rules_ops_template = {
 	.rule_size	= sizeof(struct fib4_rule),
 	.addr_size	= sizeof(u32),
 	.action		= fib4_rule_action,
+	.suppress	= fib4_rule_suppress,
 	.match		= fib4_rule_match,
 	.configure	= fib4_rule_configure,
 	.delete		= fib4_rule_delete,

net/ipv6/fib6_rules.c +13 -0

index 4c8bac7..554a4fb 100644
@@ -119,6 +119,18 @@ out:
 	return err;
 }
 
+static bool fib6_rule_suppress(struct fib_rule *rule, struct fib_lookup_arg *arg)
+{
+	struct rt6_info *rt = (struct rt6_info *) arg->result;
+	/* do not accept result if the route does
+	 * not meet the required prefix length
+	 */
+	if (rt->rt6i_dst.plen < rule->table_prefixlen_min) {
+		ip6_rt_put(rt);
+		return true;
+	}
+	return false;
+}
 
 static int fib6_rule_match(struct fib_rule *rule, struct flowi *fl, int flags)
 {

@@ -252,6 +264,7 @@ static const struct fib_rules_ops __net_initconst fib6_rules_ops_template = {
 	.addr_size		= sizeof(struct in6_addr),
 	.action			= fib6_rule_action,
 	.match			= fib6_rule_match,
+	.suppress		= fib6_rule_suppress,
 	.configure		= fib6_rule_configure,
 	.compare		= fib6_rule_compare,
 	.fill			= fib6_rule_fill,

Signature · 1 line

1.7.10.4

David Miller

maintainer Applied

2013-08-01 00:27 UTC #29 lore raw

Re: [PATCH v4] fib_rules: add .suppress operation

From: Stefan Tomanek <[email protected]>
Date: Thu, 1 Aug 2013 02:17:15 +0200

Quoted 19 lines

This change adds a new operation to the fib_rules_ops struct; it allows the suppression of routing decisions if certain criteria are not met by its results.
The first implemented constraint is a minimum prefix length added to the structures of routing rules. If a rule is added with a minimum prefix length
Quoted 1 line
0, only routes meeting this threshold will be considered. Any other (more
general) routing table entries will be ignored.
When configuring a system with multiple network uplinks and default routes, it is often convinient to reference the main routing table multiple times - but omitting the default route. Using this patch and a modified "ip" utility, this can be achieved by using the following command sequence:
  $ ip route add table secuplink default via 10.42.23.1
  $ ip rule add pref 100            table main prefixlength 1
  $ ip rule add pref 150 fwmark 0xA table secuplink

Quoted 7 lines

With this setup, packets marked 0xA will be processed by the additional routing table "secuplink", but only if no suitable route in the main routing table can be found. By using a minimal prefixlength of 1, the default route (/0) of the table "main" is hidden to packets processed by rule 100; packets traveling to destinations with more specific routing entries are processed as usual.
Signed-off-by Stefan Tomanek <[email protected]>

Applied, thanks.

Stefan Tomanek

author

2013-08-01 01:26 UTC #30 lore raw

Re: [PATCH v4] fib_rules: add .suppress operation

Dies schrieb David Miller ([email protected]):

Quoted 1 line

Applied, thanks.

Thank you very much; Hannes Frederic Sowa noted some irregularities with the error reporting in fib6_rules (message ID <[email protected]>) that might interfere with the function of my patch; perhaps his change should be applied as well? I've reposted his changes with git format-patch as message ID <[email protected]> ("[PATCH v2 1/2] fib6_rules: make error handling consistent with IPv4").

Hannes Frederic Sowa

2013-08-01 05:35 UTC #31 lore raw

Re: [PATCH v4] fib_rules: add .suppress operation

Quoted 11 lines · Stefan Tomanek

Dies schrieb David Miller ([email protected]):
Quoted 1 line
Applied, thanks.
Thank you very much; Hannes Frederic Sowa noted some irregularities with the error reporting in fib6_rules (message ID <[email protected]>) that might interfere with the function of my patch; perhaps his change should be applied as well? I've reposted his changes with git format-patch as message ID <[email protected]> ("[PATCH v2 1/2] fib6_rules: make error handling consistent with IPv4").

I can take care of this and will repost the patch today.

Thank you,

Hannes

Stefan Tomanek

author

2013-07-26 10:54 UTC #32 lore raw

In reply to Andrew Collins

Dies schrieb Andrew Collins ([email protected]):

Quoted 2 lines

I like the idea, but I agree with Hannes that it'd be nice to split this out into a fib constraint callback.

Here we go again:
https://github.com/wertarbyte/linux/compare/fib_rule_suppress