* Re: [PATCH] mm, vmalloc: fix vmalloc users tracking properly
From: Geert Uytterhoeven @ 2017-05-10 9:37 UTC (permalink / raw)
To: Michal Hocko
Cc: Linus Torvalds, Andrew Morton, Tobias Klauser, Linux MM, LKML
In-Reply-To: <20170509153702.GR6481@dhcp22.suse.cz>
On Tue, May 9, 2017 at 5:37 PM, Michal Hocko <mhocko@kernel.org> wrote:
> Sigh. I've apparently managed to screw up again. This should address the
> nommu breakage reported by 0-day.
> ---
> From 95d49bf93ae4467f3f918520ec03b3596e5b36cc Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.com>
> Date: Tue, 9 May 2017 16:27:39 +0200
> Subject: [PATCH] mm, vmalloc: fix vmalloc users tracking properly
>
> 1f5307b1e094 ("mm, vmalloc: properly track vmalloc users") has pulled
> asm/pgtable.h include dependency to linux/vmalloc.h and that turned out
> to be a bad idea for some architectures. E.g. m68k fails with
> In file included from arch/m68k/include/asm/pgtable_mm.h:145:0,
> from arch/m68k/include/asm/pgtable.h:4,
> from include/linux/vmalloc.h:9,
> from arch/m68k/kernel/module.c:9:
> arch/m68k/include/asm/mcf_pgtable.h: In function 'nocache_page':
>>> arch/m68k/include/asm/mcf_pgtable.h:339:43: error: 'init_mm' undeclared (first use in this function)
> #define pgd_offset_k(address) pgd_offset(&init_mm, address)
>
> as spotted by kernel build bot. nios2 fails for other reason
> In file included from ./include/asm-generic/io.h:767:0,
> from ./arch/nios2/include/asm/io.h:61,
> from ./include/linux/io.h:25,
> from ./arch/nios2/include/asm/pgtable.h:18,
> from ./include/linux/mm.h:70,
> from ./include/linux/pid_namespace.h:6,
> from ./include/linux/ptrace.h:9,
> from ./arch/nios2/include/uapi/asm/elf.h:23,
> from ./arch/nios2/include/asm/elf.h:22,
> from ./include/linux/elf.h:4,
> from ./include/linux/module.h:15,
> from init/main.c:16:
> ./include/linux/vmalloc.h: In function '__vmalloc_node_flags':
> ./include/linux/vmalloc.h:99:40: error: 'PAGE_KERNEL' undeclared (first use in this function); did you mean 'GFP_KERNEL'?
>
> which is due to the newly added #include <asm/pgtable.h>, which on nios2
> includes <linux/io.h> and thus <asm/io.h> and <asm-generic/io.h> which
> again includes <linux/vmalloc.h>.
>
> Tweaking that around just turns out a bigger headache than
> necessary. This patch reverts 1f5307b1e094 and reimplements the original
> fix in a different way. __vmalloc_node_flags can stay static inline
> which will cover vmalloc* functions. We only have one external user
> (kvmalloc_node) and we can export __vmalloc_node_flags_caller and
> provide the caller directly. This is much simpler and it doesn't really
> need any games with header files.
>
> Fixes: 1f5307b1e094 ("mm, vmalloc: properly track vmalloc users")
> Signed-off-by: Michal Hocko <mhocko@suse.com>
Tested-by: Geert Uytterhoeven <geert@linux-m68k.org>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH] mm/vmscan: fix unsequenced modification and access warning
From: Michal Hocko @ 2017-05-10 9:25 UTC (permalink / raw)
To: Nick Desaulniers
Cc: akpm, hannes, mgorman, vbabka, minchan, linux-mm, linux-kernel
In-Reply-To: <20170510084602.qchu4psnughxrmsz@lostoracle.net>
On Wed 10-05-17 01:46:03, Nick Desaulniers wrote:
> > You can add
>
> Something that's not clear to me when advised to add, should I be
> uploading a v3 with your acked by? I think I got that wrong the last
> time I asked (which was my first patch to Linux).
If there are no further changes to the patch/changelog then it is not
necessary. The maintainer usually just grabs ackes and reviewed-bys
from the list.
> > But I still do not understand which part of the code is undefined and
> > why.
>
> It's not immediately clear to me either, but it's super later here...
I would really like to understand that...
> > is this a bug in -Wunsequenced in Clang
>
> Possibly, I think I already found one earlier tonight.
>
> https://bugs.llvm.org/show_bug.cgi?id=32985
this seems unrelated. I would try to report this and clarify in the llvm
bugzilla.
> Tomorrow, I'll try to cut down a test case to see if this is indeed a
> compiler bug. Would you like me to change the commit message to call
> this just a simple clean up, in the meantime?
I would go with the following wording.
"
Clang and its -Wunsequenced emits a warning
(PUT THE FULL WARNING HERE).
While it is not clear to me whether the initialization code violates the
specification (6.7.8 par 19 (ISO/IEC 9899) looks it disagrees) the code
is quite confusing and worth cleaning up anyway. Fix this by reusing
sc.gfp_mask rather than the updated input gfp_mask parameter.
"
--
Michal Hocko
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: RFC v2: post-init-read-only protection for data allocated dynamically
From: Igor Stoppa @ 2017-05-10 8:57 UTC (permalink / raw)
To: Michal Hocko, Dave Hansen
Cc: Laura Abbott, linux-mm, linux-kernel,
kernel-hardening@lists.openwall.com
In-Reply-To: <20170510080542.GF31466@dhcp22.suse.cz>
On 10/05/17 11:05, Michal Hocko wrote:
> On Fri 05-05-17 13:42:27, Igor Stoppa wrote:
[...]
>> ... in the case I have in mind, I have various, heterogeneous chunks of
>> data, coming from various subsystems, not necessarily page aligned.
>> And, even if they were page aligned, most likely they would be far
>> smaller than a page, even a 4k page.
>
> This aspect of various sizes makes the SLAB allocator not optimal
> because it operates on caches (pools of pages) which manage objects of
> the same size.
Indeed, that's why I wasn't too excited about caches, but probably I was
not able to explain sufficiently well why in the RFC :-/
> You could use the maximum size of all objects and waste
> some memory but you would have to know this max in advance which would
> make this approach less practical. You could create more caches of
> course but that still requires to know those sizes in advance.
Yes, and even generic per-architecture or even per board profiling
wouldn't necessarily do much good: taking SE Linux as example, one could
have two identical boards with almost identical binaries installed, only
differing in the rules/policy.
That difference alone could easily lead to very different size
requirements for the sealable page pool.
> So it smells like a dedicated allocator which operates on a pool of
> pages might be a better option in the end.
ok
> This depends on what you
> expect from the allocator. NUMA awareness? Very effective hotpath? Very
> good fragmentation avoidance? CPU cache awareness? Special alignment
> requirements? Reasonable free()? Etc...
>From the perspective of selling the feature to as many subsystems as
possible, I'd say that as primary requirement, it shouldn't affect
runtime performance.
I suppose (but it's just my guess) that trading milliseconds-scale
boot-time slowdown, for additional integrity is acceptable in the vast
majority of cases.
The only alignment requirements that I can think of, are coming from the
MMU capability of dealing only with physical pages, when it comes to
write-protect them.
> To me it seems that this being an initialization mostly thingy a simple
> allocator which manages a pool of pages (one set of sealed and one for
> allocations)
Shouldn't also the set of pages used for keeping track of the others be
sealed? Once one is ro, also the other should not change.
> and which only appends new objects as they fit to unsealed
> pages would be sufficient for starter.
Any "free" that might happen during the initialization transient, would
actually result in an untracked gap, right?
What about the size of the pool of pages?
No predefined size, instead request a new page, when the memory
remaining from the page currently in use is not enough to fit the latest
allocation request?
There are also two aspect we discussed earlier:
- livepatch: how to deal with it? Identify the page it wants to modify
and temporarily un-protect it?
- modules: unloading and reloading modules will eventually lead to
permanently lost pages, in increasing number.
Loading/unloading repeatedly the same module is probably not so common,
with a major exception being USB, where almost anything can show up.
And disappear.
This seems like a major showstopper for the linear allocator you propose.
My reasoning in pursuing the kmalloc approach was that it is already
equipped with mechanisms for dealing with these sort of cases, where
memory can be fragmented.
I also wouldn't risk introducing bugs with my homebrew allocator ...
The initial thought was that there could be a master toggle to
seal/unseal all the memory affected.
But you were not too excited, iirc :-D
Alternatively, kmalloc could be enhanced to unseal only the pages it
wants to modify.
I don't think much can be done for data that is placed together, in the
same page with something that needs to be altered.
But what is outside of that page could still enjoy the protection from
the seal.
--
igor
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v3] arm64: fix the overlap between the kernel image and vmalloc address
From: Catalin Marinas @ 2017-05-10 8:55 UTC (permalink / raw)
To: zhongjiang
Cc: ard.biesheuvel, mark.rutland, labbott, linux-arm-kernel,
tanxiaojun, linux-mm
In-Reply-To: <1494387440-51703-1-git-send-email-zhongjiang@huawei.com>
Given that there are a lot more mm changes than arm64, cc'ing linux-mm
as well.
Patch below:
On Wed, May 10, 2017 at 11:37:20AM +0800, zhongjiang wrote:
> Recently, xiaojun report the following issue.
>
> [ 4544.984139] Unable to handle kernel paging request at virtual address ffff804392800000
> [ 4544.991995] pgd = ffff80096745f000
> [ 4544.995369] [ffff804392800000] *pgd=0000000000000000
> [ 4545.000297] Internal error: Oops: 96000005 [#1] PREEMPT SMP
> [ 4545.005815] Modules linked in:
> [ 4545.008843] CPU: 1 PID: 8976 Comm: cat Not tainted 4.11.0-rc6 #1
> [ 4545.014790] Hardware name: ARM Juno development board (r1) (DT)
> [ 4545.020653] task: ffff8009753fdb00 task.stack: ffff80097533c000
> [ 4545.026520] PC is at __memcpy+0x100/0x180
> [ 4545.030491] LR is at vread+0x144/0x280
> [ 4545.034202] pc : [<ffff0000083a1000>] lr : [<ffff0000081c126c>] pstate: 20000145
> [ 4545.041530] sp : ffff80097533fcb0
> [ 4545.044811] x29: ffff80097533fcb0 x28: ffff800962d24000
> [ 4545.050074] x27: 0000000000001000 x26: ffff8009753fdb00
> [ 4545.055337] x25: ffff000008200000 x24: ffff800977801380
> [ 4545.060600] x23: ffff8009753fdb00 x22: ffff800962d24000
> [ 4545.065863] x21: 0000000000001000 x20: ffff000008200000
> [ 4545.071125] x19: 0000000000001000 x18: 0000ffffefa323c0
> [ 4545.076387] x17: 0000ffffa9c87440 x16: ffff0000081fdfd0
> [ 4545.081649] x15: 0000ffffa9d01588 x14: 72a77346b2407be7
> [ 4545.086911] x13: 5299400690000000 x12: b0000001f9001a79
> [ 4545.092173] x11: 97fc098d91042260 x10: 0000000000000000
> [ 4545.097435] x9 : 0000000000000000 x8 : 9110626091260021
> [ 4545.102698] x7 : 0000000000001000 x6 : ffff800962d24000
> [ 4545.107960] x5 : ffff8009778013b0 x4 : 0000000000000000
> [ 4545.113222] x3 : 0400000000000001 x2 : 0000000000000f80
> [ 4545.118484] x1 : ffff804392800000 x0 : ffff800962d24000
> [ 4545.123745]
> [ 4545.125220] Process cat (pid: 8976, stack limit = 0xffff80097533c000)
> [ 4545.131598] Stack: (0xffff80097533fcb0 to 0xffff800975340000)
> [ 4545.137289] fca0: ffff80097533fd30 ffff000008270f64
> [ 4545.145049] fcc0: 000000000000e000 000000003956f000 ffff000008f950d0 ffff80097533feb8
> [ 4545.152809] fce0: 0000000000002000 ffff8009753fdb00 ffff800962d24000 ffff000008e8d3d8
> [ 4545.160568] fd00: 0000000000001000 ffff000008200000 0000000000001000 ffff800962d24000
> [ 4545.168327] fd20: 0000000000001000 ffff000008e884a0 ffff80097533fdb0 ffff00000826340c
> [ 4545.176086] fd40: ffff800976bf2800 fffffffffffffffb 000000003956d000 ffff80097533feb8
> [ 4545.183846] fd60: 0000000060000000 0000000000000015 0000000000000124 000000000000003f
> [ 4545.191605] fd80: ffff000008962000 ffff8009753fdb00 ffff8009753fdb00 ffff8009753fdb00
> [ 4545.199364] fda0: 0000000300000124 0000000000002000 ffff80097533fdd0 ffff0000081fb83c
> [ 4545.207123] fdc0: 0000000000010000 ffff80097514f900 ffff80097533fe50 ffff0000081fcb28
> [ 4545.214883] fde0: 0000000000010000 ffff80097514f900 0000000000000000 0000000000000000
> [ 4545.222642] fe00: ffff80097533fe30 ffff0000081fca1c ffff80097514f900 0000000000000000
> [ 4545.230401] fe20: 000000003956d000 ffff80097533feb8 ffff80097533fe50 ffff0000081fcb04
> [ 4545.238160] fe40: 0000000000010000 ffff80097514f900 ffff80097533fe80 ffff0000081fe014
> [ 4545.245919] fe60: ffff80097514f900 ffff80097514f900 000000003956d000 0000000000010000
> [ 4545.253678] fe80: 0000000000000000 ffff000008082f30 0000000000000000 0000800977146000
> [ 4545.261438] fea0: ffffffffffffffff 0000ffffa9c8745c 0000000000000124 0000000008202000
> [ 4545.269197] fec0: 0000000000000003 000000003956d000 0000000000010000 0000000000000000
> [ 4545.276956] fee0: 0000000000011011 0000000000000001 0000000000000011 0000000000000002
> [ 4545.284715] ff00: 000000000000003f 1f3c201f7372686b 00000000ffffffff 0000000000000030
> [ 4545.292474] ff20: 0000000000000038 0000000000000000 0000ffffa9bcca94 0000ffffa9d01588
> [ 4545.300233] ff40: 0000000000000000 0000ffffa9c87440 0000ffffefa323c0 0000000000010000
> [ 4545.307993] ff60: 000000000041a310 000000003956d000 0000000000000003 000000007fffe000
> [ 4545.315751] ff80: 00000000004088d0 0000000000010000 0000000000000000 0000000000000000
> [ 4545.323511] ffa0: 0000000000010000 0000ffffefa32690 0000000000404dcc 0000ffffefa32690
> [ 4545.331270] ffc0: 0000ffffa9c8745c 0000000060000000 0000000000000003 000000000000003f
> [ 4545.339029] ffe0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> [ 4545.346786] Call trace:
> [ 4545.349207] Exception stack(0xffff80097533fae0 to 0xffff80097533fc10)
> [ 4545.355586] fae0: 0000000000001000 0001000000000000 ffff80097533fcb0 ffff0000083a1000
> [ 4545.363345] fb00: 000000003957c000 ffff80097533fc00 0000000020000145 0000000000000025
> [ 4545.371105] fb20: ffff800962d24000 ffff000008e8d3d8 0000000000001000 ffff8009753fdb00
> [ 4545.378864] fb40: 0000000000000000 0000000000000002 ffff80097533fd30 ffff000008082604
> [ 4545.386623] fb60: 0000000000001000 0001000000000000 ffff80097533fd30 ffff0000083a0a90
> [ 4545.394382] fb80: ffff800962d24000 ffff804392800000 0000000000000f80 0400000000000001
> [ 4545.402140] fba0: 0000000000000000 ffff8009778013b0 ffff800962d24000 0000000000001000
> [ 4545.409899] fbc0: 9110626091260021 0000000000000000 0000000000000000 97fc098d91042260
> [ 4545.417658] fbe0: b0000001f9001a79 5299400690000000 72a77346b2407be7 0000ffffa9d01588
> [ 4545.425416] fc00: ffff0000081fdfd0 0000ffffa9c87440
> [ 4545.430248] [<ffff0000083a1000>] __memcpy+0x100/0x180
> [ 4545.435253] [<ffff000008270f64>] read_kcore+0x21c/0x3b0
> [ 4545.440429] [<ffff00000826340c>] proc_reg_read+0x64/0x90
> [ 4545.445691] [<ffff0000081fb83c>] __vfs_read+0x1c/0x108
> [ 4545.450779] [<ffff0000081fcb28>] vfs_read+0x80/0x130
> [ 4545.455696] [<ffff0000081fe014>] SyS_read+0x44/0xa0
> [ 4545.460528] [<ffff000008082f30>] el0_svc_naked+0x24/0x28
> [ 4545.465790] Code: d503201f d503201f d503201f d503201f (a8c12027)
> [ 4545.471852] ---[ end trace 4d1897f94759f461 ]---
> [ 4545.476435] note: cat[8976] exited with preempt_count 2
>
> I find the issue is introduced when applying commit f9040773b7bb
> ("arm64: move kernel image to base of vmalloc area"). This patch
> make the kernel image overlap with vmalloc area. It will result in
> vmalloc area have the huge page table. but the vmalloc_to_page is
> not realize the change. and the function is public to any arch.
>
> I fix it by adding the another kernel image condition in vmalloc_to_page
> to make it keep the accordance with previous vmalloc mapping.
>
> Fixes: f9040773b7bb ("arm64: move kernel image to base of vmalloc area")
> Reported-by: tan xiaojun <tanxiaojun@huawei.com>
> Signed-off-by: zhongjiang <zhongjiang@huawei.com>
> ---
> arch/arm64/mm/mmu.c | 2 +-
> include/linux/vmalloc.h | 1 +
> mm/vmalloc.c | 35 ++++++++++++++++++++++++++++-------
> 3 files changed, 30 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index 0c429ec..2265c39 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -509,7 +509,7 @@ static void __init map_kernel_segment(pgd_t *pgd, void *va_start, void *va_end,
> vma->addr = va_start;
> vma->phys_addr = pa_start;
> vma->size = size;
> - vma->flags = VM_MAP;
> + vma->flags = VM_KERNEL;
> vma->caller = __builtin_return_address(0);
>
> vm_area_add_early(vma);
> diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h
> index 0328ce0..c9245af 100644
> --- a/include/linux/vmalloc.h
> +++ b/include/linux/vmalloc.h
> @@ -17,6 +17,7 @@
> #define VM_ALLOC 0x00000002 /* vmalloc() */
> #define VM_MAP 0x00000004 /* vmap()ed pages */
> #define VM_USERMAP 0x00000008 /* suitable for remap_vmalloc_range */
> +#define VM_KERNEL 0x00000010 /* kernel pages */
> #define VM_UNINITIALIZED 0x00000020 /* vm_struct is not fully initialized */
> #define VM_NO_GUARD 0x00000040 /* don't add guard page */
> #define VM_KASAN 0x00000080 /* has allocated kasan shadow memory */
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 1dda6d8..601d940 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -1967,11 +1967,28 @@ void *vmalloc_32_user(unsigned long size)
> EXPORT_SYMBOL(vmalloc_32_user);
>
> /*
> + * kernel image overlap with the valloc area in arm64,it
> + * will make the huge talbe page existence, if we walk the
> + * all page talbe, it may be result in the panic.
> + */
> +static inline struct page *aligned_get_page(char *addr, struct vm_struct *vm)
> +{
> + struct page *p = NULL;
> +
> + if (vm->flags & VM_KERNEL)
> + p = virt_to_page(lm_alias(addr));
> + else
> + p = vmalloc_to_page(addr);
> +
> + return p;
> +}
> +
> +/*
> * small helper routine , copy contents to buf from addr.
> * If the page is not present, fill zero.
> */
> -
> -static int aligned_vread(char *buf, char *addr, unsigned long count)
> +static int aligned_vread(char *buf, char *addr, unsigned long count,
> + struct vm_struct *vm)
> {
> struct page *p;
> int copied = 0;
> @@ -1983,7 +2000,7 @@ static int aligned_vread(char *buf, char *addr, unsigned long count)
> length = PAGE_SIZE - offset;
> if (length > count)
> length = count;
> - p = vmalloc_to_page(addr);
> + p = aligned_get_page(addr, vm);
> /*
> * To do safe access to this _mapped_ area, we need
> * lock. But adding lock here means that we need to add
> @@ -2010,7 +2027,8 @@ static int aligned_vread(char *buf, char *addr, unsigned long count)
> return copied;
> }
>
> -static int aligned_vwrite(char *buf, char *addr, unsigned long count)
> +static int aligned_vwrite(char *buf, char *addr, unsigned long count,
> + struct vm_struct *vm)
> {
> struct page *p;
> int copied = 0;
> @@ -2022,7 +2040,7 @@ static int aligned_vwrite(char *buf, char *addr, unsigned long count)
> length = PAGE_SIZE - offset;
> if (length > count)
> length = count;
> - p = vmalloc_to_page(addr);
> + p = aligned_get_page(addr, vm);
> /*
> * To do safe access to this _mapped_ area, we need
> * lock. But adding lock here means that we need to add
> @@ -2109,7 +2127,7 @@ long vread(char *buf, char *addr, unsigned long count)
> if (n > count)
> n = count;
> if (!(vm->flags & VM_IOREMAP))
> - aligned_vread(buf, addr, n);
> + aligned_vread(buf, addr, n, vm);
> else /* IOREMAP area is treated as memory hole */
> memset(buf, 0, n);
> buf += n;
> @@ -2190,7 +2208,7 @@ long vwrite(char *buf, char *addr, unsigned long count)
> if (n > count)
> n = count;
> if (!(vm->flags & VM_IOREMAP)) {
> - aligned_vwrite(buf, addr, n);
> + aligned_vwrite(buf, addr, n, vm);
> copied++;
> }
> buf += n;
> @@ -2710,6 +2728,9 @@ static int s_show(struct seq_file *m, void *p)
> if (v->flags & VM_USERMAP)
> seq_puts(m, " user");
>
> + if (v->flags & VM_KERNEL)
> + seq_puts(m, " kernel");
> +
> if (is_vmalloc_addr(v->pages))
> seq_puts(m, " vpages");
>
> --
> 1.7.12.4
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* [PATCH 4/4] dax: Fix data corruption when fault races with write
From: Jan Kara @ 2017-05-10 8:54 UTC (permalink / raw)
To: Andrew Morton
Cc: Ross Zwisler, Dan Williams, linux-ext4, linux-fsdevel,
linux-nvdimm, linux-mm, Jan Kara, stable
In-Reply-To: <20170510085419.27601-1-jack@suse.cz>
Currently DAX read fault can race with write(2) in the following way:
CPU1 - write(2) CPU2 - read fault
dax_iomap_pte_fault()
->iomap_begin() - sees hole
dax_iomap_rw()
iomap_apply()
->iomap_begin - allocates blocks
dax_iomap_actor()
invalidate_inode_pages2_range()
- there's nothing to invalidate
grab_mapping_entry()
- we add zero page in the radix tree
and map it to page tables
The result is that hole page is mapped into page tables (and thus zeros
are seen in mmap) while file has data written in that place.
Fix the problem by locking exception entry before mapping blocks for the
fault. That way we are sure invalidate_inode_pages2_range() call for
racing write will either block on entry lock waiting for the fault to
finish (and unmap stale page tables after that) or read fault will see
already allocated blocks by write(2).
Fixes: 9f141d6ef6258a3a37a045842d9ba7e68f368956
CC: stable@vger.kernel.org
Reviewed-by: Ross Zwisler <ross.zwisler@linux.intel.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
fs/dax.c | 32 ++++++++++++++++----------------
1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/fs/dax.c b/fs/dax.c
index 123d9903c77d..32f020c9cedf 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -1148,6 +1148,12 @@ static int dax_iomap_pte_fault(struct vm_fault *vmf,
if ((vmf->flags & FAULT_FLAG_WRITE) && !vmf->cow_page)
flags |= IOMAP_WRITE;
+ entry = grab_mapping_entry(mapping, vmf->pgoff, 0);
+ if (IS_ERR(entry)) {
+ vmf_ret = dax_fault_return(PTR_ERR(entry));
+ goto out;
+ }
+
/*
* Note that we don't bother to use iomap_apply here: DAX required
* the file system block size to be equal the page size, which means
@@ -1156,17 +1162,11 @@ static int dax_iomap_pte_fault(struct vm_fault *vmf,
error = ops->iomap_begin(inode, pos, PAGE_SIZE, flags, &iomap);
if (error) {
vmf_ret = dax_fault_return(error);
- goto out;
+ goto unlock_entry;
}
if (WARN_ON_ONCE(iomap.offset + iomap.length < pos + PAGE_SIZE)) {
- vmf_ret = dax_fault_return(-EIO); /* fs corruption? */
- goto finish_iomap;
- }
-
- entry = grab_mapping_entry(mapping, vmf->pgoff, 0);
- if (IS_ERR(entry)) {
- vmf_ret = dax_fault_return(PTR_ERR(entry));
- goto finish_iomap;
+ error = -EIO; /* fs corruption? */
+ goto error_finish_iomap;
}
sector = dax_iomap_sector(&iomap, pos);
@@ -1188,13 +1188,13 @@ static int dax_iomap_pte_fault(struct vm_fault *vmf,
}
if (error)
- goto error_unlock_entry;
+ goto error_finish_iomap;
__SetPageUptodate(vmf->cow_page);
vmf_ret = finish_fault(vmf);
if (!vmf_ret)
vmf_ret = VM_FAULT_DONE_COW;
- goto unlock_entry;
+ goto finish_iomap;
}
switch (iomap.type) {
@@ -1214,7 +1214,7 @@ static int dax_iomap_pte_fault(struct vm_fault *vmf,
case IOMAP_HOLE:
if (!(vmf->flags & FAULT_FLAG_WRITE)) {
vmf_ret = dax_load_hole(mapping, &entry, vmf);
- goto unlock_entry;
+ goto finish_iomap;
}
/*FALLTHRU*/
default:
@@ -1223,10 +1223,8 @@ static int dax_iomap_pte_fault(struct vm_fault *vmf,
break;
}
- error_unlock_entry:
+ error_finish_iomap:
vmf_ret = dax_fault_return(error) | major;
- unlock_entry:
- put_locked_mapping_entry(mapping, vmf->pgoff, entry);
finish_iomap:
if (ops->iomap_end) {
int copied = PAGE_SIZE;
@@ -1241,7 +1239,9 @@ static int dax_iomap_pte_fault(struct vm_fault *vmf,
*/
ops->iomap_end(inode, pos, PAGE_SIZE, copied, flags, &iomap);
}
-out:
+ unlock_entry:
+ put_locked_mapping_entry(mapping, vmf->pgoff, entry);
+ out:
trace_dax_pte_fault_done(inode, vmf, vmf_ret);
return vmf_ret;
}
--
2.12.0
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related
* [PATCH 1/4] dax: prevent invalidation of mapped DAX entries
From: Jan Kara @ 2017-05-10 8:54 UTC (permalink / raw)
To: Andrew Morton
Cc: Ross Zwisler, Dan Williams, linux-ext4, linux-fsdevel,
linux-nvdimm, linux-mm, [4.10+], Jan Kara
In-Reply-To: <20170510085419.27601-1-jack@suse.cz>
From: Ross Zwisler <ross.zwisler@linux.intel.com>
dax_invalidate_mapping_entry() currently removes DAX exceptional entries
only if they are clean and unlocked. This is done via:
invalidate_mapping_pages()
invalidate_exceptional_entry()
dax_invalidate_mapping_entry()
However, for page cache pages removed in invalidate_mapping_pages() there
is an additional criteria which is that the page must not be mapped. This
is noted in the comments above invalidate_mapping_pages() and is checked in
invalidate_inode_page().
For DAX entries this means that we can can end up in a situation where a
DAX exceptional entry, either a huge zero page or a regular DAX entry,
could end up mapped but without an associated radix tree entry. This is
inconsistent with the rest of the DAX code and with what happens in the
page cache case.
We aren't able to unmap the DAX exceptional entry because according to its
comments invalidate_mapping_pages() isn't allowed to block, and
unmap_mapping_range() takes a write lock on the mapping->i_mmap_rwsem.
Since we essentially never have unmapped DAX entries to evict from the
radix tree, just remove dax_invalidate_mapping_entry().
Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
Fixes: c6dcf52c23d2 ("mm: Invalidate DAX radix tree entries only if appropriate")
Reported-by: Jan Kara <jack@suse.cz>
Cc: <stable@vger.kernel.org> [4.10+]
Signed-off-by: Jan Kara <jack@suse.cz>
---
fs/dax.c | 29 -----------------------------
include/linux/dax.h | 1 -
mm/truncate.c | 9 +++------
3 files changed, 3 insertions(+), 36 deletions(-)
diff --git a/fs/dax.c b/fs/dax.c
index 66d79067eedf..38deebb8c86e 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -461,35 +461,6 @@ int dax_delete_mapping_entry(struct address_space *mapping, pgoff_t index)
}
/*
- * Invalidate exceptional DAX entry if easily possible. This handles DAX
- * entries for invalidate_inode_pages() so we evict the entry only if we can
- * do so without blocking.
- */
-int dax_invalidate_mapping_entry(struct address_space *mapping, pgoff_t index)
-{
- int ret = 0;
- void *entry, **slot;
- struct radix_tree_root *page_tree = &mapping->page_tree;
-
- spin_lock_irq(&mapping->tree_lock);
- entry = __radix_tree_lookup(page_tree, index, NULL, &slot);
- if (!entry || !radix_tree_exceptional_entry(entry) ||
- slot_locked(mapping, slot))
- goto out;
- if (radix_tree_tag_get(page_tree, index, PAGECACHE_TAG_DIRTY) ||
- radix_tree_tag_get(page_tree, index, PAGECACHE_TAG_TOWRITE))
- goto out;
- radix_tree_delete(page_tree, index);
- mapping->nrexceptional--;
- ret = 1;
-out:
- spin_unlock_irq(&mapping->tree_lock);
- if (ret)
- dax_wake_mapping_entry_waiter(mapping, index, entry, true);
- return ret;
-}
-
-/*
* Invalidate exceptional DAX entry if it is clean.
*/
int dax_invalidate_mapping_entry_sync(struct address_space *mapping,
diff --git a/include/linux/dax.h b/include/linux/dax.h
index d3158e74a59e..d1236d16ef00 100644
--- a/include/linux/dax.h
+++ b/include/linux/dax.h
@@ -63,7 +63,6 @@ ssize_t dax_iomap_rw(struct kiocb *iocb, struct iov_iter *iter,
int dax_iomap_fault(struct vm_fault *vmf, enum page_entry_size pe_size,
const struct iomap_ops *ops);
int dax_delete_mapping_entry(struct address_space *mapping, pgoff_t index);
-int dax_invalidate_mapping_entry(struct address_space *mapping, pgoff_t index);
int dax_invalidate_mapping_entry_sync(struct address_space *mapping,
pgoff_t index);
void dax_wake_mapping_entry_waiter(struct address_space *mapping,
diff --git a/mm/truncate.c b/mm/truncate.c
index 83a059e8cd1d..706cff171a15 100644
--- a/mm/truncate.c
+++ b/mm/truncate.c
@@ -67,17 +67,14 @@ static void truncate_exceptional_entry(struct address_space *mapping,
/*
* Invalidate exceptional entry if easily possible. This handles exceptional
- * entries for invalidate_inode_pages() so for DAX it evicts only unlocked and
- * clean entries.
+ * entries for invalidate_inode_pages().
*/
static int invalidate_exceptional_entry(struct address_space *mapping,
pgoff_t index, void *entry)
{
- /* Handled by shmem itself */
- if (shmem_mapping(mapping))
+ /* Handled by shmem itself, or for DAX we do nothing. */
+ if (shmem_mapping(mapping) || dax_mapping(mapping))
return 1;
- if (dax_mapping(mapping))
- return dax_invalidate_mapping_entry(mapping, index);
clear_shadow_entry(mapping, index, entry);
return 1;
}
--
2.12.0
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related
* [PATCH 2/4] mm: Fix data corruption due to stale mmap reads
From: Jan Kara @ 2017-05-10 8:54 UTC (permalink / raw)
To: Andrew Morton
Cc: Ross Zwisler, Dan Williams, linux-ext4, linux-fsdevel,
linux-nvdimm, linux-mm, Jan Kara, stable
In-Reply-To: <20170510085419.27601-1-jack@suse.cz>
Currently, we didn't invalidate page tables during
invalidate_inode_pages2() for DAX. That could result in e.g. 2MiB zero
page being mapped into page tables while there were already underlying
blocks allocated and thus data seen through mmap were different from
data seen by read(2). The following sequence reproduces the problem:
- open an mmap over a 2MiB hole
- read from a 2MiB hole, faulting in a 2MiB zero page
- write to the hole with write(3p). The write succeeds but we
incorrectly leave the 2MiB zero page mapping intact.
- via the mmap, read the data that was just written. Since the zero
page mapping is still intact we read back zeroes instead of the new
data.
Fix the problem by unconditionally calling
invalidate_inode_pages2_range() in dax_iomap_actor() for new block
allocations and by properly invalidating page tables in
invalidate_inode_pages2_range() for DAX mappings.
Fixes: c6dcf52c23d2d3fb5235cec42d7dd3f786b87d55
CC: stable@vger.kernel.org
Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
fs/dax.c | 2 +-
mm/truncate.c | 12 +++++++++++-
2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/fs/dax.c b/fs/dax.c
index 38deebb8c86e..123d9903c77d 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -1015,7 +1015,7 @@ dax_iomap_actor(struct inode *inode, loff_t pos, loff_t length, void *data,
* into page tables. We have to tear down these mappings so that data
* written by write(2) is visible in mmap.
*/
- if ((iomap->flags & IOMAP_F_NEW) && inode->i_mapping->nrpages) {
+ if (iomap->flags & IOMAP_F_NEW) {
invalidate_inode_pages2_range(inode->i_mapping,
pos >> PAGE_SHIFT,
(end - 1) >> PAGE_SHIFT);
diff --git a/mm/truncate.c b/mm/truncate.c
index 706cff171a15..6479ed2afc53 100644
--- a/mm/truncate.c
+++ b/mm/truncate.c
@@ -686,7 +686,17 @@ int invalidate_inode_pages2_range(struct address_space *mapping,
cond_resched();
index++;
}
-
+ /*
+ * For DAX we invalidate page tables after invalidating radix tree. We
+ * could invalidate page tables while invalidating each entry however
+ * that would be expensive. And doing range unmapping before doesn't
+ * work as we have no cheap way to find whether radix tree entry didn't
+ * get remapped later.
+ */
+ if (dax_mapping(mapping)) {
+ unmap_mapping_range(mapping, (loff_t)start << PAGE_SHIFT,
+ (loff_t)(end - start + 1) << PAGE_SHIFT, 0);
+ }
out:
cleancache_invalidate_inode(mapping);
return ret;
--
2.12.0
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related
* [PATCH 3/4] ext4: Return back to starting transaction in ext4_dax_huge_fault()
From: Jan Kara @ 2017-05-10 8:54 UTC (permalink / raw)
To: Andrew Morton
Cc: Ross Zwisler, Dan Williams, linux-ext4, linux-fsdevel,
linux-nvdimm, linux-mm, Jan Kara, stable
In-Reply-To: <20170510085419.27601-1-jack@suse.cz>
DAX will return to locking exceptional entry before mapping blocks
for a page fault to fix possible races with concurrent writes. To avoid
lock inversion between exceptional entry lock and transaction start,
start the transaction already in ext4_dax_huge_fault().
Fixes: 9f141d6ef6258a3a37a045842d9ba7e68f368956
CC: stable@vger.kernel.org
Signed-off-by: Jan Kara <jack@suse.cz>
---
fs/ext4/file.c | 21 +++++++++++++++++----
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index cefa9835f275..831fd6beebf0 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -257,6 +257,7 @@ static int ext4_dax_huge_fault(struct vm_fault *vmf,
enum page_entry_size pe_size)
{
int result;
+ handle_t *handle = NULL;
struct inode *inode = file_inode(vmf->vma->vm_file);
struct super_block *sb = inode->i_sb;
bool write = vmf->flags & FAULT_FLAG_WRITE;
@@ -264,12 +265,24 @@ static int ext4_dax_huge_fault(struct vm_fault *vmf,
if (write) {
sb_start_pagefault(sb);
file_update_time(vmf->vma->vm_file);
+ down_read(&EXT4_I(inode)->i_mmap_sem);
+ handle = ext4_journal_start_sb(sb, EXT4_HT_WRITE_PAGE,
+ EXT4_DATA_TRANS_BLOCKS(sb));
+ } else {
+ down_read(&EXT4_I(inode)->i_mmap_sem);
}
- down_read(&EXT4_I(inode)->i_mmap_sem);
- result = dax_iomap_fault(vmf, pe_size, &ext4_iomap_ops);
- up_read(&EXT4_I(inode)->i_mmap_sem);
- if (write)
+ if (!IS_ERR(handle))
+ result = dax_iomap_fault(vmf, pe_size, &ext4_iomap_ops);
+ else
+ result = VM_FAULT_SIGBUS;
+ if (write) {
+ if (!IS_ERR(handle))
+ ext4_journal_stop(handle);
+ up_read(&EXT4_I(inode)->i_mmap_sem);
sb_end_pagefault(sb);
+ } else {
+ up_read(&EXT4_I(inode)->i_mmap_sem);
+ }
return result;
}
--
2.12.0
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related
* [PATCH 0/4 v4] mm,dax: Fix data corruption due to mmap inconsistency
From: Jan Kara @ 2017-05-10 8:54 UTC (permalink / raw)
To: Andrew Morton
Cc: Ross Zwisler, Dan Williams, linux-ext4, linux-fsdevel,
linux-nvdimm, linux-mm, Jan Kara
Hello,
this series fixes data corruption that can happen for DAX mounts when
page faults race with write(2) and as a result page tables get out of sync
with block mappings in the filesystem and thus data seen through mmap is
different from data seen through read(2).
The series passes testing with t_mmap_stale test program from Ross and also
other mmap related tests on DAX filesystem.
Andrew, can you please merge these patches? Thanks!
Changes since v3:
* Rebased on top of current Linus' tree due to non-trivial conflicts with
added tracepoint
Changes since v2:
* Added reviewed-by tag from Ross
Changes since v1:
* Improved performance of unmapping pages
* Changed fault locking to fix another write vs fault race
Honza
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH] mm/vmscan: fix unsequenced modification and access warning
From: Nick Desaulniers @ 2017-05-10 8:46 UTC (permalink / raw)
To: Michal Hocko
Cc: akpm, hannes, mgorman, vbabka, minchan, linux-mm, linux-kernel
In-Reply-To: <20170510071511.GA31466@dhcp22.suse.cz>
> You can add
Something that's not clear to me when advised to add, should I be
uploading a v3 with your acked by? I think I got that wrong the last
time I asked (which was my first patch to Linux).
> But I still do not understand which part of the code is undefined and
> why.
It's not immediately clear to me either, but it's super later here...
> is this a bug in -Wunsequenced in Clang
Possibly, I think I already found one earlier tonight.
https://bugs.llvm.org/show_bug.cgi?id=32985
Tomorrow, I'll try to cut down a test case to see if this is indeed a
compiler bug. Would you like me to change the commit message to call
this just a simple clean up, in the meantime?
Thanks,
~Nick
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [Patch v2] mm/vmscan: fix unsequenced modification and access warning
From: Michal Hocko @ 2017-05-10 8:38 UTC (permalink / raw)
To: Nick Desaulniers
Cc: akpm, hannes, mgorman, vbabka, minchan, linux-mm, linux-kernel
In-Reply-To: <20170510082734.2055-1-nick.desaulniers@gmail.com>
On Wed 10-05-17 01:27:34, Nick Desaulniers wrote:
> Clang flags this file with the -Wunsequenced error that GCC does not
> have.
>
> unsequenced modification and access to 'gfp_mask'
>
> It seems that gfp_mask is both read and written without a sequence point
> in between, which is undefined behavior.
>
> Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
I will definitely not object to the patch as the code is cleaner and less tricky.
You can add
Acked-by: Michal Hocko <mhocko@suse.com>
But I
still do not understand which part of the code is undefined and why. My
reading and understanding of the C specification is that
struct A {
int a;
int b;
};
struct A f = { .a = c = foo(c), .b = c};
as long as foo(c) doesn't have any side effects because because .a is
initialized before b and the assignment ordering will make sure that c
is initialized before a.
6.7.8 par 19 (ISO/IEC 9899)
19 The initialization shall occur in initializer list order, each
initializer provided for a particular subobject overriding any
previously listed initializer for the same subobject; all subobjects
that are not initialized explicitly shall be initialized implicitly
the same as objects that have static storage duration.
So is my understanding of the specification wrong or is this a bug in
-Wunsequenced in Clang?
> ---
> Changes in v2:
> - don't assign back to gfp_mask, reuse sc.gfp_mask
> - initialize reclaim_idx directly, without classzone_idx
>
> mm/vmscan.c | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 4e7ed65842af..d32c42d17935 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -2958,7 +2958,7 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
> unsigned long nr_reclaimed;
> struct scan_control sc = {
> .nr_to_reclaim = SWAP_CLUSTER_MAX,
> - .gfp_mask = (gfp_mask = current_gfp_context(gfp_mask)),
> + .gfp_mask = current_gfp_context(gfp_mask),
> .reclaim_idx = gfp_zone(gfp_mask),
> .order = order,
> .nodemask = nodemask,
> @@ -2973,12 +2973,12 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
> * 1 is returned so that the page allocator does not OOM kill at this
> * point.
> */
> - if (throttle_direct_reclaim(gfp_mask, zonelist, nodemask))
> + if (throttle_direct_reclaim(sc.gfp_mask, zonelist, nodemask))
> return 1;
>
> trace_mm_vmscan_direct_reclaim_begin(order,
> sc.may_writepage,
> - gfp_mask,
> + sc.gfp_mask,
> sc.reclaim_idx);
>
> nr_reclaimed = do_try_to_free_pages(zonelist, &sc);
> @@ -3763,16 +3763,15 @@ static int __node_reclaim(struct pglist_data *pgdat, gfp_t gfp_mask, unsigned in
> const unsigned long nr_pages = 1 << order;
> struct task_struct *p = current;
> struct reclaim_state reclaim_state;
> - int classzone_idx = gfp_zone(gfp_mask);
> struct scan_control sc = {
> .nr_to_reclaim = max(nr_pages, SWAP_CLUSTER_MAX),
> - .gfp_mask = (gfp_mask = current_gfp_context(gfp_mask)),
> + .gfp_mask = current_gfp_context(gfp_mask),
> .order = order,
> .priority = NODE_RECLAIM_PRIORITY,
> .may_writepage = !!(node_reclaim_mode & RECLAIM_WRITE),
> .may_unmap = !!(node_reclaim_mode & RECLAIM_UNMAP),
> .may_swap = 1,
> - .reclaim_idx = classzone_idx,
> + .reclaim_idx = gfp_zone(gfp_mask),
> };
>
> cond_resched();
> @@ -3782,7 +3781,7 @@ static int __node_reclaim(struct pglist_data *pgdat, gfp_t gfp_mask, unsigned in
> * and RECLAIM_UNMAP.
> */
> p->flags |= PF_MEMALLOC | PF_SWAPWRITE;
> - lockdep_set_current_reclaim_state(gfp_mask);
> + lockdep_set_current_reclaim_state(sc.gfp_mask);
> reclaim_state.reclaimed_slab = 0;
> p->reclaim_state = &reclaim_state;
>
> --
> 2.11.0
>
--
Michal Hocko
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v3 3/3] arm64: Silence first allocation with CONFIG_ARM64_MODULE_PLTS=y
From: Catalin Marinas @ 2017-05-10 8:38 UTC (permalink / raw)
To: Will Deacon
Cc: Florian Fainelli, Michal Hocko, open list, Kirill A. Shutemov,
Ard Biesheuvel, Russell King, Chris Wilson,
open list:MEMORY MANAGEMENT, zijun_hu, angus, Andrey Ryabinin,
Andrew Morton, linux-arm-kernel
In-Reply-To: <20170508100723.GF8526@arm.com>
On Mon, May 08, 2017 at 11:07:24AM +0100, Will Deacon wrote:
> On Fri, May 05, 2017 at 02:07:28PM -0700, Florian Fainelli wrote:
> > On 05/03/2017 04:18 AM, Will Deacon wrote:
> > > On Thu, Apr 27, 2017 at 11:19:02AM -0700, Florian Fainelli wrote:
> > >> When CONFIG_ARM64_MODULE_PLTS is enabled, the first allocation using the
> > >> module space fails, because the module is too big, and then the module
> > >> allocation is attempted from vmalloc space. Silence the first allocation
> > >> failure in that case by setting __GFP_NOWARN.
> > >>
> > >> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> > >> ---
> > >> arch/arm64/kernel/module.c | 7 ++++++-
> > >> 1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > I'm not sure what the merge plan is for these, but the arm64 bit here
> > > looks fine to me:
> > >
> > > Acked-by: Will Deacon <will.deacon@arm.com>
> >
> > Thanks, not sure either, would you or Catalin want to pick this series?
>
> We'd need an Ack from Russell on the arch/arm/ part before we could take
> this series.
The first patch touches mm/vmalloc.c, so we could also merge the series
via akpm's tree. Andrew, do you have any preference?
--
Catalin
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* [Patch v2] mm/vmscan: fix unsequenced modification and access warning
From: Nick Desaulniers @ 2017-05-10 8:27 UTC (permalink / raw)
Cc: akpm, hannes, mgorman, mhocko, vbabka, minchan, linux-mm,
linux-kernel, Nick Desaulniers
In-Reply-To: <20170510071511.GA31466@dhcp22.suse.cz>
Clang flags this file with the -Wunsequenced error that GCC does not
have.
unsequenced modification and access to 'gfp_mask'
It seems that gfp_mask is both read and written without a sequence point
in between, which is undefined behavior.
Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
---
Changes in v2:
- don't assign back to gfp_mask, reuse sc.gfp_mask
- initialize reclaim_idx directly, without classzone_idx
mm/vmscan.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 4e7ed65842af..d32c42d17935 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -2958,7 +2958,7 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
unsigned long nr_reclaimed;
struct scan_control sc = {
.nr_to_reclaim = SWAP_CLUSTER_MAX,
- .gfp_mask = (gfp_mask = current_gfp_context(gfp_mask)),
+ .gfp_mask = current_gfp_context(gfp_mask),
.reclaim_idx = gfp_zone(gfp_mask),
.order = order,
.nodemask = nodemask,
@@ -2973,12 +2973,12 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
* 1 is returned so that the page allocator does not OOM kill at this
* point.
*/
- if (throttle_direct_reclaim(gfp_mask, zonelist, nodemask))
+ if (throttle_direct_reclaim(sc.gfp_mask, zonelist, nodemask))
return 1;
trace_mm_vmscan_direct_reclaim_begin(order,
sc.may_writepage,
- gfp_mask,
+ sc.gfp_mask,
sc.reclaim_idx);
nr_reclaimed = do_try_to_free_pages(zonelist, &sc);
@@ -3763,16 +3763,15 @@ static int __node_reclaim(struct pglist_data *pgdat, gfp_t gfp_mask, unsigned in
const unsigned long nr_pages = 1 << order;
struct task_struct *p = current;
struct reclaim_state reclaim_state;
- int classzone_idx = gfp_zone(gfp_mask);
struct scan_control sc = {
.nr_to_reclaim = max(nr_pages, SWAP_CLUSTER_MAX),
- .gfp_mask = (gfp_mask = current_gfp_context(gfp_mask)),
+ .gfp_mask = current_gfp_context(gfp_mask),
.order = order,
.priority = NODE_RECLAIM_PRIORITY,
.may_writepage = !!(node_reclaim_mode & RECLAIM_WRITE),
.may_unmap = !!(node_reclaim_mode & RECLAIM_UNMAP),
.may_swap = 1,
- .reclaim_idx = classzone_idx,
+ .reclaim_idx = gfp_zone(gfp_mask),
};
cond_resched();
@@ -3782,7 +3781,7 @@ static int __node_reclaim(struct pglist_data *pgdat, gfp_t gfp_mask, unsigned in
* and RECLAIM_UNMAP.
*/
p->flags |= PF_MEMALLOC | PF_SWAPWRITE;
- lockdep_set_current_reclaim_state(gfp_mask);
+ lockdep_set_current_reclaim_state(sc.gfp_mask);
reclaim_state.reclaimed_slab = 0;
p->reclaim_state = &reclaim_state;
--
2.11.0
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related
* Re: [RFC 09/10] x86/mm: Rework lazy TLB to track the actual loaded mm
From: Ingo Molnar @ 2017-05-10 8:24 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Andy Lutomirski, X86 ML, linux-kernel@vger.kernel.org,
Borislav Petkov, Linus Torvalds, Andrew Morton, Mel Gorman,
linux-mm@kvack.org, Rik van Riel, Dave Hansen, Nadav Amit,
Michal Hocko, Arjan van de Ven
In-Reply-To: <alpine.DEB.2.20.1705101017590.1979@nanos>
* Thomas Gleixner <tglx@linutronix.de> wrote:
> On Wed, 10 May 2017, Ingo Molnar wrote:
> >
> > * Thomas Gleixner <tglx@linutronix.de> wrote:
> >
> > > On Sun, 7 May 2017, Andy Lutomirski wrote:
> > > > /* context.lock is held for us, so we don't need any locking. */
> > > > static void flush_ldt(void *current_mm)
> > > > {
> > > > + struct mm_struct *mm = current_mm;
> > > > mm_context_t *pc;
> > > >
> > > > - if (current->active_mm != current_mm)
> > > > + if (this_cpu_read(cpu_tlbstate.loaded_mm) != current_mm)
> > >
> > > While functional correct, this really should compare against 'mm'.
> > >
> > > > return;
> > > >
> > > > - pc = ¤t->active_mm->context;
> > > > + pc = &mm->context;
> >
> > So this appears to be the function:
> >
> > static void flush_ldt(void *current_mm)
> > {
> > struct mm_struct *mm = current_mm;
> > mm_context_t *pc;
> >
> > if (this_cpu_read(cpu_tlbstate.loaded_mm) != current_mm)
> > return;
> >
> > pc = &mm->context;
> > set_ldt(pc->ldt->entries, pc->ldt->size);
> > }
> >
> > why not rename 'current_mm' to 'mm' and remove the 'mm' local variable?
>
> Because you cannot dereference a void pointer, i.e. &mm->context ....
Indeed, doh! The naming totally confused me. The way I'd write it is the canonical
form for such callbacks:
static void flush_ldt(void *data)
{
struct mm_struct *mm = data;
... which beyond unconfusing me would probably also have prevented any accidental
use of the 'current_mm' callback argument.
Thanks,
Ingo
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [RFC 09/10] x86/mm: Rework lazy TLB to track the actual loaded mm
From: Thomas Gleixner @ 2017-05-10 8:19 UTC (permalink / raw)
To: Ingo Molnar
Cc: Andy Lutomirski, X86 ML, linux-kernel@vger.kernel.org,
Borislav Petkov, Linus Torvalds, Andrew Morton, Mel Gorman,
linux-mm@kvack.org, Rik van Riel, Dave Hansen, Nadav Amit,
Michal Hocko, Arjan van de Ven
In-Reply-To: <20170510055727.g6wojjiis36a6nvm@gmail.com>
On Wed, 10 May 2017, Ingo Molnar wrote:
>
> * Thomas Gleixner <tglx@linutronix.de> wrote:
>
> > On Sun, 7 May 2017, Andy Lutomirski wrote:
> > > /* context.lock is held for us, so we don't need any locking. */
> > > static void flush_ldt(void *current_mm)
> > > {
> > > + struct mm_struct *mm = current_mm;
> > > mm_context_t *pc;
> > >
> > > - if (current->active_mm != current_mm)
> > > + if (this_cpu_read(cpu_tlbstate.loaded_mm) != current_mm)
> >
> > While functional correct, this really should compare against 'mm'.
> >
> > > return;
> > >
> > > - pc = ¤t->active_mm->context;
> > > + pc = &mm->context;
>
> So this appears to be the function:
>
> static void flush_ldt(void *current_mm)
> {
> struct mm_struct *mm = current_mm;
> mm_context_t *pc;
>
> if (this_cpu_read(cpu_tlbstate.loaded_mm) != current_mm)
> return;
>
> pc = &mm->context;
> set_ldt(pc->ldt->entries, pc->ldt->size);
> }
>
> why not rename 'current_mm' to 'mm' and remove the 'mm' local variable?
Because you cannot dereference a void pointer, i.e. &mm->context ....
Thanks,
tglx
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v2] mm: fix the memory leak after collapsing the huge page fails
From: zhong jiang @ 2017-05-10 7:59 UTC (permalink / raw)
To: Vlastimil Babka; +Cc: akpm, kirill.shutemov, hannes, mgorman, linux-mm
In-Reply-To: <6810ea54-faed-fcc1-7751-ff91fa3f9d8e@suse.cz>
On 2017/5/10 14:51, Vlastimil Babka wrote:
> On 05/09/2017 03:54 PM, zhong jiang wrote:
>> Hi, Vlastimil
>>
>> I review the code again. it works well for NUMA. because
>> khugepaged_prealloc_page will put_page when *hpage is true.
>>
>> the memory leak will still exist in !NUMA. because it ingore
>> the put_page. is it right? I miss something.
> No, on !NUMA the preallocated and unused new_page is freed by put_page()
> at the very end of khugepaged_do_scan().
Thank you for clarification. I should consider more before sending the patch.
Thanks
zhongjiang
>> Thanks
>> zhongjiang
>>
>> On 2017/5/9 20:41, Vlastimil Babka wrote:
>>> On 05/09/2017 02:20 PM, zhong jiang wrote:
>>>> On 2017/5/9 19:34, Vlastimil Babka wrote:
>>>>> On 05/09/2017 12:55 PM, zhongjiang wrote:
>>>>>> From: zhong jiang <zhongjiang@huawei.com>
>>>>>>
>>>>>> Current, when we prepare a huge page to collapse, due to some
>>>>>> reasons, it can fail to collapse. At the moment, we should
>>>>>> release the preallocate huge page.
>>>>>>
>>>>>> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
>>>>> Hmm, scratch that, there's no memory leak. The pointer to new_page is
>>>>> stored in *hpage, and put_page() is called all the way up in
>>>>> khugepaged_do_scan().
>>>> I see. I miss it. but why the new_page need to be release all the way.
>>> AFAIK to support preallocation and reusal of preallocated page for
>>> collapse attempt in different pmd. It only works for !NUMA so it's
>>> likely not worth all the trouble and complicated code, so I wouldn't be
>>> opposed to simplifying this.
>>>
>>>> I do not see the count increment when scan success. it save the memory,
>>>> only when page fault happen.
>>> I don't understand what you mean here?
>>>
>>>> Thanks
>>>> zhongjiang
>>>>>> ---
>>>>>> mm/khugepaged.c | 4 ++++
>>>>>> 1 file changed, 4 insertions(+)
>>>>>>
>>>>>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>>>>>> index 7cb9c88..586b1f1 100644
>>>>>> --- a/mm/khugepaged.c
>>>>>> +++ b/mm/khugepaged.c
>>>>>> @@ -1082,6 +1082,8 @@ static void collapse_huge_page(struct mm_struct *mm,
>>>>>> up_write(&mm->mmap_sem);
>>>>>> out_nolock:
>>>>>> trace_mm_collapse_huge_page(mm, isolated, result);
>>>>>> + if (page != NULL && result != SCAN_SUCCEED)
>>>>>> + put_page(new_page);
>>>>>> return;
>>>>>> out:
>>>>>> mem_cgroup_cancel_charge(new_page, memcg, true);
>>>>>> @@ -1555,6 +1557,8 @@ static void collapse_shmem(struct mm_struct *mm,
>>>>>> }
>>>>>> out:
>>>>>> VM_BUG_ON(!list_empty(&pagelist));
>>>>>> + if (page != NULL && result != SCAN_SUCCEED)
>>>>>> + put_page(new_page);
>>>>>> /* TODO: tracepoints */
>>>>>> }
>>>>>>
>>>>>>
>>>>> .
>>>>>
>>> .
>>>
>>
>
> .
>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: RFC v2: post-init-read-only protection for data allocated dynamically
From: Michal Hocko @ 2017-05-10 8:05 UTC (permalink / raw)
To: Igor Stoppa
Cc: Laura Abbott, linux-mm, linux-kernel,
kernel-hardening@lists.openwall.com
In-Reply-To: <0b55343e-4305-a9f1-2b17-51c3c734aea6@huawei.com>
On Fri 05-05-17 13:42:27, Igor Stoppa wrote:
> On 04/05/17 19:49, Laura Abbott wrote:
> > [adding kernel-hardening since I think there would be interest]
>
> thank you, I overlooked this
>
>
> > BPF takes the approach of calling set_memory_ro to mark regions as
> > read only. I'm certainly over simplifying but it sounds like this
> > is mostly a mechanism to have this happen mostly automatically.
> > Can you provide any more details about tradeoffs of the two approaches?
>
> I am not sure I understand the question ...
> For what I can understand, the bpf is marking as read only something
> that spans across various pages, which is fine.
> The payload to be protected is already organized in such pages.
>
> But in the case I have in mind, I have various, heterogeneous chunks of
> data, coming from various subsystems, not necessarily page aligned.
> And, even if they were page aligned, most likely they would be far
> smaller than a page, even a 4k page.
This aspect of various sizes makes the SLAB allocator not optimal
because it operates on caches (pools of pages) which manage objects of
the same size. You could use the maximum size of all objects and waste
some memory but you would have to know this max in advance which would
make this approach less practical. You could create more caches of
course but that still requires to know those sizes in advance.
So it smells like a dedicated allocator which operates on a pool of
pages might be a better option in the end. This depends on what you
expect from the allocator. NUMA awareness? Very effective hotpath? Very
good fragmentation avoidance? CPU cache awareness? Special alignment
requirements? Reasonable free()? Etc...
To me it seems that this being an initialization mostly thingy a simple
allocator which manages a pool of pages (one set of sealed and one for
allocations) and which only appends new objects as they fit to unsealed
pages would be sufficient for starter.
--
Michal Hocko
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: RFC v2: post-init-read-only protection for data allocated dynamically
From: Michal Hocko @ 2017-05-10 7:45 UTC (permalink / raw)
To: Igor Stoppa; +Cc: linux-mm, linux-kernel, Dave Hansen
In-Reply-To: <3e798c43-1726-ee7d-add5-762c7e17cb88@huawei.com>
On Fri 05-05-17 15:19:19, Igor Stoppa wrote:
>
>
> On 04/05/17 17:01, Michal Hocko wrote:
> > On Thu 04-05-17 16:37:55, Igor Stoppa wrote:
>
> [...]
>
> >> The disadvantage is that anything can happen, undetected, while the seal
> >> is lifted.
> >
> > Yes and I think this makes it basically pointless
>
> ok, this goes a bit beyond what I had in mind initially, but I see your
> point
>
> [...]
>
> > Just to make my proposal more clear. I suggest the following workflow
> >
> > cache = kmem_cache_create(foo, object_size, ..., SLAB_SEAL);
> >
> > obj = kmem_cache_alloc(cache, gfp_mask);
> > init_obj(obj)
> > [more allocations]
> > kmem_cache_seal(cache);
>
> In case one doesn't want the feature, at which point would it be disabled?
>
> * not creating the slab
> * not sealing it
> * something else?
If the sealing would be disabled then sealing would be a noop.
--
Michal Hocko
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v2 2/2] mm: skip HWPoisoned pages when onlining pages
From: Michal Hocko @ 2017-05-10 7:41 UTC (permalink / raw)
To: Naoya Horiguchi
Cc: Balbir Singh, Laurent Dufour, linux-kernel@vger.kernel.org,
linux-mm@kvack.org, akpm@linux-foundation.org
In-Reply-To: <20170428065131.GD8143@dhcp22.suse.cz>
On Fri 28-04-17 08:51:31, Michal Hocko wrote:
> On Fri 28-04-17 08:50:50, Michal Hocko wrote:
> > [Drop Wen Congyang because his address bounces - we will have to find
> > out ourselves...]
> > On Fri 28-04-17 08:30:48, Michal Hocko wrote:
> > > On Wed 26-04-17 03:13:04, Naoya Horiguchi wrote:
> > > > On Wed, Apr 26, 2017 at 12:10:15PM +1000, Balbir Singh wrote:
> > > > > On Tue, 2017-04-25 at 16:27 +0200, Laurent Dufour wrote:
> > > > > > The commit b023f46813cd ("memory-hotplug: skip HWPoisoned page when
> > > > > > offlining pages") skip the HWPoisoned pages when offlining pages, but
> > > > > > this should be skipped when onlining the pages too.
> > > > > >
> > > > > > Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
> > > > > > ---
> > > > > > mm/memory_hotplug.c | 4 ++++
> > > > > > 1 file changed, 4 insertions(+)
> > > > > >
> > > > > > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> > > > > > index 6fa7208bcd56..741ddb50e7d2 100644
> > > > > > --- a/mm/memory_hotplug.c
> > > > > > +++ b/mm/memory_hotplug.c
> > > > > > @@ -942,6 +942,10 @@ static int online_pages_range(unsigned long start_pfn, unsigned long nr_pages,
> > > > > > if (PageReserved(pfn_to_page(start_pfn)))
> > > > > > for (i = 0; i < nr_pages; i++) {
> > > > > > page = pfn_to_page(start_pfn + i);
> > > > > > + if (PageHWPoison(page)) {
> > > > > > + ClearPageReserved(page);
> > > > >
> > > > > Why do we clear page reserved? Also if the page is marked PageHWPoison, it
> > > > > was never offlined to begin with? Or do you expect this to be set on newly
> > > > > hotplugged memory? Also don't we need to skip the entire pageblock?
> > > >
> > > > If I read correctly, to "skip HWPoiosned page" in commit b023f46813cd means
> > > > that we skip the page status check for hwpoisoned pages *not* to prevent
> > > > memory offlining for memblocks with hwpoisoned pages. That means that
> > > > hwpoisoned pages can be offlined.
> > >
> > > Is this patch actually correct? I am trying to wrap my head around it
> > > but it smells like it tries to avoid the problem rather than fix it
> > > properly. I might be wrong here of course but to me it sounds like
> > > poisoned page should simply be offlined and keep its poison state all
> > > the time. If the memory is hot-removed and added again we have lost the
> > > struct page along with the state which is the expected behavior. If it
> > > is still broken we will re-poison it.
> > >
> > > Anyway a patch to skip over poisoned pages during online makes perfect
> > > sense to me. The PageReserved fiddling around much less so.
> > >
> > > Or am I missing something. Let's CC Wen Congyang for the clarification
> > > here.
Can we revisit this please? The PageReserved() logic for poisoned pages
is completely unclear to me. I would rather not rely on the previous
changelogs and rather build the picture from what is the expected
behavior instead.
--
Michal Hocko
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [v3 0/9] parallelized "struct page" zeroing
From: Michal Hocko @ 2017-05-10 7:24 UTC (permalink / raw)
To: Pasha Tatashin
Cc: linux-kernel, sparclinux, linux-mm, linuxppc-dev, linux-s390,
borntraeger, heiko.carstens, davem
In-Reply-To: <fae4a92c-e78c-32cb-606a-8e5087acb13f@oracle.com>
On Tue 09-05-17 14:54:50, Pasha Tatashin wrote:
[...]
> >The implementation just looks too large to what I would expect. E.g. do
> >we really need to add zero argument to the large part of the memblock
> >API? Wouldn't it be easier to simply export memblock_virt_alloc_internal
> >(or its tiny wrapper memblock_virt_alloc_core) and move the zeroing
> >outside to its 2 callers? A completely untested scratched version at the
> >end of the email.
>
> I am OK, with this change. But, I do not really see a difference between:
>
> memblock_virt_alloc_raw()
> and
> memblock_virt_alloc_core()
>
> In both cases we use memblock_virt_alloc_internal(), but the only difference
> is that in my case we tell memblock_virt_alloc_internal() to zero the pages
> if needed, and in your case the other two callers are zeroing it. I like
> moving memblock_dbg() inside memblock_virt_alloc_internal()
Well, I didn't object to this particular part. I was mostly concerned
about
http://lkml.kernel.org/r/1494003796-748672-4-git-send-email-pasha.tatashin@oracle.com
and the "zero" argument for other functions. I guess we can do without
that. I _think_ that we should simply _always_ initialize the page at the
__init_single_page time rather than during the allocation. That would
require dropping __GFP_ZERO for non-memblock allocations. Or do you
think we could regress for single threaded initialization?
> >Also it seems that this is not 100% correct either as it only cares
> >about VMEMMAP while DEFERRED_STRUCT_PAGE_INIT might be enabled also for
> >SPARSEMEM. This would suggest that we would zero out pages twice,
> >right?
>
> Thank you, I will check this combination before sending out the next patch.
>
> >
> >A similar concern would go to the memory hotplug patch which will
> >fall back to the slab/page allocator IIRC. On the other hand
> >__init_single_page is shared with the hotplug code so again we would
> >initialize 2 times.
>
> Correct, when memory it hotplugged, to gain the benefit of this fix, and
> also not to regress by actually double zeroing "struct pages" we should not
> zero it out. However, I do not really have means to test it.
It should be pretty easy to test with kvm, but I can help with testing
on the real HW as well.
Thanks!
--
Michal Hocko
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH] vmscan: scan pages until it founds eligible pages
From: Michal Hocko @ 2017-05-10 7:22 UTC (permalink / raw)
To: Minchan Kim
Cc: Johannes Weiner, Andrew Morton, Mel Gorman, kernel-team,
linux-kernel, linux-mm
In-Reply-To: <20170510070311.GA24772@bbox>
On Wed 10-05-17 16:03:11, Minchan Kim wrote:
> On Wed, May 10, 2017 at 08:13:12AM +0200, Michal Hocko wrote:
> > On Wed 10-05-17 10:46:54, Minchan Kim wrote:
> > > On Wed, May 03, 2017 at 08:00:44AM +0200, Michal Hocko wrote:
[...]
> > > > + scan++;
> > > > switch (__isolate_lru_page(page, mode)) {
> > > > case 0:
> > > > nr_pages = hpage_nr_pages(page);
> > >
> > > Confirmed.
> >
> > Hmm. I can clearly see how we could skip over too many pages and hit
> > small reclaim priorities too quickly but I am still scratching my head
> > about how we could hit the OOM killer as a result. The amount of pages
> > on the active anonymous list suggests that we are not able to rotate
> > pages quickly enough. I have to keep thinking about that.
>
> I explained it but seems to be not enouggh. Let me try again.
>
> The problem is that get_scan_count determines nr_to_scan with
> eligible zones.
>
> size = lruvec_lru_size(lruvec, lru, sc->reclaim_idx);
> size = size >> sc->priority;
Ohh, right. Who has done that ;) Now it is much more clear. We simply
reclaimed all the pages on the inactive LRU list and only very slowly
progress over active list and hit the OOM before we can actually reach
anything. I completely forgot about the scan window not being the full
LRU list.
Thanks for bearing with me!
--
Michal Hocko
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH] mm/vmscan: fix unsequenced modification and access warning
From: Michal Hocko @ 2017-05-10 7:15 UTC (permalink / raw)
To: Nick Desaulniers
Cc: akpm, hannes, mgorman, vbabka, minchan, linux-mm, linux-kernel
In-Reply-To: <20170510065328.9215-1-nick.desaulniers@gmail.com>
On Tue 09-05-17 23:53:28, Nick Desaulniers wrote:
> Clang flags this file with the -Wunsequenced error that GCC does not
> have.
>
> unsequenced modification and access to 'gfp_mask'
>
> It seems that gfp_mask is both read and written without a sequence point
> in between, which is undefined behavior.
Hmm. This is rather news to me. I thought that a = foo(a) is perfectly
valid. Same as a = b = c where c = foo(b) or is the problem in the
following .reclaim_idx = gfp_zone(gfp_mask) initialization? If that is
the case then the current code is OKish because gfp_zone doesn't depend
on the gfp_mask modification. It is messy, right, but works as expected.
Anyway, we have a similar construct __node_reclaim
If you really want to change this code, and I would agree it would be
slightly less tricky, then I would suggest doing something like the
following instead
---
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 5ebf468c5429..ba4b695e810e 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -2965,7 +2965,7 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
unsigned long nr_reclaimed;
struct scan_control sc = {
.nr_to_reclaim = SWAP_CLUSTER_MAX,
- .gfp_mask = (gfp_mask = current_gfp_context(gfp_mask)),
+ .gfp_mask = current_gfp_context(gfp_mask),
.reclaim_idx = gfp_zone(gfp_mask),
.order = order,
.nodemask = nodemask,
@@ -2980,12 +2980,12 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
* 1 is returned so that the page allocator does not OOM kill at this
* point.
*/
- if (throttle_direct_reclaim(gfp_mask, zonelist, nodemask))
+ if (throttle_direct_reclaim(sc.gfp_mask, zonelist, nodemask))
return 1;
trace_mm_vmscan_direct_reclaim_begin(order,
sc.may_writepage,
- gfp_mask,
+ sc.gfp_mask,
sc.reclaim_idx);
nr_reclaimed = do_try_to_free_pages(zonelist, &sc);
@@ -3772,17 +3772,16 @@ static int __node_reclaim(struct pglist_data *pgdat, gfp_t gfp_mask, unsigned in
const unsigned long nr_pages = 1 << order;
struct task_struct *p = current;
struct reclaim_state reclaim_state;
- int classzone_idx = gfp_zone(gfp_mask);
unsigned int noreclaim_flag;
struct scan_control sc = {
.nr_to_reclaim = max(nr_pages, SWAP_CLUSTER_MAX),
- .gfp_mask = (gfp_mask = current_gfp_context(gfp_mask)),
+ .gfp_mask = current_gfp_context(gfp_mask),
.order = order,
.priority = NODE_RECLAIM_PRIORITY,
.may_writepage = !!(node_reclaim_mode & RECLAIM_WRITE),
.may_unmap = !!(node_reclaim_mode & RECLAIM_UNMAP),
.may_swap = 1,
- .reclaim_idx = classzone_idx,
+ .reclaim_idx = gfp_znoe(gfp_mask),
};
cond_resched();
@@ -3793,7 +3792,7 @@ static int __node_reclaim(struct pglist_data *pgdat, gfp_t gfp_mask, unsigned in
*/
noreclaim_flag = memalloc_noreclaim_save();
p->flags |= PF_SWAPWRITE;
- lockdep_set_current_reclaim_state(gfp_mask);
+ lockdep_set_current_reclaim_state(sc.gfp_mask);
reclaim_state.reclaimed_slab = 0;
p->reclaim_state = &reclaim_state;
--
Michal Hocko
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related
* Re: [PATCH] vmscan: scan pages until it founds eligible pages
From: Minchan Kim @ 2017-05-10 7:03 UTC (permalink / raw)
To: Michal Hocko
Cc: Johannes Weiner, Andrew Morton, Mel Gorman, kernel-team,
linux-kernel, linux-mm
In-Reply-To: <20170510061312.GB26158@dhcp22.suse.cz>
On Wed, May 10, 2017 at 08:13:12AM +0200, Michal Hocko wrote:
> On Wed 10-05-17 10:46:54, Minchan Kim wrote:
> > On Wed, May 03, 2017 at 08:00:44AM +0200, Michal Hocko wrote:
> [...]
> > > @@ -1486,6 +1486,12 @@ static unsigned long isolate_lru_pages(unsigned long nr_to_scan,
> > > continue;
> > > }
> > >
> > > + /*
> > > + * Do not count skipped pages because we do want to isolate
> > > + * some pages even when the LRU mostly contains ineligible
> > > + * pages
> > > + */
> >
> > How about adding comment about "why"?
> >
> > /*
> > * Do not count skipped pages because it makes the function to return with
> > * none isolated pages if the LRU mostly contains inelgible pages so that
> > * VM cannot reclaim any pages and trigger premature OOM.
> > */
>
> I am not sure this is necessarily any better. Mentioning a pre-mature
> OOM would require a much better explanation because a first immediate
> question would be "why don't we scan those pages at priority 0". Also
> decision about the OOM is at a different layer and it might change in
> future when this doesn't apply any more. But it is not like I would
> insist...
>
> > > + scan++;
> > > switch (__isolate_lru_page(page, mode)) {
> > > case 0:
> > > nr_pages = hpage_nr_pages(page);
> >
> > Confirmed.
>
> Hmm. I can clearly see how we could skip over too many pages and hit
> small reclaim priorities too quickly but I am still scratching my head
> about how we could hit the OOM killer as a result. The amount of pages
> on the active anonymous list suggests that we are not able to rotate
> pages quickly enough. I have to keep thinking about that.
I explained it but seems to be not enouggh. Let me try again.
The problem is that get_scan_count determines nr_to_scan with
eligible zones.
size = lruvec_lru_size(lruvec, lru, sc->reclaim_idx);
size = size >> sc->priority;
Assumes sc->priority is 0 and LRU list is as follows.
N-N-N-N-H-H-H-H-H-H-H-H-H-H-H-H-H-H-H-H
(Ie, small eligible pages are in the head of LRU but others are
almost ineligible pages)
In that case, size becomes 4 so VM want to scan 4 pages but 4 pages
from tail of the LRU are not eligible pages.
If get_scan_count counts skipped pages, it doesn't reclaim remained
pages after scanning 4 pages.
If it's more helpful to understand the problem, I will add it to
the description.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* [PATCH] mm/vmscan: fix unsequenced modification and access warning
From: Nick Desaulniers @ 2017-05-10 6:53 UTC (permalink / raw)
Cc: akpm, hannes, mgorman, mhocko, vbabka, minchan, linux-mm,
linux-kernel, Nick Desaulniers
Clang flags this file with the -Wunsequenced error that GCC does not
have.
unsequenced modification and access to 'gfp_mask'
It seems that gfp_mask is both read and written without a sequence point
in between, which is undefined behavior.
Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
---
mm/vmscan.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 4e7ed65842af..74785908822c 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -2958,7 +2958,7 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
unsigned long nr_reclaimed;
struct scan_control sc = {
.nr_to_reclaim = SWAP_CLUSTER_MAX,
- .gfp_mask = (gfp_mask = current_gfp_context(gfp_mask)),
+ .gfp_mask = current_gfp_context(gfp_mask),
.reclaim_idx = gfp_zone(gfp_mask),
.order = order,
.nodemask = nodemask,
@@ -2968,6 +2968,8 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
.may_swap = 1,
};
+ gfp_mask = sc.gfp_mask;
+
/*
* Do not enter reclaim if fatal signal was delivered while throttled.
* 1 is returned so that the page allocator does not OOM kill at this
--
2.11.0
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related
* Re: [PATCH v2] mm: fix the memory leak after collapsing the huge page fails
From: Vlastimil Babka @ 2017-05-10 6:51 UTC (permalink / raw)
To: zhong jiang; +Cc: akpm, kirill.shutemov, hannes, mgorman, linux-mm
In-Reply-To: <5911C9F9.3020802@huawei.com>
On 05/09/2017 03:54 PM, zhong jiang wrote:
> Hi, Vlastimil
>
> I review the code again. it works well for NUMA. because
> khugepaged_prealloc_page will put_page when *hpage is true.
>
> the memory leak will still exist in !NUMA. because it ingore
> the put_page. is it right? I miss something.
No, on !NUMA the preallocated and unused new_page is freed by put_page()
at the very end of khugepaged_do_scan().
> Thanks
> zhongjiang
>
> On 2017/5/9 20:41, Vlastimil Babka wrote:
>> On 05/09/2017 02:20 PM, zhong jiang wrote:
>>> On 2017/5/9 19:34, Vlastimil Babka wrote:
>>>> On 05/09/2017 12:55 PM, zhongjiang wrote:
>>>>> From: zhong jiang <zhongjiang@huawei.com>
>>>>>
>>>>> Current, when we prepare a huge page to collapse, due to some
>>>>> reasons, it can fail to collapse. At the moment, we should
>>>>> release the preallocate huge page.
>>>>>
>>>>> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
>>>> Hmm, scratch that, there's no memory leak. The pointer to new_page is
>>>> stored in *hpage, and put_page() is called all the way up in
>>>> khugepaged_do_scan().
>>> I see. I miss it. but why the new_page need to be release all the way.
>> AFAIK to support preallocation and reusal of preallocated page for
>> collapse attempt in different pmd. It only works for !NUMA so it's
>> likely not worth all the trouble and complicated code, so I wouldn't be
>> opposed to simplifying this.
>>
>>> I do not see the count increment when scan success. it save the memory,
>>> only when page fault happen.
>> I don't understand what you mean here?
>>
>>> Thanks
>>> zhongjiang
>>>>> ---
>>>>> mm/khugepaged.c | 4 ++++
>>>>> 1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>>>>> index 7cb9c88..586b1f1 100644
>>>>> --- a/mm/khugepaged.c
>>>>> +++ b/mm/khugepaged.c
>>>>> @@ -1082,6 +1082,8 @@ static void collapse_huge_page(struct mm_struct *mm,
>>>>> up_write(&mm->mmap_sem);
>>>>> out_nolock:
>>>>> trace_mm_collapse_huge_page(mm, isolated, result);
>>>>> + if (page != NULL && result != SCAN_SUCCEED)
>>>>> + put_page(new_page);
>>>>> return;
>>>>> out:
>>>>> mem_cgroup_cancel_charge(new_page, memcg, true);
>>>>> @@ -1555,6 +1557,8 @@ static void collapse_shmem(struct mm_struct *mm,
>>>>> }
>>>>> out:
>>>>> VM_BUG_ON(!list_empty(&pagelist));
>>>>> + if (page != NULL && result != SCAN_SUCCEED)
>>>>> + put_page(new_page);
>>>>> /* TODO: tracepoints */
>>>>> }
>>>>>
>>>>>
>>>> .
>>>>
>>>
>>
>> .
>>
>
>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox