diff mbox series

[v2,5/9] mm: Initialize struct vm_unmapped_area_info

Message ID 20240226190951.3240433-6-rick.p.edgecombe@intel.com
State New
Headers show
Series [RFC,v2.1,01/12] ARC: Use initializer for struct vm_unmapped_area_info | expand

Commit Message

Edgecombe, Rick P Feb. 26, 2024, 7:09 p.m. UTC
Future changes will need to add a field to struct vm_unmapped_area_info.
This would cause trouble for any archs that don't initialize the
struct. Currently every user sets each field, so if new fields are
added, the core code parsing the struct will see garbage in the new
field.

It could be possible to initialize the new field for each arch to 0, but
instead simply inialize the field with a C99 struct inializing syntax.

Cc: linux-mm@kvack.org
Cc: linux-alpha@vger.kernel.org
Cc: linux-snps-arc@lists.infradead.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-csky@vger.kernel.org
Cc: loongarch@lists.linux.dev
Cc: linux-mips@vger.kernel.org
Cc: linux-parisc@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-s390@vger.kernel.org
Cc: linux-sh@vger.kernel.org
Cc: sparclinux@vger.kernel.org
Cc: x86@kernel.org
Suggested-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
Link: https://lore.kernel.org/lkml/3ynogxcgokc6i6xojbxzzwqectg472laes24u7jmtktlxcch5e@dfytra3ia3zc/#t
---
Hi archs,

For some context, this is part of a larger series to improve shadow stack
guard gaps. It involves plumbing a new field via
struct vm_unmapped_area_info. The first user is x86, but arm and riscv may
likely use it as well. The change is compile tested only for non-x86 but
seems like a relatively safe one.

Thanks,

Rick

v2:
 - New patch
---
 arch/alpha/kernel/osf_sys.c      | 2 +-
 arch/arc/mm/mmap.c               | 2 +-
 arch/arm/mm/mmap.c               | 4 ++--
 arch/csky/abiv1/mmap.c           | 2 +-
 arch/loongarch/mm/mmap.c         | 2 +-
 arch/mips/mm/mmap.c              | 2 +-
 arch/parisc/kernel/sys_parisc.c  | 2 +-
 arch/powerpc/mm/book3s64/slice.c | 4 ++--
 arch/s390/mm/hugetlbpage.c       | 4 ++--
 arch/s390/mm/mmap.c              | 4 ++--
 arch/sh/mm/mmap.c                | 4 ++--
 arch/sparc/kernel/sys_sparc_32.c | 2 +-
 arch/sparc/kernel/sys_sparc_64.c | 4 ++--
 arch/sparc/mm/hugetlbpage.c      | 4 ++--
 arch/x86/kernel/sys_x86_64.c     | 4 ++--
 arch/x86/mm/hugetlbpage.c        | 4 ++--
 fs/hugetlbfs/inode.c             | 4 ++--
 mm/mmap.c                        | 4 ++--
 18 files changed, 29 insertions(+), 29 deletions(-)

Comments

Christophe Leroy Feb. 27, 2024, 7:02 a.m. UTC | #1
Le 26/02/2024 à 20:09, Rick Edgecombe a écrit :
> Future changes will need to add a field to struct vm_unmapped_area_info.
> This would cause trouble for any archs that don't initialize the
> struct. Currently every user sets each field, so if new fields are
> added, the core code parsing the struct will see garbage in the new
> field.
> 
> It could be possible to initialize the new field for each arch to 0, but
> instead simply inialize the field with a C99 struct inializing syntax.

Why doing a full init of the struct when all fields are re-written a few 
lines after ?

If I take the exemple of powerpc function slice_find_area_bottomup():

	struct vm_unmapped_area_info info;

	info.flags = 0;
	info.length = len;
	info.align_mask = PAGE_MASK & ((1ul << pshift) - 1);
	info.align_offset = 0;

For me it looks better to just add:

	info.new_field = 0; /* or whatever value it needs to have */

Christophe


