linux-kernel - Re: [PATCH v41 10/24] mm: Add 'mprotect' hook to struct vm_operations

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: <96318679-3320-8d7c-d178-7fa34ed11fdf@intel.com>
Date:   Sun, 15 Nov 2020 10:36:51 -0800
From:   Dave Hansen <dave.hansen@...el.com>
To:     Matthew Wilcox <willy@...radead.org>,
        Jarkko Sakkinen <jarkko@...nel.org>
Cc:     x86@...nel.org, linux-sgx@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Sean Christopherson <sean.j.christopherson@...el.com>,
        linux-mm@...ck.org, Andrew Morton <akpm@...ux-foundation.org>,
        Mel Gorman <mgorman@...hsingularity.net>,
        Jethro Beekman <jethro@...tanix.com>,
        andriy.shevchenko@...ux.intel.com, asapek@...gle.com, bp@...en8.de,
        cedric.xing@...el.com, chenalexchen@...gle.com,
        conradparker@...gle.com, cyhanish@...gle.com,
        haitao.huang@...el.com, kai.huang@...el.com, kai.svahn@...el.com,
        kmoy@...gle.com, ludloff@...gle.com, luto@...nel.org,
        nhorman@...hat.com, npmccallum@...hat.com, puiterwijk@...hat.com,
        rientjes@...gle.com, tglx@...utronix.de, yaozhangx@...gle.com,
        mikko.ylinen@...el.com
Subject: Re: [PATCH v41 10/24] mm: Add 'mprotect' hook to struct
 vm_operations_struct

On 11/15/20 9:32 AM, Matthew Wilcox wrote:
> On Fri, Nov 13, 2020 at 12:01:21AM +0200, Jarkko Sakkinen wrote:
>> +++ b/include/linux/mm.h
>> @@ -559,6 +559,13 @@ struct vm_operations_struct {
>>  	void (*close)(struct vm_area_struct * area);
>>  	int (*split)(struct vm_area_struct * area, unsigned long addr);
>>  	int (*mremap)(struct vm_area_struct * area);
>> +	/*
>> +	 * Called by mprotect() to make driver-specific permission
>> +	 * checks before mprotect() is finalised.   The VMA must not
>> +	 * be modified.  Returns 0 if eprotect() can proceed.
>> +	 */
> 
> This is the wrong place for this documentation, and it's absurdly
> specific to your implementation.  It should be in
> Documentation/filesystems/locking.rst.

I'll let you and Mel duke that one out:

> https://lore.kernel.org/linux-mm/20201106100409.GD3371@techsingularity.net/

As for 'locking.rst', I didn't even know that was there.  It's also a
_bit_ silly that we would depend on folks finding that for code like SGX
that has nothing to do with filesystems.  If we expect folks to update
that, we need a comment spelling that out in the struct.

The comment also _looks_ reasonably generic to me.  None of the
parameters can be modified, so the impact on the core mm code must only
be from the result of the return code, even if the handler does some
other magic in addition to plain checks.

For one thing, I guess we could take the literal mention of "driver"
out.  vm_operations are certainly supplied by code that isn't a "driver"
per se.

>> +	int (*mprotect)(struct vm_area_struct *vma, unsigned long start,
>> +			unsigned long end, unsigned long newflags);
>> +
>> +		if (vma->vm_ops && vma->vm_ops->mprotect)
>> +			error = vma->vm_ops->mprotect(vma, nstart, tmp, newflags);
>> +		if (error)
>> +			goto out;
>> +
>>  		error = mprotect_fixup(vma, &prev, nstart, tmp, newflags);
>>  		if (error)
>>  			goto out;
>> +
>>  		nstart = tmp;
> 
> Spurious newline added.

I don't actually think this is spurious.  It ends up grouping the code
into the logical chunks with the ->mprotect() and goto in one chunk and
mprotect_fixup() and its goto in another.  Without this newline, the two
logical parts don't look separate.

It's kinda hard to see with just the diff context, though.