[PATCH 0/4] ipv4: small set of fixes

[PATCH 0/4] ipv4: small set of fixes

[Sergey Popovich]

In this patch series I present my attempt to address
some issues in IPv4 implementation.

Please take time to review my patches.

Thanks.

Sergey Popovich (4):
  ipv4: don't disable interface if last ipv4 address is removed
  ipv4: fib_semantics: increment fib_info_cnt after fib_info allocation
  ipv4: use SNMP macro assuming softirq context in ip_forward().
  ipv4: mark nexthop as dead when it's subnet becomes unreachable

 net/ipv4/fib_frontend.c  | 14 ++++----------
 net/ipv4/fib_semantics.c | 38 +++++++++++++++++++++-----------------
 net/ipv4/ip_forward.c    |  2 +-
 3 files changed, 26 insertions(+), 28 deletions(-)

-- 
1.8.3.4

[PATCH 1/4] ipv4: don't disable interface if last ipv4 address is removed

[Sergey Popovich]

Since

  commit 876fd05ddbae03166e7037fca957b55bb3be6594
  Author: Hannes Frederic Sowa <>
  Date:   Mon Jun 24 00:22:20 2013 +0200

    ipv6: don't disable interface if last ipv6 address is removed

we have disabled this behavior for IPv6. Adjust behavior for IPv4.

There is at least one additional reason to do the same for IPv4.
Suppose we have following configuration:

  # ip link add dev dummy1 type dummy
  # ip -4 addr add 192.168.1.254/24 dev dummy1
  # ip link set up dev dummy1

  # ip link add dev veth1a type veth peer name veth1b
  # ip link set up dev veth1a
  # ip -4 route add 192.168.1.1/32 dev veth1a src 192.168.1.254 proto static

  # ip netns add pc1
  # ip link set netns pc1 dev veth1b
  # ip netns exec pc1 ip -4 addr add 192.168.1.1/24 dev veth1b
  # ip netns exec pc1 ip link set up dev veth1b
  # ip netns exec pc1 ip link set up dev lo

  # ip -4 addr add 10.0.1.1/30 dev veth1a
  # ip -4 addr sh dev veth1a
  32: veth1a: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP qlen 1000
    inet 10.0.1.1/30 scope global veth1a
       valid_lft forever preferred_lft forever
  # ip -4 route show dev veth1a
  10.0.1.0/30  proto kernel  scope link  src 10.0.1.1
  192.168.1.1  proto static  scope link  src 192.168.1.254

  # ping -I192.168.1.254 192.168.1.1 -c1
  PING 192.168.1.1 (192.168.1.1) 56(84) bytes of data.
  64 bytes from 192.168.1.1: icmp_req=1 ttl=64 time=0.039 ms

  --- 192.168.1.1 ping statistics ---
  1 packets transmitted, 1 received, 0% packet loss, time 0ms
  rtt min/avg/max/mdev = 0.039/0.039/0.039/0.000 ms
  # ip -4 neigh sh dev veth1a
  192.168.1.1 lladdr 9a:7e:a5:aa:1b:a0 REACHABLE

  and after removing last ipv4 address from veth1a
  ------------------------------------------------

  # ip -4 addr del 10.0.1.1/30 dev veth1a
  # ip -4 neigh sh dev veth1a
  # ip -4 route show dev veth1a

Signed-off-by: Sergey Popovich <popovich_sergei@mail.ru>
---
 net/ipv4/fib_frontend.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
index d846304..ae5f35f 100644
--- a/net/ipv4/fib_frontend.c
+++ b/net/ipv4/fib_frontend.c
@@ -1021,14 +1021,7 @@ static int fib_inetaddr_event(struct notifier_block *this, unsigned long event,
 	case NETDEV_DOWN:
 		fib_del_ifaddr(ifa, NULL);
 		atomic_inc(&net->ipv4.dev_addr_genid);
-		if (ifa->ifa_dev->ifa_list == NULL) {
-			/* Last address was deleted from this interface.
-			 * Disable IP.
-			 */
-			fib_disable_ip(dev, 1);
-		} else {
-			rt_cache_flush(dev_net(dev));
-		}
+		rt_cache_flush(dev_net(dev));
 		break;
 	}
 	return NOTIFY_DONE;
