Fix use after free when closing log/rt devices

PATCH In review Lachlan McIlroy 5 participants 7 messages Jun 27, 2008 – Jul 2, 2008

Share & tools

Lachlan McIlroy

author

2008-06-27 05:09 UTC #0 lore raw

PATCH 1/1 Fix use after free when closing log/rt devices

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

xfs/linux-2.6/xfs_super.c renamed from xfs/linux-2.6/xfs_super.c_1.432 +0 -0

@@ -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);
}

Christoph Hellwig

2008-06-27 06:31 UTC #1 lore raw

Quoted 24 lines · Lachlan McIlroy

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
xfs/linux-2.6/xfs_super.c renamed from xfs/linux-2.6/xfs_super.c_1.432 +0 -0
@@ -781,13 +781,17 @@ STATIC void
xfs_close_devices(
struct xfs_mount *mp)
{
xfs/linux-2.6/xfs_super.c renamed from xfs/linux-2.6/xfs_super.c_1.432 +2 -0
+	struct block_device	*bdev;
+
	if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp) {
xfs/linux-2.6/xfs_super.c renamed from xfs/linux-2.6/xfs_super.c_1.432 +1 -0
+		bdev = mp->m_logdev_targp->bt_bdev;
		xfs_free_buftarg(mp->m_logdev_targp);
xfs/linux-2.6/xfs_super.c renamed from xfs/linux-2.6/xfs_super.c_1.432 +1 -1
-		xfs_blkdev_put(mp->m_logdev_targp->bt_bdev);
+		xfs_blkdev_put(bdev);
	}
	if (mp->m_rtdev_targp) {
xfs/linux-2.6/xfs_super.c renamed from xfs/linux-2.6/xfs_super.c_1.432 +1 -0
+		bdev = mp->m_rtdev_targp->bt_bdev;
		xfs_free_buftarg(mp->m_rtdev_targp);
xfs/linux-2.6/xfs_super.c renamed from xfs/linux-2.6/xfs_super.c_1.432 +1 -1
-		xfs_blkdev_put(mp->m_rtdev_targp->bt_bdev);
+		xfs_blkdev_put(bdev);

Quoted 1 line

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);
	}

	...

Mark Goodwin

2008-06-27 06:38 UTC #2 lore raw

Quoted 29 lines · Christoph Hellwig

Quoted 24 lines · Lachlan McIlroy
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
xfs/linux-2.6/xfs_super.c renamed from xfs/linux-2.6/xfs_super.c_1.432 +0 -0
@@ -781,13 +781,17 @@ STATIC void
xfs_close_devices(
struct xfs_mount *mp)
{
xfs/linux-2.6/xfs_super.c renamed from xfs/linux-2.6/xfs_super.c_1.432 +2 -0
+	struct block_device	*bdev;
+
	if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp) {
xfs/linux-2.6/xfs_super.c renamed from xfs/linux-2.6/xfs_super.c_1.432 +1 -0
+		bdev = mp->m_logdev_targp->bt_bdev;
		xfs_free_buftarg(mp->m_logdev_targp);
xfs/linux-2.6/xfs_super.c renamed from xfs/linux-2.6/xfs_super.c_1.432 +1 -1
-		xfs_blkdev_put(mp->m_logdev_targp->bt_bdev);
+		xfs_blkdev_put(bdev);
	}
	if (mp->m_rtdev_targp) {
xfs/linux-2.6/xfs_super.c renamed from xfs/linux-2.6/xfs_super.c_1.432 +1 -0
+		bdev = mp->m_rtdev_targp->bt_bdev;
		xfs_free_buftarg(mp->m_rtdev_targp);
xfs/linux-2.6/xfs_super.c renamed from xfs/linux-2.6/xfs_super.c_1.432 +1 -1
-		xfs_blkdev_put(mp->m_rtdev_targp->bt_bdev);
+		xfs_blkdev_put(bdev);
Quoted 1 line
	}
Looks good, alhough two local variables inside the ifs might be cleaner:

Quoted 8 lines

	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

Christoph Hellwig

2008-06-27 09:07 UTC #3 lore raw

Quoted 1 line · Mark Goodwin

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.

Lachlan McIlroy

author

2008-07-01 06:16 UTC #6 lore raw

In reply to Christoph Hellwig

Quoted 29 lines · Christoph Hellwig

Quoted 24 lines · Lachlan McIlroy
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
xfs/linux-2.6/xfs_super.c renamed from xfs/linux-2.6/xfs_super.c_1.432 +0 -0
@@ -781,13 +781,17 @@ STATIC void
xfs_close_devices(
struct xfs_mount *mp)
{
xfs/linux-2.6/xfs_super.c renamed from xfs/linux-2.6/xfs_super.c_1.432 +2 -0
+	struct block_device	*bdev;
+
	if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp) {
xfs/linux-2.6/xfs_super.c renamed from xfs/linux-2.6/xfs_super.c_1.432 +1 -0
+		bdev = mp->m_logdev_targp->bt_bdev;
		xfs_free_buftarg(mp->m_logdev_targp);
xfs/linux-2.6/xfs_super.c renamed from xfs/linux-2.6/xfs_super.c_1.432 +1 -1
-		xfs_blkdev_put(mp->m_logdev_targp->bt_bdev);
+		xfs_blkdev_put(bdev);
	}
	if (mp->m_rtdev_targp) {
xfs/linux-2.6/xfs_super.c renamed from xfs/linux-2.6/xfs_super.c_1.432 +1 -0
+		bdev = mp->m_rtdev_targp->bt_bdev;
		xfs_free_buftarg(mp->m_rtdev_targp);
xfs/linux-2.6/xfs_super.c renamed from xfs/linux-2.6/xfs_super.c_1.432 +1 -1
-		xfs_blkdev_put(mp->m_rtdev_targp->bt_bdev);
+		xfs_blkdev_put(bdev);
Quoted 1 line
	}
Looks good, alhough two local variables inside the ifs might be cleaner:

Quoted 9 lines

	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.

Timothy Shimmin

2008-07-02 01:58 UTC #4 lore raw

In reply to Christoph Hellwig

Quoted 5 lines · Christoph Hellwig

Quoted 1 line · Mark Goodwin
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

Dave Chinner

2008-07-02 02:53 UTC #5 lore raw

Quoted 4 lines · Timothy Shimmin

Quoted 3 lines · Christoph Hellwig
Quoted 1 line · Mark Goodwin
do we have any QA tests that test external log?

Quoted 8 lines

Quoted 2 lines
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....

Quoted 1 line

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.

Signature · 2 lines

Dave Chinner
[email protected]