diff mbox

[LEDE-DEV] kernel: fix VDSO problem on MIPS

Message ID 20170810221427.23260-1-hauke@hauke-m.de
State Changes Requested
Delegated to: John Crispin
Headers show

Commit Message

Hauke Mehrtens Aug. 10, 2017, 10:14 p.m. UTC 
This fixes the VDSO problems on the Lantiq VR9 MIPS SoC.
The gettimeofday() sometimes returned old data because of a cache
aliasing problems on MIPS CPUs, to work around this problem VDSO
gettimeofday support was deactivated for MIPS in LEDE.
This problem made ping show very high times like a delay of 4289967.657
ms.

1.000.000 calls to clock_gettime(CLOCK_MONOTONIC, &tp); take 1 second
without VDSO support and 0.35 seconds with VDSO support on Lantiq VR9.

Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
---
 ...MIPS-work-around-aliasing-issue-with-VDSO.patch | 63 ++++++++++++++++++++++
 .../pending-4.4/206-mips-disable-vdso.patch        | 21 --------
 ...MIPS-work-around-aliasing-issue-with-VDSO.patch | 63 ++++++++++++++++++++++
 .../pending-4.9/206-mips-disable-vdso.patch        | 28 ----------
 4 files changed, 126 insertions(+), 49 deletions(-)
 create mode 100644 target/linux/generic/pending-4.4/206-MIPS-work-around-aliasing-issue-with-VDSO.patch
 delete mode 100644 target/linux/generic/pending-4.4/206-mips-disable-vdso.patch
 create mode 100644 target/linux/generic/pending-4.9/206-MIPS-work-around-aliasing-issue-with-VDSO.patch
 delete mode 100644 target/linux/generic/pending-4.9/206-mips-disable-vdso.patch

Comments

Rafał Miłecki Aug. 28, 2017, 12:08 p.m. UTC | #1 
On 11 August 2017 at 00:14, Hauke Mehrtens <hauke@hauke-m.de> wrote:
> This fixes the VDSO problems on the Lantiq VR9 MIPS SoC.
> The gettimeofday() sometimes returned old data because of a cache
> aliasing problems on MIPS CPUs, to work around this problem VDSO
> gettimeofday support was deactivated for MIPS in LEDE.
> This problem made ping show very high times like a delay of 4289967.657
> ms.
>
> 1.000.000 calls to clock_gettime(CLOCK_MONOTONIC, &tp); take 1 second
> without VDSO support and 0.35 seconds with VDSO support on Lantiq VR9.

Any comments on this? It sounds exciting to have this fixed upstream.

