* Re: [s390x vDSO Bug?] clock_gettime(CLOCK_MONOTONIC_RAW, ...) gets abnormal ts value [not found] <CAEemH2cELFSMzEYM-Gd1LxNuFzVE2PcG1chzyaVhW2YCJjjzdw@mail.gmail.com> @ 2021-03-23 7:11 ` Heiko Carstens 2021-03-23 13:48 ` Heiko Carstens 2021-03-23 21:58 ` [PATCH 0/3] s390 vdso fixes Heiko Carstens 0 siblings, 2 replies; 11+ messages in thread From: Heiko Carstens @ 2021-03-23 7:11 UTC (permalink / raw) To: Li Wang Cc: Alexander Gordeev, Vasily Gorbik, Sven Schnelle, LTP List, linux-kernel, linux-s390, Viresh Kumar 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") ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [s390x vDSO Bug?] clock_gettime(CLOCK_MONOTONIC_RAW, ...) gets abnormal ts value 2021-03-23 7:11 ` [s390x vDSO Bug?] clock_gettime(CLOCK_MONOTONIC_RAW, ...) gets abnormal ts value Heiko Carstens @ 2021-03-23 13:48 ` Heiko Carstens 2021-03-23 21:58 ` [PATCH 0/3] s390 vdso fixes Heiko Carstens 1 sibling, 0 replies; 11+ messages in thread From: Heiko Carstens @ 2021-03-23 13:48 UTC (permalink / raw) To: Heiko Carstens Cc: Li Wang, Alexander Gordeev, Vasily Gorbik, Sven Schnelle, LTP List, linux-kernel, linux-s390, Viresh Kumar 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... :/ ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 0/3] s390 vdso fixes 2021-03-23 7:11 ` [s390x vDSO Bug?] clock_gettime(CLOCK_MONOTONIC_RAW, ...) gets abnormal ts value Heiko Carstens 2021-03-23 13:48 ` Heiko Carstens @ 2021-03-23 21:58 ` Heiko Carstens 2021-03-23 21:58 ` [PATCH 1/3] s390/vdso: fix tod clock steering Heiko Carstens ` (3 more replies) 1 sibling, 4 replies; 11+ messages in thread From: Heiko Carstens @ 2021-03-23 21:58 UTC (permalink / raw) To: Li Wang, Alexander Gordeev, Vasily Gorbik, Sven Schnelle, Christian Borntraeger, Viresh Kumar, Thomas Gleixner Cc: ltp, linux-kernel, linux-s390 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/3] s390/vdso: fix tod clock steering 2021-03-23 21:58 ` [PATCH 0/3] s390 vdso fixes Heiko Carstens @ 2021-03-23 21:58 ` Heiko Carstens 2021-03-24 9:50 ` Heiko Carstens 2021-03-23 21:58 ` [PATCH 2/3] s390/vdso: fix arch_data access for __arch_get_hw_counter() Heiko Carstens ` (2 subsequent siblings) 3 siblings, 1 reply; 11+ messages in thread From: Heiko Carstens @ 2021-03-23 21:58 UTC (permalink / raw) To: Li Wang, Alexander Gordeev, Vasily Gorbik, Sven Schnelle, Christian Borntraeger, Viresh Kumar, Thomas Gleixner Cc: ltp, linux-kernel, linux-s390 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] s390/vdso: fix tod clock steering 2021-03-23 21:58 ` [PATCH 1/3] s390/vdso: fix tod clock steering Heiko Carstens @ 2021-03-24 9:50 ` Heiko Carstens 0 siblings, 0 replies; 11+ messages in thread From: Heiko Carstens @ 2021-03-24 9:50 UTC (permalink / raw) To: Heiko Carstens Cc: Li Wang, Alexander Gordeev, Vasily Gorbik, Sven Schnelle, Christian Borntraeger, Viresh Kumar, Thomas Gleixner, ltp, linux-kernel, linux-s390 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; }; ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/3] s390/vdso: fix arch_data access for __arch_get_hw_counter() 2021-03-23 21:58 ` [PATCH 0/3] s390 vdso fixes Heiko Carstens 2021-03-23 21:58 ` [PATCH 1/3] s390/vdso: fix tod clock steering Heiko Carstens @ 2021-03-23 21:58 ` Heiko Carstens 2021-03-24 5:53 ` Heiko Carstens 2021-03-23 21:58 ` [PATCH 3/3] lib/vdso: remove struct arch_vdso_data from vdso data struct Heiko Carstens [not found] ` <CAEemH2cSk=doHL51uD5Qu6uCRRTCgYd0EN0iij=X+538J53XsQ@mail.gmail.com> 3 siblings, 1 reply; 11+ messages in thread From: Heiko Carstens @ 2021-03-23 21:58 UTC (permalink / raw) To: Li Wang, Alexander Gordeev, Vasily Gorbik, Sven Schnelle, Christian Borntraeger, Viresh Kumar, Thomas Gleixner Cc: ltp, linux-kernel, linux-s390 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] s390/vdso: fix arch_data access for __arch_get_hw_counter() 2021-03-23 21:58 ` [PATCH 2/3] s390/vdso: fix arch_data access for __arch_get_hw_counter() Heiko Carstens @ 2021-03-24 5:53 ` Heiko Carstens 0 siblings, 0 replies; 11+ messages in thread From: Heiko Carstens @ 2021-03-24 5:53 UTC (permalink / raw) To: Heiko Carstens Cc: Li Wang, Alexander Gordeev, Vasily Gorbik, Sven Schnelle, Christian Borntraeger, Viresh Kumar, Thomas Gleixner, ltp, linux-kernel, linux-s390 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) ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/3] lib/vdso: remove struct arch_vdso_data from vdso data struct 2021-03-23 21:58 ` [PATCH 0/3] s390 vdso fixes Heiko Carstens 2021-03-23 21:58 ` [PATCH 1/3] s390/vdso: fix tod clock steering Heiko Carstens 2021-03-23 21:58 ` [PATCH 2/3] s390/vdso: fix arch_data access for __arch_get_hw_counter() Heiko Carstens @ 2021-03-23 21:58 ` Heiko Carstens 2021-03-25 17:55 ` Thomas Gleixner [not found] ` <CAEemH2cSk=doHL51uD5Qu6uCRRTCgYd0EN0iij=X+538J53XsQ@mail.gmail.com> 3 siblings, 1 reply; 11+ messages in thread From: Heiko Carstens @ 2021-03-23 21:58 UTC (permalink / raw) To: Li Wang, Alexander Gordeev, Vasily Gorbik, Sven Schnelle, Christian Borntraeger, Viresh Kumar, Thomas Gleixner Cc: ltp, linux-kernel, linux-s390 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] lib/vdso: remove struct arch_vdso_data from vdso data struct 2021-03-23 21:58 ` [PATCH 3/3] lib/vdso: remove struct arch_vdso_data from vdso data struct Heiko Carstens @ 2021-03-25 17:55 ` Thomas Gleixner 2021-03-25 17:57 ` Thomas Gleixner 0 siblings, 1 reply; 11+ messages in thread From: Thomas Gleixner @ 2021-03-25 17:55 UTC (permalink / raw) To: Heiko Carstens, Li Wang, Alexander Gordeev, Vasily Gorbik, Sven Schnelle, Christian Borntraeger, Viresh Kumar Cc: ltp, linux-kernel, linux-s390 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> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] lib/vdso: remove struct arch_vdso_data from vdso data struct 2021-03-25 17:55 ` Thomas Gleixner @ 2021-03-25 17:57 ` Thomas Gleixner 0 siblings, 0 replies; 11+ messages in thread From: Thomas Gleixner @ 2021-03-25 17:57 UTC (permalink / raw) To: Heiko Carstens, Li Wang, Alexander Gordeev, Vasily Gorbik, Sven Schnelle, Christian Borntraeger, Viresh Kumar Cc: ltp, linux-kernel, linux-s390 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <CAEemH2cSk=doHL51uD5Qu6uCRRTCgYd0EN0iij=X+538J53XsQ@mail.gmail.com>]
* Re: [PATCH 0/3] s390 vdso fixes [not found] ` <CAEemH2cSk=doHL51uD5Qu6uCRRTCgYd0EN0iij=X+538J53XsQ@mail.gmail.com> @ 2021-03-25 12:33 ` Heiko Carstens 0 siblings, 0 replies; 11+ messages in thread From: Heiko Carstens @ 2021-03-25 12:33 UTC (permalink / raw) To: Li Wang Cc: Alexander Gordeev, Vasily Gorbik, Sven Schnelle, Christian Borntraeger, Viresh Kumar, Thomas Gleixner, LTP List, linux-kernel, linux-s390 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? ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-03-25 17:58 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CAEemH2cELFSMzEYM-Gd1LxNuFzVE2PcG1chzyaVhW2YCJjjzdw@mail.gmail.com>
2021-03-23 7:11 ` [s390x vDSO Bug?] clock_gettime(CLOCK_MONOTONIC_RAW, ...) gets abnormal ts value Heiko Carstens
2021-03-23 13:48 ` Heiko Carstens
2021-03-23 21:58 ` [PATCH 0/3] s390 vdso fixes Heiko Carstens
2021-03-23 21:58 ` [PATCH 1/3] s390/vdso: fix tod clock steering Heiko Carstens
2021-03-24 9:50 ` Heiko Carstens
2021-03-23 21:58 ` [PATCH 2/3] s390/vdso: fix arch_data access for __arch_get_hw_counter() Heiko Carstens
2021-03-24 5:53 ` Heiko Carstens
2021-03-23 21:58 ` [PATCH 3/3] lib/vdso: remove struct arch_vdso_data from vdso data struct Heiko Carstens
2021-03-25 17:55 ` Thomas Gleixner
2021-03-25 17:57 ` Thomas Gleixner
[not found] ` <CAEemH2cSk=doHL51uD5Qu6uCRRTCgYd0EN0iij=X+538J53XsQ@mail.gmail.com>
2021-03-25 12:33 ` [PATCH 0/3] s390 vdso fixes Heiko Carstens
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).