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

time_change_fd doesn't work on time64 userspace with pre-time64 kernel #14362

Closed
richfelker opened this issue Dec 16, 2019 · 6 comments · Fixed by #14364
Closed

time_change_fd doesn't work on time64 userspace with pre-time64 kernel #14362

richfelker opened this issue Dec 16, 2019 · 6 comments · Fixed by #14364

Comments

@richfelker
Copy link

richfelker commented Dec 16, 2019

In time_change_fd, a value of TIME_T_MAX is used to set a timerfd timer for which no expiration even is ever desired (only the cancellation when clock is set is wanted):

https://github.com/systemd/systemd/blob/master/src/basic/time-util.c#L1485

However, it's possible with 64-bit time_t ("time64") on a 32-bit arch that the kernel does not support such large timeouts. The timerfd_settime function is expected to use the old time32 syscall if the new time64 one is not available (this is what musl does and it's my understanding of what glibc intends to do when they eventually upstream time64 support), but silently setting the incorrect timeout, which the application could observe via reading it back with timerfd_gettime, is not reasonable behavior, so it must fail in this case (ENOTSUP is the error musl uses for this; I expect glibc will do the same but I'm not sure).

I'm not sure what the cleanest fix is, but if callers can tolerate a spurious timer expiration event after 68 years of uptime, just setting INT_MAX should be safe and easy.

@poettering
Copy link
Member

Uh, systems that claim 64bit time_t support but don't actually do, and without any way to determine what the actual supported range is? Couldn't there at least be clock_getmax() á la clock_getres()?

Hmm, it sounds strange to me to support 64bit time_t apis on kernels that can't do them. What's the point?

(Also, on Linux ENOSUP is actually an alias for EOPNOTSUPP, and the actual name is the latter. Don't tell me musl does it the other way round? Meh.)

@poettering
Copy link
Member

@fweimer, what's glibc's plan on this?

@poettering
Copy link
Member

I prepped a potential fix in #14364. Lacking an installation of an affected system it's pretty much written from thin air though and untested...

@richfelker
Copy link
Author

Fix LGTM.

Re: ENOTSUP, I think we define the "alias" in the same direction glibc and kernel do. I just prefer the ENOTSUP name in source and documentation because it matches the documented meanings:

[ENOTSUP]

Not supported. The implementation does not support the requested feature or value.

[EOPNOTSUPP]

Operation not supported on socket. The type of socket (address family or protocol) does not support the requested operation. A conforming implementation may assign the same values for [EOPNOTSUPP] and [ENOTSUP].

Re: 64-bit time_t on a kernel that can't support it, the size of time_t is ABI, and if you have an ABI that's incompatible with running on pre-5.1 kernels, that's a sure way to ensure nobody adopts it until it's too late. In musl the decision was made to drop official support for building new programs with 32-bit time_t (although you can still do it with an old set of headers since it's ABI-compatible with old binaries built with them) and move everything to 64-bit time_t so we don't get in a situation where near-future time values are overflowing and everything is blowing up in people's faces. Lots of users are on embedded platforms with vendor-patched kernels that don't keep up with mainline, so "tell them to upgrade the kernel too" isn't really an option even if I were satisfied with telling them that (which goes against longstanding compatibility policy anyway, so I wouldn't be).

I would think distros using glibc will do similar (start building everything with 64-bit time_t in the near future once it's supported), and although they may be shipping their own 5.1+ kernels, deployment in containers very well may not have time64 syscalls. My understanding is that they're using their normal mechanism for setting the minimum supported kernel version and including compatibility with pre-time64 kernels for now.

@fweimer
Copy link

fweimer commented Dec 17, 2019

@fweimer, what's glibc's plan on this?

I haven't seen a patch, but glibc will likely use EOVERFLOW in this case, based on the already implemented wrappers. See git grep -A9 in_time_t_range in the glibc source tree.

@richfelker
Copy link
Author

@fweimer: We should take this back to libc-alpha, but I think that should be changed based on standard semantics of these error codes. EOVERFLOW is specified for when the value to be stored by the implementation to some C data type would exceed the range of that data type (e.g. offset out of 32-bit range with 32-bit off_t, or current time past 2038 with clock_gettime and 32-bit time_t). ENOTSUP was the best we could find for the other direction: a value which is representable in the programming model's types, but which the implementation can't accept/support for some reason. See: https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_03

poettering added a commit to poettering/systemd that referenced this issue Dec 18, 2019
poettering added a commit to poettering/systemd that referenced this issue Dec 19, 2019
poettering added a commit that referenced this issue Dec 19, 2019
As per
#14362 (comment)
let's also prepare for EOVERFLOW.
Yamakuzure pushed a commit to elogind/elogind that referenced this issue Feb 1, 2020
keszybz pushed a commit to systemd/systemd-stable that referenced this issue Feb 5, 2020
As per
systemd/systemd#14362 (comment)
let's also prepare for EOVERFLOW.

(cherry picked from commit 9e7c8f6)
(cherry picked from commit 9afd65f)
keszybz pushed a commit to systemd/systemd-stable that referenced this issue Feb 5, 2020
As per
systemd/systemd#14362 (comment)
let's also prepare for EOVERFLOW.

(cherry picked from commit 9e7c8f6)
keszybz pushed a commit to systemd/systemd-stable that referenced this issue Feb 7, 2020
As per
systemd/systemd#14362 (comment)
let's also prepare for EOVERFLOW.

(cherry picked from commit 9e7c8f6)
(cherry picked from commit 9afd65f)
(cherry picked from commit 55e0f99)
Yamakuzure pushed a commit to elogind/elogind that referenced this issue Feb 12, 2020
As per
systemd/systemd#14362 (comment)
let's also prepare for EOVERFLOW.

(cherry picked from commit 9e7c8f64cfda101496f56f5546097221e8ad5d6a)
(cherry picked from commit 9afd65f15e931f777e2ba3743560d63505c90ac7)
flokli pushed a commit to flokli/systemd that referenced this issue Feb 18, 2020
…nel does not

Fixes: systemd#14362
(cherry picked from commit 601f91b)
(cherry picked from commit 608d882)
bluca pushed a commit to bluca/systemd that referenced this issue Nov 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

5 participants