xfs: fix incorrect return type for fsdax fault handlers

Commit Message

Darrick J. Wong Oct. 24, 2022, 9:32 p.m. UTC

From: Darrick J. Wong <[e-mail]>

The kernel robot complained about this:

>> fs/xfs/xfs_file.c:1266:31: sparse: sparse: incorrect type in return expression (different base types) @@     expected int @@     got restricted vm_fault_t @@
   fs/xfs/xfs_file.c:1266:31: sparse:     expected int
   fs/xfs/xfs_file.c:1266:31: sparse:     got restricted vm_fault_t
   fs/xfs/xfs_file.c:1314:21: sparse: sparse: incorrect type in assignment (different base types) @@     expected restricted vm_fault_t [usertype] ret @@     got int @@
   fs/xfs/xfs_file.c:1314:21: sparse:     expected restricted vm_fault_t [usertype] ret
   fs/xfs/xfs_file.c:1314:21: sparse:     got int

Fix the incorrect return type for these two functions, and make the
!fsdax version return SIGBUS since there is no vm_fault_t that maps to
zero.

Fixes: ea6c49b784f0 ("xfs: support CoW in fsdax mode")
Signed-off-by: Darrick J. Wong <[e-mail]>
---
 fs/xfs/xfs_file.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Matthew Wilcox (Oracle) Oct. 24, 2022, 9:56 p.m. UTC | #1

On Mon, Oct 24, 2022 at 02:32:18PM -0700, Darrick J. Wong wrote:
> Fix the incorrect return type for these two functions, and make the
> !fsdax version return SIGBUS since there is no vm_fault_t that maps to
> zero.

Hmm?  You should be able to return 0 without sparse complaining.

Darrick J. Wong Oct. 24, 2022, 11:22 p.m. UTC | #2

On Mon, Oct 24, 2022 at 10:56:48PM +0100, Matthew Wilcox wrote:
> On Mon, Oct 24, 2022 at 02:32:18PM -0700, Darrick J. Wong wrote:
> > Fix the incorrect return type for these two functions, and make the
> > !fsdax version return SIGBUS since there is no vm_fault_t that maps to
> > zero.
> 
> Hmm?  You should be able to return 0 without sparse complaining.

What does (vm_fault_t)0 do?  And why wouldn't we want to induce a
segfault if someone calls the fsdax fault function when fsdax isn't
supported?

--D

Darrick J. Wong Oct. 25, 2022, 12:18 a.m. UTC | #3

On Mon, Oct 24, 2022 at 10:56:48PM +0100, Matthew Wilcox wrote:
> On Mon, Oct 24, 2022 at 02:32:18PM -0700, Darrick J. Wong wrote:
> > Fix the incorrect return type for these two functions, and make the
> > !fsdax version return SIGBUS since there is no vm_fault_t that maps to
> > zero.
> 
> Hmm?  You should be able to return 0 without sparse complaining.

Yes I know, but is that the correct return value for "someone is calling
the wrong function, everything is fubar, please stop the world now"?

--D

Matthew Wilcox (Oracle) Oct. 25, 2022, 6:02 p.m. UTC | #4

On Mon, Oct 24, 2022 at 05:18:33PM -0700, Darrick J. Wong wrote:
> On Mon, Oct 24, 2022 at 10:56:48PM +0100, Matthew Wilcox wrote:
> > On Mon, Oct 24, 2022 at 02:32:18PM -0700, Darrick J. Wong wrote:
> > > Fix the incorrect return type for these two functions, and make the
> > > !fsdax version return SIGBUS since there is no vm_fault_t that maps to
> > > zero.
> > 
> > Hmm?  You should be able to return 0 without sparse complaining.
> 
> Yes I know, but is that the correct return value for "someone is calling
> the wrong function, everything is fubar, please stop the world now"?

No, it's "success, but I didn't bother to lock the page myself, please
do it for me", which doesn't really make any sense.  I think in this
case, having not initialised vmf->page, we'd probably take a NULL
ptr dereference in lock_page().

From your changelog, it seemed like you were trying to come up with the
vm_fault_t equivalent of 0, rather than trying to change the semantics
of the !fsdax version.

Darrick J. Wong Oct. 25, 2022, 10:30 p.m. UTC | #5

On Tue, Oct 25, 2022 at 07:02:53PM +0100, Matthew Wilcox wrote:
> On Mon, Oct 24, 2022 at 05:18:33PM -0700, Darrick J. Wong wrote:
> > On Mon, Oct 24, 2022 at 10:56:48PM +0100, Matthew Wilcox wrote:
> > > On Mon, Oct 24, 2022 at 02:32:18PM -0700, Darrick J. Wong wrote:
> > > > Fix the incorrect return type for these two functions, and make the
> > > > !fsdax version return SIGBUS since there is no vm_fault_t that maps to
> > > > zero.
> > > 
> > > Hmm?  You should be able to return 0 without sparse complaining.
> > 
> > Yes I know, but is that the correct return value for "someone is calling
> > the wrong function, everything is fubar, please stop the world now"?
> 
> No, it's "success, but I didn't bother to lock the page myself, please
> do it for me", which doesn't really make any sense.  I think in this
> case, having not initialised vmf->page, we'd probably take a NULL
> ptr dereference in lock_page().

Yes, that's why I don't want to leave the !fsdax stub returning zero.

--D

> From your changelog, it seemed like you were trying to come up with the
> vm_fault_t equivalent of 0, rather than trying to change the semantics
> of the !fsdax version.

Patch

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index c6c80265c0b2..6b328ffaf629 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -1261,7 +1261,7 @@  xfs_file_llseek(
 }
 
 #ifdef CONFIG_FS_DAX
-static int
+static inline vm_fault_t
 xfs_dax_fault(
 	struct vm_fault		*vmf,
 	enum page_entry_size	pe_size,
@@ -1274,14 +1274,14 @@  xfs_dax_fault(
 				&xfs_read_iomap_ops);
 }
 #else
-static int
+static inline vm_fault_t
 xfs_dax_fault(
 	struct vm_fault		*vmf,
 	enum page_entry_size	pe_size,
 	bool			write_fault,
 	pfn_t			*pfn)
 {
-	return 0;
+	return VM_FAULT_SIGBUS;
 }
 #endif