Re: [RFC v2.1 06/12] parisc: Use initializer for struct vm_unmapped_area_info

From: Helge Deller <deller@gmx.de>
To: Rick Edgecombe <rick.p.edgecombe@intel.com>
Cc: Liam.Howlett@oracle.com, akpm@linux-foundation.org, bp@alien8.de,
	broonie@kernel.org, dave.hansen@linux.intel.com,
	debug@rivosinc.com, hpa@zytor.com, keescook@chromium.org,
	kirill.shutemov@linux.intel.com, luto@kernel.org,
	mingo@redhat.com, peterz@infradead.org,
	sparclinux@vger.kernel.org, tglx@linutronix.de, x86@kernel.org,
	"James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>,
	linux-parisc@vger.kernel.org
Subject: Re: [RFC v2.1 06/12] parisc: Use initializer for struct vm_unmapped_area_info
Date: Sat, 2 Mar 2024 07:35:15 +0100	[thread overview]
Message-ID: <ce73b0b4-a720-4e1b-8ab9-1990d76a6425@gmx.de> (raw)
In-Reply-To: <20240302001714.674091-6-rick.p.edgecombe@intel.com>

On 3/2/24 01:17, Rick Edgecombe wrote:
> Future changes will need to add a new member to struct
> vm_unmapped_area_info. This would cause trouble for any call site that
> doesn't initialize the struct. Currently every caller sets each field
> manually, so if new fields are added they will be unitialized and the core
> code parsing the struct will see garbage in the new field.
>
> It could be possible to initialize the new field manually to 0 at each
> call site. This and a couple other options were discussed, and the
> consensus (see links) was that in general the best way to accomplish this
> would be via static initialization with designated field initiators.
> Having some struct vm_unmapped_area_info instances not zero initialized
> will put those sites at risk of feeding garbage into vm_unmapped_area() if
> the convention is to zero initialize the struct and any new field addition
> misses a call site that initializes each field manually.
>
> It could be possible to leave the code mostly untouched, and just change
> the line:
> struct vm_unmapped_area_info info
> to:
> struct vm_unmapped_area_info info = {};
>
> However, that would leave cleanup for the fields that are manually set
> to zero, as it would no longer be required.
>
> So to be reduce the chance of bugs via uninitialized fields, instead
> simply continue the process to initialize the struct this way tree wide.
> This will zero any unspecified members. Move the field initializers to the
> struct declaration when they are known at that time. Leave the fields out
> that were manually initialized to zero, as this would be redundant for
> designated initializers.
>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
> Cc: Helge Deller <deller@gmx.de>

Acked-by: Helge Deller <deller@gmx.de>

Thanks!
Helge

> Cc: linux-parisc@vger.kernel.org
> Link: https://lore.kernel.org/lkml/202402280912.33AEE7A9CF@keescook/#t
> Link: https://lore.kernel.org/lkml/j7bfvig3gew3qruouxrh7z7ehjjafrgkbcmg6tcghhfh3rhmzi@wzlcoecgy5rs/
> ---
> Hi,
>
> This patch was split and refactored out of a tree-wide change [0] to just
> zero-init each struct vm_unmapped_area_info. The overall goal of the
> series is to help shadow stack guard gaps. Currently, there is only one
> arch with shadow stacks, but two more are in progress. It is 0day tested
> only.
>
> Thanks,
>
> Rick
>
> [0] https://lore.kernel.org/lkml/20240226190951.3240433-6-rick.p.edgecombe@intel.com/
> ---
>   arch/parisc/kernel/sys_parisc.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/arch/parisc/kernel/sys_parisc.c b/arch/parisc/kernel/sys_parisc.c
> index 98af719d5f85..1f0b60766aac 100644
> --- a/arch/parisc/kernel/sys_parisc.c
> +++ b/arch/parisc/kernel/sys_parisc.c
> @@ -104,7 +104,9 @@ 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 = {
> +		.length = len
> +	};
>
>   	if (unlikely(len > TASK_SIZE))
>   		return -ENOMEM;
> @@ -139,7 +141,6 @@ static unsigned long arch_get_unmapped_area_common(struct file *filp,
>   			return addr;
>   	}
>
> -	info.length = len;
>   	info.align_mask = do_color_align ? (PAGE_MASK & (SHM_COLOUR - 1)) : 0;
>   	info.align_offset = shared_align_offset(filp_pgoff, pgoff);
>