* [PATCH] Fix use after free when closing log/rt devices
@ 2008-06-27 5:14 Lachlan McIlroy
2008-06-27 6:32 ` Christoph Hellwig
0 siblings, 1 reply; 7+ messages in thread
From: Lachlan McIlroy @ 2008-06-27 5:14 UTC (permalink / raw)
To: xfs-dev, xfs-oss
The call to xfs_free_buftarg() will free the memory used by it's argument
so we need to save the bdev to pass to xfs_blkdev_put()
Lachlan
--- fs/xfs/linux-2.6/xfs_super.c_1.432 2008-06-27 14:51:17.000000000 +1000
+++ fs/xfs/linux-2.6/xfs_super.c 2008-06-27 14:59:26.000000000 +1000
@@ -781,13 +781,17 @@ STATIC void
xfs_close_devices(
struct xfs_mount *mp)
{
+ struct block_device *bdev;
+
if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp) {
+ bdev = mp->m_logdev_targp->bt_bdev;
xfs_free_buftarg(mp->m_logdev_targp);
- xfs_blkdev_put(mp->m_logdev_targp->bt_bdev);
+ xfs_blkdev_put(bdev);
}
if (mp->m_rtdev_targp) {
+ bdev = mp->m_rtdev_targp->bt_bdev;
xfs_free_buftarg(mp->m_rtdev_targp);
- xfs_blkdev_put(mp->m_rtdev_targp->bt_bdev);
+ xfs_blkdev_put(bdev);
}
xfs_free_buftarg(mp->m_ddev_targp);
}
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] Fix use after free when closing log/rt devices
2008-06-27 5:14 [PATCH] Fix use after free when closing log/rt devices Lachlan McIlroy
@ 2008-06-27 6:32 ` Christoph Hellwig
2008-06-27 6:39 ` Mark Goodwin
2008-07-01 6:21 ` Lachlan McIlroy
0 siblings, 2 replies; 7+ messages in thread
From: Christoph Hellwig @ 2008-06-27 6:32 UTC (permalink / raw)
To: Lachlan McIlroy; +Cc: xfs-dev, xfs-oss
On Fri, Jun 27, 2008 at 03:14:46PM +1000, Lachlan McIlroy wrote:
> The call to xfs_free_buftarg() will free the memory used by it's argument
> so we need to save the bdev to pass to xfs_blkdev_put()
>
> Lachlan
>
> --- fs/xfs/linux-2.6/xfs_super.c_1.432 2008-06-27 14:51:17.000000000 +1000
> +++ fs/xfs/linux-2.6/xfs_super.c 2008-06-27 14:59:26.000000000 +1000
> @@ -781,13 +781,17 @@ STATIC void
> xfs_close_devices(
> struct xfs_mount *mp)
> {
> + struct block_device *bdev;
> +
> if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp) {
> + bdev = mp->m_logdev_targp->bt_bdev;
> xfs_free_buftarg(mp->m_logdev_targp);
> - xfs_blkdev_put(mp->m_logdev_targp->bt_bdev);
> + xfs_blkdev_put(bdev);
> }
> if (mp->m_rtdev_targp) {
> + bdev = mp->m_rtdev_targp->bt_bdev;
> xfs_free_buftarg(mp->m_rtdev_targp);
> - xfs_blkdev_put(mp->m_rtdev_targp->bt_bdev);
> + xfs_blkdev_put(bdev);
> }
Looks good, alhough two local variables inside the ifs might be cleaner:
if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp) {
struct block_device *logdev = mp->m_logdev_targp->bt_bdev;
xfs_free_buftarg(mp->m_logdev_targp);
xfs_blkdev_put(logdev);
}
...
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] Fix use after free when closing log/rt devices
2008-06-27 6:32 ` Christoph Hellwig
@ 2008-06-27 6:39 ` Mark Goodwin
2008-06-27 9:08 ` Christoph Hellwig
2008-07-01 6:21 ` Lachlan McIlroy
1 sibling, 1 reply; 7+ messages in thread
From: Mark Goodwin @ 2008-06-27 6:39 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Lachlan McIlroy, xfs-dev, xfs-oss
Christoph Hellwig wrote:
> On Fri, Jun 27, 2008 at 03:14:46PM +1000, Lachlan McIlroy wrote:
>> The call to xfs_free_buftarg() will free the memory used by it's argument
>> so we need to save the bdev to pass to xfs_blkdev_put()
>>
>> Lachlan
>>
>> --- fs/xfs/linux-2.6/xfs_super.c_1.432 2008-06-27 14:51:17.000000000 +1000
>> +++ fs/xfs/linux-2.6/xfs_super.c 2008-06-27 14:59:26.000000000 +1000
>> @@ -781,13 +781,17 @@ STATIC void
>> xfs_close_devices(
>> struct xfs_mount *mp)
>> {
>> + struct block_device *bdev;
>> +
>> if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp) {
>> + bdev = mp->m_logdev_targp->bt_bdev;
>> xfs_free_buftarg(mp->m_logdev_targp);
>> - xfs_blkdev_put(mp->m_logdev_targp->bt_bdev);
>> + xfs_blkdev_put(bdev);
>> }
>> if (mp->m_rtdev_targp) {
>> + bdev = mp->m_rtdev_targp->bt_bdev;
>> xfs_free_buftarg(mp->m_rtdev_targp);
>> - xfs_blkdev_put(mp->m_rtdev_targp->bt_bdev);
>> + xfs_blkdev_put(bdev);
>> }
>
> Looks good, alhough two local variables inside the ifs might be cleaner:
>
> if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp) {
> struct block_device *logdev = mp->m_logdev_targp->bt_bdev;
>
> xfs_free_buftarg(mp->m_logdev_targp);
> xfs_blkdev_put(logdev);
> }
>
> ...
do we have any QA tests that test external log?
-- Mark
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] Fix use after free when closing log/rt devices
2008-06-27 6:39 ` Mark Goodwin
@ 2008-06-27 9:08 ` Christoph Hellwig
2008-07-02 1:59 ` Timothy Shimmin
0 siblings, 1 reply; 7+ messages in thread
From: Christoph Hellwig @ 2008-06-27 9:08 UTC (permalink / raw)
To: Mark Goodwin; +Cc: Christoph Hellwig, Lachlan McIlroy, xfs-dev, xfs-oss
On Fri, Jun 27, 2008 at 04:39:39PM +1000, Mark Goodwin wrote:
> do we have any QA tests that test external log?
Most QA tests will use the external log if you set it up that way. But
ithout slab poisoning this won't be noticed either.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix use after free when closing log/rt devices
2008-06-27 9:08 ` Christoph Hellwig
@ 2008-07-02 1:59 ` Timothy Shimmin
2008-07-02 2:54 ` Dave Chinner
0 siblings, 1 reply; 7+ messages in thread
From: Timothy Shimmin @ 2008-07-02 1:59 UTC (permalink / raw)
To: Mark Goodwin; +Cc: Christoph Hellwig, Lachlan McIlroy, xfs-dev, xfs-oss
Christoph Hellwig wrote:
> On Fri, Jun 27, 2008 at 04:39:39PM +1000, Mark Goodwin wrote:
>> do we have any QA tests that test external log?
>
> Most QA tests will use the external log if you set it up that way. But
> ithout slab poisoning this won't be noticed either.
I think you need:
USE_EXTERNAL=yes
SCRATCH_LOGDEV=somelogdevice
TEST_LOGDEV=somelogdevice
to get the scratch and test mounts using an external log.
There are no explicit external log tests (logdev=) that I can see.
--Tim
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix use after free when closing log/rt devices
2008-07-02 1:59 ` Timothy Shimmin
@ 2008-07-02 2:54 ` Dave Chinner
0 siblings, 0 replies; 7+ messages in thread
From: Dave Chinner @ 2008-07-02 2:54 UTC (permalink / raw)
To: Timothy Shimmin
Cc: Mark Goodwin, Christoph Hellwig, Lachlan McIlroy, xfs-dev,
xfs-oss
On Wed, Jul 02, 2008 at 11:59:20AM +1000, Timothy Shimmin wrote:
> Christoph Hellwig wrote:
> > On Fri, Jun 27, 2008 at 04:39:39PM +1000, Mark Goodwin wrote:
> >> do we have any QA tests that test external log?
> >
> > Most QA tests will use the external log if you set it up that way. But
> > ithout slab poisoning this won't be noticed either.
>
> I think you need:
> USE_EXTERNAL=yes
> SCRATCH_LOGDEV=somelogdevice
> TEST_LOGDEV=somelogdevice
> to get the scratch and test mounts using an external log.
Yes. Typically I used to use SCRATCH_LOGDEV=/dev/ram0 so I didn't
need another block device on the machine just for an external
log on a throw-away filesystem....
> There are no explicit external log tests (logdev=) that I can see.
It should be easy to write one using files and multiple
loopback devices.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix use after free when closing log/rt devices
2008-06-27 6:32 ` Christoph Hellwig
2008-06-27 6:39 ` Mark Goodwin
@ 2008-07-01 6:21 ` Lachlan McIlroy
1 sibling, 0 replies; 7+ messages in thread
From: Lachlan McIlroy @ 2008-07-01 6:21 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs-dev, xfs-oss
Christoph Hellwig wrote:
> On Fri, Jun 27, 2008 at 03:14:46PM +1000, Lachlan McIlroy wrote:
>> The call to xfs_free_buftarg() will free the memory used by it's argument
>> so we need to save the bdev to pass to xfs_blkdev_put()
>>
>> Lachlan
>>
>> --- fs/xfs/linux-2.6/xfs_super.c_1.432 2008-06-27 14:51:17.000000000 +1000
>> +++ fs/xfs/linux-2.6/xfs_super.c 2008-06-27 14:59:26.000000000 +1000
>> @@ -781,13 +781,17 @@ STATIC void
>> xfs_close_devices(
>> struct xfs_mount *mp)
>> {
>> + struct block_device *bdev;
>> +
>> if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp) {
>> + bdev = mp->m_logdev_targp->bt_bdev;
>> xfs_free_buftarg(mp->m_logdev_targp);
>> - xfs_blkdev_put(mp->m_logdev_targp->bt_bdev);
>> + xfs_blkdev_put(bdev);
>> }
>> if (mp->m_rtdev_targp) {
>> + bdev = mp->m_rtdev_targp->bt_bdev;
>> xfs_free_buftarg(mp->m_rtdev_targp);
>> - xfs_blkdev_put(mp->m_rtdev_targp->bt_bdev);
>> + xfs_blkdev_put(bdev);
>> }
>
> Looks good, alhough two local variables inside the ifs might be cleaner:
>
> if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp) {
> struct block_device *logdev = mp->m_logdev_targp->bt_bdev;
>
> xfs_free_buftarg(mp->m_logdev_targp);
> xfs_blkdev_put(logdev);
> }
>
> ...
>
Thought someone might suggest that. I'll make the changes, thanks.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2008-07-02 2:53 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-06-27 5:14 [PATCH] Fix use after free when closing log/rt devices Lachlan McIlroy
2008-06-27 6:32 ` Christoph Hellwig
2008-06-27 6:39 ` Mark Goodwin
2008-06-27 9:08 ` Christoph Hellwig
2008-07-02 1:59 ` Timothy Shimmin
2008-07-02 2:54 ` Dave Chinner
2008-07-01 6:21 ` Lachlan McIlroy
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox