| Message ID | 1236202436.28664.8.camel@hydrogen.msp.redhat.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Alasdair Kergon |
| Headers | show |
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
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
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;
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> -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel