Re: [PATCH] xfs: fix GPF in swapfile_activate of file from overlayfs

[Dave Chinner <david@fromorbit.com>]

On Fri, Aug 24, 2018 at 12:02:51PM +0300, Amir Goldstein wrote:
> Since overlayfs implements stacked file operations, f_inode
> is no longer euqivalent to f_mapping->host and xfs should use
> the latter, same as generic_swapfile_activate().

Since when has file_inode() not pointed to the inode backing the
struct file?

> Using f_inode results in an attempt to dereference an xfs_inode
> struct from an ovl_inode pointer:
> 
>  CPU: 0 PID: 2462 Comm: swapon Not tainted
>  4.18.0-xfstests-12721-g33e17876ea4e #3402
>  RIP: 0010:xfs_find_bdev_for_inode+0x23/0x2f
>  Call Trace:
>   xfs_iomap_swapfile_activate+0x1f/0x43
>   __se_sys_swapon+0xb1a/0xee9
> 
> Fixes: d1d04ef8572b ("ovl: stack file ops")

Oh, since about 3 days ago.

So now we've got to deal with a vfs interface change that isn't
documented, hasn't been clearly communicated prior to merging, and
it subtly breaks a subset of callers.

So, please enlighten me with a documentation patch before changing
any XFS code: What is the new behaviour and the rules we must follow
for calling file_inode()?

I'm also interested in understanding whether anyone auditted all the
callers to see if they follow those rules.  There are another
20-odd file_inode() calls in XFS, and ~750 across fs/. How many
other places have been checked for correctness under the new rules?

i.e.  I can't review this patch without also confirming that all the
other file_inode() callers in XFS are still correct, and I can't do
that until I understand what the new behaviour of file_inode() is
and when it is not safe to use.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com