Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences

[Ross Zwisler]

On Tue, Feb 02, 2016 at 11:41:34AM -0700, Ross Zwisler wrote:
> On Tue, Feb 02, 2016 at 12:17:23PM +0100, Jan Kara wrote:
> > On Tue 02-02-16 08:47:30, Dave Chinner wrote:
> > > On Mon, Feb 01, 2016 at 03:51:47PM +0100, Jan Kara wrote:
> > > > On Sat 30-01-16 00:28:33, Matthew Wilcox wrote:
> > > > > On Fri, Jan 29, 2016 at 11:28:15AM -0700, Ross Zwisler wrote:
> > > > > > I guess I need to go off and understand if we can have DAX mappings on such a
> > > > > > device.  If we can, we may have a problem - we can get the block_device from
> > > > > > get_block() in I/O path and the various fault paths, but we don't have access
> > > > > > to get_block() when flushing via dax_writeback_mapping_range().  We avoid
> > > > > > needing it the normal case by storing the sector results from get_block() in
> > > > > > the radix tree.
> > > > > 
> > > > > I think we're doing it wrong by storing the sector in the radix tree; we'd
> > > > > really need to store both the sector and the bdev which is too much data.
> > > > > 
> > > > > If we store the PFN of the underlying page instead, we don't have this
> > > > > problem.  Instead, we have a different problem; of the device going
> > > > > away under us.  I'm trying to find the code which tears down PTEs when
> > > > > the device goes away, and I'm not seeing it.  What do we do about user
> > > > > mappings of the device?
> > > > 
> > > > So I don't have a strong opinion whether storing PFN or sector is better.
> > > > Maybe PFN is somewhat more generic but OTOH turning DAX off for special
> > > > cases like inodes on XFS RT devices would be IMHO fine.
> > > 
> > > We need to support alternate devices.
> > > 
> > > There is a strong case for using the XFS RT device with DAX,
> > > especially for applications that know they are going to always use
> > > large/huge/giant pages to access their data files. The XFS RT device
> > > can guarantee allocation is always aligned to large/huge/giant page
> > > constraints right up to ENOSPC and throughout the production life of
> > > the filesystem. We have no other filesystem capable of providing
> > > such guarantees, which means the XFS RT device is uniquely suited to
> > > certain aplications with DAX...
> > 
> > I see, thanks for explanation. So I'm OK with changing what is stored in
> > the radix tree to accommodate this use case but my reservation that we IHMO
> > have other more pressing things to fix remains...
> 
> IMO this is pretty pressing - without it neither XFS RT devices nor DAX raw
> block devices work.  The case has been made above for XFS RT devices, and with
> DAX raw block devices we really need a fix because the current code will cause
> a kernel BUG when a user tries to fsync/msync a raw block device mmap().  This
> is especially bad because, unlike with filesystems where you mount with the
> dax mount option, there is no opt-in step for raw block devices.
> 
> This has to be fixed - it seems like we either figure out how to fix DAX
> fsync, or we have to disable DAX on raw block devices for a kernel cycle.  I'm
> hoping for the former. :)

Well, I guess a third option would be to keep DAX raw block device in and just
take this patch as a temporary fix:

https://lkml.org/lkml/2016/1/28/679

This would leave XFS RT broken, though, so we may want to explicitly disable
DAX + XFS RT configs for now, but at least we wouldn't have the raw block
device kernel BUG.