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@xxxxxxxxxxxxx
