XFS: __xfs_get_blocks check pointer to the target device

The __xfs_get_blocks function does not check if the pointer to the target device is valid before dereferencing it.

Quoted 2 lines · Ramon de Carvalho Valle

The __xfs_get_blocks function does not check if the pointer to the target device is valid before dereferencing it.

It should never be zero. It's set by xfs_imap_to_bmap to either mp->m_ddev_targp which is always allocated, or to mp->m_rtdev_targp which is always allocated if we have a realtime device, and XFS_IS_REALTIME_INODE should only be true in that case.

Quoted 9 lines · Christoph Hellwig

Quoted 2 lines · Ramon de Carvalho Valle

The __xfs_get_blocks function does not check if the pointer to the target device is valid before dereferencing it.

It should never be zero. It's set by xfs_imap_to_bmap to either mp->m_ddev_targp which is always allocated, or to mp->m_rtdev_targp which is always allocated if we have a realtime device, and XFS_IS_REALTIME_INODE should only be true in that case.

While testing XFS code with a modified version of fsfuzzer on SLES 10 SP3 (Kernel 2.6.16.60-0.49.3.ramon-ppc64), I came across the following Oops:

Quoted 8 lines · Christoph Hellwig

Quoted 2 lines · Ramon de Carvalho Valle

The __xfs_get_blocks function does not check if the pointer to the target device is valid before dereferencing it.

It should never be zero. It's set by xfs_imap_to_bmap to either mp->m_ddev_targp which is always allocated, or to mp->m_rtdev_targp which is always allocated if we have a realtime device, and XFS_IS_REALTIME_INODE should only be true in that case.

While testing XFS code with a modified version of fsfuzzer on SLES 10 SP3 (Kernel 2.6.16.60-0.49.3.ramon-ppc64), I came across the following Oops:

I added a printk() line just before the:

	bh_result->b_bdev = iomap.iomap_target->bt_bdev;

and as we can see iomap.iomap_target is NULL.

My guess is that the XFS_DIFLAG_REALTIME flag is being set incorrectly on the xfs inode structure, setting iomapp->iomap_target to the wrong device pointer (probably NULL).

I don't know if this is the best place to add a check, neither if returning -XFS_ERROR(EIO) is correct at this point. Maybe doing:

	if (iomap.iomap_target && omap.iomap_flags & IOMAP_REALTIME)
		bh_result->b_bdev = iomap.iomap_target->bt_bdev;

would be a better solution.

Quoted 10 lines · Ramon de Carvalho Valle

Quoted 8 lines · Christoph Hellwig

Quoted 2 lines · Ramon de Carvalho Valle

The __xfs_get_blocks function does not check if the pointer to the target device is valid before dereferencing it.

It should never be zero. It's set by xfs_imap_to_bmap to either mp->m_ddev_targp which is always allocated, or to mp->m_rtdev_targp which is always allocated if we have a realtime device, and XFS_IS_REALTIME_INODE should only be true in that case.

Quoted 3 lines

While testing XFS code with a modified version of fsfuzzer on SLES 10 SP3 (Kernel 2.6.16.60-0.49.3.ramon-ppc64), I came across the following Oops:

...

ahah, useful information that would have been great in the original patch submission. :)

Quoted 14 lines

I added a printk() line just before the:

	bh_result->b_bdev = iomap.iomap_target->bt_bdev;

and as we can see iomap.iomap_target is NULL.

My guess is that the XFS_DIFLAG_REALTIME flag is being set incorrectly on the xfs inode structure, setting iomapp->iomap_target to the wrong device pointer (probably NULL).

I don't know if this is the best place to add a check, neither if returning -XFS_ERROR(EIO) is correct at this point. Maybe doing:

Quoted 4 lines

	if (iomap.iomap_target && omap.iomap_flags & IOMAP_REALTIME)
		bh_result->b_bdev = iomap.iomap_target->bt_bdev;

would be a better solution.

Can you test hch's patch w/ your fuzzed image?

Thanks,
-Eric

The xfs_iformat function does not check if the realtime device target pointer is valid when the XFS_DIFLAG_REALTIME flag is set on the ondisk inode structure.

