public inbox for linux-xfs@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] [XFS] Validate log feature fields correctly
  2009-03-15 10:52 [PATCH 0/2] " Dave Chinner
@ 2009-03-15 10:52 ` Dave Chinner
  0 siblings, 0 replies; 11+ messages in thread
From: Dave Chinner @ 2009-03-15 10:52 UTC (permalink / raw)
  To: xfs

From: Dave Chinner <dgc@evostor.com>

If the large log sector size feature bit is set in the
superblock by accident (say disk corruption), the then
fields that are now considered valid are not checked on
production kernels. The checks are present as ASSERT
statements so cause a panic on a debug kernel.

Change this so that the fields are validity checked if
the feature bit is set and abort the log mount if the
fields do not contain valid values.

Reported-by: Eric Sesterhenn <snakebyte@gmx.de>
Signed-off-by: Dave Chinner <david@fromorbit.com>
---
 fs/xfs/xfs_log.c |   41 ++++++++++++++++++++++++++++++-----------
 1 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index c8f3008..60e6e63 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -562,9 +562,8 @@ xfs_log_mount(
 	}
 
 	mp->m_log = xlog_alloc_log(mp, log_target, blk_offset, num_bblks);
-	if (!mp->m_log) {
-		cmn_err(CE_WARN, "XFS: Log allocation failed: No memory!");
-		error = ENOMEM;
+	if (IS_ERR(mp->m_log)) {
+		error = -PTR_ERR(mp->m_log);
 		goto out;
 	}
 
@@ -1193,10 +1192,13 @@ xlog_alloc_log(xfs_mount_t	*mp,
 	xfs_buf_t		*bp;
 	int			i;
 	int			iclogsize;
+	int			error = ENOMEM;
 
 	log = kmem_zalloc(sizeof(xlog_t), KM_MAYFAIL);
-	if (!log)
-		return NULL;
+	if (!log) {
+		xlog_warn("XFS: Log allocation failed: No memory!");
+		goto out;
+	}
 
 	log->l_mp	   = mp;
 	log->l_targ	   = log_target;
@@ -1214,19 +1216,35 @@ xlog_alloc_log(xfs_mount_t	*mp,
 	log->l_grant_reserve_cycle = 1;
 	log->l_grant_write_cycle = 1;
 
+	error = EFSCORRUPTED;
 	if (xfs_sb_version_hassector(&mp->m_sb)) {
 		log->l_sectbb_log = mp->m_sb.sb_logsectlog - BBSHIFT;
-		ASSERT(log->l_sectbb_log <= mp->m_sectbb_log);
+		if (log->l_sectbb_log < 0 ||
+		    log->l_sectbb_log > mp->m_sectbb_log) {
+			xlog_warn("XFS: Log sector size (0x%x) out of range.",
+						log->l_sectbb_log);
+			goto out_free_log;
+		}
+
 		/* for larger sector sizes, must have v2 or external log */
-		ASSERT(log->l_sectbb_log == 0 ||
-			log->l_logBBstart == 0 ||
-			xfs_sb_version_haslogv2(&mp->m_sb));
-		ASSERT(mp->m_sb.sb_logsectlog >= BBSHIFT);
+		if (log->l_sectbb_log != 0 &&
+		    (log->l_logBBstart != 0 &&
+		     !xfs_sb_version_haslogv2(&mp->m_sb))) {
+			xlog_warn("XFS: log sector size (0x%x) invalid "
+				  "for configuration.", log->l_sectbb_log);
+			goto out_free_log;
+		}
+		if (mp->m_sb.sb_logsectlog < BBSHIFT) {
+			xlog_warn("XFS: Log sector log (0x%x) too small.",
+						mp->m_sb.sb_logsectlog);
+			goto out_free_log;
+		}
 	}
 	log->l_sectbb_mask = (1 << log->l_sectbb_log) - 1;
 
 	xlog_get_iclog_buffer_size(mp, log);
 
+	error = ENOMEM;
 	bp = xfs_buf_get_empty(log->l_iclog_size, mp->m_logdev_targp);
 	if (!bp)
 		goto out_free_log;
@@ -1326,7 +1344,8 @@ out_free_iclog:
 	xfs_buf_free(log->l_xbuf);
 out_free_log:
 	kmem_free(log);
-	return NULL;
+out:
+	return ERR_PTR(-error);
 }	/* xlog_alloc_log */
 
 
-- 
1.6.2

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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 0/2, RESEND] [XFS] a couple of fixes
@ 2009-03-15 11:25 Dave Chinner
  2009-03-15 11:25 ` [PATCH 1/2] [XFS] Validate log feature fields correctly Dave Chinner
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Dave Chinner @ 2009-03-15 11:25 UTC (permalink / raw)
  To: xfs

