[PATCH net 0/2] ipv6: Fix listing and flushing of cached route exceptions

[PATCH net 0/2] ipv6: Fix listing and flushing of cached route exceptions

[Stefano Brivio]

The commands 'ip -6 route list cache' and 'ip -6 route flush cache'
don't work at all after route exceptions have been moved to a separate
hash table in commit 2b760fcf5cfb ("ipv6: hook up exception table to store
dst cache"). Fix that.

Stefano Brivio (2):
  ipv6: Dump route exceptions too in rt6_dump_route()
  ip6_fib: Don't discard nodes with valid routing information in
    fib6_locate_1()

 net/ipv6/ip6_fib.c | 10 ++++------
 net/ipv6/route.c   | 38 +++++++++++++++++++++++++++++++++++---
 2 files changed, 39 insertions(+), 9 deletions(-)

-- 
2.20.1

[PATCH net 1/2] ipv6: Dump route exceptions too in rt6_dump_route()

[Stefano Brivio]

Since commit 2b760fcf5cfb ("ipv6: hook up exception table to store dst
cache"), route exceptions reside in a separate hash table, and won't be
found by walking the FIB, so they won't be dumped to userspace on a
RTM_GETROUTE message.

This causes 'ip -6 route list cache' and 'ip -6 route flush cache' to
have no function anymore:

 # ip -6 route get fc00:3::1
 fc00:3::1 via fc00:1::2 dev veth_A-R1 src fc00:1::1 metric 1024 expires 539sec mtu 1400 pref medium
 # ip -6 route get fc00:4::1
 fc00:4::1 via fc00:2::2 dev veth_A-R2 src fc00:2::1 metric 1024 expires 536sec mtu 1500 pref medium
 # ip -6 route list cache
 # ip -6 route flush cache
 # ip -6 route get fc00:3::1
 fc00:3::1 via fc00:1::2 dev veth_A-R1 src fc00:1::1 metric 1024 expires 520sec mtu 1400 pref medium
 # ip -6 route get fc00:4::1
 fc00:4::1 via fc00:2::2 dev veth_A-R2 src fc00:2::1 metric 1024 expires 519sec mtu 1500 pref medium

because iproute2 lists cached routes using RTM_GETROUTE, and flushes them
by listing all the routes, and deleting them with RTM_DELROUTE one by one.

Look up exceptions in the hash table associated with the current fib6_info
in rt6_dump_route(), and, if present and not expired, add them to the
dump.

Re-allow userspace to get FIB results by passing the RTM_F_CLONED flag as
filter, by reverting commit 08e814c9e8eb ("net/ipv6: Bail early if user
only wants cloned entries").

As we do this, we also have to honour this flag while filtering routes in
rt6_dump_route() and, if this filter effectively causes some results to be
discarded, by passing the NLM_F_DUMP_FILTERED flag back.

To flush cached routes, a procfs entry could be introduced instead: that's
how it works for IPv4. We already have a rt6_flush_exception() function
ready to be wired to it. However, this would not solve the issue for
listing, and wouldn't fix the issue with current and previous versions of
iproute2.

Reported-by: Jianlin Shi <jishi@redhat.com>
Fixes: 2b760fcf5cfb ("ipv6: hook up exception table to store dst cache")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
This will cause a non-trivial conflict with commit cc5c073a693f
("ipv6: Move exception bucket to fib6_nh") on net-next. I can submit
an equivalent patch against net-next, if it helps.

 net/ipv6/ip6_fib.c |  7 ++-----
 net/ipv6/route.c   | 38 +++++++++++++++++++++++++++++++++++---
 2 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index 008421b550c6..5be133565819 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -581,13 +581,10 @@ static int inet6_dump_fib(struct sk_buff *skb, struct netlink_callback *cb)
 	} else if (nlmsg_len(nlh) >= sizeof(struct rtmsg)) {
 		struct rtmsg *rtm = nlmsg_data(nlh);
 
-		arg.filter.flags = rtm->rtm_flags & (RTM_F_PREFIX|RTM_F_CLONED);
+		if (rtm->rtm_flags & RTM_F_PREFIX)
+			arg.filter.flags = RTM_F_PREFIX;
 	}
 
-	/* fib entries are never clones */
-	if (arg.filter.flags & RTM_F_CLONED)
-		goto out;
-
 	w = (void *)cb->args[2];
 	if (!w) {
 		/* New dump:
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 848e944f07df..51f923b3ad26 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -4862,8 +4862,11 @@ int rt6_dump_route(struct fib6_info *rt, void *p_arg)
 {
 	struct rt6_rtnl_dump_arg *arg = (struct rt6_rtnl_dump_arg *) p_arg;
 	struct fib_dump_filter *filter = &arg->filter;
+	struct rt6_exception_bucket *bucket;
 	unsigned int flags = NLM_F_MULTI;
+	struct rt6_exception *rt6_ex;
 	struct net *net = arg->net;
+	int i, err;
 
 	if (rt == net->ipv6.fib6_null_entry)
 		return 0;
@@ -4882,9 +4885,38 @@ int rt6_dump_route(struct fib6_info *rt, void *p_arg)
 		flags |= NLM_F_DUMP_FILTERED;
 	}
 
-	return rt6_fill_node(net, arg->skb, rt, NULL, NULL, NULL, 0,
-			     RTM_NEWROUTE, NETLINK_CB(arg->cb->skb).portid,
-			     arg->cb->nlh->nlmsg_seq, flags);
+	if (!(filter->flags & RTM_F_CLONED)) {
+		err = rt6_fill_node(net, arg->skb, rt, NULL, NULL, NULL, 0,
+				    RTM_NEWROUTE,
+				    NETLINK_CB(arg->cb->skb).portid,
+				    arg->cb->nlh->nlmsg_seq, flags);
+		if (err)
+			return err;
+	} else {
+		flags |= NLM_F_DUMP_FILTERED;
+	}
+
+	bucket = rcu_dereference(rt->rt6i_exception_bucket);
+	if (!bucket)
+		return 0;
+
+	for (i = 0; i < FIB6_EXCEPTION_BUCKET_SIZE; i++) {
+		hlist_for_each_entry(rt6_ex, &bucket->chain, hlist) {
+			if (rt6_check_expired(rt6_ex->rt6i))
+				continue;
+
+			err = rt6_fill_node(net, arg->skb, rt,
+					    &rt6_ex->rt6i->dst,
+					    NULL, NULL, 0, RTM_NEWROUTE,
+					    NETLINK_CB(arg->cb->skb).portid,
+					    arg->cb->nlh->nlmsg_seq, flags);
+			if (err)
+				return err;
+		}
+		bucket++;
+	}
+
+	return 0;
 }
 
 static int inet6_rtm_valid_getroute_req(struct sk_buff *skb,
-- 
2.20.1

Re: [PATCH net 1/2] ipv6: Dump route exceptions too in rt6_dump_route()

[David Ahern]

On 6/6/19 2:13 PM, Stefano Brivio wrote:
> Since commit 2b760fcf5cfb ("ipv6: hook up exception table to store dst
> cache"), route exceptions reside in a separate hash table, and won't be
> found by walking the FIB, so they won't be dumped to userspace on a
> RTM_GETROUTE message.
> 
> This causes 'ip -6 route list cache' and 'ip -6 route flush cache' to
> have no function anymore:
> 
>  # ip -6 route get fc00:3::1
>  fc00:3::1 via fc00:1::2 dev veth_A-R1 src fc00:1::1 metric 1024 expires 539sec mtu 1400 pref medium
>  # ip -6 route get fc00:4::1
>  fc00:4::1 via fc00:2::2 dev veth_A-R2 src fc00:2::1 metric 1024 expires 536sec mtu 1500 pref medium
>  # ip -6 route list cache
>  # ip -6 route flush cache
>  # ip -6 route get fc00:3::1
>  fc00:3::1 via fc00:1::2 dev veth_A-R1 src fc00:1::1 metric 1024 expires 520sec mtu 1400 pref medium
>  # ip -6 route get fc00:4::1
>  fc00:4::1 via fc00:2::2 dev veth_A-R2 src fc00:2::1 metric 1024 expires 519sec mtu 1500 pref medium
> 
> because iproute2 lists cached routes using RTM_GETROUTE, and flushes them
> by listing all the routes, and deleting them with RTM_DELROUTE one by one.
> 
> Look up exceptions in the hash table associated with the current fib6_info
> in rt6_dump_route(), and, if present and not expired, add them to the
> dump.
> 
> Re-allow userspace to get FIB results by passing the RTM_F_CLONED flag as
> filter, by reverting commit 08e814c9e8eb ("net/ipv6: Bail early if user
> only wants cloned entries").
> 
> As we do this, we also have to honour this flag while filtering routes in
> rt6_dump_route() and, if this filter effectively causes some results to be
> discarded, by passing the NLM_F_DUMP_FILTERED flag back.
> 
> To flush cached routes, a procfs entry could be introduced instead: that's
> how it works for IPv4. We already have a rt6_flush_exception() function
> ready to be wired to it. However, this would not solve the issue for
> listing, and wouldn't fix the issue with current and previous versions of
> iproute2.
> 
> Reported-by: Jianlin Shi <jishi@redhat.com>
> Fixes: 2b760fcf5cfb ("ipv6: hook up exception table to store dst cache")
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
> This will cause a non-trivial conflict with commit cc5c073a693f
> ("ipv6: Move exception bucket to fib6_nh") on net-next. I can submit
> an equivalent patch against net-next, if it helps.
> 

Thanks for doing this. It is on my to-do list.

Can you do the same for IPv4?

Re: [PATCH net 1/2] ipv6: Dump route exceptions too in rt6_dump_route()

[Stefano Brivio]

On Thu, 6 Jun 2019 14:57:33 -0600
David Ahern <dsahern@gmail.com> wrote:

> > This will cause a non-trivial conflict with commit cc5c073a693f
> > ("ipv6: Move exception bucket to fib6_nh") on net-next. I can submit
> > an equivalent patch against net-next, if it helps.
> >   
> 
> Thanks for doing this. It is on my to-do list.
> 
> Can you do the same for IPv4?

You mean this same fix? On IPv4, for flushing, iproute2
uses /proc/sys/net/ipv4/route/flush in iproute_flush_cache(), and that
works.

Listing doesn't work instead, for some different reason I haven't
looked into yet. That doesn't look as critical as the situation on IPv6
where one can't even flush the cache: exceptions can also be fetched
with 'ip route get', and that works.

Still, it's bad, I can look into it within a few days.

-- 
Stefano

Re: [PATCH net 1/2] ipv6: Dump route exceptions too in rt6_dump_route()

[David Ahern]

On 6/6/19 3:18 PM, Stefano Brivio wrote:
> On Thu, 6 Jun 2019 14:57:33 -0600
> David Ahern <dsahern@gmail.com> wrote:
> 
>>> This will cause a non-trivial conflict with commit cc5c073a693f
>>> ("ipv6: Move exception bucket to fib6_nh") on net-next. I can submit
>>> an equivalent patch against net-next, if it helps.
>>>   
>>
>> Thanks for doing this. It is on my to-do list.
>>
>> Can you do the same for IPv4?
> 
> You mean this same fix? On IPv4, for flushing, iproute2
> uses /proc/sys/net/ipv4/route/flush in iproute_flush_cache(), and that
> works.
> 
> Listing doesn't work instead, for some different reason I haven't
> looked into yet. That doesn't look as critical as the situation on IPv6
> where one can't even flush the cache: exceptions can also be fetched
> with 'ip route get', and that works.
> 
> Still, it's bad, I can look into it within a few days.
> 

I meant the ability to dump the exception cache.

Currently, we do not get the exceptions in a fib dump. There is a flag
to only show cloned (cached) entries, but no way to say 'no cloned
entries'. Maybe these should only be dumped if the cloned flag is set.
That's the use case I was targeting:
1. fib dumps - RTM_F_CLONED not set
2. exception dump - RTM_F_CLONED set

Re: [PATCH net 1/2] ipv6: Dump route exceptions too in rt6_dump_route()

[Stefano Brivio]

On Thu, 6 Jun 2019 16:47:00 -0600
David Ahern <dsahern@gmail.com> wrote:

> On 6/6/19 3:18 PM, Stefano Brivio wrote:
> > On Thu, 6 Jun 2019 14:57:33 -0600
> > David Ahern <dsahern@gmail.com> wrote:
> >   
> >>> This will cause a non-trivial conflict with commit cc5c073a693f
> >>> ("ipv6: Move exception bucket to fib6_nh") on net-next. I can submit
> >>> an equivalent patch against net-next, if it helps.
> >>>     
> >>
> >> Thanks for doing this. It is on my to-do list.
> >>
> >> Can you do the same for IPv4?  
> > 
> > You mean this same fix? On IPv4, for flushing, iproute2
> > uses /proc/sys/net/ipv4/route/flush in iproute_flush_cache(), and that
> > works.
> > 
> > Listing doesn't work instead, for some different reason I haven't
> > looked into yet. That doesn't look as critical as the situation on IPv6
> > where one can't even flush the cache: exceptions can also be fetched
> > with 'ip route get', and that works.
> > 
> > Still, it's bad, I can look into it within a few days.
> >   
> 
> I meant the ability to dump the exception cache.
> 
> Currently, we do not get the exceptions in a fib dump. There is a flag
> to only show cloned (cached) entries, but no way to say 'no cloned
> entries'. Maybe these should only be dumped if the cloned flag is set.
> That's the use case I was targeting:
> 1. fib dumps - RTM_F_CLONED not set
> 2. exception dump - RTM_F_CLONED set

I think it would make a lot of sense. But don't we risk breaking
userspace even further, by skipping exceptions if RTM_F_CLONED is not
set? On the other hand, this was broken for almost two years, maybe
it's not too bad after all.

-- 
Stefano

Re: [PATCH net 1/2] ipv6: Dump route exceptions too in rt6_dump_route()

[Martin Lau]

On Thu, Jun 06, 2019 at 04:47:00PM -0600, David Ahern wrote:
> On 6/6/19 3:18 PM, Stefano Brivio wrote:
> > On Thu, 6 Jun 2019 14:57:33 -0600
> > David Ahern <dsahern@gmail.com> wrote:
> > 
> >>> This will cause a non-trivial conflict with commit cc5c073a693f
> >>> ("ipv6: Move exception bucket to fib6_nh") on net-next. I can submit
> >>> an equivalent patch against net-next, if it helps.
> >>>   
> >>
> >> Thanks for doing this. It is on my to-do list.
> >>
> >> Can you do the same for IPv4?
> > 
> > You mean this same fix? On IPv4, for flushing, iproute2
> > uses /proc/sys/net/ipv4/route/flush in iproute_flush_cache(), and that
> > works.
> > 
> > Listing doesn't work instead, for some different reason I haven't
> > looked into yet. That doesn't look as critical as the situation on IPv6
> > where one can't even flush the cache: exceptions can also be fetched
> > with 'ip route get', and that works.
> > 
> > Still, it's bad, I can look into it within a few days.
> > 
> 
> I meant the ability to dump the exception cache.
> 
> Currently, we do not get the exceptions in a fib dump. There is a flag
> to only show cloned (cached) entries, but no way to say 'no cloned
> entries'. Maybe these should only be dumped if the cloned flag is set.
> That's the use case I was targeting:
> 1. fib dumps - RTM_F_CLONED not set
I also think the fib dump should stay as is.

To be clear, I do not expect exception routes output from the
'ip [-6] r l'.  Otherwise, I will get pages of exceptions
that I am not interested at.  This should apply for both
v4 and v6.

> 2. exception dump - RTM_F_CLONED set

Re: [PATCH net 1/2] ipv6: Dump route exceptions too in rt6_dump_route()

[Stefano Brivio]

On Sat, 8 Jun 2019 05:40:06 +0000
Martin Lau <kafai@fb.com> wrote:

> On Thu, Jun 06, 2019 at 04:47:00PM -0600, David Ahern wrote:
> > On 6/6/19 3:18 PM, Stefano Brivio wrote:  
> > > On Thu, 6 Jun 2019 14:57:33 -0600
> > > David Ahern <dsahern@gmail.com> wrote:
> > >   
> > >>> This will cause a non-trivial conflict with commit cc5c073a693f
> > >>> ("ipv6: Move exception bucket to fib6_nh") on net-next. I can submit
> > >>> an equivalent patch against net-next, if it helps.
> > >>>     
> > >>
> > >> Thanks for doing this. It is on my to-do list.
> > >>
> > >> Can you do the same for IPv4?  
> > > 
> > > You mean this same fix? On IPv4, for flushing, iproute2
> > > uses /proc/sys/net/ipv4/route/flush in iproute_flush_cache(), and that
> > > works.
> > > 
> > > Listing doesn't work instead, for some different reason I haven't
> > > looked into yet. That doesn't look as critical as the situation on IPv6
> > > where one can't even flush the cache: exceptions can also be fetched
> > > with 'ip route get', and that works.
> > > 
> > > Still, it's bad, I can look into it within a few days.
> > >   
> > 
> > I meant the ability to dump the exception cache.
> > 
> > Currently, we do not get the exceptions in a fib dump. There is a flag
> > to only show cloned (cached) entries, but no way to say 'no cloned
> > entries'. Maybe these should only be dumped if the cloned flag is set.
> > That's the use case I was targeting:
> > 1. fib dumps - RTM_F_CLONED not set  
> I also think the fib dump should stay as is.
> 
> To be clear, I do not expect exception routes output from the
> 'ip [-6] r l'.  Otherwise, I will get pages of exceptions
> that I am not interested at.  This should apply for both
> v4 and v6.

I also agree it makes more sense to filter routes this way.

But it wasn't like this before 2b760fcf5cfb, so this smells like
breaking userspace expectations, even though iproute already filters
routes this way: with 'cache' it only displays routes with
RTM_F_CLONED, without, it won't display exceptions, see filter_nlmsg():

	if (filter.cloned == !(r->rtm_flags & RTM_F_CLONED))
		return 0;

This, together with the fact it's been like that for almost two years
now, makes it acceptable in my opinion. What do you think?

If we agree on this, I'll go ahead and start changing this in my patch
for IPv6.

-- 
Stefano

Re: [PATCH net 1/2] ipv6: Dump route exceptions too in rt6_dump_route()

[Martin Lau]

On Sat, Jun 08, 2019 at 07:59:11AM +0200, Stefano Brivio wrote:
> On Sat, 8 Jun 2019 05:40:06 +0000
> Martin Lau <kafai@fb.com> wrote:
> 
> > On Thu, Jun 06, 2019 at 04:47:00PM -0600, David Ahern wrote:
> > > On 6/6/19 3:18 PM, Stefano Brivio wrote:  
> > > > On Thu, 6 Jun 2019 14:57:33 -0600
> > > > David Ahern <dsahern@gmail.com> wrote:
> > > >   
> > > >>> This will cause a non-trivial conflict with commit cc5c073a693f
> > > >>> ("ipv6: Move exception bucket to fib6_nh") on net-next. I can submit
> > > >>> an equivalent patch against net-next, if it helps.
> > > >>>     
> > > >>
> > > >> Thanks for doing this. It is on my to-do list.
> > > >>
> > > >> Can you do the same for IPv4?  
> > > > 
> > > > You mean this same fix? On IPv4, for flushing, iproute2
> > > > uses /proc/sys/net/ipv4/route/flush in iproute_flush_cache(), and that
> > > > works.
> > > > 
> > > > Listing doesn't work instead, for some different reason I haven't
> > > > looked into yet. That doesn't look as critical as the situation on IPv6
> > > > where one can't even flush the cache: exceptions can also be fetched
> > > > with 'ip route get', and that works.
> > > > 
> > > > Still, it's bad, I can look into it within a few days.
> > > >   
> > > 
> > > I meant the ability to dump the exception cache.
> > > 
> > > Currently, we do not get the exceptions in a fib dump. There is a flag
> > > to only show cloned (cached) entries, but no way to say 'no cloned
> > > entries'. Maybe these should only be dumped if the cloned flag is set.
> > > That's the use case I was targeting:
> > > 1. fib dumps - RTM_F_CLONED not set  
> > I also think the fib dump should stay as is.
> > 
> > To be clear, I do not expect exception routes output from the
> > 'ip [-6] r l'.  Otherwise, I will get pages of exceptions
> > that I am not interested at.  This should apply for both
> > v4 and v6.
> 
> I also agree it makes more sense to filter routes this way.
> 
> But it wasn't like this before 2b760fcf5cfb, so this smells like
> breaking userspace expectations, even though iproute already filters
> routes this way: with 'cache' it only displays routes with
> RTM_F_CLONED, without, it won't display exceptions, see filter_nlmsg():
Thanks for pointing it out.

> 
> 	if (filter.cloned == !(r->rtm_flags & RTM_F_CLONED))
> 		return 0;
> 
> This, together with the fact it's been like that for almost two years
> now, makes it acceptable in my opinion. What do you think?
With learning the above fact on iproute2,
it makes even less sense to dump exceptions from the kernel side
when RTM_F_CLONED is not set.

> If we agree on this, I'll go ahead and start changing this in my patch
> for IPv6.

Re: [PATCH net 1/2] ipv6: Dump route exceptions too in rt6_dump_route()

[Stefano Brivio]

On Sat, 8 Jun 2019 07:19:23 +0000
Martin Lau <kafai@fb.com> wrote:

> On Sat, Jun 08, 2019 at 07:59:11AM +0200, Stefano Brivio wrote:
> > I also agree it makes more sense to filter routes this way.
> > 
> > But it wasn't like this before 2b760fcf5cfb, so this smells like
> > breaking userspace expectations, even though iproute already filters
> > routes this way: with 'cache' it only displays routes with
> > RTM_F_CLONED, without, it won't display exceptions, see filter_nlmsg():  
> Thanks for pointing it out.
> 
> > 	if (filter.cloned == !(r->rtm_flags & RTM_F_CLONED))
> > 		return 0;
> > 
> > This, together with the fact it's been like that for almost two years
> > now, makes it acceptable in my opinion. What do you think?  
> With learning the above fact on iproute2,
> it makes even less sense to dump exceptions from the kernel side
> when RTM_F_CLONED is not set.

I just hit a more fundamental problem though: iproute2 filters on the
flag, but never sets it on a dump request. Flags will be NLM_F_DUMP |
NLM_F_REQUEST, no matter what, see rtnl_routedump_req(). So the current
iproute2 would have no way to dump cached routes.

It could from 2007, iproute2 9ab4c85b9af1 ("Fix bug in display of ipv6
cloned/cached routes"), to 2017, kernel 2b760fcf5cfb ("ipv6: hook up
exception table to store dst cache").

Something tells me it's wrong to fix userspace, because userspace "is
always right". There has been by the way a similar discussion on this
list in 2011, see https://lists.openwall.net/netdev/2011/12/28/27.

I would proceed like this:

- stick to the original semantic of RTM_F_CLONED and fix the issue at
  hand, which would be v2 with your suggested clean-up and without
  check on RTM_F_CLONED. Exceptions are always dumped and iproute2 will
  filter them as it always did. Result: kernel sends exceptions on
  netlink even if not "requested" but iproute2 works again and won't
  spam you anyway, and the issue is fixed for the users

- fix this on IPv4 (as I mentioned, I think it's less critical, because
  at least flushing works, and listing with 'route get' is awkward but
  possible)

- retry adding NLM_F_MATCH (for net-next and iproute-next) according
  to RFC 3549. Things changed a bit from 2011: we now have
  NLM_F_DUMP_FILTERED, iproute2 already uses it (ip neigh) and we
  wouldn't need to make iproute2 more complicated by handling old/new
  kernel cases. So I think this would be reasonable now.

-- 
Stefano

Re: [PATCH net 1/2] ipv6: Dump route exceptions too in rt6_dump_route()

[Stefano Brivio]

On Sat, 8 Jun 2019 17:02:06 +0200
Stefano Brivio <sbrivio@redhat.com> wrote:

> On Sat, 8 Jun 2019 07:19:23 +0000
> Martin Lau <kafai@fb.com> wrote:
> 
> > On Sat, Jun 08, 2019 at 07:59:11AM +0200, Stefano Brivio wrote:  
> > > I also agree it makes more sense to filter routes this way.
> > > 
> > > But it wasn't like this before 2b760fcf5cfb, so this smells like
> > > breaking userspace expectations, even though iproute already filters
> > > routes this way: with 'cache' it only displays routes with
> > > RTM_F_CLONED, without, it won't display exceptions, see filter_nlmsg():    
> > Thanks for pointing it out.
> >   
> > > 	if (filter.cloned == !(r->rtm_flags & RTM_F_CLONED))
> > > 		return 0;
> > > 
> > > This, together with the fact it's been like that for almost two years
> > > now, makes it acceptable in my opinion. What do you think?    
> > With learning the above fact on iproute2,
> > it makes even less sense to dump exceptions from the kernel side
> > when RTM_F_CLONED is not set.  
> 
> I just hit a more fundamental problem though: iproute2 filters on the
> flag, but never sets it on a dump request. Flags will be NLM_F_DUMP |
> NLM_F_REQUEST, no matter what, see rtnl_routedump_req(). So the current
> iproute2 would have no way to dump cached routes.

Partially wrong: it actually sets it on 'list':

	if (rtnl_routedump_req(&rth, dump_family, iproute_dump_filter) < 0) {

[...]
static int iproute_dump_filter(struct nlmsghdr *nlh, int reqlen)
[...]
	if (filter.cloned)
		rtm->rtm_flags |= RTM_F_CLONED;

but not on 'flush':

		if (rtnl_routedump_req(&rth, family, NULL) < 0) {

but this doesn't change things much: it still has no way to flush the
cache, because the dump to get the routes to flush doesn't contain the
exceptions.

So I would stick to my latest plan.

-- 
Stefano

Re: [PATCH net 1/2] ipv6: Dump route exceptions too in rt6_dump_route()

[Martin Lau]

On Sat, Jun 08, 2019 at 05:47:07PM +0200, Stefano Brivio wrote:
> On Sat, 8 Jun 2019 17:02:06 +0200
> Stefano Brivio <sbrivio@redhat.com> wrote:
> 
> > On Sat, 8 Jun 2019 07:19:23 +0000
> > Martin Lau <kafai@fb.com> wrote:
> > 
> > > On Sat, Jun 08, 2019 at 07:59:11AM +0200, Stefano Brivio wrote:  
> > > > I also agree it makes more sense to filter routes this way.
> > > > 
> > > > But it wasn't like this before 2b760fcf5cfb, so this smells like
> > > > breaking userspace expectations, even though iproute already filters
> > > > routes this way: with 'cache' it only displays routes with
> > > > RTM_F_CLONED, without, it won't display exceptions, see filter_nlmsg():    
> > > Thanks for pointing it out.
> > >   
> > > > 	if (filter.cloned == !(r->rtm_flags & RTM_F_CLONED))
> > > > 		return 0;
> > > > 
> > > > This, together with the fact it's been like that for almost two years
> > > > now, makes it acceptable in my opinion. What do you think?    
> > > With learning the above fact on iproute2,
> > > it makes even less sense to dump exceptions from the kernel side
> > > when RTM_F_CLONED is not set.  
> > 
> > I just hit a more fundamental problem though: iproute2 filters on the
> > flag, but never sets it on a dump request. Flags will be NLM_F_DUMP |
> > NLM_F_REQUEST, no matter what, see rtnl_routedump_req(). So the current
> > iproute2 would have no way to dump cached routes.
> 
> Partially wrong: it actually sets it on 'list':
> 
> 	if (rtnl_routedump_req(&rth, dump_family, iproute_dump_filter) < 0) {
> 
> [...]
> static int iproute_dump_filter(struct nlmsghdr *nlh, int reqlen)
> [...]
> 	if (filter.cloned)
> 		rtm->rtm_flags |= RTM_F_CLONED;
> 
> but not on 'flush':
> 
> 		if (rtnl_routedump_req(&rth, family, NULL) < 0) {
> 
> but this doesn't change things much: it still has no way to flush the
> cache, because the dump to get the routes to flush doesn't contain the
> exceptions.
'ip -6 r l table cache' can be limited to dump the cache only, right?

I am still missing something about why the kernel is required
to output everything and then filtered out in the iproute2.

You meant either:
The kernel needs to dump everything first. iproute2 can then figure out
which one is cache and then flush them?
or
the iproute2 can be changed to only get the cache from the kernel and then
flush them?

AFAIK, the kernel has never dumped the cache routes for IPv4.
What is done here has to be consistent with the future patch in IPv4.
Each node can hold up to 5*1024 caches which is ok-ish but still a waste
to dump it and then not printing it.

Re: [PATCH net 1/2] ipv6: Dump route exceptions too in rt6_dump_route()

[Stefano Brivio]

On Mon, 10 Jun 2019 19:42:41 +0000
Martin Lau <kafai@fb.com> wrote:

> On Sat, Jun 08, 2019 at 05:47:07PM +0200, Stefano Brivio wrote:
> > On Sat, 8 Jun 2019 17:02:06 +0200
> > Stefano Brivio <sbrivio@redhat.com> wrote:
> >   
> > > On Sat, 8 Jun 2019 07:19:23 +0000
> > > Martin Lau <kafai@fb.com> wrote:
> > >   
> > > > On Sat, Jun 08, 2019 at 07:59:11AM +0200, Stefano Brivio wrote:    
> > > > > I also agree it makes more sense to filter routes this way.
> > > > > 
> > > > > But it wasn't like this before 2b760fcf5cfb, so this smells like
> > > > > breaking userspace expectations, even though iproute already filters
> > > > > routes this way: with 'cache' it only displays routes with
> > > > > RTM_F_CLONED, without, it won't display exceptions, see filter_nlmsg():      
> > > > Thanks for pointing it out.
> > > >     
> > > > > 	if (filter.cloned == !(r->rtm_flags & RTM_F_CLONED))
> > > > > 		return 0;
> > > > > 
> > > > > This, together with the fact it's been like that for almost two years
> > > > > now, makes it acceptable in my opinion. What do you think?      
> > > > With learning the above fact on iproute2,
> > > > it makes even less sense to dump exceptions from the kernel side
> > > > when RTM_F_CLONED is not set.    
> > > 
> > > I just hit a more fundamental problem though: iproute2 filters on the
> > > flag, but never sets it on a dump request. Flags will be NLM_F_DUMP |
> > > NLM_F_REQUEST, no matter what, see rtnl_routedump_req(). So the current
> > > iproute2 would have no way to dump cached routes.  
> > 
> > Partially wrong: it actually sets it on 'list':
> > 
> > 	if (rtnl_routedump_req(&rth, dump_family, iproute_dump_filter) < 0) {
> > 
> > [...]
> > static int iproute_dump_filter(struct nlmsghdr *nlh, int reqlen)
> > [...]
> > 	if (filter.cloned)
> > 		rtm->rtm_flags |= RTM_F_CLONED;
> > 
> > but not on 'flush':
> > 
> > 		if (rtnl_routedump_req(&rth, family, NULL) < 0) {
> > 
> > but this doesn't change things much: it still has no way to flush the
> > cache, because the dump to get the routes to flush doesn't contain the
> > exceptions.  
> 'ip -6 r l table cache' can be limited to dump the cache only, right?

Yes, at it was in v1 and v2. But that's arbitrary and inconsistent,
because without RTM_F_CLONED we would anyway need (with current
iproute2) to dump exceptions too.

RTM_F_CLONED just isn't a filter right now. Let's add support for
NLM_F_MATCH later and have clear semantics. I think this whole mess
comes from the fact we miss that.

> I am still missing something about why the kernel is required
> to output everything and then filtered out in the iproute2.
> 
> You meant either:
> The kernel needs to dump everything first. iproute2 can then figure out
> which one is cache and then flush them?

Yes, this is the case, right now.

> or
> the iproute2 can be changed to only get the cache from the kernel and then
> flush them?

I didn't mean this. In theory, we could, but I wouldn't fix a kernel
regression with a userspace change. I'm not adding NLM_F_MATCH support
in this fix because it wouldn't be a fix for past and current iproute2
versions, so it wouldn't be a fix.

> AFAIK, the kernel has never dumped the cache routes for IPv4.

Right now, it doesn't, but I can't believe that 'ip route list cache'
was implemented in iproute2 and it never worked. I haven't tested IPv4
with older kernels, though.

> What is done here has to be consistent with the future patch in IPv4.
> Each node can hold up to 5*1024 caches which is ok-ish but still a waste
> to dump it and then not printing it.

Indeed, I agree. For both IPv4 and IPv6, we need to have clear
semantics to avoid this waste. NLM_F_MATCH is specified by RFC 3549
exactly for this purpose, but we don't implement that.

Without NLM_F_MATCH, RTM_F_CLONED is just what the comment in UAPI says:

#define RTM_F_CLONED		0x200	/* This route is cloned		*/

So the future patch for IPv4 would be consistent: without NLM_F_MATCH
we'll dump everything and iproute2 filters (as it was until 2017 at
least for IPv6).

Then I'd add, for both IPv4 and IPv6, support for NLM_F_MATCH (both
for kernel and iproute2): kernel filters, sends NLM_F_DUMP_FILTERED
back and iproute2 doesn't (need to) filter. But this will be a (kind of
needed) optimisation, not a fix.

