[LTP] [s390x vDSO Bug?] clock_gettime(CLOCK_MONOTONIC_RAW, ...) gets abnormal ts value

[LTP] [s390x vDSO Bug?] clock_gettime(CLOCK_MONOTONIC_RAW, ...) gets abnormal ts value

[Li Wang]

Hi linux-s390 experts,

We observed that LTP/clock_gettime04 always FAIL on s390x with
kernel-v5.12-rc3.
To simply show the problem, I rewrite the LTP reproducer as a simple C
below.
Maybe it's a new bug introduced from the kernel-5.12 series branch?

PASS:
------------
# uname -r
5.11.0-*.s390x

# grep TIME_NS /boot/config-5.11.0-*.s390x
no TIME_NS enabled

## ./test-timer
vdso_ts_nsec = 898169901815, vdso_ts.tv_sec = 898, vdso_ts.tv_nsec =
169901815
sys_ts_nsec  = 898169904269, sys_ts.tv_sec  = 898, sys_ts.tv_nsec  =
169904269
===> PASS

FAIL:
----------
# uname -r
5.12.0-0.rc3.*.s390x

# grep TIME_NS /boot/config-5.12.0-0.rc3.s390x
CONFIG_TIME_NS=y
CONFIG_GENERIC_VDSO_TIME_NS=y

# ./test-timer
vdso_ts_nsec = 4484351380985507, vdso_ts.tv_sec = 4484351, vdso_ts.tv_nsec
= 380985507
sys_ts_nsec  = 1446923235377, sys_ts.tv_sec  = 1446, sys_ts.tv_nsec  =
923235377
===> FAIL


# cat test-timer.c

#include <stdio.h>
#include <unistd.h>
#include <sys/types.h>
#include <time.h>
#include <sys/time.h>
#include <asm/unistd_64.h>

long long ts_to_ns(struct timespec ts)
{
        return (ts.tv_sec * 1000000000 + ts.tv_nsec);
}

