* [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
* 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
* [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 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
* [PATCH 0/2] [XFS] a couple of fixes @ 2009-03-15 10:52 Dave Chinner 2009-03-15 10:52 ` [PATCH 2/2] [XFS] Fix double free of inode Dave Chinner 0 siblings, 1 reply; 11+ messages in thread From: Dave Chinner @ 2009-03-15 10:52 UTC (permalink / raw) To: xfs The first fixes a fuzzer log corruption crash, and second is a null pointer dereference I found by inspection. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] [XFS] Fix double free of inode 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 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
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 2/2] [XFS] Fix double free of inode Dave Chinner
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.