linux-kernel - Re: [PATCH v2 5/9] mm: Initialize struct vm_unmapped_area

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <def71a27-2d5f-40da-867e-979648afc4cf@csgroup.eu>
Date: Wed, 28 Feb 2024 13:22:09 +0000
From: Christophe Leroy <christophe.leroy@...roup.eu>
To: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>, "keescook@...omium.org"
	<keescook@...omium.org>
CC: "linux-s390@...r.kernel.org" <linux-s390@...r.kernel.org>,
	"linux-sh@...r.kernel.org" <linux-sh@...r.kernel.org>, "luto@...nel.org"
	<luto@...nel.org>, "dave.hansen@...ux.intel.com"
	<dave.hansen@...ux.intel.com>, "debug@...osinc.com" <debug@...osinc.com>,
	"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
	"Liam.Howlett@...cle.com" <Liam.Howlett@...cle.com>, "mingo@...hat.com"
	<mingo@...hat.com>, "linux-csky@...r.kernel.org"
	<linux-csky@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "tglx@...utronix.de" <tglx@...utronix.de>,
	"linux-mm@...ck.org" <linux-mm@...ck.org>, "kirill.shutemov@...ux.intel.com"
	<kirill.shutemov@...ux.intel.com>, "linux-snps-arc@...ts.infradead.org"
	<linux-snps-arc@...ts.infradead.org>, "linux-parisc@...r.kernel.org"
	<linux-parisc@...r.kernel.org>, "loongarch@...ts.linux.dev"
	<loongarch@...ts.linux.dev>, "hpa@...or.com" <hpa@...or.com>,
	"peterz@...radead.org" <peterz@...radead.org>, "sparclinux@...r.kernel.org"
	<sparclinux@...r.kernel.org>, "linuxppc-dev@...ts.ozlabs.org"
	<linuxppc-dev@...ts.ozlabs.org>, "bp@...en8.de" <bp@...en8.de>,
	"linux-alpha@...r.kernel.org" <linux-alpha@...r.kernel.org>,
	"linux-mips@...r.kernel.org" <linux-mips@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "broonie@...nel.org"
	<broonie@...nel.org>, "x86@...nel.org" <x86@...nel.org>
Subject: 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 +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