linux-kernel - Re: [PATCH v3 01/12] mm: Switch mm->get_unmapped

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: <94e818d7-eca8-4067-b39f-81a447d2a50e@csgroup.eu>
Date: Wed, 13 Mar 2024 17:20:11 +0000
From: Christophe Leroy <christophe.leroy@...roup.eu>
To: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>, "debug@...osinc.com"
	<debug@...osinc.com>, "luto@...nel.org" <luto@...nel.org>, "x86@...nel.org"
	<x86@...nel.org>, "Liam.Howlett@...cle.com" <Liam.Howlett@...cle.com>,
	"broonie@...nel.org" <broonie@...nel.org>, "keescook@...omium.org"
	<keescook@...omium.org>, "dave.hansen@...ux.intel.com"
	<dave.hansen@...ux.intel.com>, "hpa@...or.com" <hpa@...or.com>,
	"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>, "mingo@...hat.com"
	<mingo@...hat.com>, "kirill.shutemov@...ux.intel.com"
	<kirill.shutemov@...ux.intel.com>, "bp@...en8.de" <bp@...en8.de>,
	"tglx@...utronix.de" <tglx@...utronix.de>, "peterz@...radead.org"
	<peterz@...radead.org>
CC: "linux-mm@...ck.org" <linux-mm@...ck.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 01/12] mm: Switch mm->get_unmapped_area() to a flag



Le 13/03/2024 à 15:48, Edgecombe, Rick P a écrit :
> On Wed, 2024-03-13 at 07:19 +0000, Christophe Leroy wrote:
>> This patch is quite big and un-easy to follow. Would be worth
>> splitting
>> in several patches if possible. Some of the changes seem to go
>> further
>> than just switching mm->get_unmapped_area() to a flag.
>>
>> First patch could add the new flag and necessary helpers, then
>> following
>> patches could convert sub-systems one by one then last patch would
>> remove mm->get_unmapped_area() once all users are converted.
> 
> So you are saying to do the tracking in both the new flag and mm-
>> get_unmapped_area() during the conversion process and then remove the
> pointer at the end? I guess it could be broken out, but most of the
> conversions are trivial one line changes. Hmm, I'm not sure.
> 
> [snip]
>>
>>>     #ifdef CONFIG_MMU
>>> -       if (!get_area)
>>> -               get_area = current->mm->get_unmapped_area;
>>> +       else
>>> +               return mm_get_unmapped_area(current->mm, file,
>>> orig_addr,
>>> +                                           len, pgoff, flags);
>>>     #endif
>>> -       if (get_area)
>>> -               return get_area(file, orig_addr, len, pgoff,
>>> flags);
>>> +
>>>          return orig_addr;
>>>     }
>>
>> The change looks unclear at first look. Ok after looking a second
>> time
>> it seems to simplify things, but would be better as a separate patch.
>> Don't know.
> 
> Hmm. I think the only way to do it in smaller chunks is to do both
> methods of tracking the direction during the conversion process. And
> then the smaller pieces would be really small. So it would probably
> help for changes like this, but otherwise would generate a lot of
> patches with small changes.

Yes. Maybe the best would be to have a preparation patch to churn this 
function a bit so that when it comes to the conservion it is trivial.

Something like:

	if (pde->proc_ops->proc_get_unmapped_area)
		return pde->proc_ops->proc_get_unmapped_area(file, orig_addr, len, 
pgoff, flags);

#ifdef CONFIG_MMU
	return current->mm->get_unmapped_area(file, orig_addr, len, pgoff, flags);
#endif
	return orig_addr;


Note that a length of 100 chars is now tolerated when it eases reading 
so you should avoid those 3 lines.

And the else inside #ifdef CONFIG_MMU is not needed because 'if' has 
returned.

> 
> The steps are basically:
> 1. Introduce flag and helpers
> 2. convert arch's to use it one by one
> 3. convert callers to use mm_get_unmapped_area() one by one
> 4. remove setting get_unmapped_area in each arch
> 5. remove get_unmapped_area
> 
> Step 3 is where the few non-oneline changes would be, but most would
> still be one liners. 1, 2, 4 and 5 seem simpler as a tree wide patch
> because of the one line changes.

I missed the setting of get_unmapped_area by each arch, you are right it 
might be complicated at the end.

> 
> I don't know any other variations are a ton simpler. Hopefully others
> will weigh in.
> 
> 
> 
> [snip]
>>>     
>>> +unsigned long
>>> +mm_get_unmapped_area(struct mm_struct *mm, struct file *file,
>>> +                    unsigned long addr, unsigned long len,
>>> +                    unsigned long pgoff, unsigned long flags)
>>> +{
>>> +       if (test_bit(MMF_TOPDOWN, &mm->flags))
>>> +               return arch_get_unmapped_area_topdown(file, addr,
>>> len, pgoff, flags);
>>> +       return arch_get_unmapped_area(file, addr, len, pgoff,
>>> flags);
>>> +}
>>
>> This function seems quite simple, wouldn't it be better to make it a
>> static inline ?
> 
> Then all of the arch_get_unmapped_area() and
> arch_get_unmapped_area_topdown() would need to be exported. I think it
> is better to only export the higher level functions.

Right.