netdev - Re: [PATCH] ipv6: don't call addrconf_dst

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: <20140108085554.GF9007@order.stressinduktion.org>
Date:	Wed, 8 Jan 2014 09:55:54 +0100
From:	Hannes Frederic Sowa <hannes@...essinduktion.org>
To:	Gao feng <gaofeng@...fujitsu.com>
Cc:	chenweilong <chenweilong@...wei.com>,
	David Miller <davem@...emloft.net>, kumaran.4353@...il.com,
	netdev@...r.kernel.org
Subject: Re: [PATCH] ipv6: don't call addrconf_dst_alloc again when enable lo

On Wed, Jan 08, 2014 at 04:42:46PM +0800, Gao feng wrote:
> On 01/08/2014 04:05 PM, Hannes Frederic Sowa wrote:
> > On Wed, Jan 08, 2014 at 03:50:09PM +0800, Gao feng wrote:
> >> On 01/03/2014 02:53 PM, Hannes Frederic Sowa wrote:
> >>> On Thu, Jan 02, 2014 at 05:33:15PM +0800, chenweilong wrote:
> >>>> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> >>>> index 62d1799..d2f8c0a 100644
> >>>> --- a/net/ipv6/addrconf.c
> >>>> +++ b/net/ipv6/addrconf.c
> >>>> @@ -2422,8 +2422,9 @@ static void init_loopback(struct net_device *dev)
> >>>>  			if (sp_ifa->flags & (IFA_F_DADFAILED | IFA_F_TENTATIVE))
> >>>>  				continue;
> >>>>
> >>>> -			if (sp_ifa->rt)
> >>>> -				continue;
> >>>> +			if (sp_ifa->rt && sp_ifa->rt->dst.dev == dev) {
> >>>> +				ip6_del_rt(sp_ifa->rt);
> >>>> +			}
> >>>>
> >>>>  			sp_rt = addrconf_dst_alloc(idev, &sp_ifa->addr, 0);
> >>>>
> >>>
> >>> Maybe this change would not be that bad after all, as those ifa attached dsts
> >>> are already dead and queued up for gc and should not get inserted back.
> >>
> >> I like this idea, maybe the below patch is better. we only need to delete this
> >> route when it has been added to garbage list.
> >>
> >> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> >> index 1a341f7..4dca886 100644
> >> --- a/net/ipv6/addrconf.c
> >> +++ b/net/ipv6/addrconf.c
> >> @@ -2610,8 +2610,16 @@ static void init_loopback(struct net_device *dev)
> >>                         if (sp_ifa->flags & (IFA_F_DADFAILED | IFA_F_TENTATIVE))
> >>                                 continue;
> >>
> >> -                       if (sp_ifa->rt)
> >> -                               continue;
> >> +                       if (sp_ifa->rt) {
> >> +                               /* This dst has been added to garbage list when
> >> +                                * lo device down, delete this obsolete dst and
> >> +                                * reallocate new router for ifa. */
> >> +                               if (sp_ifa->rt->dst.obsolete > 0) {
> >> +                                       ip6_del_rt(sp_ifa->rt);
> >> +                                       sp_ifa->rt = NULL;
> >> +                               } else
> >> +                                       continue;
> >> +                       }
> >>
> >>                         sp_rt = addrconf_dst_alloc(idev, &sp_ifa->addr, false);
> > 
> > It looks like it can work but I don't know if we should just fix this the
> > clean way (see below).
> > 
> >>> I'll try to just disable routes without removing them at all when we set an
> >>> interface to down at the weekend.
> >>>
> >>
> >> How do you decide which route should be disabled?  use rt6_flags? I don't know
> >> if your way will cause miscarriage.
> > 
> > What I did so far is that I added a new function next to rt6_ifdown that
> > only gets called if interface gets shutdown but not unregistered (from
> > addrconf_ifdown).
> > 
> 
> rt6_ifdown has alreay put this device related routes to the garbage list.
> 
> > fib6_clean_all then iterates over the whole routing table with a new predicate
> > function which checks in the same way like fib6_ifdown, if it is a matching route
> > (the interfaces match up) and if so, toggles a new "DEAD" flag in rt6i_flags.
> > 
> > When bringing up the interface I distinguish between up and register and just
> > clear this death flag from the routes on bringing it up.
> > 
> > fib lookup code then does not honour those routes.
> > 
> > I had no time to test this thoroughly at the weekend and still have some code
> > paths were I am unsure. Do you see any problems with this so far? We could
> > then delete the special cases on loopback interface init.
> 
> So you add a special case in the general network interface down logic. I think this
> is more complex...

The problem is, that we only have a reference to ifp->rt, the loopback
RTF_LOCAL route. Currently we silently remove all other routes this interface
has. Sure, they come back soon with RAs and such, but this is not the way this
should work.

Greetings,

  Hannes

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html