The first fixes a fuzzer log corruption crash, and second is
a null pointer dereference I found by inspection.

These should now have the correct sign-off on them.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/2] [XFS] Validate log feature fields correctly
  2009-03-15 11:25 [PATCH 0/2, RESEND] [XFS] a couple of fixes Dave Chinner
@ 2009-03-15 11:25 ` Dave Chinner
  2009-03-15 15:15   ` Christoph Hellwig
  2009-03-15 11:25 ` [PATCH 2/2] [XFS] Fix double free of inode Dave Chinner
  2009-04-06 22:28 ` [PATCH 0/2, RESEND] [XFS] a couple of fixes Christoph Hellwig
  2 siblings, 1 reply; 11+ messages in thread
From: Dave Chinner @ 2009-03-15 11:25 UTC (permalink / raw)
  To: xfs

If the large log sector size feature bit is set in the
superblock by accident (say disk corruption), the then
fields that are now considered valid are not checked on
production kernels. The checks are present as ASSERT
statements so cause a panic on a debug kernel.

Change this so that the fields are validity checked if
the feature bit is set and abort the log mount if the
fields do not contain valid values.

Reported-by: Eric Sesterhenn <snakebyte@gmx.de>
Signed-off-by: Dave Chinner <david@fromorbit.com>
---
 fs/xfs/xfs_log.c |   41 ++++++++++++++++++++++++++++++-----------
 1 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index c8f3008..60e6e63 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -562,9 +562,8 @@ xfs_log_mount(
 	}
 
 	mp->m_log = xlog_alloc_log(mp, log_target, blk_offset, num_bblks);
