linux-kernel - Re: [PATCHv3 15/17] x86/mm: Implement sync_direct

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 25 Jun 2018 09:36:43 -0700
From:   Dave Hansen <dave.hansen@...el.com>
To:     "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
Cc:     Ingo Molnar <mingo@...hat.com>, x86@...nel.org,
        Thomas Gleixner <tglx@...utronix.de>,
        "H. Peter Anvin" <hpa@...or.com>,
        Tom Lendacky <thomas.lendacky@....com>,
        Kai Huang <kai.huang@...ux.intel.com>,
        Jacob Pan <jacob.jun.pan@...ux.intel.com>,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCHv3 15/17] x86/mm: Implement sync_direct_mapping()

On 06/25/2018 02:29 AM, Kirill A. Shutemov wrote:
> On Mon, Jun 18, 2018 at 04:28:27PM +0000, Dave Hansen wrote:
>>>  
>>>  	remove_pagetable(start, end, true, NULL);
>>> +	ret = sync_direct_mapping();
>>> +	WARN_ON(ret);
>>>  }
>>
>> I understand why you implemented it this way, I really do.  It's
>> certainly the quickest way to hack something together and make a
>> standalone piece of code.  But, I don't think it's maintainable.
>>
>> For instance, this call to sync_direct_mapping() could be entirely
>> replaced by a call to:
>>
>> 	for_each_keyid(k)...
>> 		remove_pagetable(start + offset_per_keyid * k,
>> 			         end   + offset_per_keyid * k,
>> 				 true, NULL);
>>
>> No?
> 
> Yes. But what's the point if we need to have the sync routine anyway for
> the add path?

Because you are working to remove the sync routine and make an effort to
share more code with the regular direct map manipulation.  Right?

My point is that this patch did not even make an _effort_ to reuse code
where it would have been quite trivial to do so.  I think such an effort
needs to be put forth before we add 400 more lines of page table
manipulation.

>>>  int __ref arch_remove_memory(u64 start, u64 size, struct vmem_altmap *altmap)
>>> @@ -1290,6 +1295,7 @@ void mark_rodata_ro(void)
>>>  			(unsigned long) __va(__pa_symbol(rodata_end)),
>>>  			(unsigned long) __va(__pa_symbol(_sdata)));
>>>  
>>> +	sync_direct_mapping();
>>>  	debug_checkwx();
>>
>> Huh, checking the return code in some cases and not others.  Curious.
>> Why is it that way?
> 
> There's no sensible way to handle failure in any of these path. But in
> remove path we don't expect the failure -- no allocation required.
> It can only happen if we missed sync_direct_mapping() somewhere else.

So, should we just silently drop the error?  Or, would it be sensible to
make this a WARN_ON_ONCE()?