> 
> Cc: linux-mm@kvack.org
> Cc: linux-alpha@vger.kernel.org
> Cc: linux-snps-arc@lists.infradead.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-csky@vger.kernel.org
> Cc: loongarch@lists.linux.dev
> Cc: linux-mips@vger.kernel.org
> Cc: linux-parisc@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: linux-s390@vger.kernel.org
> Cc: linux-sh@vger.kernel.org
> Cc: sparclinux@vger.kernel.org
> Cc: x86@kernel.org
> Suggested-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> Link: https://lore.kernel.org/lkml/3ynogxcgokc6i6xojbxzzwqectg472laes24u7jmtktlxcch5e@dfytra3ia3zc/#t
> ---
> Hi archs,
> 
> For some context, this is part of a larger series to improve shadow stack
> guard gaps. It involves plumbing a new field via
> struct vm_unmapped_area_info. The first user is x86, but arm and riscv may
> likely use it as well. The change is compile tested only for non-x86 but
> seems like a relatively safe one.
> 
> Thanks,
> 
> Rick
> 
> v2:
>   - New patch
> ---
>   arch/alpha/kernel/osf_sys.c      | 2 +-
>   arch/arc/mm/mmap.c               | 2 +-
>   arch/arm/mm/mmap.c               | 4 ++--
>   arch/csky/abiv1/mmap.c           | 2 +-
>   arch/loongarch/mm/mmap.c         | 2 +-
>   arch/mips/mm/mmap.c              | 2 +-
>   arch/parisc/kernel/sys_parisc.c  | 2 +-
>   arch/powerpc/mm/book3s64/slice.c | 4 ++--
>   arch/s390/mm/hugetlbpage.c       | 4 ++--
>   arch/s390/mm/mmap.c              | 4 ++--
>   arch/sh/mm/mmap.c                | 4 ++--
>   arch/sparc/kernel/sys_sparc_32.c | 2 +-
>   arch/sparc/kernel/sys_sparc_64.c | 4 ++--
>   arch/sparc/mm/hugetlbpage.c      | 4 ++--
>   arch/x86/kernel/sys_x86_64.c     | 4 ++--
>   arch/x86/mm/hugetlbpage.c        | 4 ++--
>   fs/hugetlbfs/inode.c             | 4 ++--
>   mm/mmap.c                        | 4 ++--
>   18 files changed, 29 insertions(+), 29 deletions(-)
> 
> diff --git a/arch/alpha/kernel/osf_sys.c b/arch/alpha/kernel/osf_sys.c
> index 5db88b627439..dd6801bb9240 100644
> --- a/arch/alpha/kernel/osf_sys.c
> +++ b/arch/alpha/kernel/osf_sys.c
> @@ -1218,7 +1218,7 @@ static unsigned long
>   arch_get_unmapped_area_1(unsigned long addr, unsigned long len,
>   		         unsigned long limit)
>   {
> -	struct vm_unmapped_area_info info;
> +	struct vm_unmapped_area_info info = {};
>   
>   	info.flags = 0;
>   	info.length = len;
> diff --git a/arch/arc/mm/mmap.c b/arch/arc/mm/mmap.c
> index 3c1c7ae73292..6549b3375f54 100644
> --- a/arch/arc/mm/mmap.c
> +++ b/arch/arc/mm/mmap.c
> @@ -27,7 +27,7 @@ arch_get_unmapped_area(struct file *filp, unsigned long addr,
>   {
>   	struct mm_struct *mm = current->mm;
>   	struct vm_area_struct *vma;
> -	struct vm_unmapped_area_info info;
> +	struct vm_unmapped_area_info info = {};
>   
>   	/*
>   	 * We enforce the MAP_FIXED case.
> diff --git a/arch/arm/mm/mmap.c b/arch/arm/mm/mmap.c
> index a0f8a0ca0788..525795578c29 100644
> --- a/arch/arm/mm/mmap.c
> +++ b/arch/arm/mm/mmap.c
> @@ -34,7 +34,7 @@ arch_get_unmapped_area(struct file *filp, unsigned long addr,
>   	struct vm_area_struct *vma;
>   	int do_align = 0;
>   	int aliasing = cache_is_vipt_aliasing();
> -	struct vm_unmapped_area_info info;
> +	struct vm_unmapped_area_info info = {};
>   
>   	/*
>   	 * We only need to do colour alignment if either the I or D
> @@ -87,7 +87,7 @@ arch_get_unmapped_area_topdown(struct file *filp, const unsigned long addr0,
>   	unsigned long addr = addr0;
>   	int do_align = 0;
>   	int aliasing = cache_is_vipt_aliasing();
> -	struct vm_unmapped_area_info info;
> +	struct vm_unmapped_area_info info = {};
>   
>   	/*
>   	 * We only need to do colour alignment if either the I or D
> diff --git a/arch/csky/abiv1/mmap.c b/arch/csky/abiv1/mmap.c
> index 6792aca49999..726659d41fa9 100644
> --- a/arch/csky/abiv1/mmap.c
> +++ b/arch/csky/abiv1/mmap.c
> @@ -28,7 +28,7 @@ arch_get_unmapped_area(struct file *filp, unsigned long addr,
>   	struct mm_struct *mm = current->mm;
>   	struct vm_area_struct *vma;
>   	int do_align = 0;
> -	struct vm_unmapped_area_info info;
> +	struct vm_unmapped_area_info info = {};
>   
>   	/*
>   	 * We only need to do colour alignment if either the I or D
> diff --git a/arch/loongarch/mm/mmap.c b/arch/loongarch/mm/mmap.c
> index a9630a81b38a..664bf4abfdcf 100644
> --- a/arch/loongarch/mm/mmap.c
> +++ b/arch/loongarch/mm/mmap.c
> @@ -24,7 +24,7 @@ static unsigned long arch_get_unmapped_area_common(struct file *filp,
>   	struct vm_area_struct *vma;
>   	unsigned long addr = addr0;
>   	int do_color_align;
> -	struct vm_unmapped_area_info info;
> +	struct vm_unmapped_area_info info = {};
>   
>   	if (unlikely(len > TASK_SIZE))
>   		return -ENOMEM;
> diff --git a/arch/mips/mm/mmap.c b/arch/mips/mm/mmap.c
> index 00fe90c6db3e..6321b53dc995 100644
> --- a/arch/mips/mm/mmap.c
> +++ b/arch/mips/mm/mmap.c
> @@ -34,7 +34,7 @@ static unsigned long arch_get_unmapped_area_common(struct file *filp,
>   	struct vm_area_struct *vma;
>   	unsigned long addr = addr0;
>   	int do_color_align;
> -	struct vm_unmapped_area_info info;
> +	struct vm_unmapped_area_info info = {};
>   
>   	if (unlikely(len > TASK_SIZE))
>   		return -ENOMEM;
> diff --git a/arch/parisc/kernel/sys_parisc.c b/arch/parisc/kernel/sys_parisc.c
> index 98af719d5f85..e87c0e325abf 100644
> --- a/arch/parisc/kernel/sys_parisc.c
> +++ b/arch/parisc/kernel/sys_parisc.c
> @@ -104,7 +104,7 @@ static unsigned long arch_get_unmapped_area_common(struct file *filp,
>   	struct vm_area_struct *vma, *prev;
>   	unsigned long filp_pgoff;
>   	int do_color_align;
> -	struct vm_unmapped_area_info info;
> +	struct vm_unmapped_area_info info = {};
>   
>   	if (unlikely(len > TASK_SIZE))
>   		return -ENOMEM;
> diff --git a/arch/powerpc/mm/book3s64/slice.c b/arch/powerpc/mm/book3s64/slice.c
> index c0b58afb9a47..5884f384866f 100644
> --- a/arch/powerpc/mm/book3s64/slice.c
> +++ b/arch/powerpc/mm/book3s64/slice.c
> @@ -282,7 +282,7 @@ static unsigned long slice_find_area_bottomup(struct mm_struct *mm,
>   {
>   	int pshift = max_t(int, mmu_psize_defs[psize].shift, PAGE_SHIFT);
>   	unsigned long found, next_end;
> -	struct vm_unmapped_area_info info;
> +	struct vm_unmapped_area_info info = {};
>   
>   	info.flags = 0;
>   	info.length = len;
> @@ -326,7 +326,7 @@ static unsigned long slice_find_area_topdown(struct mm_struct *mm,
>   {
>   	int pshift = max_t(int, mmu_psize_defs[psize].shift, PAGE_SHIFT);
>   	unsigned long found, prev;
> -	struct vm_unmapped_area_info info;
> +	struct vm_unmapped_area_info info = {};
>   	unsigned long min_addr = max(PAGE_SIZE, mmap_min_addr);
>   
>   	info.flags = VM_UNMAPPED_AREA_TOPDOWN;
> diff --git a/arch/s390/mm/hugetlbpage.c b/arch/s390/mm/hugetlbpage.c
> index c2d2850ec8d5..7f68485feea0 100644
> --- a/arch/s390/mm/hugetlbpage.c
> +++ b/arch/s390/mm/hugetlbpage.c
> @@ -258,7 +258,7 @@ static unsigned long hugetlb_get_unmapped_area_bottomup(struct file *file,
>   		unsigned long pgoff, unsigned long flags)
>   {
>   	struct hstate *h = hstate_file(file);
> -	struct vm_unmapped_area_info info;
> +	struct vm_unmapped_area_info info = {};
>   
>   	info.flags = 0;
>   	info.length = len;
> @@ -274,7 +274,7 @@ static unsigned long hugetlb_get_unmapped_area_topdown(struct file *file,
>   		unsigned long pgoff, unsigned long flags)
>   {
>   	struct hstate *h = hstate_file(file);
> -	struct vm_unmapped_area_info info;
> +	struct vm_unmapped_area_info info = {};
>   	unsigned long addr;
>   
>   	info.flags = VM_UNMAPPED_AREA_TOPDOWN;
> diff --git a/arch/s390/mm/mmap.c b/arch/s390/mm/mmap.c
> index cd52d72b59cf..df88496e2903 100644
> --- a/arch/s390/mm/mmap.c
> +++ b/arch/s390/mm/mmap.c
> @@ -77,7 +77,7 @@ unsigned long arch_get_unmapped_area(struct file *filp, unsigned long addr,
>   {
>   	struct mm_struct *mm = current->mm;
>   	struct vm_area_struct *vma;
> -	struct vm_unmapped_area_info info;
> +	struct vm_unmapped_area_info info = {};
>   
>   	if (len > TASK_SIZE - mmap_min_addr)
>   		return -ENOMEM;
> @@ -116,7 +116,7 @@ unsigned long arch_get_unmapped_area_topdown(struct file *filp, unsigned long ad
>   {
>   	struct vm_area_struct *vma;
>   	struct mm_struct *mm = current->mm;
> -	struct vm_unmapped_area_info info;
> +	struct vm_unmapped_area_info info = {};
>   
>   	/* requested length too big for entire address space */
>   	if (len > TASK_SIZE - mmap_min_addr)
> diff --git a/arch/sh/mm/mmap.c b/arch/sh/mm/mmap.c
> index b82199878b45..6aee5f761e08 100644
> --- a/arch/sh/mm/mmap.c
> +++ b/arch/sh/mm/mmap.c
> @@ -57,7 +57,7 @@ unsigned long arch_get_unmapped_area(struct file *filp, unsigned long addr,
>   	struct mm_struct *mm = current->mm;
>   	struct vm_area_struct *vma;
>   	int do_colour_align;
> -	struct vm_unmapped_area_info info;
> +	struct vm_unmapped_area_info info = {};
>   
>   	if (flags & MAP_FIXED) {
>   		/* We do not accept a shared mapping if it would violate
> @@ -106,7 +106,7 @@ arch_get_unmapped_area_topdown(struct file *filp, const unsigned long addr0,
>   	struct mm_struct *mm = current->mm;
>   	unsigned long addr = addr0;
>   	int do_colour_align;
> -	struct vm_unmapped_area_info info;
> +	struct vm_unmapped_area_info info = {};
>   
>   	if (flags & MAP_FIXED) {
>   		/* We do not accept a shared mapping if it would violate
> diff --git a/arch/sparc/kernel/sys_sparc_32.c b/arch/sparc/kernel/sys_sparc_32.c
> index 082a551897ed..7e781dbfd052 100644
> --- a/arch/sparc/kernel/sys_sparc_32.c
> +++ b/arch/sparc/kernel/sys_sparc_32.c
> @@ -41,7 +41,7 @@ SYSCALL_DEFINE0(getpagesize)
>   
>   unsigned long arch_get_unmapped_area(struct file *filp, unsigned long addr, unsigned long len, unsigned long pgoff, unsigned long flags)
>   {
> -	struct vm_unmapped_area_info info;
> +	struct vm_unmapped_area_info info = {};
>   
>   	if (flags & MAP_FIXED) {
>   		/* We do not accept a shared mapping if it would violate
> diff --git a/arch/sparc/kernel/sys_sparc_64.c b/arch/sparc/kernel/sys_sparc_64.c
> index 1dbf7211666e..fc48ab3f83af 100644
> --- a/arch/sparc/kernel/sys_sparc_64.c
> +++ b/arch/sparc/kernel/sys_sparc_64.c
> @@ -93,7 +93,7 @@ unsigned long arch_get_unmapped_area(struct file *filp, unsigned long addr, unsi
>   	struct vm_area_struct * vma;
>   	unsigned long task_size = TASK_SIZE;
>   	int do_color_align;
> -	struct vm_unmapped_area_info info;
> +	struct vm_unmapped_area_info info = {};
>   
>   	if (flags & MAP_FIXED) {
>   		/* We do not accept a shared mapping if it would violate
> @@ -154,7 +154,7 @@ arch_get_unmapped_area_topdown(struct file *filp, const unsigned long addr0,
>   	unsigned long task_size = STACK_TOP32;
>   	unsigned long addr = addr0;
>   	int do_color_align;
> -	struct vm_unmapped_area_info info;
> +	struct vm_unmapped_area_info info = {};
>   
>   	/* This should only ever run for 32-bit processes.  */
>   	BUG_ON(!test_thread_flag(TIF_32BIT));
> diff --git a/arch/sparc/mm/hugetlbpage.c b/arch/sparc/mm/hugetlbpage.c
> index 38a1bef47efb..614e2c46d781 100644
> --- a/arch/sparc/mm/hugetlbpage.c
> +++ b/arch/sparc/mm/hugetlbpage.c
> @@ -31,7 +31,7 @@ static unsigned long hugetlb_get_unmapped_area_bottomup(struct file *filp,
>   {
>   	struct hstate *h = hstate_file(filp);
>   	unsigned long task_size = TASK_SIZE;
> -	struct vm_unmapped_area_info info;
> +	struct vm_unmapped_area_info info = {};
>   
>   	if (test_thread_flag(TIF_32BIT))
>   		task_size = STACK_TOP32;
> @@ -63,7 +63,7 @@ hugetlb_get_unmapped_area_topdown(struct file *filp, const unsigned long addr0,
>   	struct hstate *h = hstate_file(filp);
>   	struct mm_struct *mm = current->mm;
>   	unsigned long addr = addr0;
> -	struct vm_unmapped_area_info info;
> +	struct vm_unmapped_area_info info = {};
>   
>   	/* This should only ever run for 32-bit processes.  */
>   	BUG_ON(!test_thread_flag(TIF_32BIT));
> diff --git a/arch/x86/kernel/sys_x86_64.c b/arch/x86/kernel/sys_x86_64.c
> index c783aeb37dce..6e5d4fa5fc42 100644
> --- a/arch/x86/kernel/sys_x86_64.c
> +++ b/arch/x86/kernel/sys_x86_64.c
> @@ -125,7 +125,7 @@ arch_get_unmapped_area(struct file *filp, unsigned long addr,
>   {
>   	struct mm_struct *mm = current->mm;
>   	struct vm_area_struct *vma;
> -	struct vm_unmapped_area_info info;
> +	struct vm_unmapped_area_info info = {};
>   	unsigned long begin, end;
>   
>   	if (flags & MAP_FIXED)
> @@ -165,7 +165,7 @@ arch_get_unmapped_area_topdown(struct file *filp, const unsigned long addr0,
>   	struct vm_area_struct *vma;
>   	struct mm_struct *mm = current->mm;
>   	unsigned long addr = addr0;
> -	struct vm_unmapped_area_info info;
> +	struct vm_unmapped_area_info info = {};
>   
>   	/* requested length too big for entire address space */
>   	if (len > TASK_SIZE)
> diff --git a/arch/x86/mm/hugetlbpage.c b/arch/x86/mm/hugetlbpage.c
> index 6d77c0039617..88726bd1f72d 100644
> --- a/arch/x86/mm/hugetlbpage.c
> +++ b/arch/x86/mm/hugetlbpage.c
> @@ -51,7 +51,7 @@ static unsigned long hugetlb_get_unmapped_area_bottomup(struct file *file,
>   		unsigned long pgoff, unsigned long flags)
>   {
>   	struct hstate *h = hstate_file(file);
> -	struct vm_unmapped_area_info info;
> +	struct vm_unmapped_area_info info = {};
>   
>   	info.flags = 0;
>   	info.length = len;
> @@ -74,7 +74,7 @@ static unsigned long hugetlb_get_unmapped_area_topdown(struct file *file,
>   		unsigned long pgoff, unsigned long flags)
>   {
>   	struct hstate *h = hstate_file(file);
> -	struct vm_unmapped_area_info info;
> +	struct vm_unmapped_area_info info = {};
>   
>   	info.flags = VM_UNMAPPED_AREA_TOPDOWN;
>   	info.length = len;
> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> index a63d2eee086f..848b2239a215 100644
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -165,7 +165,7 @@ hugetlb_get_unmapped_area_bottomup(struct file *file, unsigned long addr,
>   		unsigned long len, unsigned long pgoff, unsigned long flags)
>   {
>   	struct hstate *h = hstate_file(file);
> -	struct vm_unmapped_area_info info;
> +	struct vm_unmapped_area_info info = {};
>   
>   	info.flags = 0;
>   	info.length = len;
> @@ -181,7 +181,7 @@ hugetlb_get_unmapped_area_topdown(struct file *file, unsigned long addr,
>   		unsigned long len, unsigned long pgoff, unsigned long flags)
>   {
>   	struct hstate *h = hstate_file(file);
> -	struct vm_unmapped_area_info info;
> +	struct vm_unmapped_area_info info = {};
>   
>   	info.flags = VM_UNMAPPED_AREA_TOPDOWN;
>   	info.length = len;
> diff --git a/mm/mmap.c b/mm/mmap.c
> index e02bb17fef5b..33af683a643f 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1699,7 +1699,7 @@ generic_get_unmapped_area(struct file *filp, unsigned long addr,
>   {
>   	struct mm_struct *mm = current->mm;
>   	struct vm_area_struct *vma, *prev;
> -	struct vm_unmapped_area_info info;
> +	struct vm_unmapped_area_info info = {};
>   	const unsigned long mmap_end = arch_get_mmap_end(addr, len, flags);
>   
>   	if (len > mmap_end - mmap_min_addr)
> @@ -1747,7 +1747,7 @@ generic_get_unmapped_area_topdown(struct file *filp, unsigned long addr,
>   {
>   	struct vm_area_struct *vma, *prev;
>   	struct mm_struct *mm = current->mm;
> -	struct vm_unmapped_area_info info;
> +	struct vm_unmapped_area_info info = {};
>   	const unsigned long mmap_end = arch_get_mmap_end(addr, len, flags);
>   
>   	/* requested length too big for entire address space */
Edgecombe, Rick P Feb. 27, 2024, 3 p.m. UTC | #2
On Tue, 2024-02-27 at 07:02 +0000, Christophe Leroy wrote:
> > It could be possible to initialize the new field for each arch to
> > 0, but
> > instead simply inialize the field with a C99 struct inializing
> > syntax.
> 
> Why doing a full init of the struct when all fields are re-written a
> few 
> lines after ?
> 
> If I take the exemple of powerpc function slice_find_area_bottomup():
> 
>         struct vm_unmapped_area_info info;
> 
>         info.flags = 0;
>         info.length = len;
>         info.align_mask = PAGE_MASK & ((1ul << pshift) - 1);
>         info.align_offset = 0;
> 
> For me it looks better to just add:
> 
>         info.new_field = 0; /* or whatever value it needs to have */

Hi,

Thanks for taking a look. Yes, I guess that should have some
justification. I was thinking of two reasons:
1. No future additions of optional parameters would need to make tree
wide changes like this.
2. The change is easier to review and get correct because the necessary
context is within a single line. For example, in that function some of
members are set within a while loop. The place you pointed seems to be
the correct one, but a diff that had the new field set after:
   info.high_limit = addr;
...would look correct too, but not be.

What is the concern with C99 initialization? FWIW, the full series also
removes an indirect branch, and probably is a net win for performance
in this path.
Kees Cook Feb. 27, 2024, 6:07 p.m. UTC | #3
On Tue, Feb 27, 2024 at 07:02:59AM +0000, Christophe Leroy wrote:
> 
> 
> Le 26/02/2024 à 20:09, Rick Edgecombe a écrit :
> > Future changes will need to add a field to struct vm_unmapped_area_info.
> > This would cause trouble for any archs that don't initialize the
> > struct. Currently every user sets each field, so if new fields are
> > added, the core code parsing the struct will see garbage in the new
> > field.
> > 
> > It could be possible to initialize the new field for each arch to 0, but
> > instead simply inialize the field with a C99 struct inializing syntax.
> 
> Why doing a full init of the struct when all fields are re-written a few 
> lines after ?

It's a nice change for robustness and makes future changes easier. It's
not actually wasteful since the compiler will throw away all redundant
stores.

> If I take the exemple of powerpc function slice_find_area_bottomup():
> 
> 	struct vm_unmapped_area_info info;
> 
> 	info.flags = 0;
> 	info.length = len;
> 	info.align_mask = PAGE_MASK & ((1ul << pshift) - 1);
> 	info.align_offset = 0;

But one cleanup that is possible from explicitly zero-initializing the
whole structure would be dropping all the individual "= 0" assignments.
:)
Christophe Leroy Feb. 27, 2024, 6:16 p.m. UTC | #4
Le 27/02/2024 à 19:07, Kees Cook a écrit :
> On Tue, Feb 27, 2024 at 07:02:59AM +0000, Christophe Leroy wrote:
>>
>>
>> Le 26/02/2024 à 20:09, Rick Edgecombe a écrit :
>>> Future changes will need to add a field to struct vm_unmapped_area_info.
>>> This would cause trouble for any archs that don't initialize the
>>> struct. Currently every user sets each field, so if new fields are
>>> added, the core code parsing the struct will see garbage in the new
>>> field.
>>>
>>> It could be possible to initialize the new field for each arch to 0, but
>>> instead simply inialize the field with a C99 struct inializing syntax.
>>
>> Why doing a full init of the struct when all fields are re-written a few
>> lines after ?
> 
> It's a nice change for robustness and makes future changes easier. It's
> not actually wasteful since the compiler will throw away all redundant
> stores.

Well, I tend to dislike default init at declaration because it often 
hides missed real init. When a field is not initialized GCC should emit 
a Warning, at least when built with W=2 which sets 
-Wmissing-field-initializers ?

> 
>> If I take the exemple of powerpc function slice_find_area_bottomup():
>>
>> 	struct vm_unmapped_area_info info;
>>
>> 	info.flags = 0;
>> 	info.length = len;
>> 	info.align_mask = PAGE_MASK & ((1ul << pshift) - 1);
>> 	info.align_offset = 0;
> 
> But one cleanup that is possible from explicitly zero-initializing the
> whole structure would be dropping all the individual "= 0" assignments.
> :)
> 

Sure if we decide to go that direction all those 0 assignments void.
Edgecombe, Rick P Feb. 27, 2024, 8:25 p.m. UTC | #5
On Tue, 2024-02-27 at 18:16 +0000, Christophe Leroy wrote:
> > > Why doing a full init of the struct when all fields are re-
> > > written a few
> > > lines after ?
> > 
> > It's a nice change for robustness and makes future changes easier.
> > It's
> > not actually wasteful since the compiler will throw away all
> > redundant
> > stores.
> 
> Well, I tend to dislike default init at declaration because it often 
> hides missed real init. When a field is not initialized GCC should
> emit 
> a Warning, at least when built with W=2 which sets 
> -Wmissing-field-initializers ?

Sorry, I'm not following where you are going with this. There aren't
any struct vm_unmapped_area_info users that use initializers today, so
that warning won't apply in this case. Meanwhile, designated style
struct initialization (which would zero new members) is very common, as
well as not get anything checked by that warning. Anything with this
many members is probably going to use the designated style.

If we are optimizing to avoid bugs, the way this struct is used today
is not great. It is essentially being used as an argument passer.
Normally when a function signature changes, but a caller is missed, of
course the compiler will notice loudly. But not here. So I think
probably zero initializing it is safer than being setup to pass
garbage.

I'm trying to figure out what to do here. If I changed it so that just
powerpc set the new field manually, then the convention across the
kernel would be for everything to be default zero, and future other new
parameters could have a greater chance of turning into garbage on
powerpc. Since it could be easy to miss that powerpc was special. Would
you prefer it?

Or maybe I could try a new vm_unmapped_area() that takes the extra
argument separately? The old callers could call the old function and
not need any arch updates. It all seems strange though, because
automatic zero initializing struct members is so common in the kernel.
But it also wouldn't add the cleanup Kees was pointing out. Hmm.

Any preference? Or maybe am I missing your point and talking nonsense?
Kirill A. Shutemov Feb. 28, 2024, 11:51 a.m. UTC | #6
On Mon, Feb 26, 2024 at 11:09:47AM -0800, Rick Edgecombe wrote:
> diff --git a/arch/alpha/kernel/osf_sys.c b/arch/alpha/kernel/osf_sys.c
> index 5db88b627439..dd6801bb9240 100644
> --- a/arch/alpha/kernel/osf_sys.c
> +++ b/arch/alpha/kernel/osf_sys.c
> @@ -1218,7 +1218,7 @@ static unsigned long
>  arch_get_unmapped_area_1(unsigned long addr, unsigned long len,
>  		         unsigned long limit)
>  {
> -	struct vm_unmapped_area_info info;
> +	struct vm_unmapped_area_info info = {};
>  
>  	info.flags = 0;
>  	info.length = len;

Can we make a step forward and actually move initialization inside the
initializator? Something like below.

I understand that it is substantially more work, but I think it is useful.

diff --git a/arch/alpha/kernel/osf_sys.c b/arch/alpha/kernel/osf_sys.c
index 5db88b627439..c40ddede3b13 100644
--- a/arch/alpha/kernel/osf_sys.c
+++ b/arch/alpha/kernel/osf_sys.c
@@ -1218,14 +1218,12 @@ static unsigned long
 arch_get_unmapped_area_1(unsigned long addr, unsigned long len,
 		         unsigned long limit)
 {
-	struct vm_unmapped_area_info info;
+	struct vm_unmapped_area_info info = {
+		.length = len;
+		.low_limit = addr,
+		.high_limit = limit,
+	};

-	info.flags = 0;
-	info.length = len;
-	info.low_limit = addr;
-	info.high_limit = limit;
-	info.align_mask = 0;
-	info.align_offset = 0;
 	return vm_unmapped_area(&info);
 }
Christophe Leroy Feb. 28, 2024, 1:22 p.m. UTC | #7
Le 27/02/2024 à 21:25, Edgecombe, Rick P a écrit :
> On Tue, 2024-02-27 at 18:16 +0000, Christophe Leroy wrote:
>>>> Why doing a full init of the struct when all fields are re-
>>>> written a few
>>>> lines after ?
>>>
>>> It's a nice change for robustness and makes future changes easier.
>>> It's
>>> not actually wasteful since the compiler will throw away all
>>> redundant
>>> stores.
>>
>> Well, I tend to dislike default init at declaration because it often
>> hides missed real init. When a field is not initialized GCC should
>> emit
>> a Warning, at least when built with W=2 which sets
>> -Wmissing-field-initializers ?
> 
> Sorry, I'm not following where you are going with this. There aren't
> any struct vm_unmapped_area_info users that use initializers today, so
> that warning won't apply in this case. Meanwhile, designated style
> struct initialization (which would zero new members) is very common, as
> well as not get anything checked by that warning. Anything with this
> many members is probably going to use the designated style.
> 
> If we are optimizing to avoid bugs, the way this struct is used today
> is not great. It is essentially being used as an argument passer.
> Normally when a function signature changes, but a caller is missed, of
> course the compiler will notice loudly. But not here. So I think
> probably zero initializing it is safer than being setup to pass
> garbage.

No worry, if everybody thinks that init at declaration is worth it in 
that case it is OK for me and I'm not going to ask for something special 
on powerpc, my comment was more general allthough I used powerpc as an 
exemple.

My worry with initialisation at declaration is it often hides missing 
assignments. Let's take following simple exemple:

char *colour(int num)
{
	char *name;

	if (num == 0) {
		name = "black";
	} else if (num == 1) {
		name = "white";
	} else if (num == 2) {
	} else {
		name = "no colour";
	}

	return name;
}


Here, GCC warns about a missing initialisation of variable 'name'.

But if I declare it as

	char *name = "no colour";

Then GCC won't warn anymore that we are missing a value for when num is 2.

During my life I have so many times spent huge amount of time 
investigating issues and bugs due to missing assignments that were going 
undetected due to default initialisation at declaration.

> 
> I'm trying to figure out what to do here. If I changed it so that just
> powerpc set the new field manually, then the convention across the
> kernel would be for everything to be default zero, and future other new
> parameters could have a greater chance of turning into garbage on
> powerpc. Since it could be easy to miss that powerpc was special. Would
> you prefer it?
> 
> Or maybe I could try a new vm_unmapped_area() that takes the extra
> argument separately? The old callers could call the old function and
> not need any arch updates. It all seems strange though, because
> automatic zero initializing struct members is so common in the kernel.
> But it also wouldn't add the cleanup Kees was pointing out. Hmm.
> 
> Any preference? Or maybe am I missing your point and talking nonsense?
> 

So my preference would go to the addition of:

	info.new_field = 0;

But that's very minor and if you think it is easier to manage and 
maintain by performing {} initialisation at declaration, lets go for that.

Christophe
Edgecombe, Rick P Feb. 28, 2024, 5:01 p.m. UTC | #8
On Wed, 2024-02-28 at 13:22 +0000, Christophe Leroy wrote:
> > Any preference? Or maybe am I missing your point and talking
> > nonsense?
> > 
> 
> So my preference would go to the addition of:
> 
>         info.new_field = 0;
> 
> But that's very minor and if you think it is easier to manage and 
> maintain by performing {} initialisation at declaration, lets go for
> that.

Appreciate the clarification and help getting this right. I'm thinking
Kees' and now Kirill's point about this patch resulting in unnecessary
manual zero initialization of the structs is probably something that
needs to be addressed.

If I created a bunch of patches to change each call site, I think the
the best is probably to do the designated field zero initialization
way.

But I can do something for powerpc special if you want. I'll first try
with powerpc matching the others, and if it seems objectionable, please
let me know.

Thanks,

Rick
Kees Cook Feb. 28, 2024, 5:21 p.m. UTC | #9
On Wed, Feb 28, 2024 at 01:22:09PM +0000, Christophe Leroy wrote:
> [...]
> My worry with initialisation at declaration is it often hides missing 
> assignments. Let's take following simple exemple:
> 
> char *colour(int num)
> {
> 	char *name;
> 
> 	if (num == 0) {
> 		name = "black";
> 	} else if (num == 1) {
> 		name = "white";
> 	} else if (num == 2) {
> 	} else {
> 		name = "no colour";
> 	}
> 
> 	return name;
> }
> 
> Here, GCC warns about a missing initialisation of variable 'name'.

Sometimes. :( We build with -Wno-maybe-uninitialized because GCC gets
this wrong too often. Also, like with large structs like this, all
uninit warnings get suppressed if anything takes it by reference. So, if
before your "return name" statement above, you had something like:

	do_something(&name);

it won't warn with any option enabled.

> But if I declare it as
> 
> 	char *name = "no colour";
> 
> Then GCC won't warn anymore that we are missing a value for when num is 2.
> 
> During my life I have so many times spent huge amount of time 
> investigating issues and bugs due to missing assignments that were going 
> undetected due to default initialisation at declaration.

I totally understand. If the "uninitialized" warnings were actually
reliable, I would agree. I look at it this way:

- initializations can be missed either in static initializers or via
  run time initializers. (So the risk of mistake here is matched --
  though I'd argue it's easier to *find* static initializers when adding
  new struct members.)
- uninitialized warnings are inconsistent (this becomes an unknown risk)
- when a run time initializer is missed, the contents are whatever was
  on the stack (high risk)
- what a static initializer is missed, the content is 0 (low risk)

I think unambiguous state (always 0) is significantly more important for
the safety of the system as a whole. Yes, individual cases maybe bad
("what uid should this be? root?!") but from a general memory safety
perspective the value doesn't become potentially influenced by order of
operations, leftover stack memory, etc.

I'd agree, lifting everything into a static initializer does seem
cleanest of all the choices.

-Kees
Christophe Leroy Feb. 28, 2024, 11:10 p.m. UTC | #10
Le 28/02/2024 à 18:01, Edgecombe, Rick P a écrit :
> On Wed, 2024-02-28 at 13:22 +0000, Christophe Leroy wrote:
>>> Any preference? Or maybe am I missing your point and talking
>>> nonsense?
>>>
>>
>> So my preference would go to the addition of:
>>
>>          info.new_field = 0;
>>
>> But that's very minor and if you think it is easier to manage and
>> maintain by performing {} initialisation at declaration, lets go for
>> that.
> 
> Appreciate the clarification and help getting this right. I'm thinking
> Kees' and now Kirill's point about this patch resulting in unnecessary
> manual zero initialization of the structs is probably something that
> needs to be addressed.
> 
> If I created a bunch of patches to change each call site, I think the
> the best is probably to do the designated field zero initialization
> way.
> 
> But I can do something for powerpc special if you want. I'll first try
> with powerpc matching the others, and if it seems objectionable, please
> let me know.
> 

My comments were generic, it was not powerpc oriented. Please keep 
powerpc as similar as possible with others.

Christophe
Edgecombe, Rick P March 2, 2024, 12:47 a.m. UTC | #11
On Wed, 2024-02-28 at 09:21 -0800, Kees Cook wrote:
> I totally understand. If the "uninitialized" warnings were actually
> reliable, I would agree. I look at it this way:
> 
> - initializations can be missed either in static initializers or via
>   run time initializers. (So the risk of mistake here is matched --
>   though I'd argue it's easier to *find* static initializers when
> adding
>   new struct members.)
> - uninitialized warnings are inconsistent (this becomes an unknown
> risk)
> - when a run time initializer is missed, the contents are whatever
> was
>   on the stack (high risk)
> - what a static initializer is missed, the content is 0 (low risk)
> 
> I think unambiguous state (always 0) is significantly more important
> for
> the safety of the system as a whole. Yes, individual cases maybe bad
> ("what uid should this be? root?!") but from a general memory safety
> perspective the value doesn't become potentially influenced by order
> of
> operations, leftover stack memory, etc.
> 
> I'd agree, lifting everything into a static initializer does seem
> cleanest of all the choices.

Hi Kees,

Well, I just gave this a try. It is giving me flashbacks of when I last
had to do a tree wide change that I couldn't fully test and the
breakage was caught by Linus.

Could you let me know if you think this is additionally worthwhile
cleanup outside of the guard gap improvements of this series? Because I
was thinking a more cowardly approach could be a new vm_unmapped_area()
variant that takes the new start gap member as a separate argument
outside of struct vm_unmapped_area_info. It would be kind of strange to
keep them separate, but it would be less likely to bump something.

Thanks,

Rick
Kees Cook March 2, 2024, 1:51 a.m. UTC | #12
On Sat, Mar 02, 2024 at 12:47:08AM +0000, Edgecombe, Rick P wrote:
> On Wed, 2024-02-28 at 09:21 -0800, Kees Cook wrote:
> > I totally understand. If the "uninitialized" warnings were actually
> > reliable, I would agree. I look at it this way:
> > 
> > - initializations can be missed either in static initializers or via
> >   run time initializers. (So the risk of mistake here is matched --
> >   though I'd argue it's easier to *find* static initializers when
> > adding
> >   new struct members.)
> > - uninitialized warnings are inconsistent (this becomes an unknown
> > risk)
> > - when a run time initializer is missed, the contents are whatever
> > was
> >   on the stack (high risk)
> > - what a static initializer is missed, the content is 0 (low risk)
> > 
> > I think unambiguous state (always 0) is significantly more important
> > for
> > the safety of the system as a whole. Yes, individual cases maybe bad
> > ("what uid should this be? root?!") but from a general memory safety
> > perspective the value doesn't become potentially influenced by order
> > of
> > operations, leftover stack memory, etc.
> > 
> > I'd agree, lifting everything into a static initializer does seem
> > cleanest of all the choices.
> 
> Hi Kees,
> 
> Well, I just gave this a try. It is giving me flashbacks of when I last
> had to do a tree wide change that I couldn't fully test and the
> breakage was caught by Linus.

Yeah, testing isn't fun for these kinds of things. This is traditionally
why the "obviously correct" changes tend to have an easier time landing
(i.e. adding "= {}" to all of them).

> Could you let me know if you think this is additionally worthwhile
> cleanup outside of the guard gap improvements of this series? Because I
> was thinking a more cowardly approach could be a new vm_unmapped_area()
> variant that takes the new start gap member as a separate argument
> outside of struct vm_unmapped_area_info. It would be kind of strange to
> keep them separate, but it would be less likely to bump something.

I think you want a new member -- AIUI, that's what that struct is for.

Looking at this resulting set of patches, I do kinda think just adding
the "= {}" in a single patch is more sensible. Having to split things
that are know at the top of the function from the stuff known at the
existing initialization time is rather awkward.

Personally, I think a single patch that sets "= {}" for all of them and
drop the all the "= 0" or "= NULL" assignments would be the cleanest way
to go.

-Kees
Christophe Leroy March 4, 2024, 6 p.m. UTC | #13
Le 02/03/2024 à 02:51, Kees Cook a écrit :
> On Sat, Mar 02, 2024 at 12:47:08AM +0000, Edgecombe, Rick P wrote:
>> On Wed, 2024-02-28 at 09:21 -0800, Kees Cook wrote:
>>> I totally understand. If the "uninitialized" warnings were actually
>>> reliable, I would agree. I look at it this way:
>>>
>>> - initializations can be missed either in static initializers or via
>>>    run time initializers. (So the risk of mistake here is matched --
>>>    though I'd argue it's easier to *find* static initializers when
>>> adding
>>>    new struct members.)
>>> - uninitialized warnings are inconsistent (this becomes an unknown
>>> risk)
>>> - when a run time initializer is missed, the contents are whatever
>>> was
>>>    on the stack (high risk)
>>> - what a static initializer is missed, the content is 0 (low risk)
>>>
>>> I think unambiguous state (always 0) is significantly more important
>>> for
>>> the safety of the system as a whole. Yes, individual cases maybe bad
>>> ("what uid should this be? root?!") but from a general memory safety
>>> perspective the value doesn't become potentially influenced by order
>>> of
>>> operations, leftover stack memory, etc.
>>>
>>> I'd agree, lifting everything into a static initializer does seem
>>> cleanest of all the choices.
>>
>> Hi Kees,
>>
>> Well, I just gave this a try. It is giving me flashbacks of when I last
>> had to do a tree wide change that I couldn't fully test and the
>> breakage was caught by Linus.
> 
> Yeah, testing isn't fun for these kinds of things. This is traditionally
> why the "obviously correct" changes tend to have an easier time landing
> (i.e. adding "= {}" to all of them).
> 
>> Could you let me know if you think this is additionally worthwhile
>> cleanup outside of the guard gap improvements of this series? Because I
>> was thinking a more cowardly approach could be a new vm_unmapped_area()
>> variant that takes the new start gap member as a separate argument
>> outside of struct vm_unmapped_area_info. It would be kind of strange to
>> keep them separate, but it would be less likely to bump something.
> 
> I think you want a new member -- AIUI, that's what that struct is for.
> 
> Looking at this resulting set of patches, I do kinda think just adding
> the "= {}" in a single patch is more sensible. Having to split things
> that are know at the top of the function from the stuff known at the
> existing initialization time is rather awkward.
> 
> Personally, I think a single patch that sets "= {}" for all of them and
> drop the all the "= 0" or "= NULL" assignments would be the cleanest way
> to go.

I agree with Kees, set = {} and drop all the "something = 0;" stuff.

Christophe
Edgecombe, Rick P March 4, 2024, 6:03 p.m. UTC | #14
On Mon, 2024-03-04 at 18:00 +0000, Christophe Leroy wrote:
> > Personally, I think a single patch that sets "= {}" for all of them
> > and
> > drop the all the "= 0" or "= NULL" assignments would be the
> > cleanest way
> > to go.
> 
> I agree with Kees, set = {} and drop all the "something = 0;" stuff.

Thanks. Now some of the arch's have very nicely acked and reviewed the
existing patches. I'll leave those as is, and do this for anyone that
doesn't respond.
diff mbox series

Patch

diff --git a/arch/alpha/kernel/osf_sys.c b/arch/alpha/kernel/osf_sys.c
index 5db88b627439..dd6801bb9240 100644
--- a/arch/alpha/kernel/osf_sys.c
+++ b/arch/alpha/kernel/osf_sys.c
@@ -1218,7 +1218,7 @@  static unsigned long
 arch_get_unmapped_area_1(unsigned long addr, unsigned long len,
 		         unsigned long limit)
 {
-	struct vm_unmapped_area_info info;
+	struct vm_unmapped_area_info info = {};
 
 	info.flags = 0;
 	info.length = len;
diff --git a/arch/arc/mm/mmap.c b/arch/arc/mm/mmap.c
index 3c1c7ae73292..6549b3375f54 100644
--- a/arch/arc/mm/mmap.c
+++ b/arch/arc/mm/mmap.c
@@ -27,7 +27,7 @@  arch_get_unmapped_area(struct file *filp, unsigned long addr,
 {
 	struct mm_struct *mm = current->mm;
 	struct vm_area_struct *vma;
-	struct vm_unmapped_area_info info;
+	struct vm_unmapped_area_info info = {};
 
 	/*
 	 * We enforce the MAP_FIXED case.
diff --git a/arch/arm/mm/mmap.c b/arch/arm/mm/mmap.c
index a0f8a0ca0788..525795578c29 100644
--- a/arch/arm/mm/mmap.c
+++ b/arch/arm/mm/mmap.c
@@ -34,7 +34,7 @@  arch_get_unmapped_area(struct file *filp, unsigned long addr,
 	struct vm_area_struct *vma;
 	int do_align = 0;
 	int aliasing = cache_is_vipt_aliasing();
-	struct vm_unmapped_area_info info;
+	struct vm_unmapped_area_info info = {};
 
 	/*
 	 * We only need to do colour alignment if either the I or D
@@ -87,7 +87,7 @@  arch_get_unmapped_area_topdown(struct file *filp, const unsigned long addr0,
 	unsigned long addr = addr0;
 	int do_align = 0;
 	int aliasing = cache_is_vipt_aliasing();
-	struct vm_unmapped_area_info info;
+	struct vm_unmapped_area_info info = {};
 
 	/*
 	 * We only need to do colour alignment if either the I or D
diff --git a/arch/csky/abiv1/mmap.c b/arch/csky/abiv1/mmap.c
index 6792aca49999..726659d41fa9 100644
--- a/arch/csky/abiv1/mmap.c
+++ b/arch/csky/abiv1/mmap.c
@@ -28,7 +28,7 @@  arch_get_unmapped_area(struct file *filp, unsigned long addr,
 	struct mm_struct *mm = current->mm;
 	struct vm_area_struct *vma;
 	int do_align = 0;
-	struct vm_unmapped_area_info info;
+	struct vm_unmapped_area_info info = {};
 
 	/*
 	 * We only need to do colour alignment if either the I or D
diff --git a/arch/loongarch/mm/mmap.c b/arch/loongarch/mm/mmap.c
index a9630a81b38a..664bf4abfdcf 100644
--- a/arch/loongarch/mm/mmap.c
+++ b/arch/loongarch/mm/mmap.c
@@ -24,7 +24,7 @@  static unsigned long arch_get_unmapped_area_common(struct file *filp,
 	struct vm_area_struct *vma;
 	unsigned long addr = addr0;
 	int do_color_align;
-	struct vm_unmapped_area_info info;
+	struct vm_unmapped_area_info info = {};
 
 	if (unlikely(len > TASK_SIZE))
 		return -ENOMEM;
diff --git a/arch/mips/mm/mmap.c b/arch/mips/mm/mmap.c
index 00fe90c6db3e..6321b53dc995 100644
--- a/arch/mips/mm/mmap.c
+++ b/arch/mips/mm/mmap.c
@@ -34,7 +34,7 @@  static unsigned long arch_get_unmapped_area_common(struct file *filp,
 	struct vm_area_struct *vma;
 	unsigned long addr = addr0;
 	int do_color_align;
-	struct vm_unmapped_area_info info;
+	struct vm_unmapped_area_info info = {};
 
 	if (unlikely(len > TASK_SIZE))
 		return -ENOMEM;
diff --git a/arch/parisc/kernel/sys_parisc.c b/arch/parisc/kernel/sys_parisc.c
index 98af719d5f85..e87c0e325abf 100644
--- a/arch/parisc/kernel/sys_parisc.c
+++ b/arch/parisc/kernel/sys_parisc.c
@@ -104,7 +104,7 @@  static unsigned long arch_get_unmapped_area_common(struct file *filp,
 	struct vm_area_struct *vma, *prev;
 	unsigned long filp_pgoff;
 	int do_color_align;
-	struct vm_unmapped_area_info info;
+	struct vm_unmapped_area_info info = {};
 
 	if (unlikely(len > TASK_SIZE))
 		return -ENOMEM;
diff --git a/arch/powerpc/mm/book3s64/slice.c b/arch/powerpc/mm/book3s64/slice.c
index c0b58afb9a47..5884f384866f 100644
--- a/arch/powerpc/mm/book3s64/slice.c
+++ b/arch/powerpc/mm/book3s64/slice.c
@@ -282,7 +282,7 @@  static unsigned long slice_find_area_bottomup(struct mm_struct *mm,
 {
 	int pshift = max_t(int, mmu_psize_defs[psize].shift, PAGE_SHIFT);
 	unsigned long found, next_end;
-	struct vm_unmapped_area_info info;
+	struct vm_unmapped_area_info info = {};
 
 	info.flags = 0;
 	info.length = len;
@@ -326,7 +326,7 @@  static unsigned long slice_find_area_topdown(struct mm_struct *mm,
 {
 	int pshift = max_t(int, mmu_psize_defs[psize].shift, PAGE_SHIFT);
 	unsigned long found, prev;
-	struct vm_unmapped_area_info info;
+	struct vm_unmapped_area_info info = {};
 	unsigned long min_addr = max(PAGE_SIZE, mmap_min_addr);
 
 	info.flags = VM_UNMAPPED_AREA_TOPDOWN;
diff --git a/arch/s390/mm/hugetlbpage.c b/arch/s390/mm/hugetlbpage.c
index c2d2850ec8d5..7f68485feea0 100644
--- a/arch/s390/mm/hugetlbpage.c
+++ b/arch/s390/mm/hugetlbpage.c
@@ -258,7 +258,7 @@  static unsigned long hugetlb_get_unmapped_area_bottomup(struct file *file,
 		unsigned long pgoff, unsigned long flags)
 {
 	struct hstate *h = hstate_file(file);
-	struct vm_unmapped_area_info info;
+	struct vm_unmapped_area_info info = {};
 
 	info.flags = 0;
 	info.length = len;
@@ -274,7 +274,7 @@  static unsigned long hugetlb_get_unmapped_area_topdown(struct file *file,
 		unsigned long pgoff, unsigned long flags)
 {
 	struct hstate *h = hstate_file(file);
-	struct vm_unmapped_area_info info;
+	struct vm_unmapped_area_info info = {};
 	unsigned long addr;
 
 	info.flags = VM_UNMAPPED_AREA_TOPDOWN;
diff --git a/arch/s390/mm/mmap.c b/arch/s390/mm/mmap.c
index cd52d72b59cf..df88496e2903 100644
--- a/arch/s390/mm/mmap.c
+++ b/arch/s390/mm/mmap.c
@@ -77,7 +77,7 @@  unsigned long arch_get_unmapped_area(struct file *filp, unsigned long addr,
 {
 	struct mm_struct *mm = current->mm;
 	struct vm_area_struct *vma;
-	struct vm_unmapped_area_info info;
+	struct vm_unmapped_area_info info = {};
 
 	if (len > TASK_SIZE - mmap_min_addr)
 		return -ENOMEM;
@@ -116,7 +116,7 @@  unsigned long arch_get_unmapped_area_topdown(struct file *filp, unsigned long ad
 {
 	struct vm_area_struct *vma;
 	struct mm_struct *mm = current->mm;
-	struct vm_unmapped_area_info info;
+	struct vm_unmapped_area_info info = {};
 
 	/* requested length too big for entire address space */
 	if (len > TASK_SIZE - mmap_min_addr)
diff --git a/arch/sh/mm/mmap.c b/arch/sh/mm/mmap.c
index b82199878b45..6aee5f761e08 100644
--- a/arch/sh/mm/mmap.c
+++ b/arch/sh/mm/mmap.c
@@ -57,7 +57,7 @@  unsigned long arch_get_unmapped_area(struct file *filp, unsigned long addr,
 	struct mm_struct *mm = current->mm;
 	struct vm_area_struct *vma;
 	int do_colour_align;
-	struct vm_unmapped_area_info info;
+	struct vm_unmapped_area_info info = {};
 
 	if (flags & MAP_FIXED) {
 		/* We do not accept a shared mapping if it would violate
@@ -106,7 +106,7 @@  arch_get_unmapped_area_topdown(struct file *filp, const unsigned long addr0,
 	struct mm_struct *mm = current->mm;
 	unsigned long addr = addr0;
 	int do_colour_align;
-	struct vm_unmapped_area_info info;
+	struct vm_unmapped_area_info info = {};
 
 	if (flags & MAP_FIXED) {
 		/* We do not accept a shared mapping if it would violate
diff --git a/arch/sparc/kernel/sys_sparc_32.c b/arch/sparc/kernel/sys_sparc_32.c
index 082a551897ed..7e781dbfd052 100644
--- a/arch/sparc/kernel/sys_sparc_32.c
+++ b/arch/sparc/kernel/sys_sparc_32.c
@@ -41,7 +41,7 @@  SYSCALL_DEFINE0(getpagesize)
 
 unsigned long arch_get_unmapped_area(struct file *filp, unsigned long addr, unsigned long len, unsigned long pgoff, unsigned long flags)
 {
-	struct vm_unmapped_area_info info;
+	struct vm_unmapped_area_info info = {};
 
 	if (flags & MAP_FIXED) {
 		/* We do not accept a shared mapping if it would violate
diff --git a/arch/sparc/kernel/sys_sparc_64.c b/arch/sparc/kernel/sys_sparc_64.c
index 1dbf7211666e..fc48ab3f83af 100644
--- a/arch/sparc/kernel/sys_sparc_64.c
+++ b/arch/sparc/kernel/sys_sparc_64.c
@@ -93,7 +93,7 @@  unsigned long arch_get_unmapped_area(struct file *filp, unsigned long addr, unsi
 	struct vm_area_struct * vma;
 	unsigned long task_size = TASK_SIZE;
 	int do_color_align;
-	struct vm_unmapped_area_info info;
+	struct vm_unmapped_area_info info = {};
 
 	if (flags & MAP_FIXED) {
 		/* We do not accept a shared mapping if it would violate
@@ -154,7 +154,7 @@  arch_get_unmapped_area_topdown(struct file *filp, const unsigned long addr0,
 	unsigned long task_size = STACK_TOP32;
 	unsigned long addr = addr0;
 	int do_color_align;
-	struct vm_unmapped_area_info info;
+	struct vm_unmapped_area_info info = {};
 
 	/* This should only ever run for 32-bit processes.  */
 	BUG_ON(!test_thread_flag(TIF_32BIT));
diff --git a/arch/sparc/mm/hugetlbpage.c b/arch/sparc/mm/hugetlbpage.c
index 38a1bef47efb..614e2c46d781 100644
--- a/arch/sparc/mm/hugetlbpage.c
+++ b/arch/sparc/mm/hugetlbpage.c
@@ -31,7 +31,7 @@  static unsigned long hugetlb_get_unmapped_area_bottomup(struct file *filp,
 {
 	struct hstate *h = hstate_file(filp);
 	unsigned long task_size = TASK_SIZE;
-	struct vm_unmapped_area_info info;
+	struct vm_unmapped_area_info info = {};
 
 	if (test_thread_flag(TIF_32BIT))
 		task_size = STACK_TOP32;
@@ -63,7 +63,7 @@  hugetlb_get_unmapped_area_topdown(struct file *filp, const unsigned long addr0,
 	struct hstate *h = hstate_file(filp);
 	struct mm_struct *mm = current->mm;
 	unsigned long addr = addr0;
-	struct vm_unmapped_area_info info;
+	struct vm_unmapped_area_info info = {};
 
 	/* This should only ever run for 32-bit processes.  */
 	BUG_ON(!test_thread_flag(TIF_32BIT));
diff --git a/arch/x86/kernel/sys_x86_64.c b/arch/x86/kernel/sys_x86_64.c
index c783aeb37dce..6e5d4fa5fc42 100644
--- a/arch/x86/kernel/sys_x86_64.c
+++ b/arch/x86/kernel/sys_x86_64.c
@@ -125,7 +125,7 @@  arch_get_unmapped_area(struct file *filp, unsigned long addr,
 {
 	struct mm_struct *mm = current->mm;
 	struct vm_area_struct *vma;
-	struct vm_unmapped_area_info info;
+	struct vm_unmapped_area_info info = {};
 	unsigned long begin, end;
 
 	if (flags & MAP_FIXED)
@@ -165,7 +165,7 @@  arch_get_unmapped_area_topdown(struct file *filp, const unsigned long addr0,
 	struct vm_area_struct *vma;
 	struct mm_struct *mm = current->mm;
 	unsigned long addr = addr0;
-	struct vm_unmapped_area_info info;
+	struct vm_unmapped_area_info info = {};
 
 	/* requested length too big for entire address space */
 	if (len > TASK_SIZE)
diff --git a/arch/x86/mm/hugetlbpage.c b/arch/x86/mm/hugetlbpage.c
index 6d77c0039617..88726bd1f72d 100644
--- a/arch/x86/mm/hugetlbpage.c
+++ b/arch/x86/mm/hugetlbpage.c
@@ -51,7 +51,7 @@  static unsigned long hugetlb_get_unmapped_area_bottomup(struct file *file,
 		unsigned long pgoff, unsigned long flags)
 {
 	struct hstate *h = hstate_file(file);
-	struct vm_unmapped_area_info info;
+	struct vm_unmapped_area_info info = {};
 
 	info.flags = 0;
 	info.length = len;
@@ -74,7 +74,7 @@  static unsigned long hugetlb_get_unmapped_area_topdown(struct file *file,
 		unsigned long pgoff, unsigned long flags)
 {
 	struct hstate *h = hstate_file(file);
-	struct vm_unmapped_area_info info;
+	struct vm_unmapped_area_info info = {};
 
 	info.flags = VM_UNMAPPED_AREA_TOPDOWN;
 	info.length = len;
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index a63d2eee086f..848b2239a215 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -165,7 +165,7 @@  hugetlb_get_unmapped_area_bottomup(struct file *file, unsigned long addr,
 		unsigned long len, unsigned long pgoff, unsigned long flags)
 {
 	struct hstate *h = hstate_file(file);
-	struct vm_unmapped_area_info info;
+	struct vm_unmapped_area_info info = {};
 
 	info.flags = 0;
 	info.length = len;
@@ -181,7 +181,7 @@  hugetlb_get_unmapped_area_topdown(struct file *file, unsigned long addr,
 		unsigned long len, unsigned long pgoff, unsigned long flags)
 {
 	struct hstate *h = hstate_file(file);
-	struct vm_unmapped_area_info info;
+	struct vm_unmapped_area_info info = {};
 
 	info.flags = VM_UNMAPPED_AREA_TOPDOWN;
 	info.length = len;
diff --git a/mm/mmap.c b/mm/mmap.c
index e02bb17fef5b..33af683a643f 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1699,7 +1699,7 @@  generic_get_unmapped_area(struct file *filp, unsigned long addr,
 {
 	struct mm_struct *mm = current->mm;
 	struct vm_area_struct *vma, *prev;
-	struct vm_unmapped_area_info info;
+	struct vm_unmapped_area_info info = {};
 	const unsigned long mmap_end = arch_get_mmap_end(addr, len, flags);
 
 	if (len > mmap_end - mmap_min_addr)
@@ -1747,7 +1747,7 @@  generic_get_unmapped_area_topdown(struct file *filp, unsigned long addr,
 {
 	struct vm_area_struct *vma, *prev;
 	struct mm_struct *mm = current->mm;
-	struct vm_unmapped_area_info info;
+	struct vm_unmapped_area_info info = {};
 	const unsigned long mmap_end = arch_get_mmap_end(addr, len, flags);
 
 	/* requested length too big for entire address space */