this is a bit horrific … there are specific switches to exclude some devices … i.e. you never want to re-trigger your rootfs HDD while it is mounted …
https://bugzilla.redhat.com/show_bug.cgi?id=570355 is an example …
you really want to limit the reload with some filtering here
What should we do instead?
still use trigger, but use some intelligent filtering so not all devices get reloaded blindly
imagine you plug in a usb key and when removing a snap suddenly your rootfs disk switches from being sda to sdb (there is no guarantee it keeps the name, it can be a matter of order)
you can filter by type, subsystem etc …
What would be the rule that you suggest?
well, what for do you do that reload ? it depends … like … do you hae a record of devices that have been touched by the snap ? then you could explicitly only reload these devices for example … just blindly reloading all rules for all devices sounds risky to me.
We reload those rules whenever a snap has interfaces that use the udev backend. If we rewrite the generated udev rules then we reload them. Doing this by subsystem would require us to understand what prior version of the interface did in all cases. We could do that but a) is that a generic solution? b) does it really fix the problem.
We don’t really know what is going on yet.
well, you are talking about rules your snap has put in place (and removed alongside) so you should know which subsystems, device names or wnatnot have been added/removed from the rule itself … i.e.:
SUBSYSTEM=="drm", KERNEL=="card[0-9]*", TAG+="<mysnap>"
knows exactly that the drm subsystem was changed … so adding or removing this specific rule should use
udevadm trigger --reload-rules --subsystem=drm
to make sure only this bit gets updated …
We don’t parse those snippets so we don’t really “know”.
We’d have to adjust the logic to parse the before/after rules and then
see which subsystems are affected. Alternatively we’d have to annotate snippets with side information about subsystems and then store that in a machine-readable way somewhere.
Edit: again, that’s something we can do but we need to understand what is wrong with the current approach exactly.
Well, in general your call above is triggering a change event for all devices by default, that is surely never desired …
note that the udevadm control you call first should actually be sufficient, has someone tested if calling trigger is actually necessary at all ?
I don’t know actually. Let me do some digging to see what that does. This code is 15.04-old.
@zyga-snapd, recently I was playing with udevadm and our cgroups handling and thought I found bugs in our cgroups handling. On reboot I found everything was working fine, so I thought of this thread. I then thought I remembered something about what is safe and not safe and remembered this thread:
This is of course an ancient thread, but this struck me:
"Here’s a quick guide to what is safe to call:
…
I’ve installed udev rules and want udev to do something about it.
udevadm trigger --action=change
...
The action argument is of utmost importance. Without it, the
kernel will tell udev to treat all devices as *NEW*. This will
have lots of side-effects you are probably not expecting.
"change" is completely safe. It tells udev just to refresh
devices, and make sure everything's as it should be.
...
"
I then recalled that udev automatically detects changes to udev rules.
It seems all we need to do is udevadm trigger --action=change and skip everything else. Indeed, looking in /var/lib/dpkg/info/* I see most postinst scripts doing this (some include --subsystem, but thinking that isn’t strictly required).
I then checked this manually and by doing:
$ udevadm info /sys/devices/virtual/mem/kmsg
P: /devices/virtual/mem/kmsg
N: kmsg
E: DEVMODE=0644
E: DEVNAME=/dev/kmsg
E: DEVPATH=/devices/virtual/mem/kmsg
E: MAJOR=1
E: MINOR=11
E: SUBSYSTEM=mem
$ sudo sh -c 'echo "KERNEL==\"kmsg\", TAG+=\"snap_hello-world_sh\"" > /etc/udev/rules.d/snap.hello-world.rules'
$ cat /etc/udev/rules.d/snap.hello-world.rules
KERNEL=="kmsg", TAG+="snap_hello-world_sh"
# before udevadm trigger --action=change
$ udevadm info /sys/devices/virtual/mem/kmsgP: /devices/virtual/mem/kmsg
N: kmsg
E: DEVMODE=0644
E: DEVNAME=/dev/kmsg
E: DEVPATH=/devices/virtual/mem/kmsg
E: MAJOR=1
E: MINOR=11
E: SUBSYSTEM=mem
$ sudo udevadm trigger --action=change
# after udevadm trigger --action=change
$ udevadm info /sys/devices/virtual/mem/kmsg
P: /devices/virtual/mem/kmsg
N: kmsg
E: DEVMODE=0644
E: DEVNAME=/dev/kmsg
E: DEVPATH=/devices/virtual/mem/kmsg
E: MAJOR=1
E: MINOR=11
E: SUBSYSTEM=mem
E: TAGS=:snap_hello-world_sh:
E: USEC_INITIALIZED=8295994829
# remove the file and then run udevadm trigger --action=change
$ sudo rm -f /etc/udev/rules.d/snap.hello-world.rules
$ sudo udevadm trigger --action=change
$ udevadm info /sys/devices/virtual/mem/kmsg
P: /devices/virtual/mem/kmsg
N: kmsg
E: DEVMODE=0644
E: DEVNAME=/dev/kmsg
E: DEVPATH=/devices/virtual/mem/kmsg
E: MAJOR=1
E: MINOR=11
E: SUBSYSTEM=mem
I’m going to create a PR for this since I helped with the initial implementation and spread tests.
Looking at this more closely and getting input from others, I don’t think this PR is necessary and is probably unrelated to this issue. I’m closing it now.
I still think the PR makes sense though, forcing a full set of coldplug events for all available devices is pretty wasteful … but yeah, not ugent and not actually related …
I reviewed the bug https://bugs.launchpad.net/ubuntu/+source/gdm3/+bug/1710637 and see the underlying cause was an interaction between console-setup udev rules and systemd and it has since been fixed in console-setup, so I will mark this topic as resolved.
I personally think this is a bug in gnome-shell/wayland/gdm since it doesn’t affect gnome-shell/X, only gnome-shell/wayland.
That said, I’d like to take a look to see if we can do something better than our use of udevadm trigger, so I’ve added it to my backlog.
So my notes are public:
Importantly, need udevadm control --reload-rules which means we don’t need to wait up to 3 seconds for udevd to detect changes and process the new rules. This command is safe as verified by code inspection in src/udev of systemd.
The test is:
sudo sh -c 'echo "KERNEL==\"kmsg\", TAG+=\"snap_hello-world_sh\"" > /etc/udev/rules.d/snap.hello-world.rules'
udevadm info /dev/kmsg # it should not show a tagudevadm info /dev/kmsg # it should show a tagThe SOMETHINGs that didn’t work
udevadm trigger --dry-run aloneudevadm trigger aloneudevadm control --reload-rules aloneThese work (if rules reloaded):
udevadm trigger --subsystem-match=memudevadm trigger /dev/kmsgudevadm trigger --subsystem-nomatch=tty (wayland workaround)Perhaps other trigger options are possible. Ideally we would trigger on tag match (eg, sudo udevadm trigger --tag-match=snap_hello-world_sh) but this doesn’t work.
I just want to say that this isn’t actually correct. It does affect X, but in that case ctrl-c doesn’t terminate your session for some reason I don’t understand. alt-left/right still switch the VT, your input goes to the console underneath Shell, etc. It is always a very bad idea to change the keyboard mode underneath your graphical environment. The environment itself can’t really do anything sensible about that - it’s really a “don’t do that then” thing.
The script was buggy in that it did exactly that - that’s been fixed (by removing it completely since it wasn’t required with systemd, but it could have been fixed in another way). Still, it is a problem that it was being invoked by udev via snapd in the first place. It’s not expected that this will happen. Thanks for adding it to your backlog - I hope it is resolved sooner or later.
AIUI, while snapd may be able to do something different, it isn’t actually doing anything wrong since the action is supposed to be safe. There are many examples of udevadm trigger in postinst scripts in the Debian and Ubuntu archives for example, some with --subsystem-match, but certainly not all.
Again, we may be able to do something more targeted to the device(s) being connected by the interface, but the fact remains that upon device assignment we need the devices to be udev tagged immediately so that the snap can start using the device before rebooting, and the way to make that take effect is via some call to udevadm trigger. The tty susbsystem is one of the subsystems snappy is interested in connecting, so even if we use udevadm trigger in a more targetted fashion, it will necessarily need to call udevadm trigger --subsystem-match=tty or udevadm trigger /dev/tty... for some snaps.
All that said, note that the long term goal is to have AppArmor solely mediate devices via static rules for devices with dependable names (something we already do) and via inode tagging hotplug/hotunplug-able devices (future) and not using device cgroups at all (and therefore no need to call udevadm trigger in any fashion). While this work is currently on the roadmap, it is unscheduled (but there is actually some work being contributed to the AppArmor upstream project to support storing security labels in inodes that we may be able to build on).