[PATCH 0/4] set_memory_xx fixes

[PATCH 0/4] set_memory_xx fixes

[mika.penttila]

Recent changes (4.4.0+) in module loader triggered oops on ARM.

The module in question is in-tree module :
drivers/misc/ti-st/st_drv.ko

The BUG is here :

[ 53.638335] ------------[ cut here ]------------
[ 53.642967] kernel BUG at mm/memory.c:1878!
[ 53.647153] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
[ 53.652987] Modules linked in:
[ 53.656061] CPU: 0 PID: 483 Comm: insmod Not tainted 4.4.0 #3
[ 53.661808] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
[ 53.668338] task: a989d400 ti: 9e6a2000 task.ti: 9e6a2000
[ 53.673751] PC is at apply_to_page_range+0x204/0x224
[ 53.678723] LR is at change_memory_common+0x90/0xdc
[ 53.683604] pc : [<800ca0ec>] lr : [<8001d668>] psr: 600b0013
[ 53.683604] sp : 9e6a3e38 ip : 8001d6b4 fp : 7f0042fc
[ 53.695082] r10: 00000000 r9 : 9e6a3e90 r8 : 00000080
[ 53.700309] r7 : 00000000 r6 : 7f008000 r5 : 7f008000 r4 : 7f008000
[ 53.706837] r3 : 8001d5a4 r2 : 7f008000 r1 : 7f008000 r0 : 80b8d3c0
[ 53.713368] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user
[ 53.720504] Control: 10c5387d Table: 2e6b804a DAC: 00000055
[ 53.726252] Process insmod (pid: 483, stack limit = 0x9e6a2210)
[ 53.732173] Stack: (0x9e6a3e38 to 0x9e6a4000)
[ 53.736532] 3e20: 7f007fff 7f008000
[ 53.744714] 3e40: 80b8d3c0 80b8d3c0 00000000 7f007000 7f00426c 7f008000 00000000 7f008000
[ 53.752895] 3e60: 7f004140 7f008000 00000000 00000080 00000000 00000000 7f0042fc 8001d668
[ 53.761076] 3e80: 9e6a3e90 00000000 8001d6b4 7f00426c 00000080 00000000 9e6a3f58 7f004140
[ 53.769257] 3ea0: 7f004240 7f00414c 00000000 8008bbe0 00000000 7f000000 00000000 00000000
[ 53.777438] 3ec0: a8b12f00 0001cfd4 7f004250 7f004240 80b8159c 00000000 000000e0 7f0042fc
[ 53.785619] 3ee0: c183d000 000074f8 000018fd 00000000 0b30000c 00000000 00000000 7f002024
[ 53.793800] 3f00: 00000002 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[ 53.801980] 3f20: 00000000 00000000 00000000 00000000 00000040 00000000 00000003 0001cfd4
[ 53.810161] 3f40: 0000017b 8000f7e4 9e6a2000 00000000 00000002 8008c498 c183d000 000074f8
[ 53.818342] 3f60: c1841588 c1841409 c1842950 00005000 000052a0 00000000 00000000 00000000
[ 53.826523] 3f80: 00000023 00000024 0000001a 0000001e 00000016 00000000 00000000 00000000
[ 53.834703] 3fa0: 003e3d60 8000f640 00000000 00000000 00000003 0001cfd4 00000000 003e3d60
[ 53.842884] 3fc0: 00000000 00000000 003e3d60 0000017b 003e3d20 7eabc9d4 76f2c000 00000002
[ 53.851065] 3fe0: 7eabc990 7eabc980 00016320 76e81d00 600b0010 00000003 00000000 00000000
[ 53.859256] [<800ca0ec>] (apply_to_page_range) from [<8001d668>] (change_memory_common+0x90/0xdc)
[ 53.868139] [<8001d668>] (change_memory_common) from [<8008bbe0>] (load_module+0x194c/0x2068)
[ 53.876671] [<8008bbe0>] (load_module) from [<8008c498>] (SyS_finit_module+0x64/0x74)
[ 53.884512] [<8008c498>] (SyS_finit_module) from [<8000f640>] (ret_fast_syscall+0x0/0x34)
[ 53.892694] Code: e0834104 eaffffbc e51a1008 eaffffac (e7f001f2)
[ 53.898792] ---[ end trace fe43fc78ebde29a3 ]---


apply_to_page_range gets zero length resulting in triggering :

  BUG_ON(addr >= end)

This is regression and a consequence of changes in module section handling.

Fix by making arm and arm64 check for zero size update in change_memory_common(),
letting set_memory_xx(addr, 0); succeed. This makes behavior similar to x86.

Also, BUG_ON() in apply_to_page_range is too strong, make it WARN_ON()
and return -EINVAL instead. There may be other caller expecting !size
to succeed.

Patch 1/4 fixes an unrelated but wrong check in the same codepath.

--Mika


