netdev - Re: [PATCH net 1/2] ipv6: Dump route exceptions too in rt6_dump

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190608170206.4fa108f5@redhat.com>
Date:   Sat, 8 Jun 2019 17:02:06 +0200
From:   Stefano Brivio <sbrivio@...hat.com>
To:     Martin Lau <kafai@...com>
Cc:     David Ahern <dsahern@...il.com>,
        David Miller <davem@...emloft.net>,
        Jianlin Shi <jishi@...hat.com>, Wei Wang <weiwan@...gle.com>,
        Eric Dumazet <edumazet@...gle.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Matti Vaittinen <matti.vaittinen@...rohmeurope.com>
Subject: Re: [PATCH net 1/2] ipv6: Dump route exceptions too in
 rt6_dump_route()

On Sat, 8 Jun 2019 07:19:23 +0000
Martin Lau <kafai@...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