udev-node: use flock() for symlink stack directory #23043

yuwata · 2022-04-11T03:47:28Z

No description provided.

src/udev/udev-node.c

yuwata · 2022-04-11T14:07:31Z

@poettering Thank you for the review. Most of comments are addressed. PTAL.

src/udev/udev-node.c

yuwata · 2022-04-12T15:12:08Z

@poettering Thank you for the review. The commit message and the comment in the code is updated. Also, the cleanup logic is updated. PTAL.

src/udev/udev-node.c

src/basic/path-util.c

src/udev/udev-node.c

poettering · 2022-04-13T07:10:29Z

Hmm, shouldn't we just use a stack directory with a different name to avoid the locking issues.

I kinda like the idea, but how to handle upgrades then?

poettering · 2022-04-13T07:11:12Z

I kinda like the idea, but how to handle upgrades then?

Maybe we should just tell people to retrigger everything on update. Which they really should do anyway. If so, then the old dir just doesn't matter (though we wouldn't clean it up either, but I'd be OK with that)

poettering · 2022-04-13T07:13:16Z

think this should be handled in tempfn_xxxxxxx instead. If we assume tempfn_xxxxxxx can fail, we'll have to have the same fallback in every caller, or at least in those where the filename is controlled externally or unpredictable.
Let's make tempfn_xxxxxxx smarter: only use up to NAME_MAX - strlen(extra) - strlen(".#" "XXXXXX") of the original file name. This way the function can only fail on oom, and callers become simpler.

So I thought about this approach, and I think we should implement this, but this doesn't solve the problem fully, because if you want to create a temporary path for something that has NAME_MAX length already, and the last compononent (i.e. the filename) is just one character, then you cannot possibly stick six XXXXXX characters in there anymore. So while it pushes the problem out, it doesn't solve it...

keszybz · 2022-04-13T07:27:04Z

Can you give an example? I don't understand the comment.

No functionality is changed.

Then, we can always assume the directory exists, and the code become slightly simpler. Note, unused directories are removed by the main udevd process in a later commit.

And try to read it only when the file is symlink.

Also shorten code a bit. Just for consistency with other part and readability of the code.

By locking the stack directory, we can safely determine the device node with the highest priority for a symlink. So, the multiple try-and-wait loops can be dropped, and the code becomes quite simple.

By the previous commit, the stack directories are not removed even if it is empty. To reduce the inode usage of /run, let's cleanup the directories.

yuwata · 2022-09-02T20:19:58Z

@poettering Thank you for the review. All comments are addressed. PTAL.

@fbuihuu Sorry for late response again.

If the lock file is readable by an unprivileged user, then the user can lock the file, and udev workers are blocked to access the dir. If the directory is not workd-accessible, then there should be no problem.

Again the lock file is created with permissions 600, hence let me ask again how an unprivileged process could open the lock file.

Hm, yeah, the file is created with 0600. There is no problem.

If the lock file is removed by a worker on unlock, then another worker needs to recreate it before lock the file. Hence, simple open() + flock() does not work...

make_lock_file_for() seems to be prepared to handle this case, no ?

Not sure, maybe. If so, we can rewrite with it. But let's do that later.

If the filename of a device symlink is too long, then the temporary filename may become invalid, and we fail to create symlink. The function `tempfn_random()` used in symlink_atomic_full() generates a safe temporary filename. Note that, thanks to the PR systemd#23043, now only one worker can handle the same symlink simultaneously. Hence, the device ID based temporary filename is not necessary.

yuwata added the udev label Apr 11, 2022

yuwata mentioned this pull request Apr 11, 2022

udev: use readlinkat_malloc() #23040

Closed

yuwata force-pushed the udev-node-use-flock branch from c3d23a8 to 2c48e52 Compare April 11, 2022 04:38