-- 
Stefano

Re: [PATCH net 1/2] ipv6: Dump route exceptions too in rt6_dump_route()

[Vaittinen, Matti]

Hi Dee Ho Peeps!

Wow Stefano, you seem to be quite a detective :) How on earth did you
match my new email to this sole netdev intrusion done back at the 2011
%) Impressive!

On Sat, 2019-06-08 at 17:02 +0200, Stefano Brivio wrote:

> 
> - retry adding NLM_F_MATCH (for net-next and iproute-next) according
>   to RFC 3549. Things changed a bit from 2011: we now have
>   NLM_F_DUMP_FILTERED, iproute2 already uses it (ip neigh) and we
>   wouldn't need to make iproute2 more complicated by handling old/new
>   kernel cases. So I think this would be reasonable now.
> 
I am pretty sure the iproute would not have become more complicated
back in 2011 even if we did push this change back then. iproute2 could
have chosen to stick with own userspace filtering - supporting the
NLM_F_MATCH flag back then would not have broken that. And if we did it
back then - there now probably was some other tools utilizing the
kernel filtering - and today the iproute2 could pretty safely drop the
user-space route filtering code and transition to do filtering already
in kernel. Well, that's a bit late to say today :)

But yes, this unfinished thing has indeed haunted me during some black
nights =) I would be delighted to see the proper NLM_F_MATCH support in
kernel.

