Skip to content

Commit

Permalink
udev: set fewer process properties
Browse files Browse the repository at this point in the history
On systemd systems we generally don't need to chdir() to root, we don't
need to setup /dev/ ourselves (as PID 1 does that during earliest boot),
and we don't need to set the OOM adjustment values, as that's done via
unit files.

Hence, drop this. if people want to use udev from other init systems
they should do this on their own, I am very sure it's a good thing to do
it from outside of udevd, so that fewer privileges are required by udevd. In
particular the dev_setup() stuff is something that people who build
their own non-systemd distros want to set up themselves anyway, in
particular as they already have to mount devtmpfs themselves anyway.

Note that this only drops stuff that isn't really necessary for testing
stuff, i.e. process properties and settings that don't matter if you
quickly want to invoke udev from a terminal session to test something.
  • Loading branch information
poettering committed Jun 9, 2020
1 parent fe56acd commit 6b2229c
Showing 1 changed file with 0 additions and 10 deletions.
10 changes: 0 additions & 10 deletions src/udev/udevd.c
Original file line number Diff line number Diff line change
Expand Up @@ -1859,10 +1859,6 @@ int run_udevd(int argc, char *argv[]) {
}

/* set umask before creating any file/directory */
r = chdir("/");
if (r < 0)
return log_error_errno(errno, "Failed to change dir to '/': %m");

umask(022);

r = mac_selinux_init();
Expand All @@ -1873,8 +1869,6 @@ int run_udevd(int argc, char *argv[]) {
if (r < 0 && r != -EEXIST)
return log_error_errno(r, "Failed to create /run/udev: %m");

dev_setup(NULL, UID_INVALID, GID_INVALID);

if (getppid() == 1 && sd_booted() > 0) {
/* Get our own cgroup, we regularly kill everything udev has left behind.
* We only do this on systemd systems, and only if we are directly spawned
Expand Down Expand Up @@ -1917,10 +1911,6 @@ int run_udevd(int argc, char *argv[]) {

/* child */
(void) setsid();

r = set_oom_score_adjust(-1000);
if (r < 0)
log_debug_errno(r, "Failed to adjust OOM score, ignoring: %m");
}

return main_loop(manager);
Expand Down

5 comments on commit 6b2229c

@mbiebl
Copy link
Contributor

@mbiebl mbiebl commented on 6b2229c Sep 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @poettering
Seems this change has caused a few regressions in Debian.
Our installer (d-i) uses udev which lead to https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=970678
and we also have our initramfs generator (initramfs-tools) which uses udev (but not systemd) which is affected by this.
Are you still convinced, it's better to duplicate dev_setup() everywhere?
Would you be open to a patch to re-introduce dev_setup() in udev but conditionalizes it on sd_booted()?

@poettering
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nah, let's not readd that. If you maintain an initrd, then maintain an initrd, i.e. create what you need yourself, it's a bit weird if udev carries all kinds of additional support for such environments, that do again what the service manager already does anyway, just because you don't want to use a proper service manager.

In a way, we subscribe to the idea "do one thing and do it well" here, i.e. pid 1 sets up those extra /dev symlinks, and manages OOM adjust and stuff, and udev just manages devices. ;-)

@mbiebl
Copy link
Contributor

@mbiebl mbiebl commented on 6b2229c Sep 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fwiw, I think it's udev's responsibility to setup /dev symlinks, not PID1. udev doesn't manage devices, that's what devtmpfs is for. udev is mainly for setting up symlinks and applying permissions.

@mbiebl
Copy link
Contributor

@mbiebl mbiebl commented on 6b2229c Sep 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm with you, if you want to drop the chdir and oom adjustment. The dev_setup(), not so much.

@poettering
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

udev is too late in the general case. This needs to be done much earlier, given that there's plenty stuff tht already runs before udev. Maybe in some very specific minimal cases you get away with udev doing that for you (or doing some of it for you while you already did some of it yourself), but in the general case this cannot work, if you want to allow arbitrary services to plug before udev, and we do want to allow that.

The code is really not particularly complicated. If you maintain your own initrd in shell it should be trivial to add, there's really no need to make udev do that, it's redundant there and generically too late.

Please sign in to comment.