It seems the old patch was added in commit 1185e645a773c ("kernel:
disable MIPS VDSO by default until the cache issues have been
resolved").

Felix?
Hauke Mehrtens Aug. 28, 2017, 1:13 p.m. UTC | #2 
On 08/28/2017 02:08 PM, Rafał Miłecki wrote:
> On 11 August 2017 at 00:14, Hauke Mehrtens <hauke@hauke-m.de> wrote:
>> This fixes the VDSO problems on the Lantiq VR9 MIPS SoC.
>> The gettimeofday() sometimes returned old data because of a cache
>> aliasing problems on MIPS CPUs, to work around this problem VDSO
>> gettimeofday support was deactivated for MIPS in LEDE.
>> This problem made ping show very high times like a delay of 4289967.657
>> ms.
>>
>> 1.000.000 calls to clock_gettime(CLOCK_MONOTONIC, &tp); take 1 second
>> without VDSO support and 0.35 seconds with VDSO support on Lantiq VR9.
> 
> Any comments on this? It sounds exciting to have this fixed upstream.
> 
> It seems the old patch was added in commit 1185e645a773c ("kernel:
> disable MIPS VDSO by default until the cache issues have been
> resolved").
> 
> Felix?

Hi,

For me this looks more like a hack which James suggested here:
https://www.linux-mips.org/archives/linux-mips/2017-06/msg00659.html
But I am unable to improve this, this needs too much low level MIPS
knowledge.

This fixes the problem on the Lantiq SoC, I used a version where I could
reproduce this problem, added the patch and then it was gone.

Hauke
diff mbox

Patch

diff --git a/target/linux/generic/pending-4.4/206-MIPS-work-around-aliasing-issue-with-VDSO.patch b/target/linux/generic/pending-4.4/206-MIPS-work-around-aliasing-issue-with-VDSO.patch
new file mode 100644
index 0000000000..b62557b7b5
--- /dev/null
+++ b/target/linux/generic/pending-4.4/206-MIPS-work-around-aliasing-issue-with-VDSO.patch
@@ -0,0 +1,63 @@ 
+From a5513a09cbb946e9d031fb65d6c2ac7c32c9042c Mon Sep 17 00:00:00 2001
+From: James Hogan <james.hogan@imgtec.com>
+Date: Thu, 10 Aug 2017 22:56:47 +0200
+Subject: MIPS: work around aliasing issue with VDSO
+
+I notice that the mips_vdso_data is updated by update_vsyscall() via
+kseg0, however userland will be accessing it via the mapping 1 page
+below the VDSO.
+
+If the kernel data happened to be placed such that the mips_vdso_data in
+kseg0 and the user mapping had different page colours then you could
+easily hit aliasing issues. A couple of well placed flushes or some more
+careful placement of the VDSO data might well fix it, as could some
+random patch changing the positioning of the data such that it
+coincidentally lined up on the same colour.
+
+The patch unfortunately hacks arch_get_unmapped_area_common so that it
+does the colour alignment on non-shared anonymous pages, as long as
+non-zero pgoff is provided. Hopefully no userland code would try
+mmap'ing anonymous memory with a file offset, and so it should be
+harmless.
+
+It doesn't look like we can just pass MAP_SHARED to avoid the hack as
+then pgoff will get cleared by get_unmapped_area()).
+---
+ arch/mips/kernel/vdso.c | 7 ++++++-
+ arch/mips/mm/mmap.c     | 2 +-
+ 2 files changed, 7 insertions(+), 2 deletions(-)
+
+diff --git a/arch/mips/kernel/vdso.c b/arch/mips/kernel/vdso.c
+index f9dbfb14af33..9509b5122e70 100644
+--- a/arch/mips/kernel/vdso.c
++++ b/arch/mips/kernel/vdso.c
+@@ -129,7 +129,12 @@ int arch_setup_additional_pages(struct linux_binprm *bprm, int uses_interp)
+ 	vvar_size = gic_size + PAGE_SIZE;
+ 	size = vvar_size + image->size;
+ 
+-	base = get_unmapped_area(NULL, 0, size, 0, 0);
++	/*
++	 * Hack to get the user mapping of the VDSO data page matching the cache
++	 * colour of its kseg0 address.
++	 */
++	base = get_unmapped_area(NULL, 0, size,
++			(virt_to_phys(&vdso_data) - gic_size) >> PAGE_SHIFT, 0);
+ 	if (IS_ERR_VALUE(base)) {
+ 		ret = base;
+ 		goto out;
+diff --git a/arch/mips/mm/mmap.c b/arch/mips/mm/mmap.c
+index d08ea3ff0f53..cb468b0ea45c 100644
+--- a/arch/mips/mm/mmap.c
++++ b/arch/mips/mm/mmap.c
+@@ -80,7 +80,7 @@ static unsigned long arch_get_unmapped_area_common(struct file *filp,
+ 	}
+ 
+ 	do_color_align = 0;
+-	if (filp || (flags & MAP_SHARED))
++	if (filp || (flags & MAP_SHARED) || pgoff)
+ 		do_color_align = 1;
+ 
+ 	/* requesting a specific address */
+-- 
+2.11.0
+
diff --git a/target/linux/generic/pending-4.4/206-mips-disable-vdso.patch b/target/linux/generic/pending-4.4/206-mips-disable-vdso.patch
deleted file mode 100644
index 51b1d041bf..0000000000
--- a/target/linux/generic/pending-4.4/206-mips-disable-vdso.patch
+++ /dev/null
@@ -1,21 +0,0 @@ 
-Disable MIPS VDSO until the cache issues have been sorted out.
-
-Signed-off-by: Felix Fietkau <nbd@nbd.name>
-
---- a/arch/mips/vdso/Makefile
-+++ b/arch/mips/vdso/Makefile
-@@ -28,11 +28,11 @@ aflags-vdso := $(ccflags-vdso) \
- # the comments on that file.
- #
- ifndef CONFIG_CPU_MIPSR6
--  ifeq ($(call ld-ifversion, -lt, 22500000, y),y)
--    $(warning MIPS VDSO requires binutils >= 2.25)
-+#  ifeq ($(call ld-ifversion, -lt, 22500000, y),y)
-+#    $(warning MIPS VDSO requires binutils >= 2.25)
-     obj-vdso-y := $(filter-out gettimeofday.o, $(obj-vdso-y))
-     ccflags-vdso += -DDISABLE_MIPS_VDSO
--  endif
-+#  endif
- endif
- 
- # VDSO linker flags.
diff --git a/target/linux/generic/pending-4.9/206-MIPS-work-around-aliasing-issue-with-VDSO.patch b/target/linux/generic/pending-4.9/206-MIPS-work-around-aliasing-issue-with-VDSO.patch
new file mode 100644
index 0000000000..b62557b7b5
--- /dev/null
+++ b/target/linux/generic/pending-4.9/206-MIPS-work-around-aliasing-issue-with-VDSO.patch
@@ -0,0 +1,63 @@ 
+From a5513a09cbb946e9d031fb65d6c2ac7c32c9042c Mon Sep 17 00:00:00 2001
+From: James Hogan <james.hogan@imgtec.com>
+Date: Thu, 10 Aug 2017 22:56:47 +0200
+Subject: MIPS: work around aliasing issue with VDSO
+
+I notice that the mips_vdso_data is updated by update_vsyscall() via
+kseg0, however userland will be accessing it via the mapping 1 page
+below the VDSO.
+
+If the kernel data happened to be placed such that the mips_vdso_data in
+kseg0 and the user mapping had different page colours then you could
+easily hit aliasing issues. A couple of well placed flushes or some more
+careful placement of the VDSO data might well fix it, as could some
+random patch changing the positioning of the data such that it
+coincidentally lined up on the same colour.
+
+The patch unfortunately hacks arch_get_unmapped_area_common so that it
+does the colour alignment on non-shared anonymous pages, as long as
+non-zero pgoff is provided. Hopefully no userland code would try
+mmap'ing anonymous memory with a file offset, and so it should be
+harmless.
+
+It doesn't look like we can just pass MAP_SHARED to avoid the hack as
+then pgoff will get cleared by get_unmapped_area()).
+---
+ arch/mips/kernel/vdso.c | 7 ++++++-
+ arch/mips/mm/mmap.c     | 2 +-
+ 2 files changed, 7 insertions(+), 2 deletions(-)
+
+diff --git a/arch/mips/kernel/vdso.c b/arch/mips/kernel/vdso.c
+index f9dbfb14af33..9509b5122e70 100644
+--- a/arch/mips/kernel/vdso.c
++++ b/arch/mips/kernel/vdso.c
+@@ -129,7 +129,12 @@ int arch_setup_additional_pages(struct linux_binprm *bprm, int uses_interp)
+ 	vvar_size = gic_size + PAGE_SIZE;
+ 	size = vvar_size + image->size;
+ 
+-	base = get_unmapped_area(NULL, 0, size, 0, 0);
++	/*
++	 * Hack to get the user mapping of the VDSO data page matching the cache
++	 * colour of its kseg0 address.
++	 */
++	base = get_unmapped_area(NULL, 0, size,
++			(virt_to_phys(&vdso_data) - gic_size) >> PAGE_SHIFT, 0);
+ 	if (IS_ERR_VALUE(base)) {
+ 		ret = base;
+ 		goto out;
+diff --git a/arch/mips/mm/mmap.c b/arch/mips/mm/mmap.c
+index d08ea3ff0f53..cb468b0ea45c 100644
+--- a/arch/mips/mm/mmap.c
++++ b/arch/mips/mm/mmap.c
+@@ -80,7 +80,7 @@ static unsigned long arch_get_unmapped_area_common(struct file *filp,
+ 	}
+ 
+ 	do_color_align = 0;
+-	if (filp || (flags & MAP_SHARED))
++	if (filp || (flags & MAP_SHARED) || pgoff)
+ 		do_color_align = 1;
+ 
+ 	/* requesting a specific address */
+-- 
+2.11.0
+
diff --git a/target/linux/generic/pending-4.9/206-mips-disable-vdso.patch b/target/linux/generic/pending-4.9/206-mips-disable-vdso.patch
deleted file mode 100644
index 9785f932e7..0000000000
--- a/target/linux/generic/pending-4.9/206-mips-disable-vdso.patch
+++ /dev/null
@@ -1,28 +0,0 @@ 
-From: Felix Fietkau <nbd@nbd.name>
-Subject: kernel: disable MIPS VDSO by default until the cache issues have been resolved
-
-lede-commit: 1185e645a773c86aa88cf04d0e2911dc62eb43f5
-Signed-off-by: Felix Fietkau <nbd@nbd.name>
----
- arch/mips/vdso/Makefile | 4 ++--
- 1 file changed, 2 insertions(+), 2 deletions(-)
-
-diff --git a/arch/mips/vdso/Makefile b/arch/mips/vdso/Makefile
-index c3dc12a8b7d9..28f66e3bb2c3 100644
---- a/arch/mips/vdso/Makefile
-+++ b/arch/mips/vdso/Makefile
-@@ -28,9 +28,9 @@ aflags-vdso := $(ccflags-vdso) \
- ifndef CONFIG_CPU_MIPSR6
-   ifeq ($(call ld-ifversion, -lt, 225000000, y),y)
-     $(warning MIPS VDSO requires binutils >= 2.25)
--    obj-vdso-y := $(filter-out gettimeofday.o, $(obj-vdso-y))
--    ccflags-vdso += -DDISABLE_MIPS_VDSO
-   endif
-+  obj-vdso-y := $(filter-out gettimeofday.o, $(obj-vdso-y))
-+  ccflags-vdso += -DDISABLE_MIPS_VDSO
- endif
- 
- # VDSO linker flags.
--- 
-2.11.0
-