linux-kernel - Re: [PATCH v3] 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 for Android: free password hash cracker in your pocket

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

Date:   Mon, 23 Apr 2018 01:09:48 +0200
From:   Jan Kara <jack@...e.cz>
To:     Souptick Joarder <jrdr.linux@...il.com>
Cc:     viro@...iv.linux.org.uk, mawilcox@...rosoft.com,
        ross.zwisler@...ux.intel.com, akpm@...ux-foundation.org,
        dan.j.williams@...el.com, mhocko@...e.com, jack@...e.cz,
        kirill.shutemov@...ux.intel.com, linux-fsdevel@...r.kernel.org,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] fs: dax: Adding new return type vm_fault_t

On Sun 22-04-18 02:35:29, 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.
> 
> As new function vmf_insert_mixed_mkwrite() only called
> from fs/dax.c, so keeping both the changes in a single
> patch.
> 
> Signed-off-by: Souptick Joarder <jrdr.linux@...il.com>

The patch looks good to me. Just one question:

> diff --git a/mm/memory.c b/mm/memory.c
> index 01f5464..721cfd5 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -1955,12 +1955,19 @@ int vm_insert_mixed(struct vm_area_struct *vma, unsigned long addr,
>  }
>  EXPORT_SYMBOL(vm_insert_mixed);
>  
> -int vm_insert_mixed_mkwrite(struct vm_area_struct *vma, unsigned long addr,
> -			pfn_t pfn)
> +vm_fault_t vmf_insert_mixed_mkwrite(struct vm_area_struct *vma,
> +		unsigned long addr, pfn_t pfn)
>  {
> -	return __vm_insert_mixed(vma, addr, pfn, true);
> +	int err;
> +
> +	err =  __vm_insert_mixed(vma, addr, pfn, true);
> +	if (err == -ENOMEM)
> +		return VM_FAULT_OOM;
> +	if (err < 0 && err != -EBUSY)
> +		return VM_FAULT_SIGBUS;
> +	return VM_FAULT_NOPAGE;
>  }
> -EXPORT_SYMBOL(vm_insert_mixed_mkwrite);
> +EXPORT_SYMBOL(vmf_insert_mixed_mkwrite);

So are we sure that all the callers of this function (and also of
vmf_insert_mixed()) are OK with EBUSY? Because especially in the
vmf_insert_mixed() case other page than the caller provided is in page
tables and thus possibly the caller needs to do some error recovery (such
as drop page refcount) in such case...

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR