Messages in this thread |  | | From | Dmitry Torokhov <> | Date | Sun, 16 Jun 2019 09:50:57 -0700 | Subject | Re: [PATCH net-next 2/2 v5] netns: restrict uevents |
| |
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
|  |