int main(void)
{
        long long vdso_ts_nsec, sys_ts_nsec;

        struct timespec vdso_ts, sys_ts;

        clock_gettime(CLOCK_MONOTONIC_RAW, &vdso_ts); //vdso way
        syscall(__NR_clock_gettime, CLOCK_MONOTONIC_RAW, &sys_ts);

        vdso_ts_nsec = ts_to_ns(vdso_ts);
        sys_ts_nsec  = ts_to_ns(sys_ts);


        printf("vdso_ts_nsec = %lld, vdso_ts.tv_sec = %ld, vdso_ts.tv_nsec
= %ld\n", vdso_ts_nsec, vdso_ts.tv_sec, vdso_ts.tv_nsec);
        printf("sys_ts_nsec  = %lld, sys_ts.tv_sec  = %ld, sys_ts.tv_nsec
 = %ld\n", sys_ts_nsec, sys_ts.tv_sec, sys_ts.tv_nsec);

        if ((vdso_ts_nsec - sys_ts_nsec) > 1000000)
                printf("===> FAIL\n");
        else
                printf("===> PASS\n");

        return 0;
}

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20210323/05629b65/attachment.htm>

Re: [s390x vDSO Bug?] clock_gettime(CLOCK_MONOTONIC_RAW, ...) gets abnormal ts value

[Heiko Carstens]

On Tue, Mar 23, 2021 at 02:21:52PM +0800, Li Wang wrote:
> Hi linux-s390 experts,
> 
> We observed that LTP/clock_gettime04 always FAIL on s390x with
> kernel-v5.12-rc3.
> To simply show the problem, I rewrite the LTP reproducer as a simple C
> below.
> Maybe it's a new bug introduced from the kernel-5.12 series branch?
> 
> PASS:
> ------------
> # uname -r
> 5.11.0-*.s390x
> 
> # grep TIME_NS /boot/config-5.11.0-*.s390x
> no TIME_NS enabled
> 
> ## ./test-timer
> vdso_ts_nsec = 898169901815, vdso_ts.tv_sec = 898, vdso_ts.tv_nsec =
> 169901815
> sys_ts_nsec  = 898169904269, sys_ts.tv_sec  = 898, sys_ts.tv_nsec  =
> 169904269
> ===> PASS
> 
> FAIL:
> ----------
> # uname -r
> 5.12.0-0.rc3.*.s390x
> 
> # grep TIME_NS /boot/config-5.12.0-0.rc3.s390x
> CONFIG_TIME_NS=y
> CONFIG_GENERIC_VDSO_TIME_NS=y
> 
> # ./test-timer
> vdso_ts_nsec = 4484351380985507, vdso_ts.tv_sec = 4484351, vdso_ts.tv_nsec
> = 380985507
> sys_ts_nsec  = 1446923235377, sys_ts.tv_sec  = 1446, sys_ts.tv_nsec  =
> 923235377
> ===> FAIL

Thanks for reporting!

I'll look later today into this. I would nearly bet that I broke it
with commit f8d8977a3d97 ("s390/time: convert tod_clock_base to
union")

[LTP] [s390x vDSO Bug?] clock_gettime(CLOCK_MONOTONIC_RAW, ...) gets abnormal ts value

[Heiko Carstens]

On Tue, Mar 23, 2021 at 02:21:52PM +0800, Li Wang wrote:
> Hi linux-s390 experts,
> 
> We observed that LTP/clock_gettime04 always FAIL on s390x with
> kernel-v5.12-rc3.
> To simply show the problem, I rewrite the LTP reproducer as a simple C
> below.
> Maybe it's a new bug introduced from the kernel-5.12 series branch?
> 
> PASS:
> ------------
> # uname -r
> 5.11.0-*.s390x
> 
> # grep TIME_NS /boot/config-5.11.0-*.s390x
> no TIME_NS enabled
> 
> ## ./test-timer
> vdso_ts_nsec = 898169901815, vdso_ts.tv_sec = 898, vdso_ts.tv_nsec =
> 169901815
> sys_ts_nsec  = 898169904269, sys_ts.tv_sec  = 898, sys_ts.tv_nsec  =
> 169904269
> ===> PASS
> 
> FAIL:
> ----------
> # uname -r
> 5.12.0-0.rc3.*.s390x
> 
> # grep TIME_NS /boot/config-5.12.0-0.rc3.s390x
> CONFIG_TIME_NS=y
> CONFIG_GENERIC_VDSO_TIME_NS=y
> 
> # ./test-timer
> vdso_ts_nsec = 4484351380985507, vdso_ts.tv_sec = 4484351, vdso_ts.tv_nsec
> = 380985507
> sys_ts_nsec  = 1446923235377, sys_ts.tv_sec  = 1446, sys_ts.tv_nsec  =
> 923235377
> ===> FAIL

Thanks for reporting!

I'll look later today into this. I would nearly bet that I broke it
with commit f8d8977a3d97 ("s390/time: convert tod_clock_base to
union")

Re: [s390x vDSO Bug?] clock_gettime(CLOCK_MONOTONIC_RAW, ...) gets abnormal ts value

[Heiko Carstens]

On Tue, Mar 23, 2021 at 08:11:41AM +0100, Heiko Carstens wrote:
> On Tue, Mar 23, 2021 at 02:21:52PM +0800, Li Wang wrote:
> > Hi linux-s390 experts,
> > 
> > We observed that LTP/clock_gettime04 always FAIL on s390x with
> > kernel-v5.12-rc3.
> > To simply show the problem, I rewrite the LTP reproducer as a simple C
> > below.
> > Maybe it's a new bug introduced from the kernel-5.12 series branch?
> > 
> > PASS:
> > ------------
> > # uname -r
> > 5.11.0-*.s390x
> > 
> > # grep TIME_NS /boot/config-5.11.0-*.s390x
> > no TIME_NS enabled
> > 
> > ## ./test-timer
> > vdso_ts_nsec = 898169901815, vdso_ts.tv_sec = 898, vdso_ts.tv_nsec =
> > 169901815
> > sys_ts_nsec  = 898169904269, sys_ts.tv_sec  = 898, sys_ts.tv_nsec  =
> > 169904269
> > ===> PASS
> > 
> > FAIL:
> > ----------
> > # uname -r
> > 5.12.0-0.rc3.*.s390x
> > 
> > # grep TIME_NS /boot/config-5.12.0-0.rc3.s390x
> > CONFIG_TIME_NS=y
> > CONFIG_GENERIC_VDSO_TIME_NS=y
> > 
> > # ./test-timer
> > vdso_ts_nsec = 4484351380985507, vdso_ts.tv_sec = 4484351, vdso_ts.tv_nsec
> > = 380985507
> > sys_ts_nsec  = 1446923235377, sys_ts.tv_sec  = 1446, sys_ts.tv_nsec  =
> > 923235377
> > ===> FAIL
> 
> Thanks for reporting!
> 
> I'll look later today into this. I would nearly bet that I broke it
> with commit f8d8977a3d97 ("s390/time: convert tod_clock_base to
> union")

So, I broke it with commit 1ba2d6c0fd4e ("s390/vdso: simplify
__arch_get_hw_counter()"). Reverting that patch will fix it for non
time namespace processes only.

The problem is that the vdso data page contains an array of struct
vdso_data's for each clock source. However only the first member of
that array contains a/the valid struct arch_vdso_data, which is
required for __arch_get_hw_counter(). Which alone is a bit odd...

However for a process which is within a time namespace there is no
(easy) way to access that page (the time namespace specific vdso data
page does not contain valid arch_vdso_data). I guess the real fix is
to simply map yet another page into the vvar mapping and put the
arch_data there. What a mess... :/

[LTP] [s390x vDSO Bug?] clock_gettime(CLOCK_MONOTONIC_RAW, ...) gets abnormal ts value

[Heiko Carstens]

On Tue, Mar 23, 2021 at 08:11:41AM +0100, Heiko Carstens wrote:
> On Tue, Mar 23, 2021 at 02:21:52PM +0800, Li Wang wrote:
> > Hi linux-s390 experts,
> > 
> > We observed that LTP/clock_gettime04 always FAIL on s390x with
> > kernel-v5.12-rc3.
> > To simply show the problem, I rewrite the LTP reproducer as a simple C
> > below.
> > Maybe it's a new bug introduced from the kernel-5.12 series branch?
> > 
> > PASS:
> > ------------
> > # uname -r
> > 5.11.0-*.s390x
> > 
> > # grep TIME_NS /boot/config-5.11.0-*.s390x
> > no TIME_NS enabled
> > 
> > ## ./test-timer
> > vdso_ts_nsec = 898169901815, vdso_ts.tv_sec = 898, vdso_ts.tv_nsec =
> > 169901815
> > sys_ts_nsec  = 898169904269, sys_ts.tv_sec  = 898, sys_ts.tv_nsec  =
> > 169904269
> > ===> PASS
> > 
> > FAIL:
> > ----------
> > # uname -r
> > 5.12.0-0.rc3.*.s390x
> > 
> > # grep TIME_NS /boot/config-5.12.0-0.rc3.s390x
> > CONFIG_TIME_NS=y
> > CONFIG_GENERIC_VDSO_TIME_NS=y
> > 
> > # ./test-timer
> > vdso_ts_nsec = 4484351380985507, vdso_ts.tv_sec = 4484351, vdso_ts.tv_nsec
> > = 380985507
> > sys_ts_nsec  = 1446923235377, sys_ts.tv_sec  = 1446, sys_ts.tv_nsec  =
> > 923235377
> > ===> FAIL
> 
> Thanks for reporting!
> 
> I'll look later today into this. I would nearly bet that I broke it
> with commit f8d8977a3d97 ("s390/time: convert tod_clock_base to
> union")

So, I broke it with commit 1ba2d6c0fd4e ("s390/vdso: simplify
__arch_get_hw_counter()"). Reverting that patch will fix it for non
time namespace processes only.

The problem is that the vdso data page contains an array of struct
vdso_data's for each clock source. However only the first member of
that array contains a/the valid struct arch_vdso_data, which is
required for __arch_get_hw_counter(). Which alone is a bit odd...

However for a process which is within a time namespace there is no
(easy) way to access that page (the time namespace specific vdso data
page does not contain valid arch_vdso_data). I guess the real fix is
to simply map yet another page into the vvar mapping and put the
arch_data there. What a mess... :/

[PATCH 0/3] s390 vdso fixes

[Heiko Carstens]

Li Wang reported that clock_gettime(CLOCK_MONOTONIC_RAW, ...) does not
work correctly on s390 via vdso. Debugging this also revealed an
unrelated bug (first patch).

The second patch fixes the problem: the tod clock steering parameters
required by __arch_get_hw_counter() are only present within the first
element of the _vdso_data array and not at all within the _timens_data
array.

Instead of working around this simply provide an s390 specific vdso
data page which contains the tod clock steering parameters.

This allows also to remove ARCH_HAS_VDSO_DATA again.

Heiko Carstens (3):
  s390/vdso: fix tod clock steering
  s390/vdso: fix arch_data access for __arch_get_hw_counter()
  lib/vdso: remove struct arch_vdso_data from vdso data struct

 arch/Kconfig                              |  3 ---
 arch/s390/Kconfig                         |  1 -
 arch/s390/include/asm/vdso.h              |  4 +++-
 arch/s390/include/asm/vdso/data.h         | 13 ------------
 arch/s390/include/asm/vdso/datapage.h     | 17 +++++++++++++++
 arch/s390/include/asm/vdso/gettimeofday.h | 11 ++++++++--
 arch/s390/kernel/time.c                   |  5 +++--
 arch/s390/kernel/vdso.c                   | 25 ++++++++++++++++++++---
 arch/s390/kernel/vdso64/vdso64.lds.S      |  3 ++-
 include/vdso/datapage.h                   | 10 ---------
 10 files changed, 56 insertions(+), 36 deletions(-)
 delete mode 100644 arch/s390/include/asm/vdso/data.h
 create mode 100644 arch/s390/include/asm/vdso/datapage.h

-- 
2.25.1

[LTP] [PATCH 0/3] s390 vdso fixes

[Heiko Carstens]

Li Wang reported that clock_gettime(CLOCK_MONOTONIC_RAW, ...) does not
work correctly on s390 via vdso. Debugging this also revealed an
unrelated bug (first patch).

The second patch fixes the problem: the tod clock steering parameters
required by __arch_get_hw_counter() are only present within the first
element of the _vdso_data array and not at all within the _timens_data
array.

Instead of working around this simply provide an s390 specific vdso
data page which contains the tod clock steering parameters.

This allows also to remove ARCH_HAS_VDSO_DATA again.

Heiko Carstens (3):
  s390/vdso: fix tod clock steering
  s390/vdso: fix arch_data access for __arch_get_hw_counter()
  lib/vdso: remove struct arch_vdso_data from vdso data struct

 arch/Kconfig                              |  3 ---
 arch/s390/Kconfig                         |  1 -
 arch/s390/include/asm/vdso.h              |  4 +++-
 arch/s390/include/asm/vdso/data.h         | 13 ------------
 arch/s390/include/asm/vdso/datapage.h     | 17 +++++++++++++++
 arch/s390/include/asm/vdso/gettimeofday.h | 11 ++++++++--
 arch/s390/kernel/time.c                   |  5 +++--
 arch/s390/kernel/vdso.c                   | 25 ++++++++++++++++++++---
 arch/s390/kernel/vdso64/vdso64.lds.S      |  3 ++-
 include/vdso/datapage.h                   | 10 ---------
 10 files changed, 56 insertions(+), 36 deletions(-)
 delete mode 100644 arch/s390/include/asm/vdso/data.h
 create mode 100644 arch/s390/include/asm/vdso/datapage.h

-- 
2.25.1

[PATCH 1/3] s390/vdso: fix tod clock steering

[Heiko Carstens]

The s390 specific vdso function __arch_get_hw_counter() is supposed to
consider tod clock steering.

If a tod clock steering event happens and the tod clock is set to a
new value __arch_get_hw_counter() will not return the real tod clock
value but slowly drift it from the old delta until the returned value
finally matches the real tod clock value again.

When converting the assembler code to C it was forgotten to tell user
space in which direction the clock has to be adjusted.

Worst case is now that instead of drifting the clock slowly it will
jump into the opposite direction by a factor of two.

Fix this by simply providing the missing value to user space.

Fixes: 4bff8cb54502 ("s390: convert to GENERIC_VDSO")
Cc: <stable@vger.kernel.org> # 5.10
Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
---
 arch/s390/kernel/time.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/s390/kernel/time.c b/arch/s390/kernel/time.c
index 165da961f901..e37285a5101b 100644
--- a/arch/s390/kernel/time.c
+++ b/arch/s390/kernel/time.c
@@ -382,6 +382,7 @@ static void clock_sync_global(unsigned long delta)
 		      tod_steering_delta);
 	tod_steering_end = now + (abs(tod_steering_delta) << 15);
 	vdso_data->arch_data.tod_steering_end = tod_steering_end;
+	vdso_data->arch_data.tod_steering_delta = tod_steering_delta;
 
 	/* Update LPAR offset. */
 	if (ptff_query(PTFF_QTO) && ptff(&qto, sizeof(qto), PTFF_QTO) == 0)
-- 
2.25.1

[LTP] [PATCH 1/3] s390/vdso: fix tod clock steering

[Heiko Carstens]

The s390 specific vdso function __arch_get_hw_counter() is supposed to
consider tod clock steering.

If a tod clock steering event happens and the tod clock is set to a
new value __arch_get_hw_counter() will not return the real tod clock
value but slowly drift it from the old delta until the returned value
finally matches the real tod clock value again.

When converting the assembler code to C it was forgotten to tell user
space in which direction the clock has to be adjusted.

Worst case is now that instead of drifting the clock slowly it will
jump into the opposite direction by a factor of two.

Fix this by simply providing the missing value to user space.

Fixes: 4bff8cb54502 ("s390: convert to GENERIC_VDSO")
Cc: <stable@vger.kernel.org> # 5.10
Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
---
 arch/s390/kernel/time.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/s390/kernel/time.c b/arch/s390/kernel/time.c
index 165da961f901..e37285a5101b 100644
--- a/arch/s390/kernel/time.c
+++ b/arch/s390/kernel/time.c
@@ -382,6 +382,7 @@ static void clock_sync_global(unsigned long delta)
 		      tod_steering_delta);
 	tod_steering_end = now + (abs(tod_steering_delta) << 15);
 	vdso_data->arch_data.tod_steering_end = tod_steering_end;
+	vdso_data->arch_data.tod_steering_delta = tod_steering_delta;
 
 	/* Update LPAR offset. */
 	if (ptff_query(PTFF_QTO) && ptff(&qto, sizeof(qto), PTFF_QTO) == 0)
-- 
2.25.1

Re: [PATCH 1/3] s390/vdso: fix tod clock steering

[Heiko Carstens]

On Tue, Mar 23, 2021 at 10:58:17PM +0100, Heiko Carstens wrote:
> The s390 specific vdso function __arch_get_hw_counter() is supposed to
> consider tod clock steering.
> 
> If a tod clock steering event happens and the tod clock is set to a
> new value __arch_get_hw_counter() will not return the real tod clock
> value but slowly drift it from the old delta until the returned value
> finally matches the real tod clock value again.
> 
> When converting the assembler code to C it was forgotten to tell user
> space in which direction the clock has to be adjusted.
> 
> Worst case is now that instead of drifting the clock slowly it will
> jump into the opposite direction by a factor of two.
> 
> Fix this by simply providing the missing value to user space.
> 
> Fixes: 4bff8cb54502 ("s390: convert to GENERIC_VDSO")
> Cc: <stable@vger.kernel.org> # 5.10
> Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
> ---
>  arch/s390/kernel/time.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/s390/kernel/time.c b/arch/s390/kernel/time.c
> index 165da961f901..e37285a5101b 100644
> --- a/arch/s390/kernel/time.c
> +++ b/arch/s390/kernel/time.c
> @@ -382,6 +382,7 @@ static void clock_sync_global(unsigned long delta)
>  		      tod_steering_delta);
>  	tod_steering_end = now + (abs(tod_steering_delta) << 15);
>  	vdso_data->arch_data.tod_steering_end = tod_steering_end;
> +	vdso_data->arch_data.tod_steering_delta = tod_steering_delta;

..and yet another bug: __arch_get_hw_counter() tests if
tod_steering_delta is negative.
It makes sense to give tod_steering_delta the correct type:

diff --git a/arch/s390/include/asm/vdso/data.h b/arch/s390/include/asm/vdso/data.h
index 7b3cdb4a5f48..73ee89142666 100644
--- a/arch/s390/include/asm/vdso/data.h
+++ b/arch/s390/include/asm/vdso/data.h
@@ -6,7 +6,7 @@
 #include <vdso/datapage.h>
 
 struct arch_vdso_data {
-	__u64 tod_steering_delta;
+	__s64 tod_steering_delta;
 	__u64 tod_steering_end;
 };
 

[LTP] [PATCH 1/3] s390/vdso: fix tod clock steering

[Heiko Carstens]

On Tue, Mar 23, 2021 at 10:58:17PM +0100, Heiko Carstens wrote:
> The s390 specific vdso function __arch_get_hw_counter() is supposed to
> consider tod clock steering.
> 
> If a tod clock steering event happens and the tod clock is set to a
> new value __arch_get_hw_counter() will not return the real tod clock
> value but slowly drift it from the old delta until the returned value
> finally matches the real tod clock value again.
> 
> When converting the assembler code to C it was forgotten to tell user
> space in which direction the clock has to be adjusted.
> 
> Worst case is now that instead of drifting the clock slowly it will
> jump into the opposite direction by a factor of two.
> 
> Fix this by simply providing the missing value to user space.
> 
> Fixes: 4bff8cb54502 ("s390: convert to GENERIC_VDSO")
> Cc: <stable@vger.kernel.org> # 5.10
> Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
> ---
>  arch/s390/kernel/time.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/s390/kernel/time.c b/arch/s390/kernel/time.c
> index 165da961f901..e37285a5101b 100644
> --- a/arch/s390/kernel/time.c
> +++ b/arch/s390/kernel/time.c
> @@ -382,6 +382,7 @@ static void clock_sync_global(unsigned long delta)
>  		      tod_steering_delta);
>  	tod_steering_end = now + (abs(tod_steering_delta) << 15);
>  	vdso_data->arch_data.tod_steering_end = tod_steering_end;
> +	vdso_data->arch_data.tod_steering_delta = tod_steering_delta;

..and yet another bug: __arch_get_hw_counter() tests if
tod_steering_delta is negative.
It makes sense to give tod_steering_delta the correct type:

diff --git a/arch/s390/include/asm/vdso/data.h b/arch/s390/include/asm/vdso/data.h
index 7b3cdb4a5f48..73ee89142666 100644
--- a/arch/s390/include/asm/vdso/data.h
+++ b/arch/s390/include/asm/vdso/data.h
@@ -6,7 +6,7 @@
 #include <vdso/datapage.h>
 
 struct arch_vdso_data {
-	__u64 tod_steering_delta;
+	__s64 tod_steering_delta;
 	__u64 tod_steering_end;
 };
 

[PATCH 2/3] s390/vdso: fix arch_data access for __arch_get_hw_counter()

[Heiko Carstens]

Li Wang reported that clock_gettime(CLOCK_MONOTONIC_RAW, ...) returns
incorrect values when time is provided via vdso instead of system call:

vdso_ts_nsec = 4484351380985507, vdso_ts.tv_sec = 4484351, vdso_ts.tv_nsec = 380985507
sys_ts_nsec  = 1446923235377, sys_ts.tv_sec  = 1446, sys_ts.tv_nsec  = 923235377

Within the s390 specific vdso function __arch_get_hw_counter() tries
to read tod clock steering values from the arch_data member of the
passed in vdso_data structure.
However only the arch_data member of the first clock source base
(CS_HRES_COARSE) is initialized. For CS_RAW arch_data is not at all
initialized, which explains the incorrect returned values.

It is a bit odd to provide the required tod clock steering parameters
only within the first element of the _vdso_data array. However for
time namespaces even no member of the _timens_data array contains the
required data, which would make fixing __arch_get_hw_counter() quite
complicated.

Therefore simply add an s390 specific vdso data page which contains
the tod clock steering parameters. Everything else seems to be
unnecessary complex.

Reported-by: Li Wang <liwang@redhat.com>
Fixes: 1ba2d6c0fd4e ("s390/vdso: simplify __arch_get_hw_counter()")
Fixes: eeab78b05d20 ("s390/vdso: implement generic vdso time namespace support")
Link: https://lore.kernel.org/linux-s390/YFnxr1ZlMIOIqjfq@osiris
Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
---
 arch/s390/Kconfig                         |  1 -
 arch/s390/include/asm/vdso.h              |  4 +++-
 arch/s390/include/asm/vdso/data.h         | 13 ------------
 arch/s390/include/asm/vdso/datapage.h     | 17 +++++++++++++++
 arch/s390/include/asm/vdso/gettimeofday.h | 11 ++++++++--
 arch/s390/kernel/time.c                   |  6 +++---
 arch/s390/kernel/vdso.c                   | 25 ++++++++++++++++++++---
 arch/s390/kernel/vdso64/vdso64.lds.S      |  3 ++-
 8 files changed, 56 insertions(+), 24 deletions(-)
 delete mode 100644 arch/s390/include/asm/vdso/data.h
 create mode 100644 arch/s390/include/asm/vdso/datapage.h

diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index c1ff874e6c2e..532ce0fcc659 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -77,7 +77,6 @@ config S390
 	select ARCH_HAS_STRICT_MODULE_RWX
 	select ARCH_HAS_SYSCALL_WRAPPER
 	select ARCH_HAS_UBSAN_SANITIZE_ALL
-	select ARCH_HAS_VDSO_DATA
 	select ARCH_HAVE_NMI_SAFE_CMPXCHG
 	select ARCH_INLINE_READ_LOCK
 	select ARCH_INLINE_READ_LOCK_BH
diff --git a/arch/s390/include/asm/vdso.h b/arch/s390/include/asm/vdso.h
index b45e3dddd2c2..0d047f519df6 100644
--- a/arch/s390/include/asm/vdso.h
+++ b/arch/s390/include/asm/vdso.h
@@ -3,17 +3,19 @@
 #define __S390_VDSO_H__
 
 #include <vdso/datapage.h>
+#include <asm/vdso/datapage.h>
 
 /* Default link address for the vDSO */
 #define VDSO64_LBASE	0
 
-#define __VVAR_PAGES	2
+#define __VVAR_PAGES	3
 
 #define VDSO_VERSION_STRING	LINUX_2.6.29
 
 #ifndef __ASSEMBLY__
 
 extern struct vdso_data *vdso_data;
+extern struct s390_vdso_data *s390_vdso_data;
 
 int vdso_getcpu_init(void);
 
diff --git a/arch/s390/include/asm/vdso/data.h b/arch/s390/include/asm/vdso/data.h
deleted file mode 100644
index 7b3cdb4a5f48..000000000000
--- a/arch/s390/include/asm/vdso/data.h
+++ /dev/null
@@ -1,13 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-#ifndef __S390_ASM_VDSO_DATA_H
-#define __S390_ASM_VDSO_DATA_H
-
-#include <linux/types.h>
-#include <vdso/datapage.h>
-
-struct arch_vdso_data {
-	__u64 tod_steering_delta;
-	__u64 tod_steering_end;
-};
-
-#endif /* __S390_ASM_VDSO_DATA_H */
diff --git a/arch/s390/include/asm/vdso/datapage.h b/arch/s390/include/asm/vdso/datapage.h
new file mode 100644
index 000000000000..bfae78d814af
--- /dev/null
+++ b/arch/s390/include/asm/vdso/datapage.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __S390_ASM_VDSO_DATAPAGE_H
+#define __S390_ASM_VDSO_DATAPAGE_H
+
+#include <linux/types.h>
+
+#ifndef __ASSEMBLY__
+
+struct s390_vdso_data {
+	__u64 tod_steering_delta;
+	__u64 tod_steering_end;
+};
+
+extern struct s390_vdso_data _s390_data __attribute__((visibility("hidden")));
+
+#endif /* __ASSEMBLY__ */
+#endif /* __S390_ASM_VDSO_DATAPAGE_H */
diff --git a/arch/s390/include/asm/vdso/gettimeofday.h b/arch/s390/include/asm/vdso/gettimeofday.h
index ed89ef742530..bbd6da6b1651 100644
--- a/arch/s390/include/asm/vdso/gettimeofday.h
+++ b/arch/s390/include/asm/vdso/gettimeofday.h
@@ -9,6 +9,7 @@
 #include <asm/timex.h>
 #include <asm/unistd.h>
 #include <asm/vdso.h>
+#include <asm/vdso/datapage.h>
 #include <linux/compiler.h>
 
 #define vdso_calc_delta __arch_vdso_calc_delta
@@ -22,14 +23,20 @@ static __always_inline const struct vdso_data *__arch_get_vdso_data(void)
 	return _vdso_data;
 }
 
+static __always_inline const struct s390_vdso_data *__get_s390_vdso_data(void)
+{
+	return &_s390_data;
+}
+
 static inline u64 __arch_get_hw_counter(s32 clock_mode, const struct vdso_data *vd)
 {
+	const struct s390_vdso_data *svd = __get_s390_vdso_data();
 	u64 adj, now;
 
 	now = get_tod_clock();
-	adj = vd->arch_data.tod_steering_end - now;
+	adj = svd->tod_steering_end - now;
 	if (unlikely((s64) adj > 0))
-		now += (vd->arch_data.tod_steering_delta < 0) ? (adj >> 15) : -(adj >> 15);
+		now += (svd->tod_steering_delta < 0) ? (adj >> 15) : -(adj >> 15);
 	return now;
 }
 
diff --git a/arch/s390/kernel/time.c b/arch/s390/kernel/time.c
index e37285a5101b..ec5c12e541aa 100644
--- a/arch/s390/kernel/time.c
+++ b/arch/s390/kernel/time.c
@@ -83,7 +83,7 @@ void __init time_early_init(void)
 
 	/* Initialize TOD steering parameters */
 	tod_steering_end = tod_clock_base.tod;
-	vdso_data->arch_data.tod_steering_end = tod_steering_end;
+	s390_vdso_data->tod_steering_end = tod_steering_end;
 
 	if (!test_facility(28))
 		return;
@@ -381,8 +381,8 @@ static void clock_sync_global(unsigned long delta)
 		panic("TOD clock sync offset %li is too large to drift\n",
 		      tod_steering_delta);
 	tod_steering_end = now + (abs(tod_steering_delta) << 15);
-	vdso_data->arch_data.tod_steering_end = tod_steering_end;
-	vdso_data->arch_data.tod_steering_delta = tod_steering_delta;
+	s390_vdso_data->tod_steering_end = tod_steering_end;
+	s390_vdso_data->tod_steering_delta = tod_steering_delta;
 
 	/* Update LPAR offset. */
 	if (ptff_query(PTFF_QTO) && ptff(&qto, sizeof(qto), PTFF_QTO) == 0)
diff --git a/arch/s390/kernel/vdso.c b/arch/s390/kernel/vdso.c
index 8c4e07d533c8..4f1dba52b240 100644
--- a/arch/s390/kernel/vdso.c
+++ b/arch/s390/kernel/vdso.c
@@ -29,9 +29,16 @@ static union {
 	u8			page[PAGE_SIZE];
 } vdso_data_store __page_aligned_data;
 
+static union {
+	struct s390_vdso_data	data;
+	u8			page[PAGE_SIZE];
+} s390_vdso_page __page_aligned_data;
+
 struct vdso_data *vdso_data = vdso_data_store.data;
+struct s390_vdso_data *s390_vdso_data = &s390_vdso_page.data;
 
 enum vvar_pages {
+	VVAR_S390_PAGE_OFFSET,
 	VVAR_DATA_PAGE_OFFSET,
 	VVAR_TIMENS_PAGE_OFFSET,
 	VVAR_NR_PAGES,
@@ -109,14 +116,26 @@ static vm_fault_t vvar_fault(const struct vm_special_mapping *sm,
 	vm_fault_t err;
 
 	switch (vmf->pgoff) {
+	case VVAR_S390_PAGE_OFFSET:
+		pfn = virt_to_pfn(s390_vdso_data);
+		break;
 	case VVAR_DATA_PAGE_OFFSET:
+		/*
+		 * Fault in VVAR s390 page too, since it will be
+		 * accessed to get tod clock steering data anyway.
+		 */
+		addr = vma->vm_start + VVAR_S390_PAGE_OFFSET * PAGE_SIZE;
+		pfn = virt_to_pfn(s390_vdso_data);
+		err = vmf_insert_pfn(vma, addr, pfn);
+		if (unlikely(err & VM_FAULT_ERROR))
+			return err;
+		addr = vma->vm_start + VVAR_TIMENS_PAGE_OFFSET * PAGE_SIZE;
 		pfn = virt_to_pfn(vdso_data);
 		if (timens_page) {
 			/*
-			 * Fault in VVAR page too, since it will be accessed
-			 * to get clock data anyway.
+			 * Fault in VVAR data page too, since it will be
+			 * accessed to get clock data anyway.
 			 */
-			addr = vmf->address + VVAR_TIMENS_PAGE_OFFSET * PAGE_SIZE;
 			err = vmf_insert_pfn(vma, addr, pfn);
 			if (unlikely(err & VM_FAULT_ERROR))
 				return err;
diff --git a/arch/s390/kernel/vdso64/vdso64.lds.S b/arch/s390/kernel/vdso64/vdso64.lds.S
index 518f1ea405f4..d38e5475df65 100644
--- a/arch/s390/kernel/vdso64/vdso64.lds.S
+++ b/arch/s390/kernel/vdso64/vdso64.lds.S
@@ -13,7 +13,8 @@ ENTRY(_start)
 
 SECTIONS
 {
-	PROVIDE(_vdso_data = . - __VVAR_PAGES * PAGE_SIZE);
+	PROVIDE(_s390_data = . - __VVAR_PAGES * PAGE_SIZE);
+	PROVIDE(_vdso_data = _s390_data + PAGE_SIZE);
 #ifdef CONFIG_TIME_NS
 	PROVIDE(_timens_data = _vdso_data + PAGE_SIZE);
 #endif
-- 
2.25.1

[LTP] [PATCH 2/3] s390/vdso: fix arch_data access for __arch_get_hw_counter()

[Heiko Carstens]

Li Wang reported that clock_gettime(CLOCK_MONOTONIC_RAW, ...) returns
incorrect values when time is provided via vdso instead of system call:

vdso_ts_nsec = 4484351380985507, vdso_ts.tv_sec = 4484351, vdso_ts.tv_nsec = 380985507
sys_ts_nsec  = 1446923235377, sys_ts.tv_sec  = 1446, sys_ts.tv_nsec  = 923235377

Within the s390 specific vdso function __arch_get_hw_counter() tries
to read tod clock steering values from the arch_data member of the
passed in vdso_data structure.
However only the arch_data member of the first clock source base
(CS_HRES_COARSE) is initialized. For CS_RAW arch_data is not at all
initialized, which explains the incorrect returned values.

It is a bit odd to provide the required tod clock steering parameters
only within the first element of the _vdso_data array. However for
time namespaces even no member of the _timens_data array contains the
required data, which would make fixing __arch_get_hw_counter() quite
complicated.

Therefore simply add an s390 specific vdso data page which contains
the tod clock steering parameters. Everything else seems to be
unnecessary complex.

Reported-by: Li Wang <liwang@redhat.com>
Fixes: 1ba2d6c0fd4e ("s390/vdso: simplify __arch_get_hw_counter()")
Fixes: eeab78b05d20 ("s390/vdso: implement generic vdso time namespace support")
Link: https://lore.kernel.org/linux-s390/YFnxr1ZlMIOIqjfq@osiris
Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
---
 arch/s390/Kconfig                         |  1 -
 arch/s390/include/asm/vdso.h              |  4 +++-
 arch/s390/include/asm/vdso/data.h         | 13 ------------
 arch/s390/include/asm/vdso/datapage.h     | 17 +++++++++++++++
 arch/s390/include/asm/vdso/gettimeofday.h | 11 ++++++++--
 arch/s390/kernel/time.c                   |  6 +++---
 arch/s390/kernel/vdso.c                   | 25 ++++++++++++++++++++---
 arch/s390/kernel/vdso64/vdso64.lds.S      |  3 ++-
 8 files changed, 56 insertions(+), 24 deletions(-)
 delete mode 100644 arch/s390/include/asm/vdso/data.h
 create mode 100644 arch/s390/include/asm/vdso/datapage.h

diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index c1ff874e6c2e..532ce0fcc659 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -77,7 +77,6 @@ config S390
 	select ARCH_HAS_STRICT_MODULE_RWX
 	select ARCH_HAS_SYSCALL_WRAPPER
 	select ARCH_HAS_UBSAN_SANITIZE_ALL
-	select ARCH_HAS_VDSO_DATA
 	select ARCH_HAVE_NMI_SAFE_CMPXCHG
 	select ARCH_INLINE_READ_LOCK
 	select ARCH_INLINE_READ_LOCK_BH
diff --git a/arch/s390/include/asm/vdso.h b/arch/s390/include/asm/vdso.h
index b45e3dddd2c2..0d047f519df6 100644
--- a/arch/s390/include/asm/vdso.h
+++ b/arch/s390/include/asm/vdso.h
@@ -3,17 +3,19 @@
 #define __S390_VDSO_H__
 
 #include <vdso/datapage.h>
+#include <asm/vdso/datapage.h>
 
 /* Default link address for the vDSO */
 #define VDSO64_LBASE	0
 
-#define __VVAR_PAGES	2
+#define __VVAR_PAGES	3
 
 #define VDSO_VERSION_STRING	LINUX_2.6.29
 
 #ifndef __ASSEMBLY__
 
 extern struct vdso_data *vdso_data;
+extern struct s390_vdso_data *s390_vdso_data;
 
 int vdso_getcpu_init(void);
 
diff --git a/arch/s390/include/asm/vdso/data.h b/arch/s390/include/asm/vdso/data.h
deleted file mode 100644
index 7b3cdb4a5f48..000000000000
--- a/arch/s390/include/asm/vdso/data.h
+++ /dev/null
@@ -1,13 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-#ifndef __S390_ASM_VDSO_DATA_H
-#define __S390_ASM_VDSO_DATA_H
-
-#include <linux/types.h>
-#include <vdso/datapage.h>
-
-struct arch_vdso_data {
-	__u64 tod_steering_delta;
-	__u64 tod_steering_end;
-};
-
-#endif /* __S390_ASM_VDSO_DATA_H */
diff --git a/arch/s390/include/asm/vdso/datapage.h b/arch/s390/include/asm/vdso/datapage.h
new file mode 100644
index 000000000000..bfae78d814af
--- /dev/null
+++ b/arch/s390/include/asm/vdso/datapage.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __S390_ASM_VDSO_DATAPAGE_H
+#define __S390_ASM_VDSO_DATAPAGE_H
+
+#include <linux/types.h>
+
+#ifndef __ASSEMBLY__
+
+struct s390_vdso_data {
+	__u64 tod_steering_delta;
+	__u64 tod_steering_end;
+};
+
+extern struct s390_vdso_data _s390_data __attribute__((visibility("hidden")));
+
+#endif /* __ASSEMBLY__ */
+#endif /* __S390_ASM_VDSO_DATAPAGE_H */
diff --git a/arch/s390/include/asm/vdso/gettimeofday.h b/arch/s390/include/asm/vdso/gettimeofday.h
index ed89ef742530..bbd6da6b1651 100644
--- a/arch/s390/include/asm/vdso/gettimeofday.h
+++ b/arch/s390/include/asm/vdso/gettimeofday.h
@@ -9,6 +9,7 @@
 #include <asm/timex.h>
 #include <asm/unistd.h>
 #include <asm/vdso.h>
+#include <asm/vdso/datapage.h>
 #include <linux/compiler.h>
 
 #define vdso_calc_delta __arch_vdso_calc_delta
@@ -22,14 +23,20 @@ static __always_inline const struct vdso_data *__arch_get_vdso_data(void)
 	return _vdso_data;
 }
 
+static __always_inline const struct s390_vdso_data *__get_s390_vdso_data(void)
+{
+	return &_s390_data;
+}
+
 static inline u64 __arch_get_hw_counter(s32 clock_mode, const struct vdso_data *vd)
 {
+	const struct s390_vdso_data *svd = __get_s390_vdso_data();
 	u64 adj, now;
 
 	now = get_tod_clock();
-	adj = vd->arch_data.tod_steering_end - now;
+	adj = svd->tod_steering_end - now;
 	if (unlikely((s64) adj > 0))
-		now += (vd->arch_data.tod_steering_delta < 0) ? (adj >> 15) : -(adj >> 15);
+		now += (svd->tod_steering_delta < 0) ? (adj >> 15) : -(adj >> 15);
 	return now;
 }
 
diff --git a/arch/s390/kernel/time.c b/arch/s390/kernel/time.c
index e37285a5101b..ec5c12e541aa 100644
--- a/arch/s390/kernel/time.c
+++ b/arch/s390/kernel/time.c
@@ -83,7 +83,7 @@ void __init time_early_init(void)
 
 	/* Initialize TOD steering parameters */
 	tod_steering_end = tod_clock_base.tod;
-	vdso_data->arch_data.tod_steering_end = tod_steering_end;
+	s390_vdso_data->tod_steering_end = tod_steering_end;
 
 	if (!test_facility(28))
 		return;
@@ -381,8 +381,8 @@ static void clock_sync_global(unsigned long delta)
 		panic("TOD clock sync offset %li is too large to drift\n",
 		      tod_steering_delta);
 	tod_steering_end = now + (abs(tod_steering_delta) << 15);
-	vdso_data->arch_data.tod_steering_end = tod_steering_end;
-	vdso_data->arch_data.tod_steering_delta = tod_steering_delta;
+	s390_vdso_data->tod_steering_end = tod_steering_end;
+	s390_vdso_data->tod_steering_delta = tod_steering_delta;
 
 	/* Update LPAR offset. */
 	if (ptff_query(PTFF_QTO) && ptff(&qto, sizeof(qto), PTFF_QTO) == 0)
diff --git a/arch/s390/kernel/vdso.c b/arch/s390/kernel/vdso.c
index 8c4e07d533c8..4f1dba52b240 100644
--- a/arch/s390/kernel/vdso.c
+++ b/arch/s390/kernel/vdso.c
@@ -29,9 +29,16 @@ static union {
 	u8			page[PAGE_SIZE];
 } vdso_data_store __page_aligned_data;
 
+static union {
+	struct s390_vdso_data	data;
+	u8			page[PAGE_SIZE];
+} s390_vdso_page __page_aligned_data;
+
 struct vdso_data *vdso_data = vdso_data_store.data;
+struct s390_vdso_data *s390_vdso_data = &s390_vdso_page.data;
 
 enum vvar_pages {
+	VVAR_S390_PAGE_OFFSET,
 	VVAR_DATA_PAGE_OFFSET,
 	VVAR_TIMENS_PAGE_OFFSET,
 	VVAR_NR_PAGES,
@@ -109,14 +116,26 @@ static vm_fault_t vvar_fault(const struct vm_special_mapping *sm,
 	vm_fault_t err;
 
 	switch (vmf->pgoff) {
+	case VVAR_S390_PAGE_OFFSET:
+		pfn = virt_to_pfn(s390_vdso_data);
+		break;
 	case VVAR_DATA_PAGE_OFFSET:
+		/*
+		 * Fault in VVAR s390 page too, since it will be
+		 * accessed to get tod clock steering data anyway.
+		 */
+		addr = vma->vm_start + VVAR_S390_PAGE_OFFSET * PAGE_SIZE;
+		pfn = virt_to_pfn(s390_vdso_data);
+		err = vmf_insert_pfn(vma, addr, pfn);
+		if (unlikely(err & VM_FAULT_ERROR))
+			return err;
+		addr = vma->vm_start + VVAR_TIMENS_PAGE_OFFSET * PAGE_SIZE;
 		pfn = virt_to_pfn(vdso_data);
 		if (timens_page) {
 			/*
-			 * Fault in VVAR page too, since it will be accessed
-			 * to get clock data anyway.
+			 * Fault in VVAR data page too, since it will be
+			 * accessed to get clock data anyway.
 			 */
-			addr = vmf->address + VVAR_TIMENS_PAGE_OFFSET * PAGE_SIZE;
 			err = vmf_insert_pfn(vma, addr, pfn);
 			if (unlikely(err & VM_FAULT_ERROR))
 				return err;
diff --git a/arch/s390/kernel/vdso64/vdso64.lds.S b/arch/s390/kernel/vdso64/vdso64.lds.S
index 518f1ea405f4..d38e5475df65 100644
--- a/arch/s390/kernel/vdso64/vdso64.lds.S
+++ b/arch/s390/kernel/vdso64/vdso64.lds.S
@@ -13,7 +13,8 @@ ENTRY(_start)
 
 SECTIONS
 {
-	PROVIDE(_vdso_data = . - __VVAR_PAGES * PAGE_SIZE);
+	PROVIDE(_s390_data = . - __VVAR_PAGES * PAGE_SIZE);
+	PROVIDE(_vdso_data = _s390_data + PAGE_SIZE);
 #ifdef CONFIG_TIME_NS
 	PROVIDE(_timens_data = _vdso_data + PAGE_SIZE);
 #endif
-- 
2.25.1

Re: [PATCH 2/3] s390/vdso: fix arch_data access for __arch_get_hw_counter()

[Heiko Carstens]

On Tue, Mar 23, 2021 at 10:58:18PM +0100, Heiko Carstens wrote:
> Li Wang reported that clock_gettime(CLOCK_MONOTONIC_RAW, ...) returns
> incorrect values when time is provided via vdso instead of system call:
> 
> vdso_ts_nsec = 4484351380985507, vdso_ts.tv_sec = 4484351, vdso_ts.tv_nsec = 380985507
> sys_ts_nsec  = 1446923235377, sys_ts.tv_sec  = 1446, sys_ts.tv_nsec  = 923235377
> 
> Within the s390 specific vdso function __arch_get_hw_counter() tries
> to read tod clock steering values from the arch_data member of the
> passed in vdso_data structure.
> However only the arch_data member of the first clock source base
> (CS_HRES_COARSE) is initialized. For CS_RAW arch_data is not at all
> initialized, which explains the incorrect returned values.
> 
> It is a bit odd to provide the required tod clock steering parameters
> only within the first element of the _vdso_data array. However for
> time namespaces even no member of the _timens_data array contains the
> required data, which would make fixing __arch_get_hw_counter() quite
> complicated.
> 
> Therefore simply add an s390 specific vdso data page which contains
> the tod clock steering parameters. Everything else seems to be
> unnecessary complex.
> 
> Reported-by: Li Wang <liwang@redhat.com>
> Fixes: 1ba2d6c0fd4e ("s390/vdso: simplify __arch_get_hw_counter()")
> Fixes: eeab78b05d20 ("s390/vdso: implement generic vdso time namespace support")
> Link: https://lore.kernel.org/linux-s390/YFnxr1ZlMIOIqjfq@osiris
> Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
> ---
>  arch/s390/Kconfig                         |  1 -
>  arch/s390/include/asm/vdso.h              |  4 +++-
>  arch/s390/include/asm/vdso/data.h         | 13 ------------
>  arch/s390/include/asm/vdso/datapage.h     | 17 +++++++++++++++
>  arch/s390/include/asm/vdso/gettimeofday.h | 11 ++++++++--
>  arch/s390/kernel/time.c                   |  6 +++---
>  arch/s390/kernel/vdso.c                   | 25 ++++++++++++++++++++---
>  arch/s390/kernel/vdso64/vdso64.lds.S      |  3 ++-
>  8 files changed, 56 insertions(+), 24 deletions(-)
>  delete mode 100644 arch/s390/include/asm/vdso/data.h
>  create mode 100644 arch/s390/include/asm/vdso/datapage.h

FWIW, alternatively to this and the third patch we could also do the
much shorter and simpler variant below. What I personally don't like
is that data is duplicated.
But on the other hand it is much shorter, and the more I think of it
this seems to be the way to go.
Opinions?

diff --git a/arch/s390/kernel/time.c b/arch/s390/kernel/time.c
index e37285a5101b..fa095ecf0349 100644
--- a/arch/s390/kernel/time.c
+++ b/arch/s390/kernel/time.c
@@ -80,10 +80,12 @@ void __init time_early_init(void)
 {
 	struct ptff_qto qto;
 	struct ptff_qui qui;
+	int i;
 
 	/* Initialize TOD steering parameters */
 	tod_steering_end = tod_clock_base.tod;
-	vdso_data->arch_data.tod_steering_end = tod_steering_end;
+	for (i = 0; i < CS_BASES; i++)
+		vdso_data[i].arch_data.tod_steering_end = tod_steering_end;
 
 	if (!test_facility(28))
 		return;
@@ -366,6 +368,7 @@ static void clock_sync_global(unsigned long delta)
 {
 	unsigned long now, adj;
 	struct ptff_qto qto;
+	int i;
 
 	/* Fixup the monotonic sched clock. */
 	tod_clock_base.eitod += delta;
@@ -381,8 +384,10 @@ static void clock_sync_global(unsigned long delta)
 		panic("TOD clock sync offset %li is too large to drift\n",
 		      tod_steering_delta);
 	tod_steering_end = now + (abs(tod_steering_delta) << 15);
-	vdso_data->arch_data.tod_steering_end = tod_steering_end;
-	vdso_data->arch_data.tod_steering_delta = tod_steering_delta;
+	for (i = 0; i < CS_BASES; i++) {
+		vdso_data[i].arch_data.tod_steering_end = tod_steering_end;
+		vdso_data[i].arch_data.tod_steering_delta = tod_steering_delta;
+	}
 
 	/* Update LPAR offset. */
 	if (ptff_query(PTFF_QTO) && ptff(&qto, sizeof(qto), PTFF_QTO) == 0)

[LTP] [PATCH 2/3] s390/vdso: fix arch_data access for __arch_get_hw_counter()

[Heiko Carstens]

On Tue, Mar 23, 2021 at 10:58:18PM +0100, Heiko Carstens wrote:
> Li Wang reported that clock_gettime(CLOCK_MONOTONIC_RAW, ...) returns
> incorrect values when time is provided via vdso instead of system call:
> 
> vdso_ts_nsec = 4484351380985507, vdso_ts.tv_sec = 4484351, vdso_ts.tv_nsec = 380985507
> sys_ts_nsec  = 1446923235377, sys_ts.tv_sec  = 1446, sys_ts.tv_nsec  = 923235377
> 
> Within the s390 specific vdso function __arch_get_hw_counter() tries
> to read tod clock steering values from the arch_data member of the
> passed in vdso_data structure.
> However only the arch_data member of the first clock source base
> (CS_HRES_COARSE) is initialized. For CS_RAW arch_data is not at all
> initialized, which explains the incorrect returned values.
> 
> It is a bit odd to provide the required tod clock steering parameters
> only within the first element of the _vdso_data array. However for
> time namespaces even no member of the _timens_data array contains the
> required data, which would make fixing __arch_get_hw_counter() quite
> complicated.
> 
> Therefore simply add an s390 specific vdso data page which contains
> the tod clock steering parameters. Everything else seems to be
> unnecessary complex.
> 
> Reported-by: Li Wang <liwang@redhat.com>
> Fixes: 1ba2d6c0fd4e ("s390/vdso: simplify __arch_get_hw_counter()")
> Fixes: eeab78b05d20 ("s390/vdso: implement generic vdso time namespace support")
> Link: https://lore.kernel.org/linux-s390/YFnxr1ZlMIOIqjfq@osiris
> Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
> ---
>  arch/s390/Kconfig                         |  1 -
>  arch/s390/include/asm/vdso.h              |  4 +++-
>  arch/s390/include/asm/vdso/data.h         | 13 ------------
>  arch/s390/include/asm/vdso/datapage.h     | 17 +++++++++++++++
>  arch/s390/include/asm/vdso/gettimeofday.h | 11 ++++++++--
>  arch/s390/kernel/time.c                   |  6 +++---
>  arch/s390/kernel/vdso.c                   | 25 ++++++++++++++++++++---
>  arch/s390/kernel/vdso64/vdso64.lds.S      |  3 ++-
>  8 files changed, 56 insertions(+), 24 deletions(-)
>  delete mode 100644 arch/s390/include/asm/vdso/data.h
>  create mode 100644 arch/s390/include/asm/vdso/datapage.h

FWIW, alternatively to this and the third patch we could also do the
much shorter and simpler variant below. What I personally don't like
is that data is duplicated.
But on the other hand it is much shorter, and the more I think of it
this seems to be the way to go.
Opinions?

diff --git a/arch/s390/kernel/time.c b/arch/s390/kernel/time.c
index e37285a5101b..fa095ecf0349 100644
--- a/arch/s390/kernel/time.c
+++ b/arch/s390/kernel/time.c
@@ -80,10 +80,12 @@ void __init time_early_init(void)
 {
 	struct ptff_qto qto;
 	struct ptff_qui qui;
+	int i;
 
 	/* Initialize TOD steering parameters */
 	tod_steering_end = tod_clock_base.tod;
-	vdso_data->arch_data.tod_steering_end = tod_steering_end;
+	for (i = 0; i < CS_BASES; i++)
+		vdso_data[i].arch_data.tod_steering_end = tod_steering_end;
 
 	if (!test_facility(28))
 		return;
@@ -366,6 +368,7 @@ static void clock_sync_global(unsigned long delta)
 {
 	unsigned long now, adj;
 	struct ptff_qto qto;
+	int i;
 
 	/* Fixup the monotonic sched clock. */
 	tod_clock_base.eitod += delta;
@@ -381,8 +384,10 @@ static void clock_sync_global(unsigned long delta)
 		panic("TOD clock sync offset %li is too large to drift\n",
 		      tod_steering_delta);
 	tod_steering_end = now + (abs(tod_steering_delta) << 15);
-	vdso_data->arch_data.tod_steering_end = tod_steering_end;
-	vdso_data->arch_data.tod_steering_delta = tod_steering_delta;
+	for (i = 0; i < CS_BASES; i++) {
+		vdso_data[i].arch_data.tod_steering_end = tod_steering_end;
+		vdso_data[i].arch_data.tod_steering_delta = tod_steering_delta;
+	}
 
 	/* Update LPAR offset. */
 	if (ptff_query(PTFF_QTO) && ptff(&qto, sizeof(qto), PTFF_QTO) == 0)

[PATCH 3/3] lib/vdso: remove struct arch_vdso_data from vdso data struct

[Heiko Carstens]

Since commit d60d7de3e16d ("lib/vdso: Allow to add architecture-specific
vdso data") it is possible to provide arch specific VDSO data.

This was only added for s390, which doesn't make use this anymore.
Therefore remove it again.

Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
---
 arch/Kconfig            |  3 ---
 include/vdso/datapage.h | 10 ----------
 2 files changed, 13 deletions(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index ecfd3520b676..35c7114f7ea3 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -1147,9 +1147,6 @@ config HAVE_SPARSE_SYSCALL_NR
 	  entries at 4000, 5000 and 6000 locations. This option turns on syscall
 	  related optimizations for a given architecture.
 
-config ARCH_HAS_VDSO_DATA
-	bool
-
 config HAVE_STATIC_CALL
 	bool
 
diff --git a/include/vdso/datapage.h b/include/vdso/datapage.h
index 73eb622e7663..ee810cae4e1e 100644
--- a/include/vdso/datapage.h
+++ b/include/vdso/datapage.h
@@ -19,12 +19,6 @@
 #include <vdso/time32.h>
 #include <vdso/time64.h>
 
-#ifdef CONFIG_ARCH_HAS_VDSO_DATA
-#include <asm/vdso/data.h>
-#else
-struct arch_vdso_data {};
-#endif
-
 #define VDSO_BASES	(CLOCK_TAI + 1)
 #define VDSO_HRES	(BIT(CLOCK_REALTIME)		| \
 			 BIT(CLOCK_MONOTONIC)		| \
@@ -70,8 +64,6 @@ struct vdso_timestamp {
  * @tz_dsttime:		type of DST correction
  * @hrtimer_res:	hrtimer resolution
  * @__unused:		unused
- * @arch_data:		architecture specific data (optional, defaults
- *			to an empty struct)
  *
  * vdso_data will be accessed by 64 bit and compat code at the same time
  * so we should be careful before modifying this structure.
@@ -105,8 +97,6 @@ struct vdso_data {
 	s32			tz_dsttime;
 	u32			hrtimer_res;
 	u32			__unused;
-
-	struct arch_vdso_data	arch_data;
 };
 
 /*
-- 
2.25.1

[LTP] [PATCH 3/3] lib/vdso: remove struct arch_vdso_data from vdso data struct

[Heiko Carstens]

Since commit d60d7de3e16d ("lib/vdso: Allow to add architecture-specific
vdso data") it is possible to provide arch specific VDSO data.

This was only added for s390, which doesn't make use this anymore.
Therefore remove it again.

Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
---
 arch/Kconfig            |  3 ---
 include/vdso/datapage.h | 10 ----------
 2 files changed, 13 deletions(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index ecfd3520b676..35c7114f7ea3 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -1147,9 +1147,6 @@ config HAVE_SPARSE_SYSCALL_NR
 	  entries at 4000, 5000 and 6000 locations. This option turns on syscall
 	  related optimizations for a given architecture.
 
-config ARCH_HAS_VDSO_DATA
-	bool
-
 config HAVE_STATIC_CALL
 	bool
 
diff --git a/include/vdso/datapage.h b/include/vdso/datapage.h
index 73eb622e7663..ee810cae4e1e 100644
--- a/include/vdso/datapage.h
+++ b/include/vdso/datapage.h
@@ -19,12 +19,6 @@
 #include <vdso/time32.h>
 #include <vdso/time64.h>
 
-#ifdef CONFIG_ARCH_HAS_VDSO_DATA
-#include <asm/vdso/data.h>
-#else
-struct arch_vdso_data {};
-#endif
-
 #define VDSO_BASES	(CLOCK_TAI + 1)
 #define VDSO_HRES	(BIT(CLOCK_REALTIME)		| \
 			 BIT(CLOCK_MONOTONIC)		| \
@@ -70,8 +64,6 @@ struct vdso_timestamp {
  * @tz_dsttime:		type of DST correction
  * @hrtimer_res:	hrtimer resolution
  * @__unused:		unused
- * @arch_data:		architecture specific data (optional, defaults
- *			to an empty struct)
  *
  * vdso_data will be accessed by 64 bit and compat code at the same time
  * so we should be careful before modifying this structure.
@@ -105,8 +97,6 @@ struct vdso_data {
 	s32			tz_dsttime;
 	u32			hrtimer_res;
 	u32			__unused;
-
-	struct arch_vdso_data	arch_data;
 };
 
 /*
-- 
2.25.1

Re: [PATCH 3/3] lib/vdso: remove struct arch_vdso_data from vdso data struct

[Thomas Gleixner]

On Tue, Mar 23 2021 at 22:58, Heiko Carstens wrote:
> Since commit d60d7de3e16d ("lib/vdso: Allow to add architecture-specific
> vdso data") it is possible to provide arch specific VDSO data.
>
> This was only added for s390, which doesn't make use this anymore.
> Therefore remove it again.
>
> Signed-off-by: Heiko Carstens <hca@linux.ibm.com>

Please route that with the rest of the fixes.

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

[LTP] [PATCH 3/3] lib/vdso: remove struct arch_vdso_data from vdso data struct

[Thomas Gleixner]

On Tue, Mar 23 2021 at 22:58, Heiko Carstens wrote:
> Since commit d60d7de3e16d ("lib/vdso: Allow to add architecture-specific
> vdso data") it is possible to provide arch specific VDSO data.
>
> This was only added for s390, which doesn't make use this anymore.
> Therefore remove it again.
>
> Signed-off-by: Heiko Carstens <hca@linux.ibm.com>

Please route that with the rest of the fixes.

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Re: [PATCH 3/3] lib/vdso: remove struct arch_vdso_data from vdso data struct

[Thomas Gleixner]

On Thu, Mar 25 2021 at 18:55, Thomas Gleixner wrote:
> On Tue, Mar 23 2021 at 22:58, Heiko Carstens wrote:
>> Since commit d60d7de3e16d ("lib/vdso: Allow to add architecture-specific
>> vdso data") it is possible to provide arch specific VDSO data.
>>
>> This was only added for s390, which doesn't make use this anymore.
>> Therefore remove it again.
>>
>> Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
>
> Please route that with the rest of the fixes.
>
> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Ah, you decided for the simpler variant. Fine with me.

[LTP] [PATCH 3/3] lib/vdso: remove struct arch_vdso_data from vdso data struct

[Thomas Gleixner]

On Thu, Mar 25 2021 at 18:55, Thomas Gleixner wrote:
> On Tue, Mar 23 2021 at 22:58, Heiko Carstens wrote:
>> Since commit d60d7de3e16d ("lib/vdso: Allow to add architecture-specific
>> vdso data") it is possible to provide arch specific VDSO data.
>>
>> This was only added for s390, which doesn't make use this anymore.
>> Therefore remove it again.
>>
>> Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
>
> Please route that with the rest of the fixes.
>
> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Ah, you decided for the simpler variant. Fine with me.

[LTP] [PATCH 0/3] s390 vdso fixes

[Li Wang]

Hi Heiko,

On Wed, Mar 24, 2021 at 5:58 AM Heiko Carstens <hca@linux.ibm.com> wrote:

> Li Wang reported that clock_gettime(CLOCK_MONOTONIC_RAW, ...) does not
> work correctly on s390 via vdso. Debugging this also revealed an
> unrelated bug (first patch).
>
> The second patch fixes the problem: the tod clock steering parameters
> required by __arch_get_hw_counter() are only present within the first
> element of the _vdso_data array and not at all within the _timens_data
> array.
>
> Instead of working around this simply provide an s390 specific vdso
> data page which contains the tod clock steering parameters.
>
> This allows also to remove ARCH_HAS_VDSO_DATA again.
>
> Heiko Carstens (3):
>   s390/vdso: fix tod clock steering
>   s390/vdso: fix arch_data access for __arch_get_hw_counter()
>   lib/vdso: remove struct arch_vdso_data from vdso data struct
>

Thanks for the quick fix! I confirmed these patches work for me.
(tested with latest mainline kernel v5.12-rc4)

Tested-by: Li Wang <liwang@redhat.com>

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20210325/ef168269/attachment-0001.htm>

Re: [PATCH 0/3] s390 vdso fixes

[Heiko Carstens]

On Thu, Mar 25, 2021 at 04:56:18PM +0800, Li Wang wrote:
> Hi Heiko,
> 
> On Wed, Mar 24, 2021 at 5:58 AM Heiko Carstens <hca@linux.ibm.com> wrote:
> 
> > Li Wang reported that clock_gettime(CLOCK_MONOTONIC_RAW, ...) does not
> > work correctly on s390 via vdso. Debugging this also revealed an
> > unrelated bug (first patch).
> >
> > The second patch fixes the problem: the tod clock steering parameters
> > required by __arch_get_hw_counter() are only present within the first
> > element of the _vdso_data array and not at all within the _timens_data
> > array.
> >
> > Instead of working around this simply provide an s390 specific vdso
> > data page which contains the tod clock steering parameters.
> >
> > This allows also to remove ARCH_HAS_VDSO_DATA again.
> >
> > Heiko Carstens (3):
> >   s390/vdso: fix tod clock steering
> >   s390/vdso: fix arch_data access for __arch_get_hw_counter()
> >   lib/vdso: remove struct arch_vdso_data from vdso data struct
> >
> 
> Thanks for the quick fix! I confirmed these patches work for me.
> (tested with latest mainline kernel v5.12-rc4)
> 
> Tested-by: Li Wang <liwang@redhat.com>

Thanks a lot for confirming! However I decided to go with the simple variant:
https://lore.kernel.org/linux-s390/YFnxr1ZlMIOIqjfq@osiris/T/#m26f94fd8ac048421a4a8870e7259a09f97840a3e

May I add your Tested-by there as well?

[LTP] [PATCH 0/3] s390 vdso fixes

[Heiko Carstens]

On Thu, Mar 25, 2021 at 04:56:18PM +0800, Li Wang wrote:
> Hi Heiko,
> 
> On Wed, Mar 24, 2021 at 5:58 AM Heiko Carstens <hca@linux.ibm.com> wrote:
> 
> > Li Wang reported that clock_gettime(CLOCK_MONOTONIC_RAW, ...) does not
> > work correctly on s390 via vdso. Debugging this also revealed an
> > unrelated bug (first patch).
> >
> > The second patch fixes the problem: the tod clock steering parameters
> > required by __arch_get_hw_counter() are only present within the first
> > element of the _vdso_data array and not at all within the _timens_data
> > array.
> >
> > Instead of working around this simply provide an s390 specific vdso
> > data page which contains the tod clock steering parameters.
> >
> > This allows also to remove ARCH_HAS_VDSO_DATA again.
> >
> > Heiko Carstens (3):
> >   s390/vdso: fix tod clock steering
> >   s390/vdso: fix arch_data access for __arch_get_hw_counter()
> >   lib/vdso: remove struct arch_vdso_data from vdso data struct
> >
> 
> Thanks for the quick fix! I confirmed these patches work for me.
> (tested with latest mainline kernel v5.12-rc4)
> 
> Tested-by: Li Wang <liwang@redhat.com>

Thanks a lot for confirming! However I decided to go with the simple variant:
https://lore.kernel.org/linux-s390/YFnxr1ZlMIOIqjfq@osiris/T/#m26f94fd8ac048421a4a8870e7259a09f97840a3e

May I add your Tested-by there as well?

[LTP] [PATCH 0/3] s390 vdso fixes

[Li Wang]

Heiko Carstens <hca@linux.ibm.com> wrote:


> ...
> > > Heiko Carstens (3):
> > >   s390/vdso: fix tod clock steering
> > >   s390/vdso: fix arch_data access for __arch_get_hw_counter()
> > >   lib/vdso: remove struct arch_vdso_data from vdso data struct
> > >
> >
> > Thanks for the quick fix! I confirmed these patches work for me.
> > (tested with latest mainline kernel v5.12-rc4)
> >
> > Tested-by: Li Wang <liwang@redhat.com>
>
> Thanks a lot for confirming! However I decided to go with the simple
> variant:
>
> https://lore.kernel.org/linux-s390/YFnxr1ZlMIOIqjfq@osiris/T/#m26f94fd8ac048421a4a8870e7259a09f97840a3e
>
> May I add your Tested-by there as well?
>

Sure, I just reverted the 2/3 and 3/3, then apply the simple variant
scratch-patch. It also works well.

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20210325/bf84e964/attachment.htm>