[PATCH] Fix use after free when closing log/rt devices

[PATCH] Fix use after free when closing log/rt devices

[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

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

Re: [PATCH] Fix use after free when closing log/rt devices

[Christoph Hellwig]

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

	...

Re: [PATCH] Fix use after free when closing log/rt devices

[Mark Goodwin]

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

Re: [PATCH] Fix use after free when closing log/rt devices

[Christoph Hellwig]

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.

Re: [PATCH] Fix use after free when closing log/rt devices

[Lachlan McIlroy]

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.

Re: [PATCH] Fix use after free when closing log/rt devices

[Timothy Shimmin]

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

  

Re: [PATCH] Fix use after free when closing log/rt devices

[Dave Chinner]

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