-	if (!mp->m_log) {
-		cmn_err(CE_WARN, "XFS: Log allocation failed: No memory!");
-		error = ENOMEM;
+	if (IS_ERR(mp->m_log)) {
+		error = -PTR_ERR(mp->m_log);
 		goto out;
 	}
 
@@ -1193,10 +1192,13 @@ xlog_alloc_log(xfs_mount_t	*mp,
 	xfs_buf_t		*bp;
 	int			i;
 	int			iclogsize;
+	int			error = ENOMEM;
 
 	log = kmem_zalloc(sizeof(xlog_t), KM_MAYFAIL);
-	if (!log)
-		return NULL;
+	if (!log) {
+		xlog_warn("XFS: Log allocation failed: No memory!");
+		goto out;
+	}
 
 	log->l_mp	   = mp;
 	log->l_targ	   = log_target;
@@ -1214,19 +1216,35 @@ xlog_alloc_log(xfs_mount_t	*mp,
 	log->l_grant_reserve_cycle = 1;
 	log->l_grant_write_cycle = 1;
 
+	error = EFSCORRUPTED;
 	if (xfs_sb_version_hassector(&mp->m_sb)) {
 		log->l_sectbb_log = mp->m_sb.sb_logsectlog - BBSHIFT;
-		ASSERT(log->l_sectbb_log <= mp->m_sectbb_log);
+		if (log->l_sectbb_log < 0 ||
+		    log->l_sectbb_log > mp->m_sectbb_log) {
+			xlog_warn("XFS: Log sector size (0x%x) out of range.",
+						log->l_sectbb_log);
+			goto out_free_log;
+		}
+
 		/* for larger sector sizes, must have v2 or external log */
-		ASSERT(log->l_sectbb_log == 0 ||
-			log->l_logBBstart == 0 ||
-			xfs_sb_version_haslogv2(&mp->m_sb));
-		ASSERT(mp->m_sb.sb_logsectlog >= BBSHIFT);
+		if (log->l_sectbb_log != 0 &&
+		    (log->l_logBBstart != 0 &&
+		     !xfs_sb_version_haslogv2(&mp->m_sb))) {
+			xlog_warn("XFS: log sector size (0x%x) invalid "
+				  "for configuration.", log->l_sectbb_log);
+			goto out_free_log;
+		}
+		if (mp->m_sb.sb_logsectlog < BBSHIFT) {
+			xlog_warn("XFS: Log sector log (0x%x) too small.",
+						mp->m_sb.sb_logsectlog);
+			goto out_free_log;
+		}
 	}
 	log->l_sectbb_mask = (1 << log->l_sectbb_log) - 1;
 
 	xlog_get_iclog_buffer_size(mp, log);
 
+	error = ENOMEM;
 	bp = xfs_buf_get_empty(log->l_iclog_size, mp->m_logdev_targp);
 	if (!bp)
 		goto out_free_log;
@@ -1326,7 +1344,8 @@ out_free_iclog:
 	xfs_buf_free(log->l_xbuf);
 out_free_log:
 	kmem_free(log);
-	return NULL;
+out:
+	return ERR_PTR(-error);
 }	/* xlog_alloc_log */
 
 
-- 
1.6.2

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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/2] [XFS] Fix double free of inode
  2009-03-15 11:25 [PATCH 0/2, RESEND] [XFS] a couple of fixes Dave Chinner
  2009-03-15 11:25 ` [PATCH 1/2] [XFS] Validate log feature fields correctly Dave Chinner
@ 2009-03-15 11:25 ` Dave Chinner
  2009-03-15 15:13   ` Christoph Hellwig
  2009-04-06 22:28 ` [PATCH 0/2, RESEND] [XFS] a couple of fixes Christoph Hellwig
  2 siblings, 1 reply; 11+ messages in thread
From: Dave Chinner @ 2009-03-15 11:25 UTC (permalink / raw)
  To: xfs

If we fail to initialise the VFS inode in inode_init_always(),
it will call ->delete_inode internally resulting in the inode being
freed. Hence we need to delay the call to inode_init_always()
until after the XFS inode is sufficient set up to handle a
call to ->delete_inode, and then if that fails do not touch
the inode again at all as it has been freed.

Signed-off-by: Dave Chinner <david@fromorbit.com>
---
 fs/xfs/xfs_iget.c |   23 ++++++++++++++---------
 1 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/fs/xfs/xfs_iget.c b/fs/xfs/xfs_iget.c
index 478e587..89b81ee 100644
--- a/fs/xfs/xfs_iget.c
+++ b/fs/xfs/xfs_iget.c
@@ -69,15 +69,6 @@ xfs_inode_alloc(
 	ASSERT(!spin_is_locked(&ip->i_flags_lock));
 	ASSERT(completion_done(&ip->i_flush));
 
-	/*
-	 * initialise the VFS inode here to get failures
-	 * out of the way early.
-	 */
-	if (!inode_init_always(mp->m_super, VFS_I(ip))) {
-		kmem_zone_free(xfs_inode_zone, ip);
-		return NULL;
-	}
-
 	/* initialise the xfs inode */
 	ip->i_ino = ino;
 	ip->i_mount = mp;
@@ -113,6 +104,20 @@ xfs_inode_alloc(
 #ifdef XFS_DIR2_TRACE
 	ip->i_dir_trace = ktrace_alloc(XFS_DIR2_KTRACE_SIZE, KM_NOFS);
 #endif
+	/*
+	* Now initialise the VFS inode. We do this after the xfs_inode
+	* initialisation as internal failures will result in ->destroy_inode
+	* being called and that will pass down through the reclaim path and
+	* free the XFS inode. This path requires the XFS inode to already be
+	* initialised. Hence if this call fails, the xfs_inode has already
+	* been freed and we should not reference it at all in the error
+	* handling.
+	*/
+	if (!inode_init_always(mp->m_super, VFS_I(ip)))
+		return NULL;
+
+	/* prevent anyone from using this yet */
+	VFS_I(ip)->i_state = I_NEW|I_LOCK;
 
 	return ip;
 }
-- 
1.6.2

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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] [XFS] Fix double free of inode
  2009-03-15 11:25 ` [PATCH 2/2] [XFS] Fix double free of inode Dave Chinner
@ 2009-03-15 15:13   ` Christoph Hellwig
  0 siblings, 0 replies; 11+ messages in thread
From: Christoph Hellwig @ 2009-03-15 15:13 UTC (permalink / raw)
  To: Dave Chinner; +Cc: xfs

On Sun, Mar 15, 2009 at 10:25:42PM +1100, Dave Chinner wrote:
> If we fail to initialise the VFS inode in inode_init_always(),
> it will call ->delete_inode internally resulting in the inode being
> freed. Hence we need to delay the call to inode_init_always()
> until after the XFS inode is sufficient set up to handle a
> call to ->delete_inode, and then if that fails do not touch
> the inode again at all as it has been freed.

Looks good, but the changelog should mention the setting of the
I_NEW bit, too.


Reviewed-by: Christoph Hellwig <hch@lst.de>

I suspect this should go into 2.6.29-stable after some testing, too.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] [XFS] Validate log feature fields correctly
  2009-03-15 11:25 ` [PATCH 1/2] [XFS] Validate log feature fields correctly Dave Chinner
@ 2009-03-15 15:15   ` Christoph Hellwig
  2009-03-16 10:49     ` Dave Chinner
  0 siblings, 1 reply; 11+ messages in thread
From: Christoph Hellwig @ 2009-03-15 15:15 UTC (permalink / raw)
  To: Dave Chinner; +Cc: xfs

On Sun, Mar 15, 2009 at 10:25:41PM +1100, Dave Chinner wrote:
> If the large log sector size feature bit is set in the
> superblock by accident (say disk corruption), the then
> fields that are now considered valid are not checked on
> production kernels. The checks are present as ASSERT
> statements so cause a panic on a debug kernel.
> 
> Change this so that the fields are validity checked if
> the feature bit is set and abort the log mount if the
> fields do not contain valid values.
> 
> Reported-by: Eric Sesterhenn <snakebyte@gmx.de>
> Signed-off-by: Dave Chinner <david@fromorbit.com>

Looks good to me, but wouldn't be easier to rad if the various sizes
in the error messages were reported decimal instead of in hex?


Reviewed-by: Christoph Hellwig <hch@lst.de>

>  }	/* xlog_alloc_log */

any maybe remove this comment while you're at it?

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] [XFS] Validate log feature fields correctly
  2009-03-15 15:15   ` Christoph Hellwig
@ 2009-03-16 10:49     ` Dave Chinner
  2009-03-16 10:50       ` Christoph Hellwig
  0 siblings, 1 reply; 11+ messages in thread
From: Dave Chinner @ 2009-03-16 10:49 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: xfs

On Sun, Mar 15, 2009 at 11:15:51AM -0400, Christoph Hellwig wrote:
> On Sun, Mar 15, 2009 at 10:25:41PM +1100, Dave Chinner wrote:
> > If the large log sector size feature bit is set in the
> > superblock by accident (say disk corruption), the then
> > fields that are now considered valid are not checked on
> > production kernels. The checks are present as ASSERT
> > statements so cause a panic on a debug kernel.
> > 
> > Change this so that the fields are validity checked if
> > the feature bit is set and abort the log mount if the
> > fields do not contain valid values.
> > 
> > Reported-by: Eric Sesterhenn <snakebyte@gmx.de>
> > Signed-off-by: Dave Chinner <david@fromorbit.com>
> 
> Looks good to me, but wouldn't be easier to rad if the various sizes
> in the error messages were reported decimal instead of in hex?

I just find large numbers easier to parse in hex, especially as
we are expecting power-of-2 type numbers to come out of this.
I'll change it....

> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> >  }	/* xlog_alloc_log */
> 
> any maybe remove this comment while you're at it?

Ok.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] [XFS] Validate log feature fields correctly
  2009-03-16 10:49     ` Dave Chinner
@ 2009-03-16 10:50       ` Christoph Hellwig
  0 siblings, 0 replies; 11+ messages in thread
From: Christoph Hellwig @ 2009-03-16 10:50 UTC (permalink / raw)
  To: Christoph Hellwig, xfs

On Mon, Mar 16, 2009 at 09:49:04PM +1100, Dave Chinner wrote:
> I just find large numbers easier to parse in hex, especially as
> we are expecting power-of-2 type numbers to come out of this.
> I'll change it....