What stopped me back in the 2011 was actually Dave's comment that even
if he could consider applying this change he would require it for IPv4
too. And that makes perfect sense. It was just too much for me back
then. I guess this has not changed - IPv6 and IPv4 should still handle
these flags in a same way.

Br,
	Matti Vaittinen

Re: [PATCH net 1/2] ipv6: Dump route exceptions too in rt6_dump_route()

[Stefano Brivio]

Hi Matti,

On Mon, 10 Jun 2019 05:56:03 +0000
"Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com> wrote:

> Hi Dee Ho Peeps!
> 
> Wow Stefano, you seem to be quite a detective :) How on earth did you
> match my new email to this sole netdev intrusion done back at the 2011
> %) Impressive!

I was looking for lost UDP jokes. :)

> On Sat, 2019-06-08 at 17:02 +0200, Stefano Brivio wrote:
> 
> > 
> > - retry adding NLM_F_MATCH (for net-next and iproute-next) according
> >   to RFC 3549. Things changed a bit from 2011: we now have
> >   NLM_F_DUMP_FILTERED, iproute2 already uses it (ip neigh) and we
> >   wouldn't need to make iproute2 more complicated by handling old/new
> >   kernel cases. So I think this would be reasonable now.
> >   
> I am pretty sure the iproute would not have become more complicated
> back in 2011 even if we did push this change back then. iproute2 could
> have chosen to stick with own userspace filtering - supporting the
> NLM_F_MATCH flag back then would not have broken that. And if we did it
> back then - there now probably was some other tools utilizing the
> kernel filtering - and today the iproute2 could pretty safely drop the
> user-space route filtering code and transition to do filtering already
> in kernel. Well, that's a bit late to say today :)