[PATCH 1/4] arm: Fix wrong bounds check.
[PATCH 2/4] arm: let set_memory_xx(addr, 0) succeed.
[PATCH 3/4] arm64: let set_memory_xx(addr, 0) succeed.
[PATCH 4/4] make apply_to_page_range() more robust.

 arch/arm/mm/pageattr.c   | 5 ++++-
 arch/arm64/mm/pageattr.c | 3 +++
 mm/memory.c              | 4 +++-
 3 files changed, 10 insertions(+), 2 deletions(-)

[PATCH 1/4] arm: Fix wrong bounds check.

[mika.penttila]

From: Mika Penttilä <mika.penttila@nextfour.com>

Not related to this oops, but while at it, fix incorrect bounds check.

Signed-off-by: Mika Penttilä mika.penttila@nextfour.com

---
 arch/arm/mm/pageattr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mm/pageattr.c b/arch/arm/mm/pageattr.c
index cf30daf..be7fe4b 100644
--- a/arch/arm/mm/pageattr.c
+++ b/arch/arm/mm/pageattr.c
@@ -52,7 +52,7 @@ static int change_memory_common(unsigned long addr, int numpages,
 	if (start < MODULES_VADDR || start >= MODULES_END)
 		return -EINVAL;
 
-	if (end < MODULES_VADDR || start >= MODULES_END)
+	if (end < MODULES_VADDR || end >= MODULES_END)
 		return -EINVAL;
 
 	data.set_mask = set_mask;
-- 
1.9.1

[PATCH 2/4] arm: let set_memory_xx(addr, 0) succeed.

[mika.penttila]

From: Mika Penttilä <mika.penttila@nextfour.com>

This makes set_memory_xx() consistent with x86.

Signed-off-by: Mika Penttilä mika.penttila@nextfour.com

---
 arch/arm/mm/pageattr.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/mm/pageattr.c b/arch/arm/mm/pageattr.c
index be7fe4b..9edf6b0 100644
--- a/arch/arm/mm/pageattr.c
+++ b/arch/arm/mm/pageattr.c
@@ -49,6 +49,9 @@ static int change_memory_common(unsigned long addr, int numpages,
 		WARN_ON_ONCE(1);
 	}
 
+	if (!numpages)
+		return 0;
+
 	if (start < MODULES_VADDR || start >= MODULES_END)
 		return -EINVAL;
 
-- 
1.9.1

[PATCH 3/4] arm64: let set_memory_xx(addr, 0) succeed.

[mika.penttila]

From: Mika Penttilä <mika.penttila@nextfour.com>

This makes set_memory_xx() consistent with x86.

Signed-off-by: Mika Penttilä mika.penttila@nextfour.com

---
 arch/arm64/mm/pageattr.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c
index 3571c73..52220dd 100644
--- a/arch/arm64/mm/pageattr.c
+++ b/arch/arm64/mm/pageattr.c
@@ -51,6 +51,9 @@ static int change_memory_common(unsigned long addr, int numpages,
 		WARN_ON_ONCE(1);
 	}
 
+	if (!numpages)
+		return 0;
+
 	if (start < MODULES_VADDR || start >= MODULES_END)
 		return -EINVAL;
 
-- 
1.9.1

[PATCH 4/4] make apply_to_page_range() more robust.

[mika.penttila]

From: Mika Penttilä <mika.penttila@nextfour.com>


Now the arm/arm64 don't trigger this BUG_ON() any more,
but WARN_ON() is here enough to catch buggy callers
but still let potential other !size callers pass with warning.

Signed-off-by: Mika Penttilä mika.penttila@nextfour.com

---
 mm/memory.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/mm/memory.c b/mm/memory.c
index 30991f8..9178ee6 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1871,7 +1871,9 @@ int apply_to_page_range(struct mm_struct *mm, unsigned long addr,
 	unsigned long end = addr + size;
 	int err;
 
-	BUG_ON(addr >= end);
+	if (WARN_ON(addr >= end))
+		return -EINVAL;
+
 	pgd = pgd_offset(mm, addr);
 	do {
 		next = pgd_addr_end(addr, end);
-- 
1.9.1

Re: [PATCH 1/4] arm: Fix wrong bounds check.

[Laura Abbott]

On 01/23/2016 07:05 AM, mika.penttila@nextfour.com wrote:
> From: Mika Penttilä <mika.penttila@nextfour.com>
>
> Not related to this oops, but while at it, fix incorrect bounds check.
>
> Signed-off-by: Mika Penttilä mika.penttila@nextfour.com
>
> ---
>   arch/arm/mm/pageattr.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/mm/pageattr.c b/arch/arm/mm/pageattr.c
> index cf30daf..be7fe4b 100644
> --- a/arch/arm/mm/pageattr.c
> +++ b/arch/arm/mm/pageattr.c
> @@ -52,7 +52,7 @@ static int change_memory_common(unsigned long addr, int numpages,
>   	if (start < MODULES_VADDR || start >= MODULES_END)
>   		return -EINVAL;
>
> -	if (end < MODULES_VADDR || start >= MODULES_END)
> +	if (end < MODULES_VADDR || end >= MODULES_END)
>   		return -EINVAL;
>
>   	data.set_mask = set_mask;
>

This has been submitted a few times before, not sure if it is pending
in Russell's patch tracker or nobody has actually submitted it to the
patch tracker.

Russell, is this pending somewhere already?

Thanks,
Laura

Re: [PATCH 2/4] arm: let set_memory_xx(addr, 0) succeed.

[Laura Abbott]

On 01/23/2016 07:05 AM, mika.penttila@nextfour.com wrote:
> From: Mika Penttilä <mika.penttila@nextfour.com>
>
> This makes set_memory_xx() consistent with x86.
>
> Signed-off-by: Mika Penttilä mika.penttila@nextfour.com
>
> ---
>   arch/arm/mm/pageattr.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/arch/arm/mm/pageattr.c b/arch/arm/mm/pageattr.c
> index be7fe4b..9edf6b0 100644
> --- a/arch/arm/mm/pageattr.c
> +++ b/arch/arm/mm/pageattr.c
> @@ -49,6 +49,9 @@ static int change_memory_common(unsigned long addr, int numpages,
>   		WARN_ON_ONCE(1);
>   	}
>
> +	if (!numpages)
> +		return 0;
> +
>   	if (start < MODULES_VADDR || start >= MODULES_END)
>   		return -EINVAL;
>
>

Reviewed-by: Laura Abbott <labbott@redhat.com>

Re: [PATCH 3/4] arm64: let set_memory_xx(addr, 0) succeed.

[Laura Abbott]

On 01/23/2016 07:05 AM, mika.penttila@nextfour.com wrote:
> From: Mika Penttilä <mika.penttila@nextfour.com>
>
> This makes set_memory_xx() consistent with x86.
>
> Signed-off-by: Mika Penttilä mika.penttila@nextfour.com
>
> ---
>   arch/arm64/mm/pageattr.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c
> index 3571c73..52220dd 100644
> --- a/arch/arm64/mm/pageattr.c
> +++ b/arch/arm64/mm/pageattr.c
> @@ -51,6 +51,9 @@ static int change_memory_common(unsigned long addr, int numpages,
>   		WARN_ON_ONCE(1);
>   	}
>
> +	if (!numpages)
> +		return 0;
> +
>   	if (start < MODULES_VADDR || start >= MODULES_END)
>   		return -EINVAL;
>
>

I think this is going to conflict with Ard's patch
lkml.kernel.org/g/<1453125665-26627-1-git-send-email-ard.biesheuvel@linaro.org>

Can you rebase on top of that?

Thanks,
Laura

Re: [PATCH 4/4] make apply_to_page_range() more robust.

[David Rientjes]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 400 bytes --]

On Sat, 23 Jan 2016, mika.penttila@nextfour.com wrote:

> From: Mika Penttilä <mika.penttila@nextfour.com>
> 
> 
> Now the arm/arm64 don't trigger this BUG_ON() any more,
> but WARN_ON() is here enough to catch buggy callers
> but still let potential other !size callers pass with warning.
> 
> Signed-off-by: Mika Penttilä mika.penttila@nextfour.com

Acked-by: David Rientjes <rientjes@google.com>

Re: [PATCH 3/4] arm64: let set_memory_xx(addr, 0) succeed.

[David Rientjes]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1343 bytes --]

On Mon, 25 Jan 2016, Laura Abbott wrote:

> On 01/23/2016 07:05 AM, mika.penttila@nextfour.com wrote:
> > From: Mika Penttilä <mika.penttila@nextfour.com>
> > 
> > This makes set_memory_xx() consistent with x86.
> > 
> > Signed-off-by: Mika Penttilä mika.penttila@nextfour.com
> > 
> > ---
> >   arch/arm64/mm/pageattr.c | 3 +++
> >   1 file changed, 3 insertions(+)
> > 
> > diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c
> > index 3571c73..52220dd 100644
> > --- a/arch/arm64/mm/pageattr.c
> > +++ b/arch/arm64/mm/pageattr.c
> > @@ -51,6 +51,9 @@ static int change_memory_common(unsigned long addr, int
> > numpages,
> >   		WARN_ON_ONCE(1);
> >   	}
> > 
> > +	if (!numpages)
> > +		return 0;
> > +
> >   	if (start < MODULES_VADDR || start >= MODULES_END)
> >   		return -EINVAL;
> > 
> > 
> 
> I think this is going to conflict with Ard's patch
> lkml.kernel.org/g/<1453125665-26627-1-git-send-email-ard.biesheuvel@linaro.org>
> 
> Can you rebase on top of that?
> 

Also, I think patch 2 and 3 can be folded together since the change is the 
same to both functions.

I think the changelog should be expanded to explain that 
charge_memory_common() with numpages == 0 should be a no-op.

When both of those are done, and it's rebased as requested, feel free to 
add my:

	Acked-by: David Rientjes <rientjes@google.com>