linux-kernel - Re: [PATCH v6] fs: dax: Adding new return type vm_fault

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: <20180426195831.GA27127@linux.intel.com>
Date:   Thu, 26 Apr 2018 13:58:31 -0600
From:   Ross Zwisler <ross.zwisler@...ux.intel.com>
To:     Souptick Joarder <jrdr.linux@...il.com>
Cc:     willy@...radead.org, jack@...e.cz, viro@...iv.linux.org.uk,
        ross.zwisler@...ux.intel.com, akpm@...ux-foundation.org,
        dan.j.williams@...el.com, mhocko@...e.com,
        kirill.shutemov@...ux.intel.com, linux-fsdevel@...r.kernel.org,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6] fs: dax: Adding new return type vm_fault_t

On Tue, Apr 24, 2018 at 10:17:51PM +0530, Souptick Joarder wrote:
> Use new return type vm_fault_t for fault handler. For
> now, this is just documenting that the function returns
> a VM_FAULT value rather than an errno. Once all instances
> are converted, vm_fault_t will become a distinct type.
> 
> commit 1c8f422059ae ("mm: change return type to vm_fault_t")
> 
> There was an existing bug inside dax_load_hole()
> if vm_insert_mixed had failed to allocate a page table,
> we'd return VM_FAULT_NOPAGE instead of VM_FAULT_OOM.
> With new vmf_insert_mixed() this issue is addressed.
> 
> vm_insert_mixed_mkwrite has inefficiency when it returns
> an error value, driver has to convert it to vm_fault_t
> type. With new vmf_insert_mixed_mkwrite() this limitation
> will be addressed.
> 
> Signed-off-by: Souptick Joarder <jrdr.linux@...il.com>
> Reviewed-by: Jan Kara <jack@...e.cz>
> Reviewed-by: Matthew Wilcox <mawilcox@...rosoft.com>

Sure, this looks correct.  You can add:

Reviewed-by: Ross Zwisler <ross.zwisler@...ux.intel.com>

I noticed that we have the following status translation now in 4 places in 2
files:

        if (err == -ENOMEM)
                return VM_FAULT_OOM;
        if (err < 0 && err != -EBUSY)
                return VM_FAULT_SIGBUS;
        return VM_FAULT_NOPAGE;


This happens in vmf_insert_mixed_mkwrite(), vmf_insert_page(),
vmf_insert_mixed() and vmf_insert_pfn().

I think it'd be a good idea to consolidate this translation into an inline
helper, in the spirit of dax_fault_return().  This will ensure that if/when we
start changing this status translation, we won't accidentally miss some of the
places which would make them get out of sync.  No need to fold this into this
patch - it should be a separate change.