Well, yes, it wouldn't have become more complicated. The difference
today is that, with NLM_F_DUMP_FILTERED codifying this, we could safely
and simply skip userspace filtering with just two lines, something like:

	if (n->nlmsg_flags & RTM_F_DUMP_FILTERED)
		return 1;

in filter_nlmsg().

> But yes, this unfinished thing has indeed haunted me during some black
> nights =) I would be delighted to see the proper NLM_F_MATCH support in
> kernel.
> 
> What stopped me back in the 2011 was actually Dave's comment that even
> if he could consider applying this change he would require it for IPv4
> too. And that makes perfect sense. It was just too much for me back
> then. I guess this has not changed - IPv6 and IPv4 should still handle
> these flags in a same way.

Indeed, we'll have to take care of both. It's on my to do list now, I
should get to it soon.

Right now my priority is to fix the fact that, at least with iproute2,
flushing IPv6 routed caches is simply not possible anymore. This looks
like a serious breakage to me.

-- 
Stefano

Re: [PATCH net 1/2] ipv6: Dump route exceptions too in rt6_dump_route()

[Martin Lau]

On Thu, Jun 06, 2019 at 10:13:41PM +0200, Stefano Brivio wrote:
> Since commit 2b760fcf5cfb ("ipv6: hook up exception table to store dst
> cache"), route exceptions reside in a separate hash table, and won't be
> found by walking the FIB, so they won't be dumped to userspace on a
> RTM_GETROUTE message.
> 
> This causes 'ip -6 route list cache' and 'ip -6 route flush cache' to
> have no function anymore:
> 
>  # ip -6 route get fc00:3::1
>  fc00:3::1 via fc00:1::2 dev veth_A-R1 src fc00:1::1 metric 1024 expires 539sec mtu 1400 pref medium
>  # ip -6 route get fc00:4::1
>  fc00:4::1 via fc00:2::2 dev veth_A-R2 src fc00:2::1 metric 1024 expires 536sec mtu 1500 pref medium
>  # ip -6 route list cache
>  # ip -6 route flush cache
>  # ip -6 route get fc00:3::1
>  fc00:3::1 via fc00:1::2 dev veth_A-R1 src fc00:1::1 metric 1024 expires 520sec mtu 1400 pref medium
>  # ip -6 route get fc00:4::1
>  fc00:4::1 via fc00:2::2 dev veth_A-R2 src fc00:2::1 metric 1024 expires 519sec mtu 1500 pref medium
> 
> because iproute2 lists cached routes using RTM_GETROUTE, and flushes them
> by listing all the routes, and deleting them with RTM_DELROUTE one by one.
> 
> Look up exceptions in the hash table associated with the current fib6_info
> in rt6_dump_route(), and, if present and not expired, add them to the
> dump.
> 
> Re-allow userspace to get FIB results by passing the RTM_F_CLONED flag as
> filter, by reverting commit 08e814c9e8eb ("net/ipv6: Bail early if user
> only wants cloned entries").
> 
> As we do this, we also have to honour this flag while filtering routes in
> rt6_dump_route() and, if this filter effectively causes some results to be
> discarded, by passing the NLM_F_DUMP_FILTERED flag back.
> 
> To flush cached routes, a procfs entry could be introduced instead: that's
> how it works for IPv4. We already have a rt6_flush_exception() function
> ready to be wired to it. However, this would not solve the issue for
> listing, and wouldn't fix the issue with current and previous versions of
> iproute2.
> 
> Reported-by: Jianlin Shi <jishi@redhat.com>
> Fixes: 2b760fcf5cfb ("ipv6: hook up exception table to store dst cache")
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
> This will cause a non-trivial conflict with commit cc5c073a693f
> ("ipv6: Move exception bucket to fib6_nh") on net-next. I can submit
> an equivalent patch against net-next, if it helps.
> 
>  net/ipv6/ip6_fib.c |  7 ++-----
>  net/ipv6/route.c   | 38 +++++++++++++++++++++++++++++++++++---
>  2 files changed, 37 insertions(+), 8 deletions(-)
> 
> diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
> index 008421b550c6..5be133565819 100644
> --- a/net/ipv6/ip6_fib.c
> +++ b/net/ipv6/ip6_fib.c
> @@ -581,13 +581,10 @@ static int inet6_dump_fib(struct sk_buff *skb, struct netlink_callback *cb)
>  	} else if (nlmsg_len(nlh) >= sizeof(struct rtmsg)) {
>  		struct rtmsg *rtm = nlmsg_data(nlh);
>  
> -		arg.filter.flags = rtm->rtm_flags & (RTM_F_PREFIX|RTM_F_CLONED);
> +		if (rtm->rtm_flags & RTM_F_PREFIX)
> +			arg.filter.flags = RTM_F_PREFIX;
>  	}
>  
> -	/* fib entries are never clones */
> -	if (arg.filter.flags & RTM_F_CLONED)
> -		goto out;
> -
>  	w = (void *)cb->args[2];
>  	if (!w) {
>  		/* New dump:
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 848e944f07df..51f923b3ad26 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -4862,8 +4862,11 @@ int rt6_dump_route(struct fib6_info *rt, void *p_arg)
>  {
>  	struct rt6_rtnl_dump_arg *arg = (struct rt6_rtnl_dump_arg *) p_arg;
>  	struct fib_dump_filter *filter = &arg->filter;
> +	struct rt6_exception_bucket *bucket;
>  	unsigned int flags = NLM_F_MULTI;
> +	struct rt6_exception *rt6_ex;
>  	struct net *net = arg->net;
> +	int i, err;
>  
>  	if (rt == net->ipv6.fib6_null_entry)
>  		return 0;
> @@ -4882,9 +4885,38 @@ int rt6_dump_route(struct fib6_info *rt, void *p_arg)
>  		flags |= NLM_F_DUMP_FILTERED;
>  	}
>  
> -	return rt6_fill_node(net, arg->skb, rt, NULL, NULL, NULL, 0,
> -			     RTM_NEWROUTE, NETLINK_CB(arg->cb->skb).portid,
> -			     arg->cb->nlh->nlmsg_seq, flags);
> +	if (!(filter->flags & RTM_F_CLONED)) {
> +		err = rt6_fill_node(net, arg->skb, rt, NULL, NULL, NULL, 0,
> +				    RTM_NEWROUTE,
> +				    NETLINK_CB(arg->cb->skb).portid,
> +				    arg->cb->nlh->nlmsg_seq, flags);
> +		if (err)
> +			return err;
> +	} else {
> +		flags |= NLM_F_DUMP_FILTERED;
> +	}
> +
> +	bucket = rcu_dereference(rt->rt6i_exception_bucket);
> +	if (!bucket)
> +		return 0;
> +
> +	for (i = 0; i < FIB6_EXCEPTION_BUCKET_SIZE; i++) {
> +		hlist_for_each_entry(rt6_ex, &bucket->chain, hlist) {
> +			if (rt6_check_expired(rt6_ex->rt6i))
> +				continue;
> +
> +			err = rt6_fill_node(net, arg->skb, rt,
> +					    &rt6_ex->rt6i->dst,
> +					    NULL, NULL, 0, RTM_NEWROUTE,
> +					    NETLINK_CB(arg->cb->skb).portid,
> +					    arg->cb->nlh->nlmsg_seq, flags);
Thanks for the patch.

A question on when rt6_fill_node() returns -EMSGSIZE while dumping the
exception bucket here.  Where will the next inet6_dump_fib() start?

> +			if (err)
> +				return err;
> +		}
> +		bucket++;
> +	}
> +
> +	return 0;
>  }
>  
>  static int inet6_rtm_valid_getroute_req(struct sk_buff *skb,
> -- 
> 2.20.1
> 

Re: [PATCH net 1/2] ipv6: Dump route exceptions too in rt6_dump_route()

[Stefano Brivio]

On Thu, 6 Jun 2019 21:44:58 +0000
Martin Lau <kafai@fb.com> wrote:

> > +	if (!(filter->flags & RTM_F_CLONED)) {
> > +		err = rt6_fill_node(net, arg->skb, rt, NULL, NULL, NULL, 0,
> > +				    RTM_NEWROUTE,
> > +				    NETLINK_CB(arg->cb->skb).portid,
> > +				    arg->cb->nlh->nlmsg_seq, flags);
> > +		if (err)
> > +			return err;
> > +	} else {
> > +		flags |= NLM_F_DUMP_FILTERED;
> > +	}
> > +
> > +	bucket = rcu_dereference(rt->rt6i_exception_bucket);
> > +	if (!bucket)
> > +		return 0;
> > +
> > +	for (i = 0; i < FIB6_EXCEPTION_BUCKET_SIZE; i++) {
> > +		hlist_for_each_entry(rt6_ex, &bucket->chain, hlist) {
> > +			if (rt6_check_expired(rt6_ex->rt6i))
> > +				continue;
> > +
> > +			err = rt6_fill_node(net, arg->skb, rt,
> > +					    &rt6_ex->rt6i->dst,
> > +					    NULL, NULL, 0, RTM_NEWROUTE,
> > +					    NETLINK_CB(arg->cb->skb).portid,
> > +					    arg->cb->nlh->nlmsg_seq, flags);  
> Thanks for the patch.
> 
> A question on when rt6_fill_node() returns -EMSGSIZE while dumping the
> exception bucket here.  Where will the next inet6_dump_fib() start?

And thanks for reviewing.

It starts again from the same node, see fib6_dump_node(): w->leaf = rt;
where rt is the fib6_info where we failed dumping, so we won't skip
dumping any node.

This also means that to avoid sending duplicates in the case where at
least one rt6_fill_node() call goes through and one fails, we would
need to track the last bucket and entry sent, or, alternatively, to
make sure we can fit the whole node before dumping.

I don't think that can happen in practice, or at least I haven't found a
way to create enough valid exceptions for the same node.

Anyway, I guess that would be nicer, but the fix is going to be much
bigger, and I don't think we even have to guarantee that. I'd rather
take care of that as a follow-up. Any preferred solution by the way?

-- 
Stefano

Re: [PATCH net 1/2] ipv6: Dump route exceptions too in rt6_dump_route()

[Martin Lau]

On Fri, Jun 07, 2019 at 12:17:47AM +0200, Stefano Brivio wrote:
> On Thu, 6 Jun 2019 21:44:58 +0000
> Martin Lau <kafai@fb.com> wrote:
> 
> > > +	if (!(filter->flags & RTM_F_CLONED)) {
> > > +		err = rt6_fill_node(net, arg->skb, rt, NULL, NULL, NULL, 0,
> > > +				    RTM_NEWROUTE,
> > > +				    NETLINK_CB(arg->cb->skb).portid,
> > > +				    arg->cb->nlh->nlmsg_seq, flags);
> > > +		if (err)
> > > +			return err;
> > > +	} else {
> > > +		flags |= NLM_F_DUMP_FILTERED;
> > > +	}
> > > +
> > > +	bucket = rcu_dereference(rt->rt6i_exception_bucket);
> > > +	if (!bucket)
> > > +		return 0;
> > > +
> > > +	for (i = 0; i < FIB6_EXCEPTION_BUCKET_SIZE; i++) {
> > > +		hlist_for_each_entry(rt6_ex, &bucket->chain, hlist) {
> > > +			if (rt6_check_expired(rt6_ex->rt6i))
> > > +				continue;
> > > +
> > > +			err = rt6_fill_node(net, arg->skb, rt,
> > > +					    &rt6_ex->rt6i->dst,
> > > +					    NULL, NULL, 0, RTM_NEWROUTE,
> > > +					    NETLINK_CB(arg->cb->skb).portid,
> > > +					    arg->cb->nlh->nlmsg_seq, flags);  
> > Thanks for the patch.
> > 
> > A question on when rt6_fill_node() returns -EMSGSIZE while dumping the
> > exception bucket here.  Where will the next inet6_dump_fib() start?
> 
> And thanks for reviewing.
> 
> It starts again from the same node, see fib6_dump_node(): w->leaf = rt;
> where rt is the fib6_info where we failed dumping, so we won't skip
> dumping any node.
If the same node will be dumped, does it mean that it will go through this
loop and iterate all exceptions again?

> 
> This also means that to avoid sending duplicates in the case where at
> least one rt6_fill_node() call goes through and one fails, we would
> need to track the last bucket and entry sent, or, alternatively, to
> make sure we can fit the whole node before dumping.
My another concern is the dump may never finish.

> 
> I don't think that can happen in practice, or at least I haven't found a
> way to create enough valid exceptions for the same node.
That I am not sure.  It is not unusual to have many pmtu exceptions in
a gateway node.

> 
> Anyway, I guess that would be nicer, but the fix is going to be much
> bigger, and I don't think we even have to guarantee that. I'd rather
> take care of that as a follow-up. Any preferred solution by the way?
> 
> -- 
> Stefano

Re: [PATCH net 1/2] ipv6: Dump route exceptions too in rt6_dump_route()

[David Ahern]

On 6/6/19 4:37 PM, Martin Lau wrote:
>> I don't think that can happen in practice, or at least I haven't found a
>> way to create enough valid exceptions for the same node.
> That I am not sure.  It is not unusual to have many pmtu exceptions in
> a gateway node.
> 

yes.

Stefano: you could generalize this test script
   http://patchwork.ozlabs.org/patch/1110802/
to have N-remote hosts

Re: [PATCH net 1/2] ipv6: Dump route exceptions too in rt6_dump_route()

[Stefano Brivio]

On Thu, 6 Jun 2019 16:48:51 -0600
David Ahern <dsahern@gmail.com> wrote:

> On 6/6/19 4:37 PM, Martin Lau wrote:
> >> I don't think that can happen in practice, or at least I haven't found a
> >> way to create enough valid exceptions for the same node.  
> > That I am not sure.  It is not unusual to have many pmtu exceptions in
> > a gateway node.
> >   
> 
> yes.
> 
> Stefano: you could generalize this test script
>    http://patchwork.ozlabs.org/patch/1110802/
> to have N-remote hosts

Right, thanks for the pointer. I ended up doing something like that in
pmtu.sh, and it turns out that, starting from 25 exceptions in the same
node, iproute2 doesn't actually retry with a larger buffer. As Martin
predicted (thanks!) the dump doesn't terminate.

I tested a version that counts the number of routes in a partial dump
and skips them on the next one with 10,000 entries, dump terminates and
entries count is consistent (at some point, the buckets are just full,
and number of entries doesn't increase any longer).

Unfortunately, the setup of the test takes a few minutes, so I wouldn't
include it (at least as it is) in the selftest.

I'll post that as v2 soon.

-- 
Stefano

Re: [PATCH net 1/2] ipv6: Dump route exceptions too in rt6_dump_route()

[Stefano Brivio]

On Thu, 6 Jun 2019 22:37:11 +0000
Martin Lau <kafai@fb.com> wrote:

> On Fri, Jun 07, 2019 at 12:17:47AM +0200, Stefano Brivio wrote:
> > On Thu, 6 Jun 2019 21:44:58 +0000
> > Martin Lau <kafai@fb.com> wrote:
> >   
> > > > +	if (!(filter->flags & RTM_F_CLONED)) {
> > > > +		err = rt6_fill_node(net, arg->skb, rt, NULL, NULL, NULL, 0,
> > > > +				    RTM_NEWROUTE,
> > > > +				    NETLINK_CB(arg->cb->skb).portid,
> > > > +				    arg->cb->nlh->nlmsg_seq, flags);
> > > > +		if (err)
> > > > +			return err;
> > > > +	} else {
> > > > +		flags |= NLM_F_DUMP_FILTERED;
> > > > +	}
> > > > +
> > > > +	bucket = rcu_dereference(rt->rt6i_exception_bucket);
> > > > +	if (!bucket)
> > > > +		return 0;
> > > > +
> > > > +	for (i = 0; i < FIB6_EXCEPTION_BUCKET_SIZE; i++) {
> > > > +		hlist_for_each_entry(rt6_ex, &bucket->chain, hlist) {
> > > > +			if (rt6_check_expired(rt6_ex->rt6i))
> > > > +				continue;
> > > > +
> > > > +			err = rt6_fill_node(net, arg->skb, rt,
> > > > +					    &rt6_ex->rt6i->dst,
> > > > +					    NULL, NULL, 0, RTM_NEWROUTE,
> > > > +					    NETLINK_CB(arg->cb->skb).portid,
> > > > +					    arg->cb->nlh->nlmsg_seq, flags);    
> > > Thanks for the patch.
> > > 
> > > A question on when rt6_fill_node() returns -EMSGSIZE while dumping the
> > > exception bucket here.  Where will the next inet6_dump_fib() start?  
> > 
> > And thanks for reviewing.
> > 
> > It starts again from the same node, see fib6_dump_node(): w->leaf = rt;
> > where rt is the fib6_info where we failed dumping, so we won't skip
> > dumping any node.  
> If the same node will be dumped, does it mean that it will go through this
> loop and iterate all exceptions again?

Yes (well, all the exceptions for that node).

> > This also means that to avoid sending duplicates in the case where at
> > least one rt6_fill_node() call goes through and one fails, we would
> > need to track the last bucket and entry sent, or, alternatively, to
> > make sure we can fit the whole node before dumping.  
> My another concern is the dump may never finish.

That's not a guarantee in general, even without this, because in theory
the skb passed might be small enough that we can't even fit a single
node without exceptions.

We could add a guard on w->leaf not being the same before and after the
walk in inet6_dump_fib() and, if it is, terminate the dump. I just
wonder if we have to do this at all -- I can't find this being done
anywhere else (at a quick look at least).

By the way, we can also trigger a never-ending dump by touching the
tree frequently enough during a dump: it would always start again from
the root, see fib6_dump_table().

-- 
Stefano

Re: [PATCH net 1/2] ipv6: Dump route exceptions too in rt6_dump_route()

[Stefano Brivio]

On Fri, 7 Jun 2019 00:58:52 +0200
Stefano Brivio <sbrivio@redhat.com> wrote:

> On Thu, 6 Jun 2019 22:37:11 +0000
> Martin Lau <kafai@fb.com> wrote:
> 
> > On Fri, Jun 07, 2019 at 12:17:47AM +0200, Stefano Brivio wrote:  
> > > On Thu, 6 Jun 2019 21:44:58 +0000
> > > Martin Lau <kafai@fb.com> wrote:
> > >     
> > > > > +	if (!(filter->flags & RTM_F_CLONED)) {
> > > > > +		err = rt6_fill_node(net, arg->skb, rt, NULL, NULL, NULL, 0,
> > > > > +				    RTM_NEWROUTE,
> > > > > +				    NETLINK_CB(arg->cb->skb).portid,
> > > > > +				    arg->cb->nlh->nlmsg_seq, flags);
> > > > > +		if (err)
> > > > > +			return err;
> > > > > +	} else {
> > > > > +		flags |= NLM_F_DUMP_FILTERED;
> > > > > +	}
> > > > > +
> > > > > +	bucket = rcu_dereference(rt->rt6i_exception_bucket);
> > > > > +	if (!bucket)
> > > > > +		return 0;
> > > > > +
> > > > > +	for (i = 0; i < FIB6_EXCEPTION_BUCKET_SIZE; i++) {
> > > > > +		hlist_for_each_entry(rt6_ex, &bucket->chain, hlist) {
> > > > > +			if (rt6_check_expired(rt6_ex->rt6i))
> > > > > +				continue;
> > > > > +
> > > > > +			err = rt6_fill_node(net, arg->skb, rt,
> > > > > +					    &rt6_ex->rt6i->dst,
> > > > > +					    NULL, NULL, 0, RTM_NEWROUTE,
> > > > > +					    NETLINK_CB(arg->cb->skb).portid,
> > > > > +					    arg->cb->nlh->nlmsg_seq, flags);      
> > > > Thanks for the patch.
> > > > 
> > > > A question on when rt6_fill_node() returns -EMSGSIZE while dumping the
> > > > exception bucket here.  Where will the next inet6_dump_fib() start?    
> > > 
> > > And thanks for reviewing.
> > > 
> > > It starts again from the same node, see fib6_dump_node(): w->leaf = rt;
> > > where rt is the fib6_info where we failed dumping, so we won't skip
> > > dumping any node.    
> > If the same node will be dumped, does it mean that it will go through this
> > loop and iterate all exceptions again?  
> 
> Yes (well, all the exceptions for that node).
> 
> > > This also means that to avoid sending duplicates in the case where at
> > > least one rt6_fill_node() call goes through and one fails, we would
> > > need to track the last bucket and entry sent, or, alternatively, to
> > > make sure we can fit the whole node before dumping.    
> > My another concern is the dump may never finish.  
> 
> That's not a guarantee in general, even without this, because in theory
> the skb passed might be small enough that we can't even fit a single
> node without exceptions.
> 
> We could add a guard on w->leaf not being the same before and after the
> walk in inet6_dump_fib() and, if it is, terminate the dump. I just
> wonder if we have to do this at all -- I can't find this being done
> anywhere else (at a quick look at least).

I still can't convince myself this is an actual issue, but... somewhat
simpler: let's add a field to fib6_walker, that counts the entries
(both from FIB and exceptions) already dumped for the current node:

		res = rt6_dump_route(rt, w->args);
		if (res) {
			/* Frame is full, suspend walking */
			w->leaf = rt;
			w->skip_node = res;
			return 1;
		}

if the current leaf changes (tree changed), we reset it. And we use that
to skip rt6_fill_node() calls in rt6_dump_route(). What do you think?

-- 
Stefano

Re: [PATCH net 1/2] ipv6: Dump route exceptions too in rt6_dump_route()

[David Ahern]

On 6/6/19 4:58 PM, Stefano Brivio wrote:
>>> This also means that to avoid sending duplicates in the case where at
>>> least one rt6_fill_node() call goes through and one fails, we would
>>> need to track the last bucket and entry sent, or, alternatively, to
>>> make sure we can fit the whole node before dumping.  
>> My another concern is the dump may never finish.
> 
> That's not a guarantee in general, even without this, because in theory
> the skb passed might be small enough that we can't even fit a single
> node without exceptions.

That should be handled by skb->len = 0 and then returning the err back
to caller. See inet_dump_fib.

> 
> We could add a guard on w->leaf not being the same before and after the
> walk in inet6_dump_fib() and, if it is, terminate the dump. I just
> wonder if we have to do this at all -- I can't find this being done
> anywhere else (at a quick look at least).
> 
> By the way, we can also trigger a never-ending dump by touching the
> tree frequently enough during a dump: it would always start again from
> the root, see fib6_dump_table().
> 

that should be a userspace problem on a sequence mismatch. In-kernel
notifiers do restart for offload drivers (seeregister_fib_notifier), but
as I recall the kernel should not restart a dump.

libnl reference is nl_cache_refill,

                err = nl_cache_pickup(sk, cache);
                if (err == -NLE_DUMP_INTR) {
                        NL_DBG(2, "Dump interrupted, restarting!\n");
                        goto restart;
                } else if (err < 0)
                        break;

Re: [PATCH net 1/2] ipv6: Dump route exceptions too in rt6_dump_route()

[Martin Lau]

On Fri, Jun 07, 2019 at 12:58:52AM +0200, Stefano Brivio wrote:
> On Thu, 6 Jun 2019 22:37:11 +0000
> Martin Lau <kafai@fb.com> wrote:
> 
> > On Fri, Jun 07, 2019 at 12:17:47AM +0200, Stefano Brivio wrote:
> > > On Thu, 6 Jun 2019 21:44:58 +0000
> > > Martin Lau <kafai@fb.com> wrote:
> > >   
> > > > > +	if (!(filter->flags & RTM_F_CLONED)) {
> > > > > +		err = rt6_fill_node(net, arg->skb, rt, NULL, NULL, NULL, 0,
> > > > > +				    RTM_NEWROUTE,
> > > > > +				    NETLINK_CB(arg->cb->skb).portid,
> > > > > +				    arg->cb->nlh->nlmsg_seq, flags);
> > > > > +		if (err)
> > > > > +			return err;
> > > > > +	} else {
> > > > > +		flags |= NLM_F_DUMP_FILTERED;
> > > > > +	}
> > > > > +
> > > > > +	bucket = rcu_dereference(rt->rt6i_exception_bucket);
> > > > > +	if (!bucket)
> > > > > +		return 0;
> > > > > +
> > > > > +	for (i = 0; i < FIB6_EXCEPTION_BUCKET_SIZE; i++) {
> > > > > +		hlist_for_each_entry(rt6_ex, &bucket->chain, hlist) {
> > > > > +			if (rt6_check_expired(rt6_ex->rt6i))
> > > > > +				continue;
> > > > > +
> > > > > +			err = rt6_fill_node(net, arg->skb, rt,
> > > > > +					    &rt6_ex->rt6i->dst,
> > > > > +					    NULL, NULL, 0, RTM_NEWROUTE,
> > > > > +					    NETLINK_CB(arg->cb->skb).portid,
> > > > > +					    arg->cb->nlh->nlmsg_seq, flags);    
> > > > Thanks for the patch.
> > > > 
> > > > A question on when rt6_fill_node() returns -EMSGSIZE while dumping the
> > > > exception bucket here.  Where will the next inet6_dump_fib() start?  
> > > 
> > > And thanks for reviewing.
> > > 
> > > It starts again from the same node, see fib6_dump_node(): w->leaf = rt;
> > > where rt is the fib6_info where we failed dumping, so we won't skip
> > > dumping any node.  
> > If the same node will be dumped, does it mean that it will go through this
> > loop and iterate all exceptions again?
> 
> Yes (well, all the exceptions for that node).
> 
> > > This also means that to avoid sending duplicates in the case where at
> > > least one rt6_fill_node() call goes through and one fails, we would
> > > need to track the last bucket and entry sent, or, alternatively, to
> > > make sure we can fit the whole node before dumping.  
> > My another concern is the dump may never finish.
> 
> That's not a guarantee in general, even without this, because in theory
> the skb passed might be small enough that we can't even fit a single
> node without exceptions.
That is arguably the caller's responsibility to retry
with a larger buffer if it cannot even get a single route.

If caller provides a large enough buffer for a single route,
the kernel should guarantee forward progress.

I think the minimum is to remember how many exceptions have to be
skipped.

> 
> We could add a guard on w->leaf not being the same before and after the
> walk in inet6_dump_fib() and, if it is, terminate the dump. I just
> wonder if we have to do this at all -- I can't find this being done
> anywhere else (at a quick look at least).
> 
> By the way, we can also trigger a never-ending dump by touching the
> tree frequently enough during a dump: it would always start again from
> the root, see fib6_dump_table().
This case "cb->args[5] != w->root->fn_sernum"?  It seems there is a w->skip
to take care of it.

Regardless, I don't think we should make it worse.

[PATCH net 2/2] ip6_fib: Don't discard nodes with valid routing information in fib6_locate_1()

[Stefano Brivio]

When we perform an inexact match on FIB nodes via fib6_locate_1(), longer
prefixes will be preferred to shorter ones. However, it might happen that
a node, with higher fn_bit value than some other, has no valid routing
information.

In this case, we'll pick that node, but it will be discarded by the check
on RTN_RTINFO in fib6_locate(), and we might miss nodes with valid routing
information but with lower fn_bit value.

This is apparent when a routing exception is created for a default route:
 # ip -6 route list
 fc00:1::/64 dev veth_A-R1 proto kernel metric 256 pref medium
 fc00:2::/64 dev veth_A-R2 proto kernel metric 256 pref medium
 fc00:4::1 via fc00:2::2 dev veth_A-R2 metric 1024 pref medium
 fe80::/64 dev veth_A-R1 proto kernel metric 256 pref medium
 fe80::/64 dev veth_A-R2 proto kernel metric 256 pref medium
 default via fc00:1::2 dev veth_A-R1 metric 1024 pref medium
 # ip -6 route list cache
 fc00:4::1 via fc00:2::2 dev veth_A-R2 metric 1024 expires 593sec mtu 1500 pref medium
 fc00:3::1 via fc00:1::2 dev veth_A-R1 metric 1024 expires 593sec mtu 1500 pref medium
 # ip -6 route flush cache    # node for default route is discarded
 Failed to send flush request: No such process
 # ip -6 route list cache
 fc00:3::1 via fc00:1::2 dev veth_A-R1 metric 1024 expires 586sec mtu 1500 pref medium

Check right away if the node has a RTN_RTINFO flag, before replacing the
'prev' pointer, that indicates the longest matching prefix found so far.

Fixes: 38fbeeeeccdb ("ipv6: prepare fib6_locate() for exception table")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 net/ipv6/ip6_fib.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index 5be133565819..d8b9f4d801e3 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -1537,7 +1537,8 @@ static struct fib6_node *fib6_locate_1(struct fib6_node *root,
 		if (plen == fn->fn_bit)
 			return fn;
 
-		prev = fn;
+		if (fn->fn_flags & RTN_RTINFO)
+			prev = fn;
 
 next:
 		/*
-- 
2.20.1