On Thu, Jan 07, 2016 at 10:34:02AM +0100, Jan Kara wrote:

On Wed 06-01-16 11:14:09, Dan Williams wrote:

quoted

On Wed, Jan 6, 2016 at 10:00 AM, Ross Zwisler
[off-list ref] wrote:

quoted

__dax_dbg() currently assumes that bh->b_bdev is non-NULL, passing it into
bdevname() where is is dereferenced.  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, verify that we have a valid bh->b_bdev, else just say "unknown"
for the block device.

Signed-off-by: Ross Zwisler <redacted>
Cc: Dan Williams <redacted>
---
 fs/dax.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/dax.c b/fs/dax.c
index 7af8797..03cc4a3 100644
--- a/fs/dax.c
+++ b/fs/dax.c

@@ -563,7 +563,12 @@ static void __dax_dbg(struct buffer_head *bh, unsigned long address,
 {
        if (bh) {
                char bname[BDEVNAME_SIZE];
-               bdevname(bh->b_bdev, bname);
+
+               if (bh->b_bdev)
+                       bdevname(bh->b_bdev, bname);
+               else
+                       snprintf(bname, BDEVNAME_SIZE, "unknown");
+
                pr_debug("%s: %s addr: %lx dev %s state %lx start %lld "
                        "length %zd fallback: %s\n", fn, current->comm,
                        address, bname, bh->b_state, (u64)bh->b_blocknr,

I'm assuming there's no danger of a such a buffer_head ever being used
for the bdev parameter to dax_map_atomic()?  Shouldn't we also/instead
go fix ext4 to not send partially filled buffer_heads?

No. The real problem is a long-standing abuse of struct buffer_head to be
used for passing block mapping information (it's on my todo list to remove
that at least from DAX code and use cleaner block mapping interface but
first I want basic DAX functionality to settle down to avoid unnecessary
conflicts). Filesystem is not supposed to touch bh->b_bdev.

That has not been true for a long, long time. e.g. XFS always
rewrites bh->b_bdev in get_blocks because the file may not reside on
the primary block device of the filesystem. i.e.:

        /*
         * If this is a realtime file, data may be on a different device.
         * to that pointed to from the buffer_head b_bdev currently.
         */
        bh_result->b_bdev = xfs_find_bdev_for_inode(inode);

If you need
that filled in, set it yourself in before passing bh to the block mapping
function.

That may be true, but we cannot assume that the bdev coming back
out of get_block is the same one that was passed in.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs