GNOME Bugzilla – Bug 750203
GNetworkMonitorNetlink hangs in user namespace
Last modified: 2015-06-03 06:53:39 UTC
If a user namespace where uid 0 is not mapped glib apps hang on startup, because the netlink code to filter out all messages not from the kernel breaks when uid 0 is mapped to overflowuid and all the netlink replies are ignored. Instead we're supposed to look at nl_pid from the source address, as per: http://lists.linuxfoundation.org/pipermail/containers/2015-May/036032.html
Created attachment 304340 [details] [review] Add GNativeSocketAddress for handling "other" addresses Instead of just dropping address types that we're not specifically handling we return a GNativeSocketAddress which is just a dummy container for the stuct sockaddr.
Created attachment 304341 [details] [review] GNetworkMonitorNetlink: Fix check for non-kernel messages This code used to look at the SCM_CREDENTIALS and ignore every message not from uid 0. However, when user namespaces are in use this does not work, as if uid 0 is not mapped you get overflowuid instead. Right now this means we ignore all messages in such user namespaces and glib apps hang on startup. We can't look at pids either, as pid 0 is returned for processes outside your pid namespace. Instead the correct approach is to look at the sending sockaddr and if the port id (nl_pid) is zero, then its from the kernel. Source: http://lists.linuxfoundation.org/pipermail/containers/2015-May/036032.html
Review of attachment 304341 [details] [review]: ::: gio/gnetworkmonitornetlink.c @@ +338,3 @@ + g_error_free (error); + if (nl->priv->dump_networks) + finish_dump (nl); Why change the control flow here and duplicate the dump_networks handling? I.e. why not just `goto done;` like the code was doing before?
Review of attachment 304340 [details] [review]: Minor bits: ::: gio/gnativesocketaddress.c @@ +47,3 @@ +struct _GNativeSocketAddressPrivate +{ + struct sockaddr *sockaddr; It's only 2 words on x86_64, not sure it deserves its own malloc block when we could just inline it here. (Note that performance of socket addresses does matter a bit - hence the existence of cache_recv_address() ) ::: gio/gnativesocketaddress.h @@ +54,3 @@ +}; + +GLIB_AVAILABLE_IN_ALL IN_2_46
Review of attachment 304340 [details] [review]: ::: gio/gnativesocketaddress.c @@ +47,3 @@ +struct _GNativeSocketAddressPrivate +{ + struct sockaddr *sockaddr; struct sockaddr is small, but it doesn't fit all socket address types. There is sockaddr_storage which is large enough to fit a sockaddr_in or sockaddr_in6, but a sockaddr can be larger than that. That said, we could use a sockaddr_storage and then allocate when things don't fit. All performance critical addresses will use known socket address types though, not this fallback.
Review of attachment 304341 [details] [review]: ::: gio/gnetworkmonitornetlink.c @@ +338,3 @@ + g_error_free (error); + if (nl->priv->dump_networks) + finish_dump (nl); I didn't change the control flow in the previously handled case. If the sender is not from the kernel we still ignore without sending an error (i.e. goto done). However, we do report an error if reading the sending address fails, with code just like the statement before it when the receive itself fails. The reason for this is that we can't just ignore this because it may be the real reply we're waiting for, and ignoring it will cause your app to hang, but not ignoring it will make it impossible to verify that the message came from the kernel.
Created attachment 304411 [details] [review] Add GNativeSocketAddress for handling "other" addresses Instead of just dropping address types that we're not specifically handling we return a GNativeSocketAddress which is just a dummy container for the stuct sockaddr.
New version has inline struct sockaddr_storage, and the right version markers.
Review of attachment 304411 [details] [review]: Also needs an entry in docs/reference/gio/gio-sections.txt, right? Otherwise LGTM after those fixes. ::: gio/gnativesocketaddress.c @@ +140,3 @@ + * Returns: a new #GNativeSocketAddress + * + * Since: 2.22 2.46
Review of attachment 304341 [details] [review]: Makes sense to me.
Created attachment 304427 [details] [review] Add GNativeSocketAddress for handling "other" addresses Instead of just dropping address types that we're not specifically handling we return a GNativeSocketAddress which is just a dummy container for the stuct sockaddr.
Attachment 304341 [details] pushed as 7cba800 - GNetworkMonitorNetlink: Fix check for non-kernel messages Attachment 304427 [details] pushed as f8273f3 - Add GNativeSocketAddress for handling "other" addresses