Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

basic: increase UNIT_NAME_MAX to 512 #14294

Closed
wants to merge 1 commit into from

Conversation

jsynacek
Copy link
Contributor

@jsynacek jsynacek commented Dec 9, 2019

Apparently, some systems' device paths are longer than 256 bytes after escaping.

$ systemd-escape /devices/pci0000:00/0000:00:02.0/0000:02:00.0/host10/port-10:0/expander-10:0/port-10:0:0/expander-10:1/port-10:1:0/expander-10:2/port-10:2:0/expander-10:3/port-10:3:13/end_device-10:3:13/target10:0:89/10:0:89:0/scsi_device/10:0:89:0 | wc -c
263

Apparently, some systems' device paths are longer than 256 bytes after escaping.

$ systemd-escape /devices/pci0000:00/0000:00:02.0/0000:02:00.0/host10/port-10:0/expander-10:0/port-10:0:0/expander-10:1/port-10:1:0/expander-10:2/port-10:2:0/expander-10:3/port-10:3:13/end_device-10:3:13/target10:0:89/10:0:89:0/scsi_device/10:0:89:0 | wc -c
263
@poettering
Copy link
Member

Hmm, no, I am not sure we should do this change. Unit names are supposed to map to files on disk, but on Linux FILENAME_MAX is 256, hence anything longer than that means we cannot define a unit file for it anymore.

i.e. I have the suspicion we should simply ignore devices like this, mabe log about it, but otherwise just accept that some things are not manageable with systemd.

Also, maybe work with the kernel folks to use less excessively long names... i mean "expander" keeps being repeated in that path. A bit shorter name would work.

Anayway, regardless what we do I am pretty sure this specific PR is not the right approach, things are more complicated than this.

@poettering poettering closed this Dec 9, 2019
@poettering poettering reopened this Dec 9, 2019
@poettering
Copy link
Member

Oops, closed by accident. This should stay open, but marked for discussion, since the way forward is not clear.

@jsynacek
Copy link
Contributor Author

IMHO the problem is that you basically put FILENAME_MAX and PATH_MAX into the same bag, which, of course, you can't, because you just degraded path lengths to file name (single directory entries) lengths. Currently, if you run into a file name that is 255 bytes long, you can't even turn it into a unit file name as there is no room for prepending, say, a ".unit" suffix. I don't understand how bumping UNIT_NAME_MAX can currently break things.

Speaking as a RHEL maintainer, we can't tell customers that we don't support their systems just because their hardware setup is complicated and our broken software either creates or can't handle file paths representing that hardware.

And while adjusting whatever generates the paths to make them shorter would work for a while, it doesn't solve the underlying issue.

@poettering
Copy link
Member

Note that we are not the only ones doing things that way. For example sysfs itself builds USB paths this way. I got a device like this for example:

/sys/devices/pci0000:00/0000:00:14.0/usb1/1-9/1-9:1.0/

You see that the suffix is heavily redundant, since it keeps repeating the same stuff in each filename, just appending stuff to the end. Of course, they keep the overall string much shorter, but the idea is the same: fold the whole path in a filename. If you extrapolate a bit for deeper bus topologies this will hit limits too eventually.

You know, I am not saying there wasn't issue here. All I am saying just setting UNIT_NAME_MAX to 512 is not the way out. It's not nearly enough.

Also note that this isn't just an issue with systemd units btw. This also tends to spill into /dev/disk/by-path/ symlinks and such. Basically, everywhere where we fold a full path into a single filename.

@jsynacek
Copy link
Contributor Author

The way I understand this, the only real solution is to make the device paths dynamically allocated, starting on the kernel side. Otherwise, the assumption is always going to be "this limit is enough, nobody is ever going to need more" and that is, as experience shows, always wrong. Is that correct?

@Keruspe
Copy link
Contributor

Keruspe commented Dec 10, 2019

Could there be a way to use special folders for splitting long unit names?

Like if this-name-is-too-long.service is too long, we'd have something like this-name-is-too-l.part/ong.service

Yes, that's not nice, but that could just be the "filesystem implementation" which systemd would transparently transpose from/to this-name-is-too-long.service on interactive or visual intput/output

@Keruspe
Copy link
Contributor

Keruspe commented Dec 10, 2019

Also, if the current state is supposed to be FILENAME_MAX, why not use that?

Here with glibc 2.30, I get #define FILENAME_MAX 4096 from /usr/include/bits/stdio_lim.h

@poettering
Copy link
Member

well, file systems generally enforce limits on filename lengths. And probably for a good reason:

This works:

touch `base64 /dev/urandom | tr -d '\n' | tr / x | dd bs=1 count=255`

This doesn't:

touch `base64 /dev/urandom | tr -d '\n' | tr / x | dd bs=1 count=256`

There are certainly ways out of this issue. For example, ideally the kernel wouldn't even generate such long paths. But even if it continues to do so, there are ugly ways out, for example: if the path is too long, just hash it to something shorter. It's gonna be very opaque but would work. I mean, such long paths are awfully opaque anyway, a hash wouldn't be that much worse.

It's not the first time we are having this discussion btw. Last time we came to the conclusion that devices with such long paths are generally not something you#d manage with systemd anyway, and instead led LVM or some other storage subsystem assemble, and the result might be something systemd then mounts for you...

We have to base things on these sysfs paths btw. It's the only identifier that all devices in sysfs possess. The other identifiers (block device node path, major/minor, network iface index, subsystem+devname) are only defined for some nodes in the sysfs tree, not all.

@poettering
Copy link
Member

Hmm, let's close this, this PR is not the way to go, because UNIT_NAME_MAX needs to stay below FILENAME_MAX, for the aforementoned reasons.

I'd be happy to merge a patch that does what #15911 does for device units, too. But simply increasing UNIT_NAME_MAX cannot work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants