[PATCH 1/1] XFS: __xfs_get_blocks check pointer to the target device

[PATCH 1/1] XFS: __xfs_get_blocks check pointer to the target device

[Ramon de Carvalho Valle]

[-- Attachment #1: Type: text/plain, Size: 976 bytes --]

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

Signed-off-by: Ramon de Carvalho Valle <ramon@risesecurity.org>
Cc: stable <stable@kernel.org>
---
 fs/xfs/linux-2.6/xfs_aops.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/fs/xfs/linux-2.6/xfs_aops.c b/fs/xfs/linux-2.6/xfs_aops.c
index aecf251..bf482d5 100644
--- a/fs/xfs/linux-2.6/xfs_aops.c
+++ b/fs/xfs/linux-2.6/xfs_aops.c
@@ -1419,7 +1419,11 @@ __xfs_get_blocks(
 	 * 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 = iomap.iomap_target->bt_bdev;
+	if (!iomap.iomap_target)
+		return -XFS_ERROR(EIO);
+
+	if (iomap.iomap_flags & IOMAP_REALTIME)
+		bh_result->b_bdev = iomap.iomap_target->bt_bdev;
 
 	/*
 	 * If we previously allocated a block out beyond eof and we are now
-- 
1.5.6.3


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 1/1] XFS: __xfs_get_blocks check pointer to the target device

[Christoph Hellwig]

On Mon, Aug 03, 2009 at 05:03:28PM -0300, Ramon de Carvalho Valle wrote:
> 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.

Re: [PATCH 1/1] XFS: __xfs_get_blocks check pointer to the target device

[Ramon de Carvalho Valle]

[-- Attachment #1: Type: text/plain, Size: 3609 bytes --]

On Mon, 2009-08-03 at 17:49 -0400, Christoph Hellwig wrote:
> On Mon, Aug 03, 2009 at 05:03:28PM -0300, Ramon de Carvalho Valle wrote:
> > 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:

--
iomap.iomap_target = 0000000000000000
Oops: Kernel access of bad area, sig: 11 [#1]
SMP NR_CPUS=128 NUMA PSERIES LPAR 
Modules linked in: xfs_quota xfs ipv6 nfs lockd nfs_acl sunrpc apparmor loop dm_mod ehea ibmvscsic sg ipr libata firmware_class sd_mod scsi_mod
NIP: D000000000532270 LR: D000000000532268 CTR: 0000000000EEBBA0
REGS: c0000000ea2b7250 TRAP: 0300   Not tainted  (2.6.16.60-0.49.3.ramon-ppc64)
MSR: 8000000000009032 <EE,ME,IR,DR>  CR: 22244882  XER: 00000007
DAR: 0000000000000008, DSISR: 0000000040000000
TASK = c00000000f2e4d90[4369] 'run_test' THREAD: c0000000ea2b4000 CPU: 1
GPR00: D000000000532268 C0000000EA2B74D0 D000000000593F20 0000000000000029 
GPR04: 8000000000009032 0000000000000000 0000000000000000 00000000000D360A 
GPR08: 80003FBFF900000C 0000000000000000 C0000000FEFEBBE8 C0000000004F6478 
GPR12: 0000000000004000 C0000000004C3800 0000000000000005 D00000000057EC20 
GPR16: C0000000F5122480 C0000000EA7F55B0 0000000000000400 C0000000EA7F55B0 
GPR20: D00000000058A798 0000000000000000 C0000000EBF868C8 0000000000000000 
GPR24: 0000000000000000 0000000000000001 0000000000000000 0000000000000000 
GPR28: C0000000EA7F55B0 C0000000EA2B7548 D00000000058C578 C0000000EBF868C8 
NIP [D000000000532270] .__xfs_get_blocks+0x170/0x2fc [xfs]
LR [D000000000532268] .__xfs_get_blocks+0x168/0x2fc [xfs]
Call Trace:
[C0000000EA2B74D0] [D000000000532268] .__xfs_get_blocks+0x168/0x2fc [xfs] (unreliable)
[C0000000EA2B75C0] [C0000000000D8EBC] .__block_prepare_write+0x198/0x520
[C0000000EA2B76C0] [C0000000000D9818] .block_prepare_write+0x34/0x64
[C0000000EA2B7740] [D000000000531798] .xfs_vm_prepare_write+0x2c/0x44 [xfs]
[C0000000EA2B77C0] [C0000000000A2804] .generic_file_buffered_write+0x300/0x7fc
[C0000000EA2B7960] [D00000000053CB6C] .xfs_write+0x67c/0xa64 [xfs]
[C0000000EA2B7AE0] [D0000000005379B0] .xfs_file_aio_write+0x8c/0xa0 [xfs]
[C0000000EA2B7B60] [C0000000000D4518] .do_sync_write+0xd0/0x12c
[C0000000EA2B7CE0] [C0000000000D53BC] .vfs_write+0x130/0x218
[C0000000EA2B7D80] [C0000000000D55B0] .SyS_write+0x58/0xa0
[C0000000EA2B7E30] [C00000000000871C] syscall_exit+0x0/0x40
Instruction dump:
40a2fff4 38000200 7d20f8a8 7d290378 7d20f9ad 40a2fff4 48000118 e87e8058 
e8810080 4800e80d e8410028 e9210080 <e8090008> f81f0030 41920048 e81f0000 
--

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.

-Ramon

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 1/1] XFS: __xfs_get_blocks check pointer to the target device

[Christoph Hellwig]

On Mon, Aug 03, 2009 at 11:00:41PM -0300, Ramon de Carvalho Valle wrote:
> On Mon, 2009-08-03 at 17:49 -0400, Christoph Hellwig wrote:
> > On Mon, Aug 03, 2009 at 05:03:28PM -0300, Ramon de Carvalho Valle wrote:
> > > 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:

So you actually introduced the RT flag in the inode on a filesystem
where it can't happen software-wise.

I'd rather deal with this early on to protect the invariant that's
guaranteed inside the xfs code.

Something like the following untested patch:

Index: linux-2.6/fs/xfs/xfs_inode.c
===================================================================
--- linux-2.6.orig/fs/xfs/xfs_inode.c	2009-08-04 16:19:40.778532419 +0200
+++ linux-2.6/fs/xfs/xfs_inode.c	2009-08-04 16:28:06.352533058 +0200
@@ -343,6 +343,16 @@ xfs_iformat(
 		return XFS_ERROR(EFSCORRUPTED);
 	}
 
+	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, has realtime flag set.",
+			ip->i_ino);
+		XFS_CORRUPTION_ERROR("xfs_iformat(realtime)",
+				     XFS_ERRLEVEL_LOW, ip->i_mount, dip);
+		return XFS_ERROR(EFSCORRUPTED);
+	}
+
 	switch (ip->i_d.di_mode & S_IFMT) {
 	case S_IFIFO:
 	case S_IFCHR:

Re: [PATCH 1/1] XFS: __xfs_get_blocks check pointer to the target device

[Eric Sandeen]

Ramon de Carvalho Valle wrote:
> On Mon, 2009-08-03 at 17:49 -0400, Christoph Hellwig wrote:
>> On Mon, Aug 03, 2009 at 05:03:28PM -0300, Ramon de Carvalho Valle wrote:
>>> 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:

...

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

> 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.

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

Thanks,
-Eric

Re: [PATCH 1/1] XFS: __xfs_get_blocks check pointer to the target device

[Ramon de Carvalho Valle]

[-- Attachment #1: Type: text/plain, Size: 2036 bytes --]

On Tue, 2009-08-04 at 11:25 -0500, Eric Sandeen wrote:
> Ramon de Carvalho Valle wrote:
> > On Mon, 2009-08-03 at 17:49 -0400, Christoph Hellwig wrote:
> >> On Mon, Aug 03, 2009 at 05:03:28PM -0300, Ramon de Carvalho Valle wrote:
> >>> 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:
> 
> ...
> 
> ahah, useful information that would have been great in the original
> patch submission.  :)

Sorry about that. :)

> 
> > 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.
> 
> Can you test hch's patch w/ your fuzzed image?

I tested it on my SLES 10 SP3 (Kernel 2.6.16.60-0.49.3.ramon-ppc64) and
it successfully fixed the NULL pointer dereference.

I did some small modifications to the patch to xfs error print the flags
value and fixed the xfs error report tags. I am submitting the Christoph
patch again.

Thanks Christoph and Eric.

-Ramon

> 
> Thanks,
> -Eric

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[PATCH 1/1] XFS: xfs_iformat realtime device target pointer check

[Ramon de Carvalho Valle]

[-- Attachment #1: Type: text/plain, Size: 2918 bytes --]

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 <hch@infradead.org>
Signed-off-by: Ramon de Carvalho Valle <ramon@risesecurity.org>
Cc: stable <stable@kernel.org>
---
 fs/xfs/xfs_inode.c |   23 +++++++++++++++++------
 1 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 1f22d65..37d3ee5 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -343,13 +343,24 @@ xfs_iformat(
 		return XFS_ERROR(EFSCORRUPTED);
 	}
 
+	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);
+		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) {
@@ -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);
-- 
1.5.6.3


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 1/1] XFS: xfs_iformat realtime device target pointer check

[Eric Sandeen]

Ramon de Carvalho Valle wrote:
> 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 <hch@infradead.org>
> Signed-off-by: Ramon de Carvalho Valle <ramon@risesecurity.org>
> Cc: stable <stable@kernel.org>
> ---
>  fs/xfs/xfs_inode.c |   23 +++++++++++++++++------
>  1 files changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 1f22d65..37d3ee5 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -343,13 +343,24 @@ xfs_iformat(
>  		return XFS_ERROR(EFSCORRUPTED);
>  	}
>  
> +	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

> +		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) {
> @@ -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);

Re: [PATCH 1/1] XFS: xfs_iformat realtime device target pointer check

[Ramon de Carvalho Valle]

[-- Attachment #1: Type: text/plain, Size: 3959 bytes --]

On Tue, 2009-08-04 at 14:11 -0500, Eric Sandeen wrote:
> Ramon de Carvalho Valle wrote:
> > 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 <hch@infradead.org>
> > Signed-off-by: Ramon de Carvalho Valle <ramon@risesecurity.org>
> > Cc: stable <stable@kernel.org>
> > ---
> >  fs/xfs/xfs_inode.c |   23 +++++++++++++++++------
> >  1 files changed, 17 insertions(+), 6 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index 1f22d65..37d3ee5 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -343,13 +343,24 @@ xfs_iformat(
> >  		return XFS_ERROR(EFSCORRUPTED);
> >  	}
> >  
> > +	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.

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

> 
> -Eric
> 
> > +		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) {
> > @@ -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);
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 1/1] XFS: xfs_iformat realtime device target pointer check

[Eric Sandeen]

Ramon de Carvalho Valle wrote:
> On Tue, 2009-08-04 at 14:11 -0500, Eric Sandeen wrote:
>> Ramon de Carvalho Valle wrote:
>>> 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 <hch@infradead.org>
>>> Signed-off-by: Ramon de Carvalho Valle <ramon@risesecurity.org>
>>> Cc: stable <stable@kernel.org>
>>> ---
>>>  fs/xfs/xfs_inode.c |   23 +++++++++++++++++------
>>>  1 files changed, 17 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
>>> index 1f22d65..37d3ee5 100644
>>> --- a/fs/xfs/xfs_inode.c
>>> +++ b/fs/xfs/xfs_inode.c
>>> @@ -343,13 +343,24 @@ xfs_iformat(
>>>  		return XFS_ERROR(EFSCORRUPTED);
>>>  	}
>>>  
>>> +	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.
> 
> 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

> -Ramon
> 
>> -Eric

Re: [PATCH 1/1] XFS: xfs_iformat realtime device target pointer check

[Ramon de Carvalho Valle]

[-- Attachment #1: Type: text/plain, Size: 2583 bytes --]

On Tue, 2009-08-04 at 23:15 -0500, Eric Sandeen wrote:
> Ramon de Carvalho Valle wrote:
> > On Tue, 2009-08-04 at 14:11 -0500, Eric Sandeen wrote:
> >> Ramon de Carvalho Valle wrote:
> >>> 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 <hch@infradead.org>
> >>> Signed-off-by: Ramon de Carvalho Valle <ramon@risesecurity.org>
> >>> Cc: stable <stable@kernel.org>
> >>> ---
> >>>  fs/xfs/xfs_inode.c |   23 +++++++++++++++++------
> >>>  1 files changed, 17 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> >>> index 1f22d65..37d3ee5 100644
> >>> --- a/fs/xfs/xfs_inode.c
> >>> +++ b/fs/xfs/xfs_inode.c
> >>> @@ -343,13 +343,24 @@ xfs_iformat(
> >>>  		return XFS_ERROR(EFSCORRUPTED);
> >>>  	}
> >>>  
> >>> +	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.
> > 
> > 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.

Yes, whatever they decide should be ok. Thanks for your feedback Eric.

-Ramon

> 
> Thanks,
> -Eric
> 
> > -Ramon
> > 
> >> -Eric
> 
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 1/1] XFS: xfs_iformat realtime device target pointer check

[Christoph Hellwig]

On Tue, Aug 04, 2009 at 03:51:38PM -0300, Ramon de Carvalho Valle wrote:
> 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.

Re: [PATCH 1/1] XFS: xfs_iformat realtime device target pointer check

[Ramon de Carvalho Valle]

[-- Attachment #1: Type: text/plain, Size: 864 bytes --]

On Wed, 2009-08-05 at 11:17 -0400, Christoph Hellwig wrote:
> On Tue, Aug 04, 2009 at 03:51:38PM -0300, Ramon de Carvalho Valle wrote:
> > 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.

Great. Thanks.

> 
> 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.
> 

Printing the flags is just for debugging purposes and it keeps the code
consistent with the other calls to xfs_fs_repair_cmn_err.

-Ramon

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 1/1] XFS: xfs_iformat realtime device target pointer check

[Felix Blyakher]

On Aug 4, 2009, at 11:15 PM, Eric Sandeen wrote:

> Ramon de Carvalho Valle wrote:
>> On Tue, 2009-08-04 at 14:11 -0500, Eric Sandeen wrote:
>>> Ramon de Carvalho Valle wrote:
>>>> 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 <hch@infradead.org>
>>>> Signed-off-by: Ramon de Carvalho Valle <ramon@risesecurity.org>
>>>> Cc: stable <stable@kernel.org>
>>>> ---
>>>> fs/xfs/xfs_inode.c |   23 +++++++++++++++++------
>>>> 1 files changed, 17 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
>>>> index 1f22d65..37d3ee5 100644
>>>> --- a/fs/xfs/xfs_inode.c
>>>> +++ b/fs/xfs/xfs_inode.c
>>>> @@ -343,13 +343,24 @@ xfs_iformat(
>>>> 		return XFS_ERROR(EFSCORRUPTED);
>>>> 	}
>>>>
>>>> +	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.
>>
>> 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.

Agree with Eric, see the benefits of both approaches, but I think,
it'll be cleaner without shifting the numbering of all messages.
Otherwise, looks good.

Felix

>
>
> Thanks,
> -Eric
>
>> -Ramon
>>
>>> -Eric
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux- 
> kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/