Re: [PATCH v2] mips: add vdso support

[Rich Felker <dalias@libc.org>]

On Tue, Jan 26, 2016 at 12:00:12AM +0100, Hauke Mehrtens wrote:
> I just saw that the mips stuff is implemented in a bad way, it returns
> -ENOSYS when it can not handle the vdso call. I already complained to
> the mips kernel developers. ;-) This code was not tested, I am still
> building.

Thanks for catching that! It would have been a bad regression.

>  arch/mips/syscall_arch.h |  4 ++++
>  src/time/clock_gettime.c | 12 +++++++++++-
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/mips/syscall_arch.h b/arch/mips/syscall_arch.h
> index e74e0ad..39c0ea3 100644
> --- a/arch/mips/syscall_arch.h
> +++ b/arch/mips/syscall_arch.h
> @@ -161,3 +161,7 @@ static inline long __syscall6(long n, long a, long b, long c, long d, long e, lo
>  	if (n == SYS_fstatat) __stat_fix(c);
>  	return r2;
>  }
> +
> +#define VDSO_USEFUL
> +#define VDSO_CGT_SYM "__vdso_clock_gettime"
> +#define VDSO_CGT_VER "LINUX_2.6"
> diff --git a/src/time/clock_gettime.c b/src/time/clock_gettime.c
> index 1572de0..dba99ff 100644
> --- a/src/time/clock_gettime.c
> +++ b/src/time/clock_gettime.c
> @@ -26,13 +26,23 @@ void *__vdsosym(const char *, const char *);
>  int __clock_gettime(clockid_t clk, struct timespec *ts)
>  {
>  #ifdef VDSO_CGT_SYM
> +	int ret;
>  	static int (*volatile cgt)(clockid_t, struct timespec *);
>  	if (!cgt) {
>  		void *f = __vdsosym(VDSO_CGT_VER, VDSO_CGT_SYM);
>  		if (!f) f = (void *)sc_clock_gettime;
>  		a_cas_p(&cgt, 0, f);
>  	}
> -	return cgt(clk, ts);
> +	ret = cgt(clk, ts);
> +
> +	/*
> +	 * mips in linux kernel 4.4.0 returns -ENOSYS if it can not
> +	 * handle the syscall in vdso, the original syscall should be
> +	 * called by the libc in such a case.
> +	 */
> +	if (ret == -ENOSYS)
> +		return sc_clock_gettime(clk, ts);
> +	return ret;
>  #else
>  	return sc_clock_gettime(clk, ts);
>  #endif

This could probably be written better as:

	if (ret != -ENOSYS) return ret;
#endif
	return sc_clock_gettime(clk, ts);

On the other hand if the kernel is going to fix the mips bug and use a
new symbol version for the fixed one, we could just wait and define
VDSO_CGT_VER appropriately. That's the less invasive fix (doesn't
affect other archs) but I'm not really partial.

There's some value to having the fallback anyway in case some buggy
kernel breaks the vdso function on an arch where it was previously
working. I almost wonder if we shouldn't just do:

	if (ret == -EINVAL) return ret;
#endif
	return sc_clock_gettime(clk, ts);

i.e. reject any error but EINVAL from the vdso and try the syscall,
since EINVAL is the only one that should be possible.

Rich