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: <20190610194238.3gke27kflrocrpwo@kafai-mbp.dhcp.thefacebook.com>
Date:   Mon, 10 Jun 2019 19:42:41 +0000
From:   Martin Lau <kafai@...com>
To:     Stefano Brivio <sbrivio@...hat.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, Jun 08, 2019 at 05:47:07PM +0200, Stefano Brivio wrote:
> On Sat, 8 Jun 2019 17:02:06 +0200
> Stefano Brivio <sbrivio@...hat.com> wrote:
> 
> > 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.
> 
> 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.