dm-table-rework-reference-counting.patch

dm-table-rework-reference-counting.patch

[Kiyoshi Ueda]

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

Re: dm-table-rework-reference-counting.patch

[Mikulas Patocka]

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.


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?

Mikulas

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@redhat.com>
CC: Kiyoshi Ueda <k-ueda@ct.jp.nec.com>

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

self deadlock possibility during device deletion (Was: dm-table-rework-reference-counting.patch)

[Kiyoshi Ueda]

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@redhat.com>
> > CC: Kiyoshi Ueda <k-ueda@ct.jp.nec.com>
> > 
> > ---
> >  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@redhat.com
> > https://www.redhat.com/mailman/listinfo/dm-devel