Ok, so do we have a kernel issue here too?  Obviously we need to work around this in NetworkManager, but it appears the kernel does not send RTM_DELROUTE for a routes that are removed as a result of the interface being set down.

Running libnl's "nl-monitor ipv4-route" tool and taking wlan0 down with /sbin/ip, no RTM_DELROUTE is ever received:

$ ip -4 route
default via 192.168.1.1 dev wlan0 
192.168.1.0/24 dev wlan0  proto kernel  scope link  src 192.168.1.175 
$ sudo ip link set dev wlan0 down
<<< expect RTM_DELROUTEs here, but none come >>>
$ ip -4 route
$ sudo ip link set dev wlan0 up
$ ip -4 route
192.168.1.0/24 dev wlan0  proto kernel  scope link  src 192.168.1.175 
$ 

So I guess we just need to ensure that when a device gets taken down in the platform, that we refill the route cache...  That seems to be what the kernel expects us to do.  Still, would be nice to get notifications that the routes have been removed on !IFF_UP, since the automagic ones for the interface's subnet (eg 192.168.1.0/24) *do* get announced via RTM_NEWROUTE when you set the interface back up.

This makes things work:

@@ -1277,7 +1304,12 @@ link_set_up (NMPlatform *platform, int ifindex)
 static gboolean
 link_set_down (NMPlatform *platform, int ifindex)
 {
-       return link_change_flags (platform, ifindex, IFF_UP, FALSE);
+       NMLinuxPlatformPrivate *priv = NM_LINUX_PLATFORM_GET_PRIVATE (platform);
+       gboolean success;
+
+       success = link_change_flags (platform, ifindex, IFF_UP, FALSE);
+       nl_cache_refill (priv->nlh, priv->route_cache);
+       return success;
 }
 
 static gboolean

though we should probably also listen for kernel routing events and respond to them correctly too.

Created attachment 249966 [details] [review]
Fix cache sync issues in platform

(In reply to comment #2)
> This makes things work:
> 
> @@ -1277,7 +1304,12 @@ link_set_up (NMPlatform *platform, int ifindex)
>  static gboolean
>  link_set_down (NMPlatform *platform, int ifindex)
>  {
> -       return link_change_flags (platform, ifindex, IFF_UP, FALSE);
> +       NMLinuxPlatformPrivate *priv = NM_LINUX_PLATFORM_GET_PRIVATE
> (platform);
> +       gboolean success;
> +
> +       success = link_change_flags (platform, ifindex, IFF_UP, FALSE);
> +       nl_cache_refill (priv->nlh, priv->route_cache);
> +       return success;
>  }
> 
>  static gboolean
> 
> though we should probably also listen for kernel routing events and respond to
> them correctly too.

This indeed works only for interfaces taken down by NetworkManager.

(In reply to comment #3)
> Created an attachment (id=249966) [details] [review]
> Fix cache sync issues in platform

Ah, it looks like we're missing some important bits. I'll push a patch with just the left-overs, so that we can concentrate on the issue.

Created attachment 250005 [details] [review]
handle address/route deletion upon link actions

This patch handles the issue as late as possible, in order to cover *all* cases when link down/deletion is noticed by nm-platform, including both internal and external changes.

(In reply to comment #1)
> Ok, so do we have a kernel issue here too?

In the past, the kernel was known to not announce certain routing changes (in particular, any change to the IPv6 default route), and this was considered not a bug, because netlink sockets don't guarantee reliable delivery, so you have to manually resync your caches now and then to ensure they're accurate anyway.

I don't know if this is still true. (Well, the theoretical part still is. I don't know there are still specific things it intentionally doesn't announce.)

(In reply to comment #6)
> (In reply to comment #1)
> > Ok, so do we have a kernel issue here too?
> 
> In the past, the kernel was known to not announce certain routing changes (in
> particular, any change to the IPv6 default route), and this was considered not
> a bug, because netlink sockets don't guarantee reliable delivery, so you have
> to manually resync your caches now and then to ensure they're accurate anyway.

Any reliable source for that, apart from just a feeling?

> I don't know if this is still true. (Well, the theoretical part still is. I
> don't know there are still specific things it intentionally doesn't announce.)

Well, my tests didn't show such behavior but they were not very thorough. But we abandoned the kernel IPv6 autoconf which was the most problematic in various respects.

Therefore, our current situation is:

1) Most of the time we don't rely on kernel netlink notifications, as NetworkManager is the originator of the link/address/route records. This applies to static configuration, DHCP and router discovery, regardless of the IP protocol version.

AFAIK this also applies to this particular case as it's NetworkManager, which sets down interfaces on suspend, unless I'm mistaken.

2) *When* we depend on the kernel at all, it's only for external tools making changes to the kernel tables. That is a brand new feature and therefore it should be OK to request kernel fixes for it to work properly. Also, it is mostly a convenience feature, rarely a critical one.

That said, I don't see a need to workaround kernel problems any more and it sounds like a valid behavior to only accept external changes actually announced by the kernel. But that's not really about this particular bug.

(In reply to comment #7)
> Any reliable source for that, apart from just a feeling?

I discovered this when writing GNetworkMonitor. OK, actually, it's the IPv4 default route, not IPv6, and only when it's removed. (bug 620932 comment 45). I seem to recall someone pointing me to a lkml post where someone said that this was known and wasn't going to be fixed, but I can't find that now.

Thanks for more information. I still believe we should ignore the external default IPv4 route issue for now until it becomes a real problem and then at least try to get it fixed in kernel first. If we ever need to work around it in NetworkManager, I would like to have the workaround properly commented including the source of information, etc, as those are really ugly hacks around kernel's bugs and/or missing features.

Well, the theoretical point still applies: netlink sockets have a fixed-size buffer, so if too many notifications get sent out all at once (or, say, if you're running NM in gdb, and you leave it stopped at a breakpoint too long), then some notifications might get dropped.

I don't know how likely this is in practice.

(In reply to comment #10)
> Well, the theoretical point still applies: netlink sockets have a fixed-size
> buffer, so if too many notifications get sent out all at once (or, say, if
> you're running NM in gdb, and you leave it stopped at a breakpoint too long),
> then some notifications might get dropped.

But then you must be talking about NetworkManager, or more precisely libnl3, side of communication, if you're mentioning gdb. That buffer size size is specified by NetworkManager.

See for example:

commit 38a9ac5cc22a39506453a238406bd7f9d9205943
Author: Pavel Šimerda <psimerda@redhat.com>
Date:   Mon May 27 17:45:55 2013 +0200

    netlink: enlarge netlink buffer to 128k

+       nle = nl_socket_set_buffer_size (priv->nlh_event, 131072, 0);

As far as I remember, more weird things happen, and techniques like explicit refill won't help much.

> I don't know how likely this is in practice.

Actually it first happened to me with nm-platform tests and then later someone noted that it happened in a running NetworkManager. We will have to keep the size of the buffer large enough even if it requires guesswork using number of interfaces, IP addresses or whatever. We will learn more when NM is used in some complicated environments.

I forgot the important point that any netlink socket error conditions are noted in the logs.

Pushed the patch.