[PATCH v2 RESEND 0/2] set_memory_xx fixes

[PATCH v2 RESEND 0/2] set_memory_xx fixes

[mika.penttila]

Resend to CC arm64 maintainers also.. I kept patch 1/2 as one because
it touches the same function in both arches...

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.

v2:
  - drop patch 1/4 for the bounds check, it has been submitted before
  - merge arm/arm64 changes into one patch

--Mika


[PATCH 1/2] arm, arm64: change_memory_common with numpages == 0 should be no-op.
[PATCH 2/2] make apply_to_page_range() more robust.

 arch/arm/mm/pageattr.c   | 3 +++
 arch/arm64/mm/pageattr.c | 3 +++
 mm/memory.c              | 4 +++-
 3 files changed, 9 insertions(+), 1 deletion(-)

[PATCH v2 RESEND 1/2] arm, arm64: change_memory_common with numpages == 0 should be no-op.

[mika.penttila]

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

This makes the caller set_memory_xx() consistent with x86.

arm64 part is rebased on 4.5.0-rc1 with Ard's patch
 lkml.kernel.org/g/<1453125665-26627-1-git-send-email-ard.biesheuvel@linaro.org>
applied.

Signed-off-by: Mika Penttilä mika.penttila@nextfour.com
Reviewed-by: Laura Abbott <labbott@redhat.com>
Acked-by: David Rientjes <rientjes@google.com>

---
 arch/arm/mm/pageattr.c   | 3 +++
 arch/arm64/mm/pageattr.c | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/arch/arm/mm/pageattr.c b/arch/arm/mm/pageattr.c
index cf30daf..d19b1ad 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;
 
diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c
index 1360a02..b582fc2 100644
--- a/arch/arm64/mm/pageattr.c
+++ b/arch/arm64/mm/pageattr.c
@@ -53,6 +53,9 @@ static int change_memory_common(unsigned long addr, int numpages,
 		WARN_ON_ONCE(1);
 	}
 