Signed-off-by Christoph Hellwig <[email protected]>

Signed-off-by Ramon de Carvalho Valle <[email protected]>

Cc stable <[email protected]>

---
 fs/xfs/xfs_inode.c |   23 +++++++++++++++++------
 1 files changed, 17 insertions(+), 6 deletions(-)

fs/xfs/xfs_inode.c +0 -0

index 1f22d65..37d3ee5 100644
@@ -343,13 +343,24 @@ xfs_iformat(
 		return XFS_ERROR(EFSCORRUPTED);

fs/xfs/xfs_inode.c +8 -5

+		return XFS_ERROR(EFSCORRUPTED);
+	}
+
 	switch (ip->i_d.di_mode & S_IFMT) {
 	case S_IFIFO:
 	case S_IFCHR:
 	case S_IFBLK:
 	case S_IFSOCK:
 		if (unlikely(dip->di_format != XFS_DINODE_FMT_DEV)) {
-			XFS_CORRUPTION_ERROR("xfs_iformat(3)", XFS_ERRLEVEL_LOW,
+			XFS_CORRUPTION_ERROR("xfs_iformat(4)", XFS_ERRLEVEL_LOW,
 					      ip->i_mount, dip);
 			return XFS_ERROR(EFSCORRUPTED);
 		}

@@ -371,7 +382,7 @@ xfs_iformat(
 					"corrupt inode %Lu "
 					"(local format for regular file).",
 					(unsigned long long) ip->i_ino);
-				XFS_CORRUPTION_ERROR("xfs_iformat(4)",
+				XFS_CORRUPTION_ERROR("xfs_iformat(5)",
 						     XFS_ERRLEVEL_LOW,
 						     ip->i_mount, dip);
 				return XFS_ERROR(EFSCORRUPTED);

@@ -384,7 +395,7 @@ xfs_iformat(
 					"(bad size %Ld for local inode).",
 					(unsigned long long) ip->i_ino,
 					(long long) di_size);
-				XFS_CORRUPTION_ERROR("xfs_iformat(5)",
+				XFS_CORRUPTION_ERROR("xfs_iformat(6)",
 						     XFS_ERRLEVEL_LOW,
 						     ip->i_mount, dip);
 				return XFS_ERROR(EFSCORRUPTED);

@@ -400,14 +411,14 @@ xfs_iformat(
 			error = xfs_iformat_btree(ip, dip, XFS_DATA_FORK);
 			break;
 		default:
-			XFS_ERROR_REPORT("xfs_iformat(6)", XFS_ERRLEVEL_LOW,
+			XFS_ERROR_REPORT("xfs_iformat(7)", XFS_ERRLEVEL_LOW,
 					 ip->i_mount);
 			return XFS_ERROR(EFSCORRUPTED);
 		}
 		break;
 
 	default:
-		XFS_ERROR_REPORT("xfs_iformat(7)", XFS_ERRLEVEL_LOW, ip->i_mount);
+		XFS_ERROR_REPORT("xfs_iformat(8)", XFS_ERRLEVEL_LOW, ip->i_mount);
 		return XFS_ERROR(EFSCORRUPTED);
 	}
 	if (error) {

Quoted 17 lines · Ramon de Carvalho Valle

The xfs_iformat function does not check if the realtime device target pointer is valid when the XFS_DIFLAG_REALTIME flag is set on the ondisk inode structure.

Signed-off-by Christoph Hellwig <[email protected]>

Signed-off-by Ramon de Carvalho Valle <[email protected]>

Cc stable <[email protected]>

---
 fs/xfs/xfs_inode.c |   23 +++++++++++++++++------
 1 files changed, 17 insertions(+), 6 deletions(-)

fs/xfs/xfs_inode.c +0 -0

index 1f22d65..37d3ee5 100644
@@ -343,13 +343,24 @@ xfs_iformat(
 		return XFS_ERROR(EFSCORRUPTED);

fs/xfs/xfs_inode.c xfs_iformat( · line 353

Quoted 10 lines

fs/xfs/xfs_inode.c +8 -0

 	}
 
+	if (unlikely((ip->i_d.di_flags & XFS_DIFLAG_REALTIME) &&
+			!ip->i_mount->m_rtdev_targp)) {
+		xfs_fs_repair_cmn_err(CE_WARN, ip->i_mount,
+			"corrupt dinode %Lu, flags = 0x%x.",
+			(unsigned long long)ip->i_ino,
+			ip->i_d.di_flags);
+		XFS_CORRUPTION_ERROR("xfs_iformat(3)", XFS_ERRLEVEL_LOW,
+				     ip->i_mount, dip);

I think I'd rather not change all the corruption text tag ordering; it'll make it harder to track down any common occurrences of "xfs_iformat(X)" corruption in the future if they get renumbered now.

I'd either make this xfs_iformat(2.1) ;) or just leave it as Christoph had. "realtime" is a lot more informative than "3" anyway.

-Eric

Quoted 43 lines

fs/xfs/xfs_inode.c +7 -4

+		return XFS_ERROR(EFSCORRUPTED);
+	}
+
 	switch (ip->i_d.di_mode & S_IFMT) {
 	case S_IFIFO:
 	case S_IFCHR:
 	case S_IFBLK:
 	case S_IFSOCK:
 		if (unlikely(dip->di_format != XFS_DINODE_FMT_DEV)) {
-			XFS_CORRUPTION_ERROR("xfs_iformat(3)", XFS_ERRLEVEL_LOW,
+			XFS_CORRUPTION_ERROR("xfs_iformat(4)", XFS_ERRLEVEL_LOW,
 					      ip->i_mount, dip);
 			return XFS_ERROR(EFSCORRUPTED);
 		}

@@ -371,7 +382,7 @@ xfs_iformat(
 					"corrupt inode %Lu "
 					"(local format for regular file).",
 					(unsigned long long) ip->i_ino);
-				XFS_CORRUPTION_ERROR("xfs_iformat(4)",
+				XFS_CORRUPTION_ERROR("xfs_iformat(5)",
 						     XFS_ERRLEVEL_LOW,
 						     ip->i_mount, dip);
 				return XFS_ERROR(EFSCORRUPTED);

@@ -384,7 +395,7 @@ xfs_iformat(
 					"(bad size %Ld for local inode).",
 					(unsigned long long) ip->i_ino,
 					(long long) di_size);
-				XFS_CORRUPTION_ERROR("xfs_iformat(5)",
+				XFS_CORRUPTION_ERROR("xfs_iformat(6)",
 						     XFS_ERRLEVEL_LOW,
 						     ip->i_mount, dip);
 				return XFS_ERROR(EFSCORRUPTED);

@@ -400,14 +411,14 @@ xfs_iformat(
 			error = xfs_iformat_btree(ip, dip, XFS_DATA_FORK);
 			break;
 		default:
-			XFS_ERROR_REPORT("xfs_iformat(6)", XFS_ERRLEVEL_LOW,
+			XFS_ERROR_REPORT("xfs_iformat(7)", XFS_ERRLEVEL_LOW,
 					 ip->i_mount);
 			return XFS_ERROR(EFSCORRUPTED);
 		}
 		break;
 

Quoted 7 lines

fs/xfs/xfs_inode.c +1 -1

 	default:
-		XFS_ERROR_REPORT("xfs_iformat(7)", XFS_ERRLEVEL_LOW, ip->i_mount);
+		XFS_ERROR_REPORT("xfs_iformat(8)", XFS_ERRLEVEL_LOW, ip->i_mount);
 		return XFS_ERROR(EFSCORRUPTED);
 	}
 	if (error) {

fs/xfs/xfs_inode.c xfs_iformat( · line 447

Quoted 8 lines

fs/xfs/xfs_inode.c +1 -1

@@ -430,7 +441,7 @@ xfs_iformat(
 				"(bad attr fork size %Ld).",
 				(unsigned long long) ip->i_ino,
 				(long long) size);
-			XFS_CORRUPTION_ERROR("xfs_iformat(8)",
+			XFS_CORRUPTION_ERROR("xfs_iformat(9)",
 					     XFS_ERRLEVEL_LOW,
 					     ip->i_mount, dip);
 			return XFS_ERROR(EFSCORRUPTED);

Quoted 32 lines · Eric Sandeen

Quoted 17 lines · Ramon de Carvalho Valle

The xfs_iformat function does not check if the realtime device target pointer is valid when the XFS_DIFLAG_REALTIME flag is set on the ondisk inode structure.

Signed-off-by Christoph Hellwig <[email protected]>

Signed-off-by Ramon de Carvalho Valle <[email protected]>

Cc stable <[email protected]>

---
 fs/xfs/xfs_inode.c |   23 +++++++++++++++++------
 1 files changed, 17 insertions(+), 6 deletions(-)

fs/xfs/xfs_inode.c +0 -0

index 1f22d65..37d3ee5 100644
@@ -343,13 +343,24 @@ xfs_iformat(
 		return XFS_ERROR(EFSCORRUPTED);

fs/xfs/xfs_inode.c xfs_iformat( · line 353

Quoted 10 lines

fs/xfs/xfs_inode.c +8 -0

 	}
 
+	if (unlikely((ip->i_d.di_flags & XFS_DIFLAG_REALTIME) &&
+			!ip->i_mount->m_rtdev_targp)) {
+		xfs_fs_repair_cmn_err(CE_WARN, ip->i_mount,
+			"corrupt dinode %Lu, flags = 0x%x.",
+			(unsigned long long)ip->i_ino,
+			ip->i_d.di_flags);
+		XFS_CORRUPTION_ERROR("xfs_iformat(3)", XFS_ERRLEVEL_LOW,
+				     ip->i_mount, dip);

I think I'd rather not change all the corruption text tag ordering; it'll make it harder to track down any common occurrences of "xfs_iformat(X)" corruption in the future if they get renumbered now.

Quoted 2 lines

I'd either make this xfs_iformat(2.1) ;) or just leave it as Christoph had. "realtime" is a lot more informative than "3" anyway.

I don't think this is a bad decision, because the corruption errors can be easily identified by the output of xfs_fs_repair_cmn_err and the source line. I think this is a reasonable change that will keep the code clean and consistent.

-Ramon

Quoted 1 line

-Eric

Quoted 33 lines · Ramon de Carvalho Valle

Quoted 32 lines · Eric Sandeen

Quoted 17 lines · Ramon de Carvalho Valle

The xfs_iformat function does not check if the realtime device target pointer is valid when the XFS_DIFLAG_REALTIME flag is set on the ondisk inode structure.

Signed-off-by Christoph Hellwig <[email protected]>

Signed-off-by Ramon de Carvalho Valle <[email protected]>

Cc stable <[email protected]>

---
 fs/xfs/xfs_inode.c |   23 +++++++++++++++++------
 1 files changed, 17 insertions(+), 6 deletions(-)

fs/xfs/xfs_inode.c +0 -0

index 1f22d65..37d3ee5 100644
@@ -343,13 +343,24 @@ xfs_iformat(
 		return XFS_ERROR(EFSCORRUPTED);

fs/xfs/xfs_inode.c xfs_iformat( · line 353

Quoted 10 lines

fs/xfs/xfs_inode.c +8 -0

 	}
 
+	if (unlikely((ip->i_d.di_flags & XFS_DIFLAG_REALTIME) &&
+			!ip->i_mount->m_rtdev_targp)) {
+		xfs_fs_repair_cmn_err(CE_WARN, ip->i_mount,
+			"corrupt dinode %Lu, flags = 0x%x.",
+			(unsigned long long)ip->i_ino,
+			ip->i_d.di_flags);
+		XFS_CORRUPTION_ERROR("xfs_iformat(3)", XFS_ERRLEVEL_LOW,
+				     ip->i_mount, dip);

I think I'd rather not change all the corruption text tag ordering; it'll make it harder to track down any common occurrences of "xfs_iformat(X)" corruption in the future if they get renumbered now.

Quoted 7 lines

Quoted 2 lines

I'd either make this xfs_iformat(2.1) ;) or just leave it as Christoph had. "realtime" is a lot more informative than "3" anyway.

I don't think this is a bad decision, because the corruption errors can be easily identified by the output of xfs_fs_repair_cmn_err and the source line. I think this is a reasonable change that will keep the code clean and consistent.

Until you wind up looking at a problem from some old kernel, or modified vendor kernel, and you realize that now you really don't know which error "xfs_iformat(6)" is anymore, and you either have to go digging through trees that aren't handy, or you just give up and don't bother to help because now it's too much of a pain. ;)

But I can leave it up to the folks @ sgi, I can see both sides of the argument, and I won't care too much either way.

Thanks,
-Eric

Quoted 3 lines

-Ramon

Quoted 1 line

-Eric

The xfs_iformat function does not check if the realtime device target pointer is valid when the XFS_DIFLAG_REALTIME flag is set on the ondisk inode structure.

Quoted 3 lines · Ramon de Carvalho Valle

The xfs_iformat function does not check if the realtime device target pointer is valid when the XFS_DIFLAG_REALTIME flag is set on the ondisk inode structure.

Same as Eric I don't think there's much of a point renumbering the error cases. Instead I'll do another patch with a couple of cleanups in this function replacing all the numbers with short alphabetic tags.

I don't really see the point of printing the flags either, if we have this bit flipped it's pretty clear that we had random corruption of this dinode.

Quoted 33 lines · Ramon de Carvalho Valle

Quoted 32 lines · Eric Sandeen

Quoted 17 lines · Ramon de Carvalho Valle

The xfs_iformat function does not check if the realtime device target pointer is valid when the XFS_DIFLAG_REALTIME flag is set on the ondisk inode structure.

Signed-off-by Christoph Hellwig <[email protected]>

Signed-off-by Ramon de Carvalho Valle <[email protected]>

Cc stable <[email protected]>

---
fs/xfs/xfs_inode.c | 23 +++++++++++++++++------

1 files changed, 17 insertions(+), 6 deletions(-)

fs/xfs/xfs_inode.c +0 -0

index 1f22d65..37d3ee5 100644
@@ -343,13 +343,24 @@ xfs_iformat(
 		return XFS_ERROR(EFSCORRUPTED);

fs/xfs/xfs_inode.c xfs_iformat( · line 353

Quoted 10 lines

fs/xfs/xfs_inode.c +8 -0

 	}
 
+	if (unlikely((ip->i_d.di_flags & XFS_DIFLAG_REALTIME) &&
+			!ip->i_mount->m_rtdev_targp)) {
+		xfs_fs_repair_cmn_err(CE_WARN, ip->i_mount,
+			"corrupt dinode %Lu, flags = 0x%x.",
+			(unsigned long long)ip->i_ino,
+			ip->i_d.di_flags);
+		XFS_CORRUPTION_ERROR("xfs_iformat(3)", XFS_ERRLEVEL_LOW,
+				     ip->i_mount, dip);

I think I'd rather not change all the corruption text tag ordering; it'll make it harder to track down any common occurrences of "xfs_iformat(X)" corruption in the future if they get renumbered now.

Quoted 7 lines

Quoted 2 lines

I'd either make this xfs_iformat(2.1) ;) or just leave it as Christoph had. "realtime" is a lot more informative than "3" anyway.

I don't think this is a bad decision, because the corruption errors can be easily identified by the output of xfs_fs_repair_cmn_err and the source line. I think this is a reasonable change that will keep the code clean and consistent.

Until you wind up looking at a problem from some old kernel, or modified vendor kernel, and you realize that now you really don't know which error "xfs_iformat(6)" is anymore, and you either have to go digging through trees that aren't handy, or you just give up and don't bother to help because now it's too much of a pain. ;)

But I can leave it up to the folks @ sgi, I can see both sides of the argument, and I won't care too much either way.

Thanks,
-Eric

Quoted 2 lines

-Ramon

Quoted 1 line

Quoted 1 line

-Eric

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/