Hi Mikulas, Alasdair,
Mikulas Patocka wrote:
> > Hi
> >
> > You are right, thanks for the review. I grepped for other uses of
> > dm_table_put and they seem correct - just these two were bad.
> >
> > Alasdair: push this patch to Linus soon.
Thank you for the rapid work.
> > Another point: it looks somehow scary to free the table before unlocking
> > _hash_lock --- most of the time, _hash_lock is took only on behalf of
> > userspace ioctls (no problem with that), but it is also took in
> > dm_copy_name_and_uuid that can be called from uevent code:
> >
> > dm_table_event -> event_callback -> dm_send_uevents -> takes _hash_lock
> > And dm_table_event is called directly from targets, targets wait for
> > dm_table_event to finish in their destructor --- so calling the destructor
> > with locked _hash_lock seems to be a deadlock possibility --- what do you
> > think?
You are right, although there is no such target now.
But there is already a code path that could cause such a deadlock.
If a mapped_device is being deleted while there are pending uevents
in md->uevent_list, the ioctl for the deletion should cause
self-deadlock, because __hash_remove() calls dm_table_event()
with _hash_lock locked:
drivers/md/dm-ioctl.c:__hash_remove()
220 static void __hash_remove(struct hash_cell *hc)
221 {
222 struct dm_table *table;
223
224 /* remove from the dev hash */
225 list_del(&hc->uuid_list);
226 list_del(&hc->name_list);
227 dm_set_mdptr(hc->md, NULL);
228
229 table = dm_get_table(hc->md);
230 if (table) {
231 dm_table_event(table);
232 dm_table_put(table);
233 }
We should unlock _hash_lock before handling event flush.
> > Mikulas
Thanks,
Kiyoshi Ueda
> > On Tue, 6 Jan 2009, Kiyoshi Ueda wrote:
> >
>> >> Hi Alasdair, Mikulas,
>> >>
>> >> Although I'm not sure this is correct, don't we need to convert
>> >> the following 2 dm_table_put()s to dm_table_destroy()?
>> >> Otherwise, the created table won't be destroyed and memory leak
>> >> will happen in such error cases?
>> >>
>> >> drivers/md/dm-ioctl.c:table_load()
>> >> 1060 r = dm_table_create(&t, get_mode(param), param->target_count, md);
>> >> 1061 if (r)
>> >> 1062 goto out;
>> >> 1063
>> >> 1064 r = populate_table(t, param, param_size);
>> >> 1065 if (r) {
>> >> 1066 dm_table_put(t);
>> >> 1067 goto out;
>> >> 1068 }
>> >> 1069
>> >> 1070 down_write(&_hash_lock);
>> >> 1071 hc = dm_get_mdptr(md);
>> >> 1072 if (!hc || hc->md != md) {
>> >> 1073 DMWARN("device has been removed from the dev hash table.");
>> >> 1074 dm_table_put(t);
>> >> 1075 up_write(&_hash_lock);
>> >> 1076 r = -ENXIO;
>> >> 1077 goto out;
>> >> 1078 }
>> >>
>> >> Thanks,
>> >> Kiyoshi Ueda
>> >>
> >
> > Fix an error introduced in dm-table-rework-reference-counting.patch.
> >
> > When there is failure after table initialization, we need to use
> > dm_table_destroy, not dm_table_put, to free the table.
> >
> > dm_table_put may be used only after dm_table_get.
> >
> > Signed-off-by: Mikulas Patocka <mpatocka@xxxxxxxxxx>
> > CC: Kiyoshi Ueda <k-ueda@xxxxxxxxxxxxx>
> >
> > ---
> > drivers/md/dm-ioctl.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > Index: linux-2.6.28-devel/drivers/md/dm-ioctl.c
> > ===================================================================
> > --- linux-2.6.28-devel.orig/drivers/md/dm-ioctl.c 2009-01-06 22:06:01.000000000 +0100
> > +++ linux-2.6.28-devel/drivers/md/dm-ioctl.c 2009-01-06 22:07:05.000000000 +0100
> > @@ -1063,7 +1063,7 @@ static int table_load(struct dm_ioctl *p
> >
> > r = populate_table(t, param, param_size);
> > if (r) {
> > - dm_table_put(t);
> > + dm_table_destroy(t);
> > goto out;
> > }
> >
> > @@ -1071,7 +1071,7 @@ static int table_load(struct dm_ioctl *p
> > hc = dm_get_mdptr(md);
> > if (!hc || hc->md != md) {
> > DMWARN("device has been removed from the dev hash table.");
> > - dm_table_put(t);
> > + dm_table_destroy(t);
> > up_write(&_hash_lock);
> > r = -ENXIO;
> > goto out;
> >
> > --
> > dm-devel mailing list
> > dm-devel@xxxxxxxxxx
> > https://www.redhat.com/mailman/listinfo/dm-devel
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
