On Thu, Nov 16, 2006 at 10:18:26PM +0100, Jesper Juhl wrote: > (got no reply on this when I originally send it on 20061031, so resending > now that a bit of time has passed. The patch still applies cleanly to > Linus' git tree as of today.) > > > The Coverity checker spotted a potential problem in XFS. > > The problem is that if, in xfs_mount(), this code triggers: > > ... > if (!mp->m_logdev_targp) > goto error0; > ... > > Then we'll end up calling xfs_unmountfs_close() with a NULL > 'mp->m_logdev_targp'. > This in turn will result in a call to xfs_free_buftarg() with its 'btp' > argument == NULL. xfs_free_buftarg() dereferences 'btp' leading to > a NULL pointer dereference and crash.
Interesting that coverity found that, but failed to find the other leaks in that function from exactly the same code and error case..... > I think this can happen, since the fatal call to xfs_free_buftarg() > happens when 'm_logdev_targp != m_ddev_targp' and due to a check of > 'm_ddev_targp' against NULL in xfs_mount() (and subsequent return if it is > NULL) the two will never both be NULL when we hit the error0 label from > the two lines cited above. > > Comments welcome (please keep me on Cc: on replies). > > Here's a proposed patch to fix this by testing 'btp' against NULL in > xfs_free_buftarg(). Not the right fix - we should only be trying to free valid buftargs, which means xfs_unmountfs_close() is the correct place to fix this.... e.g: - if (mp->m_logdev_targp != mp->m_ddev_targp) + if (mp->m_logdev_targp && (mp->m_logdev_targp != mp->m_ddev_targp)) As to the afore-mentioned leaks, if we fail to allocate a realtime buftarg, then we will leak a reference to both the rtdev and logdev, and if we fail to allocate an external log buftarg we'll leak a reference to the logdev. i.e., we fail to do one or both of: xfs_blkdev_put(logdev); xfs_blkdev_put(rtdev); To remove the bdev references we may have gained earlier. Normally, these references are released by xfs_free_buftarg(), but because we failed to allocate the buftarg, we can't drop the references via that method.... Cheers, Dave. -- Dave Chinner Principal Engineer SGI Australian Software Group - 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/