FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > Redhat > Device-mapper Development

 
 
LinkBack Thread Tools
 
Old 03-04-2009, 08:33 PM
Jonathan Brassow
 
Default dm-snap-persistent-fix-dtr-cleanup.patch

Prerequisites for this patch are:
1) dm-exception-store-introduce-registry.patch
2) dm-exception-store-move-dm_target-pointer.patch
3) dm-exception-store-move-chunk_fields.patch
4) dm-exception-store-move-cow-pointer.patch
5) dm-snapshot-remove-dm_snap-header-use.patch
6) dm-snapshot-remove-dm_snap-header.patch
7) dm-snapshot-use-DMEMIT-macro-for-status.patch
8) dm-snapshot-move-ctr-parsing-to-exception-store.patch
9) dm-snapshot-move-status-to-exception-store.patch
10) dm-exception-store-generalize-table-args.patch
11) dm-snapshot-new-ctr-table-format.patch
12) dm-snapshot-cleanup.patch
13) dm-snap-minor-fix.patch
14) dm-snap-fix-status-output.patch

brassow

The persistent exception store destructor does not properly
account for all conditions in which it can be called. If it
is called after 'ctr' but before 'read_metadata' - like if
something else in 'snapshot_ctr' fails - then it will attempt
to free areas of memory that haven't been allocated yet.

Signed-off-by: Jonathan Brassow <jbrassow@redhat.com>

Index: linux-2.6/drivers/md/dm-snap-persistent.c
================================================== =================
--- linux-2.6.orig/drivers/md/dm-snap-persistent.c
+++ linux-2.6/drivers/md/dm-snap-persistent.c
@@ -162,9 +162,12 @@ static int alloc_area(struct pstore *ps)

static void free_area(struct pstore *ps)
{
- vfree(ps->area);
+ if (ps->area)
+ vfree(ps->area);
ps->area = NULL;
- vfree(ps->zero_area);
+
+ if (ps->zero_area)
+ vfree(ps->zero_area);
ps->zero_area = NULL;
}

@@ -481,10 +484,17 @@ static void persistent_dtr(struct dm_exc
{
struct pstore *ps = get_info(store);

- destroy_workqueue(ps->metadata_wq);
- dm_io_client_destroy(ps->io_client);
- vfree(ps->callbacks);
+ /* Created in read_header */
+ if (ps->io_client)
+ dm_io_client_destroy(ps->io_client);
free_area(ps);
+
+ /* Allocated in persistent_read_metadata */
+ if (ps->callbacks)
+ vfree(ps->callbacks);
+
+ /* Don't need to check these, because they are done in ctr */
+ destroy_workqueue(ps->metadata_wq);
kfree(ps);
}

@@ -661,7 +671,7 @@ static int persistent_ctr(struct dm_exce
struct pstore *ps;

/* allocate the pstore */
- ps = kmalloc(sizeof(*ps), GFP_KERNEL);
+ ps = kzalloc(sizeof(*ps), GFP_KERNEL);
if (!ps)
return -ENOMEM;



--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 03-19-2009, 09:42 PM
Alasdair G Kergon
 
Default dm-snap-persistent-fix-dtr-cleanup.patch

On Wed, Mar 04, 2009 at 03:33:56PM -0600, Jon Brassow wrote:
> @@ -481,10 +484,17 @@ static void persistent_dtr(struct dm_exc
> {
> struct pstore *ps = get_info(store);
>
> - destroy_workqueue(ps->metadata_wq);
> - dm_io_client_destroy(ps->io_client);
> - vfree(ps->callbacks);
> + /* Created in read_header */
> + if (ps->io_client)
> + dm_io_client_destroy(ps->io_client);
> free_area(ps);
> +
> + /* Allocated in persistent_read_metadata */
> + if (ps->callbacks)
> + vfree(ps->callbacks);
> +
> + /* Don't need to check these, because they are done in ctr */
> + destroy_workqueue(ps->metadata_wq);
> kfree(ps);
> }

I presume it's safe to reorder those operations as the workqueue is
guaranteed to be empty? It needs an inline comment though.
(Or can we just keep the original, logical, order?)

Alasdair
--
agk@redhat.com

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 03-20-2009, 01:01 PM
Jonathan Brassow
 
Default dm-snap-persistent-fix-dtr-cleanup.patch

On Mar 19, 2009, at 5:42 PM, Alasdair G Kergon wrote:


On Wed, Mar 04, 2009 at 03:33:56PM -0600, Jon Brassow wrote:

@@ -481,10 +484,17 @@ static void persistent_dtr(struct dm_exc
{
struct pstore *ps = get_info(store);

- destroy_workqueue(ps->metadata_wq);
- dm_io_client_destroy(ps->io_client);
- vfree(ps->callbacks);
+ /* Created in read_header */
+ if (ps->io_client)
+ dm_io_client_destroy(ps->io_client);
free_area(ps);
+
+ /* Allocated in persistent_read_metadata */
+ if (ps->callbacks)
+ vfree(ps->callbacks);
+
+ /* Don't need to check these, because they are done in ctr */
+ destroy_workqueue(ps->metadata_wq);
kfree(ps);
}


I presume it's safe to reorder those operations as the workqueue is
guaranteed to be empty? It needs an inline comment though.
(Or can we just keep the original, logical, order?)


The patch puts them in the order they /should/ have been in in the
first place; that is, the reverse order in which they are created.
The inline comments I put in state where/when those items are created,
and hence why they need checks before being freed. Perhaps you are
suggesting the comments could be better. (?)


brassow

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 

Thread Tools




All times are GMT. The time now is 03:43 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org