-- 
1.8.3.4

Re: [PATCH 1/4] ipv4: don't disable interface if last ipv4 address is removed

[David Miller]

From: Sergey Popovich <popovich_sergei@mail.ru>
Date: Tue, 21 Jan 2014 13:48:48 +0200

> Since
> 
>   commit 876fd05ddbae03166e7037fca957b55bb3be6594
>   Author: Hannes Frederic Sowa <>
>   Date:   Mon Jun 24 00:22:20 2013 +0200
> 
>     ipv6: don't disable interface if last ipv6 address is removed
> 
> we have disabled this behavior for IPv6. Adjust behavior for IPv4.

I do not agree with this change.

IPV4 is a lot different from ipv6, for example ipv4 lacks things like
temporary address assignments and other notifications that links listen
to even if they don't have explicit static addressed assigned.

The ipv4 behavior also has two decades of precedence, and I think you'll
have a very hard time convincing me that nobody depends upon it.

So I'm not applying this patch, sorry.

[PATCH 2/4] ipv4: fib_semantics: increment fib_info_cnt after fib_info allocation

[Sergey Popovich]

Increment fib_info_cnt in fib_create_info() right after successfuly
alllocating fib_info structure, overwise fib_metrics allocation failure
leads to fib_info_cnt incorrectly decremented in free_fib_info(), called
on error path from fib_create_info().

Signed-off-by: Sergey Popovich <popovich_sergei@mail.ru>
---
 net/ipv4/fib_semantics.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
index b53f0bf..9d43468 100644
--- a/net/ipv4/fib_semantics.c
+++ b/net/ipv4/fib_semantics.c
@@ -820,13 +820,13 @@ struct fib_info *fib_create_info(struct fib_config *cfg)
 	fi = kzalloc(sizeof(*fi)+nhs*sizeof(struct fib_nh), GFP_KERNEL);
 	if (fi == NULL)
 		goto failure;
+	fib_info_cnt++;
 	if (cfg->fc_mx) {
 		fi->fib_metrics = kzalloc(sizeof(u32) * RTAX_MAX, GFP_KERNEL);
 		if (!fi->fib_metrics)
 			goto failure;
 	} else
 		fi->fib_metrics = (u32 *) dst_default_metrics;
-	fib_info_cnt++;
 
 	fi->fib_net = hold_net(net);
 	fi->fib_protocol = cfg->fc_protocol;
-- 
1.8.3.4

[PATCH 3/4] ipv4: use SNMP macro assuming softirq context in ip_forward().

[Sergey Popovich]

ip_forward() runs entirely in softirq context, we could use
version of SNMP macro which updates stats counters assuming that.

Signed-off-by: Sergey Popovich <popovich_sergei@mail.ru>
---
 net/ipv4/ip_forward.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/ip_forward.c b/net/ipv4/ip_forward.c
