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

runtime: potential issue with mips64 VDSO clock_gettime #39046

Closed
draganmladjenovic opened this issue May 13, 2020 · 14 comments
Closed

runtime: potential issue with mips64 VDSO clock_gettime #39046

draganmladjenovic opened this issue May 13, 2020 · 14 comments
Labels
Milestone

Comments

@draganmladjenovic
Copy link

@draganmladjenovic draganmladjenovic commented May 13, 2020

Posting a drive by bug report after looking at the source.

Since 1.14 (210e367) we are using VDSO instead of direct clock_gettime syscall. For mips platform it seems that VDSO implementation didn't provide fallback path until 4.13 (torvalds/linux@180902e). I guess that, depending on available clocksource, it is possible that VDSO call silently fail with -ENOSYS on thease older kernel versions, so that needs to be checked.

@cherrymui
@ianlancetaylor

@draganmladjenovic draganmladjenovic changed the title Potential issue with with mips64 VDSO clock_gettime Potential issue with mips64 VDSO clock_gettime May 13, 2020
@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented May 13, 2020

It looks to me that it does have a fallback path: https://tip.golang.org/src/runtime/sys_linux_mips64x.s#L309

Does it handle your concern? Thanks.

@draganmladjenovic
Copy link
Author

@draganmladjenovic draganmladjenovic commented May 13, 2020

@cherrymui That path seems to be taken only when runtime.vdsoClockgettimeSym is zero (VDSO is not available or __vdso_clock_gettime is missing). This potential issue could happen when executing __vdso_clock_gettime while using clocksource that doesn't have VDSO support [1]. I believe that in that case the code should run the existing fallback path.

[1] https://github.com/torvalds/linux/blob/v4.12/arch/mips/vdso/gettimeofday.c#L134

@prattmic
Copy link
Member

@prattmic prattmic commented May 13, 2020

This does look like an issue to me, but referenced commit (torvalds/linux@180902e) does not add an appropriate fallback. That commit adds a fallback for unknown clock IDs, but Go always passes CLOCK_MONOTONIC. The ENOSYS for CLOCK_MONOTONIC when VDSO is disabled is still there.

That said, torvalds/linux@24640f2#diff-6d421860181fa4dae9db33a72099e169 (v5.4) switches to a new generic VDSO, which does have a fallback: https://github.com/torvalds/linux/blob/458ef2a25e0cbdc216012aa2b9cf549d64133b08/lib/vdso/gettimeofday.c#L293

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented May 13, 2020

Thanks. So, do you think we should fall back to syscall if the VDSO call returns ENOSYS, or maybe any negative result?

@prattmic
Copy link
Member

@prattmic prattmic commented May 13, 2020

From a quick look over the code, it shouldn't matter, as ENOSYS is literally the only possible error. FWIW, I'd probably stick with ENOSYS, but it really doesn't matter.

For reference glibc just falls back on ENOSYS: https://github.com/bminor/glibc/blob/bc2eb9321ec0d17d41596933617b2522c9aa5e0b/sysdeps/unix/sysv/linux/sysdep-vdso.h#L41

@odeke-em odeke-em changed the title Potential issue with mips64 VDSO clock_gettime runtime: potential issue with mips64 VDSO clock_gettime May 13, 2020
@cagedmantis cagedmantis added this to the Backlog milestone May 18, 2020
@mengzhuo
Copy link
Contributor

@mengzhuo mengzhuo commented Nov 5, 2020

@prattmic Is this issue resolved?

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 17, 2020

Change https://golang.org/cl/270717 mentions this issue: runtime: check mips64 VDSO clock_gettime return code

@mengzhuo
Copy link
Contributor

@mengzhuo mengzhuo commented Nov 17, 2020

@draganmladjenovic @prattmic I can't find any available kernel before 4.13, it passed the testes on 4.19/5.4.

@prattmic
Copy link
Member

@prattmic prattmic commented Nov 17, 2020

@mengzhuo that surprises me. Is this on a machine running that has VDSO_CLOCK_NONE.

From browsing the tree at torvalds/linux@24640f2, it seems that would be any hardware not using the "R4K" clock source:

$ grep -R clock_mode arch/mips
...
arch/mips/kernel/csrc-r4k.c:            clocksource_mips.archdata.vdso_clock_mode = VDSO_CLOCK_R4K;

Other clock sources, like "OCTEON_CVMCOUNT" (arch/mips/cavium-octeon/csrc-octeon.c) don't seem to provide a VDSO clock mode and would thus return ENOSYS.

I'm not at all familiar with MIPS hardware. Do these configurations not exist in practice?

@draganmladjenovic
Copy link
Author

@draganmladjenovic draganmladjenovic commented Nov 17, 2020

I'm not at all familiar with MIPS hardware. Do these configurations not exist in practice?

If I remember correctly, all linux-mips*-rtrk builders happen to use OCTEON_CVMCOUNT, but the kernel was built w/o VDSO support at all because of that.

@mengzhuo
Copy link
Contributor

@mengzhuo mengzhuo commented Nov 18, 2020

@prattmic
AFAIK It's not necessary setting clock_mode any kernel with CONFIG_HIGH_RES_TIMERS=y will got a "Constant" clocksource on mips
I did the tests on my Loongson box (linux-mips64le-mengzhuo, Linux 5.4)

cat /sys/devices/system/clocksource/clocksource0/available_clocksource
Constant hpet

Linux 5.4 will safely fallback to syscall and ret 0 if I switch to hpet, which has no vdso support.

@mengzhuo that surprises me. Is this on a machine running that has VDSO_CLOCK_NONE.

From browsing the tree at torvalds/linux@24640f2, it seems that would be any hardware not using the "R4K" clock source:

$ grep -R clock_mode arch/mips
...
arch/mips/kernel/csrc-r4k.c:            clocksource_mips.archdata.vdso_clock_mode = VDSO_CLOCK_R4K;

Other clock sources, like "OCTEON_CVMCOUNT" (arch/mips/cavium-octeon/csrc-octeon.c) don't seem to provide a VDSO clock mode and would thus return ENOSYS.

I'm not at all familiar with MIPS hardware. Do these configurations not exist in practice?

@mengzhuo
Copy link
Contributor

@mengzhuo mengzhuo commented Jan 6, 2021

@prattmic
@draganmladjenovic

Any updates? I think we can merge the fix in Go1.16

@draganmladjenovic
Copy link
Author

@draganmladjenovic draganmladjenovic commented Jan 6, 2021

@mengzhuo Sorry, I didn't have a chance to test this on the offending versions of the kernel. Your change looks fine to me. I guess that it should also be backported to 1.14 and 1.15.

@gopherbot gopherbot closed this in df81a15 Jan 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants