Re: [PATCH net-next 2/2 v5] netns: restrict uevents

Dmitry Torokhov
2019-06-16T16:51:13Z

Hi Eric,

On Sun, Jun 16, 2019 at 4:50 AM Eric W. Biederman <ebiederm@xmission.com> wrote:
>
> Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:
>
> > Hi Christian,
> >
> > On Sun, Apr 29, 2018 at 3:45 AM Christian Brauner
> > <christian.brauner@ubuntu.com> wrote:
> >>
> >> commit 07e98962fa77 ("kobject: Send hotplug events in all network namespaces")
> >>abhishekbh@google.com
> >> enabled sending hotplug events into all network namespaces back in 2010.
> >> Over time the set of uevents that get sent into all network namespaces has
> >> shrunk. We have now reached the point where hotplug events for all devices
> >> that carry a namespace tag are filtered according to that namespace.
> >> Specifically, they are filtered whenever the namespace tag of the kobject
> >> does not match the namespace tag of the netlink socket.
> >> Currently, only network devices carry namespace tags (i.e. network
> >> namespace tags). Hence, uevents for network devices only show up in the
> >> network namespace such devices are created in or moved to.
> >>
> >> However, any uevent for a kobject that does not have a namespace tag
> >> associated with it will not be filtered and we will broadcast it into all
> >> network namespaces. This behavior stopped making sense when user namespaces
> >> were introduced.
> >>
> >> This patch simplifies and fixes couple of things:
> >> - Split codepath for sending uevents by kobject namespace tags:
> >> 1. Untagged kobjects - uevent_net_broadcast_untagged():
> >> Untagged kobjects will be broadcast into all uevent sockets recorded
> >> in uevent_sock_list, i.e. into all network namespacs owned by the
> >> intial user namespace.
> >> 2. Tagged kobjects - uevent_net_broadcast_tagged():
> >> Tagged kobjects will only be broadcast into the network namespace they
> >> were tagged with.
> >> Handling of tagged kobjects in 2. does not cause any semantic changes.
> >> This is just splitting out the filtering logic that was handled by
> >> kobj_bcast_filter() before.
> >> Handling of untagged kobjects in 1. will cause a semantic change. The
> >> reasons why this is needed and ok have been discussed in [1]. Here is a
> >> short summary:
> >> - Userspace ignores uevents from network namespaces that are not owned by
> >> the intial user namespace:
> >> Uevents are filtered by userspace in a user namespace because the
> >> received uid != 0. Instead the uid associated with the event will be
> >> 65534 == "nobody" because the global root uid is not mapped.
> >> This means we can safely and without introducing regressions modify the
> >> kernel to not send uevents into all network namespaces whose owning
> >> user namespace is not the initial user namespace because we know that
> >> userspace will ignore the message because of the uid anyway.
> >> I have a) verified that is is true for every udev implementation out
> >> there b) that this behavior has been present in all udev
> >> implementations from the very beginning.
> >
> > Unfortunately udev is not the only consumer of uevents, for example on
> > Android there is healthd that also consumes uevents, and this
> > particular change broke Android running in a container on Chrome OS.
> > Can this be reverted? Or, if we want to keep this, how can containers
> > that use separate user namespace still listen to uevents?
>
> The code has been in the main tree for over a year so at a minimum
> reverting this has the real chance of causing a regression for
> folks like lxc.
>
> I don't think Android running in a container on Chrome OS was even
> available when this change was merged. So I don't think this falls
> under the ordinary no regression rules.
>
> I may be wrong but I think this is a case of developing new code on an
> old kernel and developing a dependence on a bug that had already been
> fixed in newer kernels.

No, this is not quite the case. We have been shipping Android on
Chrome OS since 2016, the concept of running Android in a container
definitely predates these series of patches.

> I know Christian did his best to reach out to
> everyone when this change came through, so only getting a bug report
> over a year after the code was merged is concerning.

This only proves that it is hard to change userspace-visible behavior
as one can't really know who might be using the interfaces and for
what reason. Again, udev is not the only consumer of uevents; as fat
as I know Android does not use udev and there are other users of
uevents as well. For example, libusb can be compiled to listen to
uevents directly.

>
> That said uevents should be completely useless in a user namespace
> except as letting you know something happened. Is that what healthd
> is using them for?

Yes, that is one of the use cases. Appearance of AC power supply can
be used to adjust system behavior, for example.

Thanks.

--
Dmitry