+	if (!numpages)
+		return 0;
+
 	/*
 	 * Kernel VA mappings are always live, and splitting live section
 	 * mappings into page mappings may cause TLB conflicts. This means
-- 
1.9.1

[PATCH V2 RESEND 2/2] 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
Reviewed-by: Pekka Enberg <penberg@kernel.org>
Acked-by: David Rientjes <rientjes@google.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 v2 RESEND 1/2] arm, arm64: change_memory_common with numpages == 0 should be no-op.

[Will Deacon]

Hi Mika,

On Tue, Jan 26, 2016 at 04:59:52PM +0200, mika.penttila@nextfour.com wrote:
> From: Mika Penttilä <mika.penttila@nextfour.com>
> 
> This makes the caller set_memory_xx() consistent with x86.
> 
> arm64 part is rebased on 4.5.0-rc1 with Ard's patch
>  lkml.kernel.org/g/<1453125665-26627-1-git-send-email-ard.biesheuvel@linaro.org>
> applied.
> 
> Signed-off-by: Mika Penttilä mika.penttila@nextfour.com
> Reviewed-by: Laura Abbott <labbott@redhat.com>
> Acked-by: David Rientjes <rientjes@google.com>
> 
> ---
>  arch/arm/mm/pageattr.c   | 3 +++
>  arch/arm64/mm/pageattr.c | 3 +++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/arch/arm/mm/pageattr.c b/arch/arm/mm/pageattr.c
> index cf30daf..d19b1ad 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;
>  
> diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c
> index 1360a02..b582fc2 100644
> --- a/arch/arm64/mm/pageattr.c
> +++ b/arch/arm64/mm/pageattr.c
> @@ -53,6 +53,9 @@ static int change_memory_common(unsigned long addr, int numpages,
>  		WARN_ON_ONCE(1);
>  	}
>  
> +	if (!numpages)
> +		return 0;
> +

Thanks for this. I can reproduce the failure on my Juno board, so I'd
like to queue this for 4.5 since it fixes a real issue. I've taken the
liberty of rebasing the arm64 part to my fixes branch and writing a
commit message. Does the patch below look ok to you?

Will

--->8

>From 57adec866c0440976c96a4b8f5b59fb411b1cacb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mika=20Penttil=C3=A4?= <mika.penttila@nextfour.com>
Date: Tue, 26 Jan 2016 15:47:25 +0000
Subject: [PATCH] arm64: mm: avoid calling apply_to_page_range on empty range
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Calling apply_to_page_range with an empty range results in a BUG_ON
from the core code. This can be triggered by trying to load the st_drv
module with CONFIG_DEBUG_SET_MODULE_RONX enabled:

  kernel BUG at mm/memory.c:1874!
  Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
  Modules linked in:
  CPU: 3 PID: 1764 Comm: insmod Not tainted 4.5.0-rc1+ #2
  Hardware name: ARM Juno development board (r0) (DT)
  task: ffffffc9763b8000 ti: ffffffc975af8000 task.ti: ffffffc975af8000
  PC is at apply_to_page_range+0x2cc/0x2d0
  LR is at change_memory_common+0x80/0x108

This patch fixes the issue by making change_memory_common (called by the
set_memory_* functions) a NOP when numpages == 0, therefore avoiding the
erroneous call to apply_to_page_range and bringing us into line with x86
and s390.

Cc: <stable@vger.kernel.org>
Reviewed-by: Laura Abbott <labbott@redhat.com>
Acked-by: David Rientjes <rientjes@google.com>
Signed-off-by: Mika Penttilä <mika.penttila@nextfour.com>
Signed-off-by: Will Deacon <will.deacon@arm.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 3571c7309c5e..cf6240741134 100644
--- a/arch/arm64/mm/pageattr.c
+++ b/arch/arm64/mm/pageattr.c
@@ -57,6 +57,9 @@ static int change_memory_common(unsigned long addr, int numpages,
 	if (end < MODULES_VADDR || end >= MODULES_END)
 		return -EINVAL;
 
+	if (!numpages)
+		return 0;
+
 	data.set_mask = set_mask;
 	data.clear_mask = clear_mask;
 
-- 
2.1.4

Re: [PATCH v2 RESEND 1/2] arm, arm64: change_memory_common with numpages == 0 should be no-op.

[David Rientjes]

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

On Tue, 26 Jan 2016, Will Deacon wrote:

> From 57adec866c0440976c96a4b8f5b59fb411b1cacb Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Mika=20Penttil=C3=A4?= <mika.penttila@nextfour.com>
> Date: Tue, 26 Jan 2016 15:47:25 +0000
> Subject: [PATCH] arm64: mm: avoid calling apply_to_page_range on empty range
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> Calling apply_to_page_range with an empty range results in a BUG_ON
> from the core code. This can be triggered by trying to load the st_drv
> module with CONFIG_DEBUG_SET_MODULE_RONX enabled:
> 
>   kernel BUG at mm/memory.c:1874!
>   Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
>   Modules linked in:
>   CPU: 3 PID: 1764 Comm: insmod Not tainted 4.5.0-rc1+ #2
>   Hardware name: ARM Juno development board (r0) (DT)
>   task: ffffffc9763b8000 ti: ffffffc975af8000 task.ti: ffffffc975af8000
>   PC is at apply_to_page_range+0x2cc/0x2d0
>   LR is at change_memory_common+0x80/0x108
> 
> This patch fixes the issue by making change_memory_common (called by the
> set_memory_* functions) a NOP when numpages == 0, therefore avoiding the
> erroneous call to apply_to_page_range and bringing us into line with x86
> and s390.
> 
> Cc: <stable@vger.kernel.org>
> Reviewed-by: Laura Abbott <labbott@redhat.com>
> Acked-by: David Rientjes <rientjes@google.com>
> Signed-off-by: Mika Penttilä <mika.penttila@nextfour.com>
> Signed-off-by: Will Deacon <will.deacon@arm.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 3571c7309c5e..cf6240741134 100644
> --- a/arch/arm64/mm/pageattr.c
> +++ b/arch/arm64/mm/pageattr.c
> @@ -57,6 +57,9 @@ static int change_memory_common(unsigned long addr, int numpages,
>  	if (end < MODULES_VADDR || end >= MODULES_END)
>  		return -EINVAL;
>  
> +	if (!numpages)
> +		return 0;
> +
>  	data.set_mask = set_mask;
>  	data.clear_mask = clear_mask;
>  

LGTM, I think this issue goes back to 3.17 due to commit 11d91a770f1f 
("arm64: Add CONFIG_DEBUG_SET_MODULE_RONX support") so perhaps annotate 
the stable@vger.kernel.org for 3.17+.

Re: [PATCH v2 RESEND 1/2] arm, arm64: change_memory_common with numpages == 0 should be no-op.

[Mika Penttilä]

Hi Will,

On 26.01.2016 17:59, Will Deacon wrote:
> Hi Mika,
>
> On Tue, Jan 26, 2016 at 04:59:52PM +0200, mika.penttila@nextfour.com wrote:
>> From: Mika Penttilä <mika.penttila@nextfour.com>
>>
>> This makes the caller set_memory_xx() consistent with x86.
>>
>> arm64 part is rebased on 4.5.0-rc1 with Ard's patch
>>  lkml.kernel.org/g/<1453125665-26627-1-git-send-email-ard.biesheuvel@linaro.org>
>> applied.
>>
>> Signed-off-by: Mika Penttilä mika.penttila@nextfour.com
>> Reviewed-by: Laura Abbott <labbott@redhat.com>
>> Acked-by: David Rientjes <rientjes@google.com>
>>
>> ---
>>  arch/arm/mm/pageattr.c   | 3 +++
>>  arch/arm64/mm/pageattr.c | 3 +++
>>  2 files changed, 6 insertions(+)
>>
>> diff --git a/arch/arm/mm/pageattr.c b/arch/arm/mm/pageattr.c
>> index cf30daf..d19b1ad 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;
>>  
>> diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c
>> index 1360a02..b582fc2 100644
>> --- a/arch/arm64/mm/pageattr.c
>> +++ b/arch/arm64/mm/pageattr.c
>> @@ -53,6 +53,9 @@ static int change_memory_common(unsigned long addr, int numpages,
>>  		WARN_ON_ONCE(1);
>>  	}
>>  
>> +	if (!numpages)
>> +		return 0;
>> +
> Thanks for this. I can reproduce the failure on my Juno board, so I'd
> like to queue this for 4.5 since it fixes a real issue. I've taken the
> liberty of rebasing the arm64 part to my fixes branch and writing a
> commit message. Does the patch below look ok to you?
>
> Will
>
> --->8
>
> From 57adec866c0440976c96a4b8f5b59fb411b1cacb Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Mika=20Penttil=C3=A4?= <mika.penttila@nextfour.com>
> Date: Tue, 26 Jan 2016 15:47:25 +0000
> Subject: [PATCH] arm64: mm: avoid calling apply_to_page_range on empty range
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> Calling apply_to_page_range with an empty range results in a BUG_ON
> from the core code. This can be triggered by trying to load the st_drv
> module with CONFIG_DEBUG_SET_MODULE_RONX enabled:
>
>   kernel BUG at mm/memory.c:1874!
>   Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
>   Modules linked in:
>   CPU: 3 PID: 1764 Comm: insmod Not tainted 4.5.0-rc1+ #2
>   Hardware name: ARM Juno development board (r0) (DT)
>   task: ffffffc9763b8000 ti: ffffffc975af8000 task.ti: ffffffc975af8000
>   PC is at apply_to_page_range+0x2cc/0x2d0
>   LR is at change_memory_common+0x80/0x108
>
> This patch fixes the issue by making change_memory_common (called by the
> set_memory_* functions) a NOP when numpages == 0, therefore avoiding the
> erroneous call to apply_to_page_range and bringing us into line with x86
> and s390.
>
> Cc: <stable@vger.kernel.org>
> Reviewed-by: Laura Abbott <labbott@redhat.com>
> Acked-by: David Rientjes <rientjes@google.com>
> Signed-off-by: Mika Penttilä <mika.penttila@nextfour.com>
> Signed-off-by: Will Deacon <will.deacon@arm.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 3571c7309c5e..cf6240741134 100644
> --- a/arch/arm64/mm/pageattr.c
> +++ b/arch/arm64/mm/pageattr.c
> @@ -57,6 +57,9 @@ static int change_memory_common(unsigned long addr, int numpages,
>  	if (end < MODULES_VADDR || end >= MODULES_END)
>  		return -EINVAL;
>  
> +	if (!numpages)
> +		return 0;
> +
>  	data.set_mask = set_mask;
>  	data.clear_mask = clear_mask;
>  

Yes I'm fine with that,
Thanks!
Mika