True, the power of two would be an argument for hex.  Feel free to keep
it.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/2, RESEND] [XFS] a couple of fixes
  2009-03-15 11:25 [PATCH 0/2, RESEND] [XFS] a couple of fixes Dave Chinner
  2009-03-15 11:25 ` [PATCH 1/2] [XFS] Validate log feature fields correctly Dave Chinner
  2009-03-15 11:25 ` [PATCH 2/2] [XFS] Fix double free of inode Dave Chinner
@ 2009-04-06 22:28 ` Christoph Hellwig
  2009-04-08  4:45   ` Felix Blyakher
  2 siblings, 1 reply; 11+ messages in thread
From: Christoph Hellwig @ 2009-04-06 22:28 UTC (permalink / raw)
  To: Felix Blyakher, Dave Chinner; +Cc: xfs

I've sucked these patches into my tree after reviewin and QAing them.
Felix, can you pull this and send it to Linus for 2.6.30?

	git://git.kernel.org/pub/scm/fs/xfs/xfs.git

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/2, RESEND] [XFS] a couple of fixes
  2009-04-06 22:28 ` [PATCH 0/2, RESEND] [XFS] a couple of fixes Christoph Hellwig
@ 2009-04-08  4:45   ` Felix Blyakher
  2009-04-08 13:33     ` Christoph Hellwig
  0 siblings, 1 reply; 11+ messages in thread
From: Felix Blyakher @ 2009-04-08  4:45 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: xfs


On Apr 6, 2009, at 5:28 PM, Christoph Hellwig wrote:

> I've sucked these patches into my tree after reviewin and QAing them.
> Felix, can you pull this and send it to Linus for 2.6.30?
>
> 	git://git.kernel.org/pub/scm/fs/xfs/xfs.git

I did pull from your tree, and pushed to oss.
But I stopped short from pushing the for-linus branch,
and generating the pull request, as I'm again seeing
the 'merge' commits. I'll try to sort it out tomorrow.

Felix

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/2, RESEND] [XFS] a couple of fixes
  2009-04-08  4:45   ` Felix Blyakher
@ 2009-04-08 13:33     ` Christoph Hellwig
  0 siblings, 0 replies; 11+ messages in thread
From: Christoph Hellwig @ 2009-04-08 13:33 UTC (permalink / raw)
  To: Felix Blyakher; +Cc: Christoph Hellwig, xfs

On Tue, Apr 07, 2009 at 11:45:33PM -0500, Felix Blyakher wrote:
>
> On Apr 6, 2009, at 5:28 PM, Christoph Hellwig wrote:
>
>> I've sucked these patches into my tree after reviewin and QAing them.
>> Felix, can you pull this and send it to Linus for 2.6.30?
>>
>> 	git://git.kernel.org/pub/scm/fs/xfs/xfs.git
>
> I did pull from your tree, and pushed to oss.
> But I stopped short from pushing the for-linus branch,
> and generating the pull request, as I'm again seeing
> the 'merge' commits. I'll try to sort it out tomorrow.

Note that you will always see some merge commits of both merged trees
touched the same files.  What Linus objected was the large amount of
merge commits.  The way to bring them down is to simply do less merges,
just stay on the same mainline level for a longer time and only merge
with mainline short before you prepare a pull or when you really need it
due to conflicting changes making it in.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2009-04-08 13:34 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-03-15 11:25 [PATCH 0/2, RESEND] [XFS] a couple of fixes Dave Chinner
2009-03-15 11:25 ` [PATCH 1/2] [XFS] Validate log feature fields correctly Dave Chinner
2009-03-15 15:15   ` Christoph Hellwig
2009-03-16 10:49     ` Dave Chinner
2009-03-16 10:50       ` Christoph Hellwig
2009-03-15 11:25 ` [PATCH 2/2] [XFS] Fix double free of inode Dave Chinner
2009-03-15 15:13   ` Christoph Hellwig
2009-04-06 22:28 ` [PATCH 0/2, RESEND] [XFS] a couple of fixes Christoph Hellwig
2009-04-08  4:45   ` Felix Blyakher
2009-04-08 13:33     ` Christoph Hellwig
  -- strict thread matches above, loose matches on Subject: below --
2009-03-15 10:52 [PATCH 0/2] " Dave Chinner
2009-03-15 10:52 ` [PATCH 1/2] [XFS] Validate log feature fields correctly Dave Chinner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox