Re: [PATCH] Netlink: add krt_scope attribute

2016-09-16 Thread Vincent Bernat 
 ❦ 16 septembre 2016 11:33 CEST, Ondrej Zajicek  :

>> > For device routes created by the kernel, scope is link, but if you
>> > create them manually, you get a global scope.
>> 
>> My point was that 'ip' tool does that automatically for users:
>> 
>> # ip r a 10.1.1.0/24 dev eth0
>> # ip r l
>> 10.1.1.0/24 dev eth0  scope link 
>> 
>> Perhaps we should do that too. The fact that we do not set link scope
>> for device routes sent to kernel is probably an oversight, not by design.
>
> Here is an attached patch, which does that. Does it work for you?

Yes, works just fine for me!
-- 
April 1

This is the day upon which we are reminded of what we are on the other three
hundred and sixty-four.
-- Mark Twain, "Pudd'nhead Wilson's Calendar"

Re: [PATCH] Netlink: add krt_scope attribute

2016-09-16 Thread Ondrej Zajicek 
On Thu, Sep 15, 2016 at 10:36:11PM +0200, Ondrej Zajicek wrote:
> On Thu, Sep 15, 2016 at 09:22:02PM +0200, Vincent Bernat wrote:
> > > I though that kernel sets scope link automatically for device routes,
> > > but in reality it is done by ip tool. Perhaps we should do that too.
> > 
> > For device routes created by the kernel, scope is link, but if you
> > create them manually, you get a global scope.
> 
> My point was that 'ip' tool does that automatically for users:
> 
> # ip r a 10.1.1.0/24 dev eth0
> # ip r l
> 10.1.1.0/24 dev eth0  scope link 
> 
> Perhaps we should do that too. The fact that we do not set link scope
> for device routes sent to kernel is probably an oversight, not by design.

Here is an attached patch, which does that. Does it work for you?

-- 
Elen sila lumenn' omentielvo

Ondrej 'Santiago' Zajicek (email: santi...@crfreenet.org)
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
"To err is human -- to blame it on a computer is even more so."
diff --git a/sysdep/linux/krt-sys.h b/sysdep/linux/krt-sys.h
index 96688e3..6d6586d 100644
--- a/sysdep/linux/krt-sys.h
+++ b/sysdep/linux/krt-sys.h
@@ -36,6 +36,7 @@ static inline struct ifa * kif_get_primary_ip(struct iface *i) { return NULL; }
 
 #define EA_KRT_PREFSRC		EA_CODE(EAP_KRT, 0x10)
 #define EA_KRT_REALM		EA_CODE(EAP_KRT, 0x11)
+#define EA_KRT_SCOPE		EA_CODE(EAP_KRT, 0x12)
 
 
 #define KRT_METRICS_MAX		0x10	/* RTAX_QUICKACK+1 */
diff --git a/sysdep/linux/netlink.Y b/sysdep/linux/netlink.Y
index a1c22f3..f577244 100644
--- a/sysdep/linux/netlink.Y
+++ b/sysdep/linux/netlink.Y
@@ -10,7 +10,7 @@ CF_HDR
 
 CF_DECLS
 
-CF_KEYWORDS(KERNEL, TABLE, METRIC, KRT_PREFSRC, KRT_REALM, KRT_MTU, KRT_WINDOW,
+CF_KEYWORDS(KERNEL, TABLE, METRIC, KRT_PREFSRC, KRT_REALM, KRT_SCOPE, KRT_MTU, KRT_WINDOW,
 	KRT_RTT, KRT_RTTVAR, KRT_SSTRESH, KRT_CWND, KRT_ADVMSS, KRT_REORDERING,
 	KRT_HOPLIMIT, KRT_INITCWND, KRT_RTO_MIN, KRT_INITRWND, KRT_QUICKACK,
 	KRT_LOCK_MTU, KRT_LOCK_WINDOW, KRT_LOCK_RTT, KRT_LOCK_RTTVAR,
@@ -28,6 +28,7 @@ kern_sys_item:
 
 CF_ADDTO(dynamic_attr, KRT_PREFSRC	{ $$ = f_new_dynamic_attr(EAF_TYPE_IP_ADDRESS, T_IP, EA_KRT_PREFSRC); })
 CF_ADDTO(dynamic_attr, KRT_REALM	{ $$ = f_new_dynamic_attr(EAF_TYPE_INT, T_INT, EA_KRT_REALM); })
+CF_ADDTO(dynamic_attr, KRT_SCOPE	{ $$ = f_new_dynamic_attr(EAF_TYPE_INT, T_INT, EA_KRT_SCOPE); })
 
 CF_ADDTO(dynamic_attr, KRT_MTU		{ $$ = f_new_dynamic_attr(EAF_TYPE_INT, T_INT, EA_KRT_MTU); })
 CF_ADDTO(dynamic_attr, KRT_WINDOW	{ $$ = f_new_dynamic_attr(EAF_TYPE_INT, T_INT, EA_KRT_WINDOW); })
diff --git a/sysdep/linux/netlink.c b/sysdep/linux/netlink.c
index 9bdcc0d..1490213 100644
--- a/sysdep/linux/netlink.c
+++ b/sysdep/linux/netlink.c
@@ -899,7 +899,7 @@ nl_send_route(struct krt_proto *p, rte *e, struct ea_list *eattrs, int op, int d
   r.r.rtm_family = BIRD_AF;
   r.r.rtm_dst_len = net->n.pxlen;
   r.r.rtm_protocol = RTPROT_BIRD;
-  r.r.rtm_scope = RT_SCOPE_UNIVERSE;
+  r.r.rtm_scope = RT_SCOPE_NOWHERE;
   nl_add_attr_ipa(&r.h, sizeof(r), RTA_DST, net->n.prefix);
 
   /*
@@ -928,6 +928,12 @@ nl_send_route(struct krt_proto *p, rte *e, struct ea_list *eattrs, int op, int d
   if (op == NL_OP_DELETE)
 goto dest;
 
+  /* Default scope is LINK for device routes, UNIVERSE otherwise */
+  if (ea = ea_find(eattrs, EA_KRT_SCOPE))
+r.r.rtm_scope = ea->u.data;
+  else
+r.r.rtm_scope = (dest == RTD_DEVICE) ? RT_SCOPE_LINK : RT_SCOPE_UNIVERSE;
+
   if (ea = ea_find(eattrs, EA_KRT_PREFSRC))
 nl_add_attr_ipa(&r.h, sizeof(r), RTA_PREFSRC, *(ip_addr *)ea->u.ptr->data);
 
@@ -1135,6 +1141,7 @@ nl_parse_route(struct nl_parse_state *s, struct nlmsghdr *h)
   u32 oif = ~0;
   u32 table;
   u32 priority = 0;
+  u32 def_scope = RT_SCOPE_UNIVERSE;
   int src;
 
   if (!(i = nl_checkin(h, sizeof(*i
@@ -1157,7 +1164,6 @@ nl_parse_route(struct nl_parse_state *s, struct nlmsghdr *h)
 	return;
 }
 
-
   if (a[RTA_DST])
 {
   memcpy(&dst, RTA_DATA(a[RTA_DST]), sizeof(dst));
@@ -1195,11 +1201,6 @@ nl_parse_route(struct nl_parse_state *s, struct nlmsghdr *h)
   if ((c < 0) || !(c & IADDR_HOST) || ((c & IADDR_SCOPE_MASK) <= SCOPE_LINK))
 SKIP("strange class/scope\n");
 
-  // ignore rtm_scope, it is not a real scope
-  // if (i->rtm_scope != RT_SCOPE_UNIVERSE)
-  //   SKIP("scope %u\n", i->rtm_scope);
-
-
   switch (i->rtm_protocol)
 {
 case RTPROT_UNSPEC:
@@ -1286,6 +1287,7 @@ nl_parse_route(struct nl_parse_state *s, struct nlmsghdr *h)
   else
 	{
 	  ra->dest = RTD_DEVICE;
+	  def_scope = RT_SCOPE_LINK;
 	}
 
   break;
@@ -1304,6 +1306,19 @@ nl_parse_route(struct nl_parse_state *s, struct nlmsghdr *h)
   return;
 }
 
+  if (i->rtm_scope != def_scope)
+{
+  ea_list *ea = lp_alloc(s->pool, sizeof(ea_list) + sizeof(eattr));
+  ea->next = ra->eattrs;
+  ra->eattrs = ea;
+  ea->flags = EALF_SORTED;
+  ea->count = 1;
+  ea->attrs[0].id = EA_KRT_SCOPE;
+  ea->attrs[0].flags = 0;
+  ea->attrs[0].type = EAF_TYP

Re: [PATCH] Netlink: add krt_scope attribute

2016-09-15 Thread Ondrej Zajicek 
On Thu, Sep 15, 2016 at 09:22:02PM +0200, Vincent Bernat wrote:
> > I though that kernel sets scope link automatically for device routes,
> > but in reality it is done by ip tool. Perhaps we should do that too.
> 
> For device routes created by the kernel, scope is link, but if you
> create them manually, you get a global scope.

My point was that 'ip' tool does that automatically for users:

# ip r a 10.1.1.0/24 dev eth0
# ip r l
10.1.1.0/24 dev eth0  scope link 

Perhaps we should do that too. The fact that we do not set link scope
for device routes sent to kernel is probably an oversight, not by design.

I think reasonable behavior would be like:

if route has krt_scope attribute
  -> use that
else if it is device route
  -> use link
else
  -> use universe

And in other direction (kernel->bird):
if route has expected scope (link for device, universe otherwise)
  -> do not set krt_scope
else
  -> set krt_scope


-- 
Elen sila lumenn' omentielvo

Ondrej 'Santiago' Zajicek (email: santi...@crfreenet.org)
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
"To err is human -- to blame it on a computer is even more so."


signature.asc
Description: Digital signature

Re: [PATCH] Netlink: add krt_scope attribute

2016-09-15 Thread Vincent Bernat 
 ❦ 15 septembre 2016 21:22 CEST, Vincent Bernat  :

>> Thans for the patch. I agree that we should handle the attribute
>> if it has some usage. I have some questions/comments:
>>
>> Perhaps we should use the same approach like we use for krt_realm?
>> i.e., handle krt_scope as an integer variable and import constants
>> from /etc/iproute2/rt_scopes. So we could handle all values as the
>> Linux kernel and we would be consistent with iproute2 tools.
>
> I didn't know that scope names could be defined in this file. So, yes,
> we should.

Everything was already in place to use this file. So, the patch is now
simplified. While the scope is correctly imported for regular routes, it
is not for device routes. I still have to do:

protocol direct {
  table public;
  import filter {
krt_scope = ips_link;
accept "ok";
  };
  interface "eth0*", "eth1*";
}

I don't see why, I will investigate more tomorrow.

>From 29787207b33162046bfae06086e248f47d947bde Mon Sep 17 00:00:00 2001
From: Vincent Bernat 
Date: Thu, 15 Sep 2016 18:04:19 +0200
Subject: [PATCH] Netlink: add krt_scope attribute

The general-purpose scope attribute is detailed in the documentation as
an attribute that BIRD won't set and won't use. Therefore, to expose the
scope of a kernel route, this commit adds a new attribute,
krt_scope. The possible values are read from
/etc/iproute2/rt_scopes (prefixed by "ips_"). Both import and export are
supported.

Signed-off-by: Vincent Bernat 
---
 sysdep/linux/krt-sys.h |  1 +
 sysdep/linux/netlink.Y |  3 ++-
 sysdep/linux/netlink.c | 23 +--
 3 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/sysdep/linux/krt-sys.h b/sysdep/linux/krt-sys.h
index 96688e34c860..6d6586d10f02 100644
--- a/sysdep/linux/krt-sys.h
+++ b/sysdep/linux/krt-sys.h
@@ -36,6 +36,7 @@ static inline struct ifa * kif_get_primary_ip(struct iface *i) { return NULL; }
 
 #define EA_KRT_PREFSRC		EA_CODE(EAP_KRT, 0x10)
 #define EA_KRT_REALM		EA_CODE(EAP_KRT, 0x11)
+#define EA_KRT_SCOPE		EA_CODE(EAP_KRT, 0x12)
 
 
 #define KRT_METRICS_MAX		0x10	/* RTAX_QUICKACK+1 */
diff --git a/sysdep/linux/netlink.Y b/sysdep/linux/netlink.Y
index a1c22f3ece17..f577244de183 100644
--- a/sysdep/linux/netlink.Y
+++ b/sysdep/linux/netlink.Y
@@ -10,7 +10,7 @@ CF_HDR
 
 CF_DECLS
 
-CF_KEYWORDS(KERNEL, TABLE, METRIC, KRT_PREFSRC, KRT_REALM, KRT_MTU, KRT_WINDOW,
+CF_KEYWORDS(KERNEL, TABLE, METRIC, KRT_PREFSRC, KRT_REALM, KRT_SCOPE, KRT_MTU, KRT_WINDOW,
 	KRT_RTT, KRT_RTTVAR, KRT_SSTRESH, KRT_CWND, KRT_ADVMSS, KRT_REORDERING,
 	KRT_HOPLIMIT, KRT_INITCWND, KRT_RTO_MIN, KRT_INITRWND, KRT_QUICKACK,
 	KRT_LOCK_MTU, KRT_LOCK_WINDOW, KRT_LOCK_RTT, KRT_LOCK_RTTVAR,
@@ -28,6 +28,7 @@ kern_sys_item:
 
 CF_ADDTO(dynamic_attr, KRT_PREFSRC	{ $$ = f_new_dynamic_attr(EAF_TYPE_IP_ADDRESS, T_IP, EA_KRT_PREFSRC); })
 CF_ADDTO(dynamic_attr, KRT_REALM	{ $$ = f_new_dynamic_attr(EAF_TYPE_INT, T_INT, EA_KRT_REALM); })
+CF_ADDTO(dynamic_attr, KRT_SCOPE	{ $$ = f_new_dynamic_attr(EAF_TYPE_INT, T_INT, EA_KRT_SCOPE); })
 
 CF_ADDTO(dynamic_attr, KRT_MTU		{ $$ = f_new_dynamic_attr(EAF_TYPE_INT, T_INT, EA_KRT_MTU); })
 CF_ADDTO(dynamic_attr, KRT_WINDOW	{ $$ = f_new_dynamic_attr(EAF_TYPE_INT, T_INT, EA_KRT_WINDOW); })
diff --git a/sysdep/linux/netlink.c b/sysdep/linux/netlink.c
index 9bdcc0d2ff19..2aeadbae4117 100644
--- a/sysdep/linux/netlink.c
+++ b/sysdep/linux/netlink.c
@@ -900,6 +900,9 @@ nl_send_route(struct krt_proto *p, rte *e, struct ea_list *eattrs, int op, int d
   r.r.rtm_dst_len = net->n.pxlen;
   r.r.rtm_protocol = RTPROT_BIRD;
   r.r.rtm_scope = RT_SCOPE_UNIVERSE;
+  if (ea = ea_find(eattrs, EA_KRT_SCOPE))
+r.r.rtm_scope = ea->u.data;
+
   nl_add_attr_ipa(&r.h, sizeof(r), RTA_DST, net->n.prefix);
 
   /*
@@ -1157,7 +1160,6 @@ nl_parse_route(struct nl_parse_state *s, struct nlmsghdr *h)
 	return;
 }
 
-
   if (a[RTA_DST])
 {
   memcpy(&dst, RTA_DATA(a[RTA_DST]), sizeof(dst));
@@ -1195,11 +1197,6 @@ nl_parse_route(struct nl_parse_state *s, struct nlmsghdr *h)
   if ((c < 0) || !(c & IADDR_HOST) || ((c & IADDR_SCOPE_MASK) <= SCOPE_LINK))
 SKIP("strange class/scope\n");
 
-  // ignore rtm_scope, it is not a real scope
-  // if (i->rtm_scope != RT_SCOPE_UNIVERSE)
-  //   SKIP("scope %u\n", i->rtm_scope);
-
-
   switch (i->rtm_protocol)
 {
 case RTPROT_UNSPEC:
@@ -1304,6 +1301,16 @@ nl_parse_route(struct nl_parse_state *s, struct nlmsghdr *h)
   return;
 }
 
+  ea_list *ea = lp_alloc(s->pool, sizeof(ea_list) + sizeof(eattr));
+  ea->next = ra->eattrs;
+  ra->eattrs = ea;
+  ea->flags = EALF_SORTED;
+  ea->count = 1;
+  ea->attrs[0].id = EA_KRT_SCOPE;
+  ea->attrs[0].flags = 0;
+  ea->attrs[0].type = EAF_TYPE_INT;
+  ea->attrs[0].u.data = i->rtm_scope;
+
   if (a[RTA_PREFSRC])
 {
   ip_addr ps;
@@ -1633,6 +1640,10 @@ krt_sys_get_attr(eattr *a, byte *buf, int buflen UNUSED)
 bsprintf(buf, "realm");
 return GA_NAME;
 
+  case EA_KRT_SCOPE:
+bsprintf(buf, "scope");
+return GA_NAME;
+
   cas

Re: [PATCH] Netlink: add krt_scope attribute

2016-09-15 Thread Vincent Bernat 
 ❦ 15 septembre 2016 20:36 CEST, Ondrej Zajicek  :

> Thans for the patch. I agree that we should handle the attribute
> if it has some usage. I have some questions/comments:
>
> Perhaps we should use the same approach like we use for krt_realm?
> i.e., handle krt_scope as an integer variable and import constants
> from /etc/iproute2/rt_scopes. So we could handle all values as the
> Linux kernel and we would be consistent with iproute2 tools.

I didn't know that scope names could be defined in this file. So, yes,
we should.

> I don't really understand how Linux handles scope argument. If i
> remember correctly, the idea is that next hop of a route could be
> resolvable through a route with higher scope value, but it seems
> that it distinguish only host (internal), link and anything other.

At least, Linux will refuse to install a route "scope global" when the
next-hop is "scope global" too.

>> A typical use case is to install "scope link" routes for directly
>> connected destinations, as the kernel won't accept routes with a broader
>> scope when issuing an internal lookup.
>
> You mean for regular routes (with next hop) or device routes?

Device routes.

> I though that kernel sets scope link automatically for device routes,
> but in reality it is done by ip tool. Perhaps we should do that too.

For device routes created by the kernel, scope is link, but if you
create them manually, you get a global scope.

> I could create regular routes with scope link directed to device routes
> with scope link (which is a bit strange) and then create regular routes
> with normal scope directed on them, but i cannot get more hierarchy.

In fact, I need top copy device routes from main table to other tables
(to workaround a limitation with Linux kernel before 3.15). Currently,
they are copied with "scope global". I need them to copy them with
"scope link", otherwise BIRD won't be able to install routes with a
next-hop in the connected subnet.
-- 
10.0 times 0.1 is hardly ever 1.0.
- The Elements of Programming Style (Kernighan & Plauger)

Re: [PATCH] Netlink: add krt_scope attribute

2016-09-15 Thread Ondrej Zajicek 
On Thu, Sep 15, 2016 at 06:05:27PM +0200, Vincent Bernat wrote:
> The general-purpose scope attribute is detailed in the documentation as
> an attribute that BIRD won't set and won't use. Therefore, to expose the
> scope of a kernel route, this commit adds a new attribute,
> krt_scope. Its possible values are the numeric values for SCOPE_*
> variables (1 is SCOPE_LINK for example), not the values from the
> kernel (253 is RT_SCOPE_LINK). Both import and export are supported.

Hi

Thans for the patch. I agree that we should handle the attribute
if it has some usage. I have some questions/comments:

Perhaps we should use the same approach like we use for krt_realm?
i.e., handle krt_scope as an integer variable and import constants
from /etc/iproute2/rt_scopes. So we could handle all values as the
Linux kernel and we would be consistent with iproute2 tools.

I don't really understand how Linux handles scope argument. If i
remember correctly, the idea is that next hop of a route could be
resolvable through a route with higher scope value, but it seems
that it distinguish only host (internal), link and anything other.

> A typical use case is to install "scope link" routes for directly
> connected destinations, as the kernel won't accept routes with a broader
> scope when issuing an internal lookup.

You mean for regular routes (with next hop) or device routes?

I though that kernel sets scope link automatically for device routes,
but in reality it is done by ip tool. Perhaps we should do that too.

I could create regular routes with scope link directed to device routes
with scope link (which is a bit strange) and then create regular routes
with normal scope directed on them, but i cannot get more hierarchy.

-- 
Elen sila lumenn' omentielvo

Ondrej 'Santiago' Zajicek (email: santi...@crfreenet.org)
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
"To err is human -- to blame it on a computer is even more so."


signature.asc
Description: Digital signature

Re: [PATCH] Netlink: add krt_scope attribute

2016-09-15 Thread Vincent Bernat 
 ❦ 15 septembre 2016 18:05 CEST, Vincent Bernat  :

> It would have been neat to be able to use "SCOPE_LINK" in a filter
> or to show "SCOPE_LINK" when displaying the route, but currently, only
> the numerical value can be used.

Any hint for this part would be welcome. The T_ENUM_SCOPE is ignored and
SCOPE_LINK is considered as non-int for some reason.
-- 
Take care to branch the right way on equality.
- The Elements of Programming Style (Kernighan & Plauger)