[v2,01/29] lustre: llite: ll_fault fixes
diff mbox series

Message ID 1558356671-29599-2-git-send-email-jsimmons@infradead.org
State New
Headers show
Series
  • More lustre patches
Related show

Commit Message

James Simmons May 20, 2019, 12:50 p.m. UTC
From: Patrick Farrell <pfarrell@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.

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.

Signed-off-by: Patrick Farrell <pfarrell@whamcloud.com>
WC-bug-id: https://jira.whamcloud.com/browse/LU-11403
Reviewed-on: https://review.whamcloud.com/34247
Reviewed-by: Alex Zhuravlev <bzzz@whamcloud.com>
Reviewed-by: Alexander Zarochentsev <c17826@cray.com>
Reviewed-by: Oleg Drokin <green@whamcloud.com>
Signed-off-by: James Simmons <jsimmons@infradead.org>
---
 fs/lustre/llite/llite_mmap.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

NeilBrown May 22, 2019, 3:54 a.m. UTC | #1
On Mon, May 20 2019, James Simmons wrote:

> From: Patrick Farrell <pfarrell@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?

(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@whamcloud.com>
> WC-bug-id: https://jira.whamcloud.com/browse/LU-11403
> Reviewed-on: https://review.whamcloud.com/34247
> Reviewed-by: Alex Zhuravlev <bzzz@whamcloud.com>
> Reviewed-by: Alexander Zarochentsev <c17826@cray.com>
> Reviewed-by: Oleg Drokin <green@whamcloud.com>
> Signed-off-by: James Simmons <jsimmons@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
Patrick Farrell May 22, 2019, 12:48 p.m. UTC | #2
Neil,

If you can’t find any, I imagine they don’t exist, at least in your branch, given that difference you cited.

The particular case we had is here:
https://jira.whamcloud.com/browse/LU-11403

Which is when the file exists but has no striping info, and hence no data.

- Patrick
James Simmons May 22, 2019, 7:06 p.m. UTC | #3
> > From: Patrick Farrell <pfarrell@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@whamcloud.com>
> > WC-bug-id: https://jira.whamcloud.com/browse/LU-11403
> > Reviewed-on: https://review.whamcloud.com/34247
> > Reviewed-by: Alex Zhuravlev <bzzz@whamcloud.com>
> > Reviewed-by: Alexander Zarochentsev <c17826@cray.com>
> > Reviewed-by: Oleg Drokin <green@whamcloud.com>
> > Signed-off-by: James Simmons <jsimmons@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
>
NeilBrown May 22, 2019, 11:26 p.m. UTC | #4
On Wed, May 22 2019, Patrick Farrell wrote:

> Neil,
>
> If you can’t find any, I imagine they don’t exist, at least in your branch, given that difference you cited.
>
> The particular case we had is here:
> https://jira.whamcloud.com/browse/LU-11403
>
> Which is when the file exists but has no striping info, and hence no data.

Hi Patrick,
Thanks for the reply.  Looking at that issue it appears that the root
cause is -EFAULT being returned from lov_io_init_empty, which then got
returned by ll_fault_io_init and then was converted by to_fault_error
into VM_FAULT_NOPAGE.

The test

 	if (!(result & (VM_FAULT_RETRY | VM_FAULT_ERROR | VM_FAULT_LOCKED))) {

in ll_fault() doesn't notice VM_FAULT_NOPAGE (it is not part of
VM_FAULT_ERROR), so the 'then' branch is run, which triggers the
problem.

Just changing to_fault_error() to convert  -EFAULT to VM_FAULT_SIGBUS,
as you did, cause the 'if' to do "the right thing", as VM_FAULT_SIGBUS
is one of the flags in VM_FAULT_ERROR.

So the extra test on vmf->page is redundant.  If there has been no
error, and we don't need to retry, then vmf->page *must* exist.


I've changed the commit message to explain this, so what I have now is
below.

Thanks,
NeilBrown

From: Patrick Farrell <pfarrell@whamcloud.com>
Date: Mon, 20 May 2019 08:50:43 -0400
Subject: [PATCH] lustre: llite: ll_fault fix

Error conditions in the fault path such as a fault on a file without
stripes (see lov_io_init_emtpy()) can cause us to
not return a page in vm_fault, and to report -EFAULT.

-EFAULT is currently mapped to VM_FAULT_NOPAGE by to_fault_error(),
and ll_fault doesn't recognize this as an error which might leave
->page unset, and tries to dereference vmf->page.

VM_FAULT_NOPAGE is *not* a valid status for .fault, only for
.page_mkwrite and .pfn_mkwrite.  So change to_fault_error() to
instead map -EFAULT to VM_FAULT_SIGBUS.  This is part of
VM_FAULT_ERROR, so ll_fault will *not* dereference vmf->page when that
error code is set.

Signed-off-by: Patrick Farrell <pfarrell@whamcloud.com>
WC-bug-id: https://jira.whamcloud.com/browse/LU-11403
Reviewed-on: https://review.whamcloud.com/34247
Reviewed-by: Alex Zhuravlev <bzzz@whamcloud.com>
Reviewed-by: Alexander Zarochentsev <c17826@cray.com>
Reviewed-by: Oleg Drokin <green@whamcloud.com>
Signed-off-by: James Simmons <jsimmons@infradead.org>
Signed-off-by: NeilBrown <neilb@suse.com>
---
 fs/lustre/llite/llite_mmap.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/lustre/llite/llite_mmap.c b/fs/lustre/llite/llite_mmap.c
index 1865db1237ce..59fb40029306 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;
Patrick Farrell May 23, 2019, 12:13 a.m. UTC | #5
Neil,

Memory has surfaced here.  There was a not-documented-where-I-can-find-it report of a null pointer on “page” here that came in around the same time running - I believe - racer (a big messy “do random things from many threads” test).  No obvious explanation for how we got the problem but I added the check since it was simple enough.

But I’m perfectly comfortable with you removing it and seeing if something occurs again.

- Patrick

Patch
diff mbox series

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 */