From: Christophe Leroy <christophe.leroy@csgroup.eu>
To: Rick Edgecombe <rick.p.edgecombe@intel.com>,
"Liam.Howlett@oracle.com" <Liam.Howlett@oracle.com>,
"akpm@linux-foundation.org" <akpm@linux-foundation.org>,
"bp@alien8.de" <bp@alien8.de>,
"broonie@kernel.org" <broonie@kernel.org>,
"dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
"debug@rivosinc.com" <debug@rivosinc.com>,
"hpa@zytor.com" <hpa@zytor.com>,
"keescook@chromium.org" <keescook@chromium.org>,
"kirill.shutemov@linux.intel.com"
<kirill.shutemov@linux.intel.com>,
"luto@kernel.org" <luto@kernel.org>,
"mingo@redhat.com" <mingo@redhat.com>,
"peterz@infradead.org" <peterz@infradead.org>,
"tglx@linutronix.de" <tglx@linutronix.de>,
"x86@kernel.org" <x86@kernel.org>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"linux-mm@kvack.org" <linux-mm@kvack.org>,
Guo Ren <guoren@kernel.org>,
"linux-csky@vger.kernel.org" <linux-csky@vger.kernel.org>
Subject: Re: [PATCH v3 05/12] csky: Use initializer for struct vm_unmapped_area_info
Date: Wed, 13 Mar 2024 09:04:48 +0000 [thread overview]
Message-ID: <06593c84-2fdb-4921-8d54-dabe13fa2227@csgroup.eu> (raw)
In-Reply-To: <20240312222843.2505560-6-rick.p.edgecombe@intel.com>
Le 12/03/2024 à 23:28, Rick Edgecombe a écrit :
> 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 member
> manually, so if new members are added they will be uninitialized and the
> core code parsing the struct will see garbage in the new member.
>
> It could be possible to initialize the new member manually to 0 at each
> call site. This and a couple other options were discussed, and a working
> consensus (see links) was that in general the best way to accomplish this
> would be via static initialization with designated member 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 member addition
> misses a call site that initializes each member 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 members that are manually set
> to zero, as it would no longer be required.
>
> So to be reduce the chance of bugs via uninitialized members, instead
> simply continue the process to initialize the struct this way tree wide.
> This will zero any unspecified members. Move the member initializers to the
> struct declaration when they are known at that time. Leave the members 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>
> Reviewed-by: Guo Ren <guoren@kernel.org>
Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> Cc: Guo Ren <guoren@kernel.org>
> Cc: linux-csky@vger.kernel.org
> Link: https://lore.kernel.org/lkml/202402280912.33AEE7A9CF@keescook/#t
> Link: https://lore.kernel.org/lkml/j7bfvig3gew3qruouxrh7z7ehjjafrgkbcmg6tcghhfh3rhmzi@wzlcoecgy5rs/
> ---
> v3:
> - Fixed spelling errors in log
> - Be consistent about field vs member in log
>
> 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 compile tested
> only.
>
> There was further discussion that this method of initializing the structs
> while nice in some ways has a greater risk of introducing bugs in some of
> the more complicated callers. Since this version was reviewed my arch
> maintainers already, leave it as was already acknowledged.
>
> Thanks,
>
> Rick
>
> [0] https://lore.kernel.org/lkml/20240226190951.3240433-6-rick.p.edgecombe@intel.com/
> ---
> arch/csky/abiv1/mmap.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/arch/csky/abiv1/mmap.c b/arch/csky/abiv1/mmap.c
> index 6792aca49999..7f826331d409 100644
> --- a/arch/csky/abiv1/mmap.c
> +++ b/arch/csky/abiv1/mmap.c
> @@ -28,7 +28,12 @@ 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 = {
I see you didn't had .flags = 0, which is good. Wondering why you didn't
do the same for powerpc.
> + .length = len,
> + .low_limit = mm->mmap_base,
> + .high_limit = TASK_SIZE,
> + .align_offset = pgoff << PAGE_SHIFT
Usually we leave the comma at the end of the last element so that the
day you add a new element you don't have to change an existing line just
to add a comma.
> + };
>
> /*
> * We only need to do colour alignment if either the I or D
> @@ -61,11 +66,6 @@ arch_get_unmapped_area(struct file *filp, unsigned long addr,
> return addr;
> }
>
> - info.flags = 0;
> - info.length = len;
> - info.low_limit = mm->mmap_base;
> - info.high_limit = TASK_SIZE;
> info.align_mask = do_align ? (PAGE_MASK & (SHMLBA - 1)) : 0;
> - info.align_offset = pgoff << PAGE_SHIFT;
> return vm_unmapped_area(&info);
> }
next prev parent reply other threads:[~2024-03-13 9:04 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20240312222843.2505560-1-rick.p.edgecombe@intel.com>
2024-03-12 22:28 ` [PATCH v3 05/12] csky: Use initializer for struct vm_unmapped_area_info Rick Edgecombe
2024-03-13 9:04 ` Christophe Leroy [this message]
2024-03-12 22:28 ` [PATCH v3 08/12] treewide: " Rick Edgecombe
2024-03-13 3:18 ` Kees Cook
2024-03-13 15:40 ` Edgecombe, Rick P
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=06593c84-2fdb-4921-8d54-dabe13fa2227@csgroup.eu \
--to=christophe.leroy@csgroup.eu \
--cc=Liam.Howlett@oracle.com \
--cc=akpm@linux-foundation.org \
--cc=bp@alien8.de \
--cc=broonie@kernel.org \
--cc=dave.hansen@linux.intel.com \
--cc=debug@rivosinc.com \
--cc=guoren@kernel.org \
--cc=hpa@zytor.com \
--cc=keescook@chromium.org \
--cc=kirill.shutemov@linux.intel.com \
--cc=linux-csky@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=luto@kernel.org \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
--cc=rick.p.edgecombe@intel.com \
--cc=tglx@linutronix.de \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).