netdev - Re: [PATCH 4/4] ipv4: mark nexthop as dead when it's subnet becomes unreachable

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]

Date:	Fri, 24 Jan 2014 12:12:28 +0200
From:	Sergey Popovich <popovich_sergei@...l.ru>
To:	netdev@...r.kernel.org
Subject: Re: [PATCH 4/4] ipv4: mark nexthop as dead when it's subnet becomes unreachable


> 	Hello,

Hello, thanks for detailed explanation.

> 
> 
> 	My idea was that without calling fib_lookup() as
> done in fib_check_nh() you can not mark NH as dead because
> you are not sure that nh_gw is still reachable in different
> subnet. GW can be part of many subnets! Your patch assumes
> that GW can be part only from one subnet.

Agree, you are completely right.

> 
> 	Lets extend your example in this way:
> 
> ip -4 addr add 10.0.0.200/8 dev dummy1
> 
> 	ip route get 10.0.10.5 should return the longest
> match route, 10.0.10.0/24 in our case. If 10.0.10.1 is
> removed ip route get 10.0.10.5 will hit 10.0.0.0/8.
> 
> 	OK, lets delete the last 10.0.10.1 address on dummy1.
> 
> 	Before your patch only fib_sync_down_addr() was called.
> You now call fib_sync_down_dev() with force=0 and ifa, with the
> goal to mark 172.16.0.0/12 as dead (it is unipath route,
> so it will be removed). inet_ifa_match() checks that nh_gw
> 10.0.10.5 is part of the removed subnet: 10.0.10.0/24. Yes, it is.
> Who will check that 10.0.10.5 is still reachable as part of
> another subnet 10.0.0.0/8 ? At this point
> ip -4 route add 172.16.0.0/12 proto static via 10.0.10.5
> should succeed again because fib_check_nh() will see with
> fib_lookup() that 10.0.10.5 is part of 10.0.0.0/8. So, the
> patch wrongly marked the NH as dead.

Really, this true thank you for pointing me on the problem. V3 of patch
makes an attempt to address this. Please review it and tell what do you
thing about.

> 
> 	If last 10.0.10.5 is deleted we can mark NHs with
> nh_gw=10.0.10.5 as dead. It is again expensive (your
> new fib_sync_down_dev call) but this time fib_lookup() is
> not needed because this is a local GW. That is what I mean
> above.
> 
> > When deleting address and removing its subnet, instead of removing route
> > from the FIB, resolve new NH with fib_lookup() if possible, as this done
> > in fib_check_nh(), and leave route with modified NH?
> 
> 	I don't say to do it. But it is the only way to
> check if nh_gw is no more reachable.
> 
> > Well, sounds good, but what to do with multipath routes?
> > Is this correct at all?
> 
> 	fib_lookup() call is correct but expensive. That is
> why it was not done before.

fib_lookup() do its job well, but I dont think we need it at all because:

  1. We known what address is removed (if any).
  2. We known device where address is removed.
  3. Each route, regargless if this unipath or multipath, has device assigned
     for each NH entry (unless NH entry is dead or route is RTN_BLACKHOLE and
     friends). fib_check_nh() checks for this.

I think we could simply iterate over interface address list to find if there
are other subnet to which NH gateway resolve, beside removed address if any?

I did tests with v3 patch and seems described problem is gone.

> 
> Regards
> 
> --
> Julian Anastasov <ja@....bg>

-- 
SP5474-RIPE
Sergey Popovich
--
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