[lustre-devel] [PATCH v2 01/29] lustre: llite: ll_fault fixes

[James Simmons]

> > From: Patrick Farrell <pfarrell at whamcloud.com>
> >
> > Various error conditions in the fault path can cause us to
> > not return a page in vm_fault.  Check if it's present
> > before accessing it.
> 
> I cannot find any error conditions that would leave ->page NULL,
> but that wouldn't set one of
>   VM_FAULT_RETRY | VM_FAULT_ERROR | VM_FAULT_LOCKED
> in 'result'.
> 
> Can someone provide an example?

Reproducer is here:

https://review.whamcloud.com/#/c/34247/7/lustre/tests/mmap_mknod_test.c

Just run mmap_mknod_test "file" and it will show this problem.

> (I have seen crashes with vmf->page being NULL, but they were caused
>  by VM_FAULT_RETRY being #defined to 0 as lustre/llite/llite_internal.h
>  still does on OpenSFS lustre)
> 
> >
> > Additionally, it's not valid to return VM_FAULT_NOPAGE for
> > page faults.  The correct return when accessing a page that
> > does not exist is VM_FAULT_SIGBUS.  Correcting this avoids
> > looping infinitely in the testcase.
> 
> I agree with that.  VM_FAULT_NOPAGE is valid for page_mkwrite - and
> ll_page_mkwrite() has separate code to choose VM_FAULT_NOPAGE.
> So the change to to_fault_error() is valid.
> 
> NeilBrown
> 
> 
> >
> > Signed-off-by: Patrick Farrell <pfarrell at whamcloud.com>
> > WC-bug-id: https://jira.whamcloud.com/browse/LU-11403
> > Reviewed-on: https://review.whamcloud.com/34247
> > Reviewed-by: Alex Zhuravlev <bzzz at whamcloud.com>
> > Reviewed-by: Alexander Zarochentsev <c17826 at cray.com>
> > Reviewed-by: Oleg Drokin <green at whamcloud.com>
> > Signed-off-by: James Simmons <jsimmons at infradead.org>
> > ---
> >  fs/lustre/llite/llite_mmap.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/lustre/llite/llite_mmap.c b/fs/lustre/llite/llite_mmap.c
> > index 1865db1..c8e57ad 100644
> > --- a/fs/lustre/llite/llite_mmap.c
> > +++ b/fs/lustre/llite/llite_mmap.c
> > @@ -238,9 +238,6 @@ static inline vm_fault_t to_fault_error(int result)
> >  	case 0:
> >  		result = VM_FAULT_LOCKED;
> >  		break;
> > -	case -EFAULT:
> > -		result = VM_FAULT_NOPAGE;
> > -		break;
> >  	case -ENOMEM:
> >  		result = VM_FAULT_OOM;
> >  		break;
> > @@ -366,7 +363,8 @@ static vm_fault_t ll_fault(struct vm_fault *vmf)
> >  
> >  restart:
> >  	result = __ll_fault(vmf->vma, vmf);
> > -	if (!(result & (VM_FAULT_RETRY | VM_FAULT_ERROR | VM_FAULT_LOCKED))) {
> > +	if (vmf->page &&
> > +	    !(result & (VM_FAULT_RETRY | VM_FAULT_ERROR | VM_FAULT_LOCKED))) {
> >  		struct page *vmpage = vmf->page;
> >  
> >  		/* check if this page has been truncated */
> > -- 
> > 1.8.3.1
>