Re: [PATCH v2 5/9] mm: Initialize struct vm_unmapped_area_info
Le 28/02/2024 à 18:01, Edgecombe, Rick P a écrit :
> On Wed, 2024-02-28 at 13:22 +, 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
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 3/4] arch: define CONFIG_PAGE_SIZE_*KB on all architectures
On Mon, Feb 26, 2024 at 05:14:13PM +0100, Arnd Bergmann wrote: > From: Arnd Bergmann > > Most architectures only support a single hardcoded page size. In order > to ensure that each one of these sets the corresponding Kconfig symbols, > change over the PAGE_SHIFT definition to the common one and allow > only the hardware page size to be selected. > > Signed-off-by: Arnd Bergmann > --- > arch/alpha/Kconfig | 1 + > arch/alpha/include/asm/page.h | 2 +- > arch/arm/Kconfig | 1 + > arch/arm/include/asm/page.h| 2 +- > arch/csky/Kconfig | 1 + > arch/csky/include/asm/page.h | 2 +- > arch/m68k/Kconfig | 3 +++ > arch/m68k/Kconfig.cpu | 2 ++ > arch/m68k/include/asm/page.h | 6 +- > arch/microblaze/Kconfig| 1 + > arch/microblaze/include/asm/page.h | 2 +- > arch/nios2/Kconfig | 1 + > arch/nios2/include/asm/page.h | 2 +- > arch/openrisc/Kconfig | 1 + > arch/openrisc/include/asm/page.h | 2 +- > arch/riscv/Kconfig | 1 + > arch/riscv/include/asm/page.h | 2 +- > arch/s390/Kconfig | 1 + > arch/s390/include/asm/page.h | 2 +- > arch/sparc/Kconfig | 2 ++ > arch/sparc/include/asm/page_32.h | 2 +- > arch/sparc/include/asm/page_64.h | 3 +-- > arch/um/Kconfig| 1 + > arch/um/include/asm/page.h | 2 +- > arch/x86/Kconfig | 1 + > arch/x86/include/asm/page_types.h | 2 +- > arch/xtensa/Kconfig| 1 + > arch/xtensa/include/asm/page.h | 2 +- > 28 files changed, 32 insertions(+), 19 deletions(-) > diff --git a/arch/openrisc/Kconfig b/arch/openrisc/Kconfig > index fd9bb76a610b..3586cda55bde 100644 > --- a/arch/openrisc/Kconfig > +++ b/arch/openrisc/Kconfig > @@ -25,6 +25,7 @@ config OPENRISC > select GENERIC_CPU_DEVICES > select HAVE_PCI > select HAVE_UID16 > + select HAVE_PAGE_SIZE_8KB > select GENERIC_ATOMIC64 > select GENERIC_CLOCKEVENTS_BROADCAST > select GENERIC_SMP_IDLE_THREAD > diff --git a/arch/openrisc/include/asm/page.h > b/arch/openrisc/include/asm/page.h > index 44fc1fd56717..7925ce09ab5a 100644 > --- a/arch/openrisc/include/asm/page.h > +++ b/arch/openrisc/include/asm/page.h > @@ -18,7 +18,7 @@ > > /* PAGE_SHIFT determines the page size */ > > -#define PAGE_SHIFT 13 > +#define PAGE_SHIFT CONFIG_PAGE_SHIFT > #ifdef __ASSEMBLY__ > #define PAGE_SIZE (1 << PAGE_SHIFT) > #else For the openrisc bits, Acked-by: Stafford Horne ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2 5/9] mm: Initialize struct vm_unmapped_area_info
On Wed, 2024-02-28 at 13:22 +, 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
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2 5/9] mm: Initialize struct vm_unmapped_area_info
On Wed, Feb 28, 2024 at 01:22:09PM +, 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();
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
--
Kees Cook
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2 5/9] mm: Initialize struct vm_unmapped_area_info
Le 27/02/2024 à 21:25, Edgecombe, Rick P a écrit :
> On Tue, 2024-02-27 at 18:16 +, 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
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2 5/9] mm: Initialize struct vm_unmapped_area_info
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();
}
--
Kiryl Shutsemau / Kirill A. Shutemov
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