index e9f1217..fd7eeec 100644
--- a/net/ipv4/ip_forward.c
+++ b/net/ipv4/ip_forward.c
@@ -93,7 +93,7 @@ int ip_forward(struct sk_buff *skb)
 	mtu = ip_dst_mtu_maybe_forward(&rt->dst, true);
 	if (unlikely(skb->len > mtu && !skb_is_gso(skb) &&
 		     (ip_hdr(skb)->frag_off & htons(IP_DF))) && !skb->local_df) {
-		IP_INC_STATS(dev_net(rt->dst.dev), IPSTATS_MIB_FRAGFAILS);
+		IP_INC_STATS_BH(dev_net(rt->dst.dev), IPSTATS_MIB_FRAGFAILS);
 		icmp_send(skb, ICMP_DEST_UNREACH, ICMP_FRAG_NEEDED,
 			  htonl(mtu));
 		goto drop;
-- 
1.8.3.4

Re: [PATCH 3/4] ipv4: use SNMP macro assuming softirq context in ip_forward().

[David Miller]

From: Sergey Popovich <popovich_sergei@mail.ru>
Date: Tue, 21 Jan 2014 13:48:50 +0200

> ip_forward() runs entirely in softirq context, we could use
> version of SNMP macro which updates stats counters assuming that.
> 
> Signed-off-by: Sergey Popovich <popovich_sergei@mail.ru>

This is not a bug fix, it's an optimization.

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

[Sergey Popovich]

Removing ip address and it's subnet route using fib_del_ifaddr() does
not purge routes with nexthop in such subnet.

This could be easily reproduced with the following config:

  # ip link add dev dummy1 type dummy
  # ip link set up dev dummy1
  # ip -4 addr add 10.0.10.1/24 dev dummy1
  # ip -4 addr add 10.0.20.1/24 dev dummy1

  # ip -4 route add 172.16.0.0/12 proto static via 10.0.10.5
  # ip -4 route show exact 172.16.0.0/12
  172.16.0.0/12 via 10.0.10.5 dev dummy1  proto static

  # ip -4 addr del 10.0.10.1/24 dev dummy1

  # ip -4 route show exact 172.16.0.0/12
  172.16.0.0/12 via 10.0.10.5 dev dummy1  proto static

Add interface address (ifa) parameter to fib_sync_down_dev()
and use it to match nexthop against it's subnet.

Use fib_sync_down_dev() in fib_del_ifaddr() among with fib_sync_down_addr()
to mark as dead routes with nexthop in ifa.

Also optimize loop a bit:
  move handling of force > 1 out of the change_nexthops() loop.

Signed-off-by: Sergey Popovich <popovich_sergei@mail.ru>
---
 net/ipv4/fib_frontend.c  |  5 +++--
 net/ipv4/fib_semantics.c | 36 ++++++++++++++++++++----------------
 2 files changed, 23 insertions(+), 18 deletions(-)

diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
index ae5f35f..fd3445e 100644
--- a/net/ipv4/fib_frontend.c
+++ b/net/ipv4/fib_frontend.c
@@ -907,7 +907,8 @@ void fib_del_ifaddr(struct in_ifaddr *ifa, struct in_ifaddr *iprim)
 			 * First of all, we scan fib_info list searching
 			 * for stray nexthop entries, then ignite fib_flush.
 			 */
-			if (fib_sync_down_addr(dev_net(dev), ifa->ifa_local))
+			if (fib_sync_down_addr(dev_net(dev), ifa->ifa_local) +
+			    fib_sync_down_dev(dev, ifa, 0))
 				fib_flush(dev_net(dev));
 		}
 	}
@@ -997,7 +998,7 @@ static void nl_fib_lookup_exit(struct net *net)
 
 static void fib_disable_ip(struct net_device *dev, int force)
 {
-	if (fib_sync_down_dev(dev, force))
+	if (fib_sync_down_dev(dev, NULL, force))
 		fib_flush(dev_net(dev));
 	rt_cache_flush(dev_net(dev));
 	arp_ifdown(dev);
diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
index 9d43468..ca920df 100644
--- a/net/ipv4/fib_semantics.c
+++ b/net/ipv4/fib_semantics.c
@@ -1112,7 +1112,7 @@ int fib_sync_down_addr(struct net *net, __be32 local)
 	return ret;
 }
 
