Re: [PATCH v8 1/9] dax: fix NULL pointer dereference in __dax_dbg()

[Ross Zwisler]

On Tue, Jan 12, 2016 at 10:34:58AM +0100, Jan Kara wrote:
> On Thu 07-01-16 22:27:51, Ross Zwisler wrote:
> > In __dax_pmd_fault() we currently assume that get_block() will always set
> > bh.b_bdev and we unconditionally dereference it in __dax_dbg().  This
> > assumption isn't always true - when called for reads of holes
> > ext4_dax_mmap_get_block() returns a buffer head where bh->b_bdev is never
> > set.  I hit this BUG while testing the DAX PMD fault path.
> > 
> > Instead, initialize bh.b_bdev before passing bh into get_block().  It is
> > possible that the filesystem's get_block() will update bh.b_bdev, and this
> > is fine - we just want to initialize bh.b_bdev to something reasonable so
> > that the calls to __dax_dbg() work and print something useful.
> > 
> > Signed-off-by: Ross Zwisler <ross.zwisler@xxxxxxxxxxxxxxx>
> > Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
> 
> Looks good. But don't you need to do the same for __dax_fault(),
> dax_zero_page_range() and similar places passing bh to dax functions?
> 
> 								Honza

I don't think we need it anywhere else.  The only reason that we need to
initialize the bh.b_bdev manually in the __dax_pmd_fault() path is that if the
get_block() call ends up finding a hole (so doesn't fill out b_bdev) we still
go through the dax_pmd_dbg() path to print an error message, which uses
b_bdev.  I believe that in the other paths where we hit a hole, such as
__dax_fault(), we don't use b_bdev because we don't have the same error path
prints, and the regular code for handling holes doesn't use b_bdev.

That being said, if you feel like it's cleaner to initialize it everywhere so
everything is consistent and we don't have to worry about it, I'm fine to make
the change.

> > ---
> >  fs/dax.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/fs/dax.c b/fs/dax.c
> > index 7af8797..513bba5 100644
> > --- a/fs/dax.c
> > +++ b/fs/dax.c
> > @@ -624,6 +624,7 @@ int __dax_pmd_fault(struct vm_area_struct *vma, unsigned long address,
> >  	}
> >  
> >  	memset(&bh, 0, sizeof(bh));
> > +	bh.b_bdev = inode->i_sb->s_bdev;
> >  	block = (sector_t)pgoff << (PAGE_SHIFT - blkbits);
> >  
> >  	bh.b_size = PMD_SIZE;
> > -- 
> > 2.5.0
> > 
> > 
> -- 
> Jan Kara <jack@xxxxxxxx>
> SUSE Labs, CR