| Message ID | cover.1560016091.git.sbrivio@redhat.com |
|---|---|
| Headers | show |
| Series | ipv6: Fix listing and flushing of cached route exceptions | expand |
On 6/8/19 12:12 PM, Stefano Brivio wrote: > 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. The breakage is the limited ability to remove exceptions. Yes, you can delete a v6 exception route if you know it exists. Without the ability to list them, you have to guess. The ability to list exceptions was deleted 2 years ago with 4.15. So far no one has complained that exceptions do not show up in route dumps. Rather than perturb the system again and worse with different behaviors, in dot releases of stable trees, I think it would be better to converge on consistent behavior between v4 and v6. By that I mean without the CLONED flag, no exceptions are returned (default FIB dump). With the CLONED flag only exceptions are returned.
On Mon, Jun 10, 2019 at 03:38:06PM -0600, David Ahern wrote: > The ability to list exceptions was deleted 2 years ago with 4.15. So far > no one has complained that exceptions do not show up in route dumps. > Rather than perturb the system again and worse with different behaviors, > in dot releases of stable trees, I think it would be better to converge > on consistent behavior between v4 and v6. By that I mean without the > CLONED flag, no exceptions are returned (default FIB dump). With the > CLONED flag only exceptions are returned. +1
On Mon, 10 Jun 2019 15:38:06 -0600 David Ahern <dsahern@gmail.com> wrote: > On 6/8/19 12:12 PM, Stefano Brivio wrote: > > 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. > > The breakage is the limited ability to remove exceptions. Yes, you can > delete a v6 exception route if you know it exists. Without the ability > to list them, you have to guess. > > The ability to list exceptions was deleted 2 years ago with 4.15. So far > no one has complained that exceptions do not show up in route dumps. I am doing it right now... > Rather than perturb the system again and worse with different behaviors, Well, I'm just trying to restore the behaviour before 2b760fcf5cfb it's not "different". I don't think 2b760fcf5cfb intended to break iproute2 like that. > in dot releases of stable trees, I think it would be better to converge > on consistent behavior between v4 and v6. By that I mean without the > CLONED flag, no exceptions are returned (default FIB dump). With the > CLONED flag only exceptions are returned. Again, this needs a change in iproute2, because RTM_F_CLONED is *not* passed on 'flush'. And sure, let's *also* do that, but not everybody runs recent versions of iproute2.
On Mon, 10 Jun 2019 23:53:15 +0200 Stefano Brivio <sbrivio@redhat.com> wrote: > On Mon, 10 Jun 2019 15:38:06 -0600 > David Ahern <dsahern@gmail.com> wrote: > > > in dot releases of stable trees, I think it would be better to converge > > on consistent behavior between v4 and v6. By that I mean without the > > CLONED flag, no exceptions are returned (default FIB dump). With the > > CLONED flag only exceptions are returned. > > Again, this needs a change in iproute2, because RTM_F_CLONED is *not* > passed on 'flush'. And sure, let's *also* do that, but not everybody > runs recent versions of iproute2. One thing that sounds a bit more acceptable to me is: - dump (in IPv4 and IPv6): - regular routes only, if !RTM_F_CLONED and NLM_F_MATCH - exceptions only, if RTM_F_CLONED and NLM_F_MATCH - everything if !NLM_F_MATCH - fix iproute2 so that RTM_F_CLONED is passed on 'flush cache', or don't pass NLM_F_MATCH in that case this way, the kernel respects the intended semantics of flags, and we fix a bug in iproute2 (that was always present). I think it's not ideal, because the kernel unexpectedly changed the behaviour and we're not guaranteeing that older iproute2 works. The fact it was broken for two years is probably a partial excuse for this, though. What do you think? I'll prepare a v4 for net-next if we all agree. I'm not entirely sure which trees I should target. I guess this introduces a feature in the kernel, so net-next, and fixes a bug in iproute2, so iproute2.git?
On Tue, Jun 11, 2019 at 12:47:58AM +0200, Stefano Brivio wrote: > On Mon, 10 Jun 2019 23:53:15 +0200 > Stefano Brivio <sbrivio@redhat.com> wrote: > > > On Mon, 10 Jun 2019 15:38:06 -0600 > > David Ahern <dsahern@gmail.com> wrote: > > > > > in dot releases of stable trees, I think it would be better to converge > > > on consistent behavior between v4 and v6. By that I mean without the > > > CLONED flag, no exceptions are returned (default FIB dump). With the > > > CLONED flag only exceptions are returned. > > > > Again, this needs a change in iproute2, because RTM_F_CLONED is *not* > > passed on 'flush'. And sure, let's *also* do that, but not everybody > > runs recent versions of iproute2. > > One thing that sounds a bit more acceptable to me is: > > - dump (in IPv4 and IPv6): > - regular routes only, if !RTM_F_CLONED and NLM_F_MATCH > - exceptions only, if RTM_F_CLONED and NLM_F_MATCH That seems reasonable since DavidAhern pointed out iproute2 already has #define NLM_F_DUMP (NLM_F_ROOT|NLM_F_MATCH) > - everything if !NLM_F_MATCH I am not sure how may the kernel change looks like. At least I don't see the current ipv6/route.c or ipv6/ip6_fib.c is handling nlmsg_flags. I would defer to DavidAhern for comment. > > - fix iproute2 so that RTM_F_CLONED is passed on 'flush cache', or I would just pass RTM_F_CLONED with NLM_F_DUMP. > don't pass NLM_F_MATCH in that case > > this way, the kernel respects the intended semantics of flags, and we > fix a bug in iproute2 (that was always present). > > I think it's not ideal, because the kernel unexpectedly changed the > behaviour and we're not guaranteeing that older iproute2 works. The > fact it was broken for two years is probably a partial excuse for this, > though. > > What do you think? I'll prepare a v4 for net-next if we all agree. > > I'm not entirely sure which trees I should target. I guess this > introduces a feature in the kernel, so net-next, and fixes a bug in > iproute2, so iproute2.git? > > -- > Stefano
On 6/11/19 2:19 PM, Martin Lau wrote: > On Tue, Jun 11, 2019 at 12:47:58AM +0200, Stefano Brivio wrote: >> On Mon, 10 Jun 2019 23:53:15 +0200 >> Stefano Brivio <sbrivio@redhat.com> wrote: >> >>> On Mon, 10 Jun 2019 15:38:06 -0600 >>> David Ahern <dsahern@gmail.com> wrote: >>> >>>> in dot releases of stable trees, I think it would be better to converge >>>> on consistent behavior between v4 and v6. By that I mean without the >>>> CLONED flag, no exceptions are returned (default FIB dump). With the >>>> CLONED flag only exceptions are returned. >>> >>> Again, this needs a change in iproute2, because RTM_F_CLONED is *not* >>> passed on 'flush'. And sure, let's *also* do that, but not everybody >>> runs recent versions of iproute2. >> >> One thing that sounds a bit more acceptable to me is: >> >> - dump (in IPv4 and IPv6): >> - regular routes only, if !RTM_F_CLONED and NLM_F_MATCH >> - exceptions only, if RTM_F_CLONED and NLM_F_MATCH > That seems reasonable since DavidAhern pointed out iproute2 already has > #define NLM_F_DUMP (NLM_F_ROOT|NLM_F_MATCH) > >> - everything if !NLM_F_MATCH > I am not sure how may the kernel change looks like. At least I don't > see the current ipv6/route.c or ipv6/ip6_fib.c is handling > nlmsg_flags. I would defer to DavidAhern for comment. We might be battling change histories in 2 different code bases. We should compare behaviors of kernel and iproute2 for 4.14 (pre-change), 4.15 (change), 4.19 (LTS), 5.0 (strict checking) and 5.2 and then look at what the proposed kernel change does with the various iproute2 versions.
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. v3: Drop check on RTM_F_CLONED and rework logic of return values of rt6_dump_route() v2: Add count of routes handled in partial dumps, and skip them, in patch 1/2. 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() include/net/ip6_fib.h | 1 + include/net/ip6_route.h | 2 +- net/ipv6/ip6_fib.c | 24 ++++++++++----- net/ipv6/route.c | 67 ++++++++++++++++++++++++++++++++++++----- 4 files changed, 78 insertions(+), 16 deletions(-)