-int fib_sync_down_dev(struct net_device *dev, int force)
+int fib_sync_down_dev(struct net_device *dev, struct in_ifaddr *ifa, int force)
 {
 	int ret = 0;
 	int scope = RT_SCOPE_NOWHERE;
@@ -1132,29 +1132,33 @@ int fib_sync_down_dev(struct net_device *dev, int force)
 		if (nh->nh_dev != dev || fi == prev_fi)
 			continue;
 		prev_fi = fi;
-		dead = 0;
-		change_nexthops(fi) {
-			if (nexthop_nh->nh_flags & RTNH_F_DEAD)
-				dead++;
-			else if (nexthop_nh->nh_dev == dev &&
-				 nexthop_nh->nh_scope != scope) {
-				nexthop_nh->nh_flags |= RTNH_F_DEAD;
 #ifdef CONFIG_IP_ROUTE_MULTIPATH
-				spin_lock_bh(&fib_multipath_lock);
-				fi->fib_power -= nexthop_nh->nh_power;
-				nexthop_nh->nh_power = 0;
-				spin_unlock_bh(&fib_multipath_lock);
+		if (force > 1)
+			goto fi_dead;
 #endif
+		dead = 0;
+		change_nexthops(fi) {
+			if (nexthop_nh->nh_flags & RTNH_F_DEAD) {
 				dead++;
+				continue;
 			}
+			if (nexthop_nh->nh_dev != dev ||
+			    nexthop_nh->nh_scope == scope ||
+			    (ifa && !inet_ifa_match(nexthop_nh->nh_gw, ifa)))
+				continue;
+			nexthop_nh->nh_flags |= RTNH_F_DEAD;
 #ifdef CONFIG_IP_ROUTE_MULTIPATH
-			if (force > 1 && nexthop_nh->nh_dev == dev) {
-				dead = fi->fib_nhs;
-				break;
-			}
+			spin_lock_bh(&fib_multipath_lock);
+			fi->fib_power -= nexthop_nh->nh_power;
+			nexthop_nh->nh_power = 0;
+			spin_unlock_bh(&fib_multipath_lock);
 #endif
+			dead++;
 		} endfor_nexthops(fi)
 		if (dead == fi->fib_nhs) {
+#ifdef CONFIG_IP_ROUTE_MULTIPATH
+fi_dead:
+#endif
 			fi->fib_flags |= RTNH_F_DEAD;
 			ret++;
 		}
-- 
1.8.3.4

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

[David Miller]

From: Sergey Popovich <popovich_sergei@mail.ru>
Date: Tue, 21 Jan 2014 13:48:51 +0200

> Also optimize loop a bit:
>   move handling of force > 1 out of the change_nexthops() loop.

Do not mix bug fixes with optimizations.

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

[Julian Anastasov]

	Hello,

On Tue, 21 Jan 2014, Sergey Popovich wrote:

> +			if (nexthop_nh->nh_dev != dev ||
> +			    nexthop_nh->nh_scope == scope ||
> +			    (ifa && !inet_ifa_match(nexthop_nh->nh_gw, ifa)))

	What if nh_gw is part from another smaller/larger subnet?
For example, what if we still have 10.0.0.200/8 ? 10.0.10.5 is
still reachable, i.e. fib_check_nh() would create such NH.
IMHO, marking NH by exact nh_gw looks more acceptable because
the exact GW becomes unreachable. Otherwise, you will need
fib_lookup() as in fib_check_nh() to check that NH becomes
unreachable.

Regards

--
Julian Anastasov <ja@ssi.bg>

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

[Sergey Popovich]

В письме от 23 января 2014 12:06:30 пользователь Julian Anastasov написал:
> 	Hello,
> 
> On Tue, 21 Jan 2014, Sergey Popovich wrote:
> > +			if (nexthop_nh->nh_dev != dev ||
> > +			    nexthop_nh->nh_scope == scope ||
> > +			    (ifa && !inet_ifa_match(nexthop_nh->nh_gw, ifa)))
> 
> 	What if nh_gw is part from another smaller/larger subnet?
> For example, what if we still have 10.0.0.200/8 ? 10.0.10.5 is
> still reachable, i.e. fib_check_nh() would create such NH.


Please correct me if I dont understand something:

1. fib_sync_down_dev() is used when interface is going down
to remove entires with stray nexthops (including multipath routes).

2. It takes as its argument device on which event (DOWN for short) is received
and force argument to force fib info entry deletion (which is true when
fib_sync_down_dev() called from fib_disable_ip() with 2 on UNREGISTER event.

Case, that patch is tries to address happens when we have two
or more addresses on interface, and NH exists in one of such subnet.

With two or more address on iface, fib_disable_ip() is not called on single 
address removal, so fib_sync_down_dev() also not called, and we end with 
routes with stray nexthop.

There is no problem with single address and NH in its subnet, as 
fib_sync_down_dev() called from fib_disable_ip().

When deleting IP address, we have net_device where address deleted
and deleted ifa entry.

Only thing that I miss is RTNH_F_ONLINK NH flag, should be consulted
before marking nexthop as dead. I will fix this in v2.


> IMHO, marking NH by exact nh_gw looks more acceptable because
> the exact GW becomes unreachable. Otherwise, you will need
> fib_lookup() as in fib_check_nh() to check that NH becomes
> unreachable.

Not sure that I fully understand you.

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?

Well, sounds good, but what to do with multipath routes?
Is this correct at all?

Thanks revieving my patches.

> 
> Regards
> 
> --
> Julian Anastasov <ja@ssi.bg>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
SP5474-RIPE
Sergey Popovich

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

[Julian Anastasov]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3858 bytes --]


	Hello,

On Thu, 23 Jan 2014, Sergey Popovich wrote:

> В письме от 23 января 2014 12:06:30 пользователь Julian Anastasov написал:
> > 	Hello,
> > 
> > On Tue, 21 Jan 2014, Sergey Popovich wrote:
> > > +			if (nexthop_nh->nh_dev != dev ||
> > > +			    nexthop_nh->nh_scope == scope ||
> > > +			    (ifa && !inet_ifa_match(nexthop_nh->nh_gw, ifa)))
> > 
> > 	What if nh_gw is part from another smaller/larger subnet?
> > For example, what if we still have 10.0.0.200/8 ? 10.0.10.5 is
> > still reachable, i.e. fib_check_nh() would create such NH.
> 
> 
> Please correct me if I dont understand something:
> 
> 1. fib_sync_down_dev() is used when interface is going down
> to remove entires with stray nexthops (including multipath routes).
> 
> 2. It takes as its argument device on which event (DOWN for short) is received
> and force argument to force fib info entry deletion (which is true when
> fib_sync_down_dev() called from fib_disable_ip() with 2 on UNREGISTER event.

	Correct. The rules are:

- force=2 => remove unipath/multipath fi with such NH dev
- force=1 => mark/remove remote and local gateways for dev
- force=0 => mark/remove remote gateways, keep local gateways

	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.

> Case, that patch is tries to address happens when we have two
> or more addresses on interface, and NH exists in one of such subnet.
> 
> With two or more address on iface, fib_disable_ip() is not called on single 
> address removal, so fib_sync_down_dev() also not called, and we end with 
> routes with stray nexthop.

	fib_disable_ip() is not a problem with your patch.
My concern is when last 10.0.10.1 is deleted on dummy1,
i.e. when fib_del_ifaddr() is called.

	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.

> > IMHO, marking NH by exact nh_gw looks more acceptable because
> > the exact GW becomes unreachable. Otherwise, you will need
> > fib_lookup() as in fib_check_nh() to check that NH becomes
> > unreachable.
> 
> Not sure that I fully understand you.

	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.

Regards

--
Julian Anastasov <ja@ssi.bg>

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

[Sergey Popovich]

> 	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@ssi.bg>

-- 
SP5474-RIPE
Sergey Popovich

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

[Sergey Popovich]

Removing ip address and it's subnet route using fib_del_ifaddr() does
not purge routes with nexthop in such subnet.

This could be easily reproduced with the following config:

  ip link add dev dummy1 type dummy
  ip link set up dev dummy1
  ip -4 addr add 10.0.10.1/24 dev dummy1
  ip -4 addr add 10.0.20.1/24 dev dummy1

  ip -4 route add 172.16.0.0/12 proto static via 10.0.10.5
  ip -4 route show exact 172.16.0.0/12
  172.16.0.0/12 via 10.0.10.5 dev dummy1  proto static

  ip -4 addr del 10.0.10.1/24 dev dummy1

  ip -4 route show exact 172.16.0.0/12
  172.16.0.0/12 via 10.0.10.5 dev dummy1  proto static

Add interface address (ifa) parameter to fib_sync_down_dev()
and use it to match nexthop against it's subnet.

Use fib_sync_down_dev() in fib_del_ifaddr() among with fib_sync_down_addr()
to mark as dead routes with nexthop in ifa.

  v3. Fix NH marking as dead when NH gateway subnet is still on
      interface (e.g. 10.0.10.1/24 and 10.0.30.1/16 and NH is 10.0.10.5).
      Thanks to Julian Anastasov.

  v2. Fix NH marking as dead when NH created with onlink option.

Signed-off-by: Sergey Popovich <popovich_sergei@mail.ru>
---
 include/net/ip_fib.h     |  3 ++-
 net/ipv4/fib_frontend.c  |  5 +++--
 net/ipv4/fib_semantics.c | 29 +++++++++++++++++++++++++++--
 3 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/include/net/ip_fib.h b/include/net/ip_fib.h
index 9922093..0405fc9 100644
--- a/include/net/ip_fib.h
+++ b/include/net/ip_fib.h
@@ -287,8 +287,9 @@ static inline int fib_num_tclassid_users(struct net *net)
 #endif
 
 /* Exported by fib_semantics.c */
+struct in_ifaddr;
 int ip_fib_check_default(__be32 gw, struct net_device *dev);
-int fib_sync_down_dev(struct net_device *dev, int force);
+int fib_sync_down_dev(struct net_device *dev, struct in_ifaddr *ifa, int force);
 int fib_sync_down_addr(struct net *net, __be32 local);
 int fib_sync_up(struct net_device *dev);
 void fib_select_multipath(struct fib_result *res);
diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
index ae5f35f..fd3445e 100644
--- a/net/ipv4/fib_frontend.c
+++ b/net/ipv4/fib_frontend.c
@@ -907,7 +907,8 @@ void fib_del_ifaddr(struct in_ifaddr *ifa, struct in_ifaddr *iprim)
 			 * First of all, we scan fib_info list searching
 			 * for stray nexthop entries, then ignite fib_flush.
 			 */
-			if (fib_sync_down_addr(dev_net(dev), ifa->ifa_local))
+			if (fib_sync_down_addr(dev_net(dev), ifa->ifa_local) +
+			    fib_sync_down_dev(dev, ifa, 0))
 				fib_flush(dev_net(dev));
 		}
 	}
@@ -997,7 +998,7 @@ static void nl_fib_lookup_exit(struct net *net)
 
 static void fib_disable_ip(struct net_device *dev, int force)
 {
-	if (fib_sync_down_dev(dev, force))
+	if (fib_sync_down_dev(dev, NULL, force))
 		fib_flush(dev_net(dev));
 	rt_cache_flush(dev_net(dev));
 	arp_ifdown(dev);
diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
index 9d43468..fbebba5 100644
--- a/net/ipv4/fib_semantics.c
+++ b/net/ipv4/fib_semantics.c
@@ -1112,7 +1112,29 @@ int fib_sync_down_addr(struct net *net, __be32 local)
 	return ret;
 }
 
-int fib_sync_down_dev(struct net_device *dev, int force)
+static inline bool fib_sync_down_gw(struct fib_nh *nh,
+				    struct in_ifaddr *ifr)
+{
+	if (!ifr)
+		return true;
+
+	if (nh->nh_flags & RTNH_F_ONLINK)
+		return false;
+
+	if (!inet_ifa_match(nh->nh_gw, ifr))
+		return false;
+
+	for_ifa(ifr->ifa_dev) {
+		if (unlikely(ifa == ifr))
+			continue;
+		if (inet_ifa_match(nh->nh_gw, ifa))
+			return false;
+	} endfor_ifa(ifr->ifa_dev);
+
+	return true;
+}
+
+int fib_sync_down_dev(struct net_device *dev, struct in_ifaddr *ifa, int force)
 {
 	int ret = 0;
 	int scope = RT_SCOPE_NOWHERE;
@@ -1124,6 +1146,8 @@ int fib_sync_down_dev(struct net_device *dev, int force)
 	if (force)
 		scope = -1;
 
+	BUG_ON(ifa && ifa->ifa_dev->dev != dev);
+
 	hlist_for_each_entry(nh, head, nh_hash) {
 		struct fib_info *fi = nh->nh_parent;
 		int dead;
@@ -1137,7 +1161,8 @@ int fib_sync_down_dev(struct net_device *dev, int force)
 			if (nexthop_nh->nh_flags & RTNH_F_DEAD)
 				dead++;
 			else if (nexthop_nh->nh_dev == dev &&
-				 nexthop_nh->nh_scope != scope) {
+				 nexthop_nh->nh_scope != scope &&
+				 fib_sync_down_gw(nexthop_nh, ifa)) {
 				nexthop_nh->nh_flags |= RTNH_F_DEAD;
 #ifdef CONFIG_IP_ROUTE_MULTIPATH
 				spin_lock_bh(&fib_multipath_lock);
-- 
1.8.3.4

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

[Julian Anastasov]

	Hello,

On Fri, 24 Jan 2014, Sergey Popovich wrote:

> -int fib_sync_down_dev(struct net_device *dev, int force)
> +static inline bool fib_sync_down_gw(struct fib_nh *nh,
> +				    struct in_ifaddr *ifr)
> +{
> +	if (!ifr)
> +		return true;
> +
> +	if (nh->nh_flags & RTNH_F_ONLINK)
> +		return false;
> +
> +	if (!inet_ifa_match(nh->nh_gw, ifr))
> +		return false;
> +

	You need to walk subnets here, not IPs, so
for_primary_ifa() instead of for_ifa() will save some
cycles. But for me such change still looks expensive
and does not fix the root of the problem:

- You fix the problem from IP address point of view.
The actual problem is that subnet is removed, i.e.
it is the route removal that is making GWs unreachable.
I can ip route delete some link route and cause GWs
to become unreachable.

- not sure that walking the ifa_list is a fast operation

- sadly, the NHs can not survive the secondary address
promotion as done in __inet_del_ifa().

	You can have additional optimization in
fib_del_ifaddr() while calling fib_sync_down_dev():
do nothing if secondary address is deleted because its
subnet (primary address) should be present. For example:

	if (fib_sync_down_addr(dev_net(dev), ifa->ifa_local) |
	    (!(ifa->ifa_flags & IFA_F_SECONDARY) &&
	     fib_sync_down_dev(dev, ifa, 0)))

> +	for_ifa(ifr->ifa_dev) {

	Below ifa == ifr check will not be needed when
for_primary_ifa() is used or when fib_sync_down_dev() is
called only for primary IPs. We can see some ifr in the
list only if it is secondary IP deleted during the promotion
process. Without promotion, the primary/secondary ifa is
unlinked before the NETDEV_DOWN notification and we do
not see it here.

> +		if (unlikely(ifa == ifr))
> +			continue;
> +		if (inet_ifa_match(nh->nh_gw, ifa))
> +			return false;
> +	} endfor_ifa(ifr->ifa_dev);
> +
> +	return true;

Regards

--
Julian Anastasov <ja@ssi.bg>

[PATCH 4/4 v3] ipv4: mark nexthop as dead when it's subnet becomes

[Sergey Popovich]

Removing ip address and it's subnet route using fib_del_ifaddr() does
not purge routes with nexthop in such subnet.

This could be easily reproduced with the following config:

  ip link add dev dummy1 type dummy
  ip link set up dev dummy1
  ip -4 addr add 10.0.10.1/24 dev dummy1
  ip -4 addr add 10.0.20.1/24 dev dummy1

  ip -4 route add 172.16.0.0/12 proto static via 10.0.10.5
  ip -4 route show exact 172.16.0.0/12
  172.16.0.0/12 via 10.0.10.5 dev dummy1  proto static

  ip -4 addr del 10.0.10.1/24 dev dummy1

  ip -4 route show exact 172.16.0.0/12
  172.16.0.0/12 via 10.0.10.5 dev dummy1  proto static

Add interface address (ifa) parameter to fib_sync_down_dev()
and use it to match nexthop against it's subnet.

Use fib_sync_down_dev() in fib_del_ifaddr() among with fib_sync_down_addr()
to mark as dead routes with nexthop in ifa.

  v3. Fix NH marking as dead when NH gateway subnet is still on
      interface (e.g. 10.0.10.1/24 and 10.0.30.1/16 and NH is 10.0.10.5).
      Thanks to Julian Anastasov.

  v2. Fix NH marking as dead when NH created with onlink option.

Signed-off-by: Sergey Popovich <popovich_sergei@mail.ru>
---
 include/net/ip_fib.h     |  3 ++-
 net/ipv4/fib_frontend.c  |  5 +++--
 net/ipv4/fib_semantics.c | 29 +++++++++++++++++++++++++++--
 3 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/include/net/ip_fib.h b/include/net/ip_fib.h
index 9922093..0405fc9 100644
--- a/include/net/ip_fib.h
+++ b/include/net/ip_fib.h
@@ -287,8 +287,9 @@ static inline int fib_num_tclassid_users(struct net *net)
 #endif
 
 /* Exported by fib_semantics.c */
+struct in_ifaddr;
 int ip_fib_check_default(__be32 gw, struct net_device *dev);
-int fib_sync_down_dev(struct net_device *dev, int force);
+int fib_sync_down_dev(struct net_device *dev, struct in_ifaddr *ifa, int 
force);
 int fib_sync_down_addr(struct net *net, __be32 local);
 int fib_sync_up(struct net_device *dev);
 void fib_select_multipath(struct fib_result *res);
diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
index ae5f35f..fd3445e 100644
--- a/net/ipv4/fib_frontend.c
+++ b/net/ipv4/fib_frontend.c
@@ -907,7 +907,8 @@ void fib_del_ifaddr(struct in_ifaddr *ifa, struct 
in_ifaddr *iprim)
 			 * First of all, we scan fib_info list searching
 			 * for stray nexthop entries, then ignite fib_flush.
 			 */
-			if (fib_sync_down_addr(dev_net(dev), ifa->ifa_local))
+			if (fib_sync_down_addr(dev_net(dev), ifa->ifa_local) +
+			    fib_sync_down_dev(dev, ifa, 0))
 				fib_flush(dev_net(dev));
 		}
 	}
@@ -997,7 +998,7 @@ static void nl_fib_lookup_exit(struct net *net)
 
 static void fib_disable_ip(struct net_device *dev, int force)
 {
-	if (fib_sync_down_dev(dev, force))
+	if (fib_sync_down_dev(dev, NULL, force))
 		fib_flush(dev_net(dev));
 	rt_cache_flush(dev_net(dev));
 	arp_ifdown(dev);
diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
index 9d43468..fbebba5 100644
--- a/net/ipv4/fib_semantics.c
+++ b/net/ipv4/fib_semantics.c
@@ -1112,7 +1112,29 @@ int fib_sync_down_addr(struct net *net, __be32 local)
 	return ret;
 }
 
-int fib_sync_down_dev(struct net_device *dev, int force)
+static inline bool fib_sync_down_gw(struct fib_nh *nh,
+				    struct in_ifaddr *ifr)
+{
+	if (!ifr)
+		return true;
+
+	if (nh->nh_flags & RTNH_F_ONLINK)
+		return false;
+
+	if (!inet_ifa_match(nh->nh_gw, ifr))
+		return false;
+
+	for_ifa(ifr->ifa_dev) {
+		if (unlikely(ifa == ifr))
+			continue;
+		if (inet_ifa_match(nh->nh_gw, ifa))
+			return false;
+	} endfor_ifa(ifr->ifa_dev);
+
+	return true;
+}
+
+int fib_sync_down_dev(struct net_device *dev, struct in_ifaddr *ifa, int 
force)
 {
 	int ret = 0;
 	int scope = RT_SCOPE_NOWHERE;
@@ -1124,6 +1146,8 @@ int fib_sync_down_dev(struct net_device *dev, int force)
 	if (force)
 		scope = -1;
 
+	BUG_ON(ifa && ifa->ifa_dev->dev != dev);
+
 	hlist_for_each_entry(nh, head, nh_hash) {
 		struct fib_info *fi = nh->nh_parent;
 		int dead;
@@ -1137,7 +1161,8 @@ int fib_sync_down_dev(struct net_device *dev, int force)
 			if (nexthop_nh->nh_flags & RTNH_F_DEAD)
 				dead++;
 			else if (nexthop_nh->nh_dev == dev &&
-				 nexthop_nh->nh_scope != scope) {
+				 nexthop_nh->nh_scope != scope &&
+				 fib_sync_down_gw(nexthop_nh, ifa)) {
 				nexthop_nh->nh_flags |= RTNH_F_DEAD;
 #ifdef CONFIG_IP_ROUTE_MULTIPATH
 				spin_lock_bh(&fib_multipath_lock);
-- 
1.8.3.4

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

[Sergey Popovich]

Please ignore this mail. MUA line wrapping issues. Sorry.

Re: [PATCH 0/4] ipv4: small set of fixes

[David Miller]

From: Sergey Popovich <popovich_sergei@mail.ru>
Date: Tue, 21 Jan 2014 13:48:47 +0200

> In this patch series I present my attempt to address
> some issues in IPv4 implementation.
> 
> Please take time to review my patches.

You're going to have to repost these with the adjustments I've
asked for.