[linux-lvm] libdm cannot swap names between two child volumes

[M.H. Tsai]

Hi All,

I found that there's a potential bug in libdm when I try to swap sub
volume's name. By executing the following code, the function
dm_tree_activate_children() will go into infinite loop since that
_rename_conflict_exists() still reports "resolvable=1" in this case.

logical_volume *parent = ...;
logical_volume *child1 = seg_lv(parent, 0);
logical_volume *child2 = seg_lv(parent, 1);
char const *tmp = child1->name;
child1->name = child2->name;
child2->name = tmp;
resume_lv(cmd, parent);

Although this scenario might not happen through command-line
operation, some programmers might need this feature. I think that this
issue could be resolve in libdm by assigning a temporary name, as the
following patch does. Is it safe to do this?

diff --git libdm/libdm-deptree.c libdm/libdm-deptree.c
index 1335351..b0c5f13 100644
--- a/libdm/libdm-deptree.c
+++ b/libdm/libdm-deptree.c
@@ -1771,7 +1771,8 @@ static int _rename_conflict_exists(struct
dm_tree_node *parent,
}

if (!strcmp(node->props.new_name, sibling_name)) {
- if (sibling->props.new_name)
+ if (sibling->props.new_name &&
+ strcmp(sibling->props.new_name,
dm_tree_node_get_name(node)))
*resolvable = 1;
return 1;
}
@@ -1790,8 +1791,10 @@ int dm_tree_activate_children(struct dm_tree_node *dnode,
struct dm_tree_node *child = dnode;
struct dm_info newinfo;
const char *name;
+ const char *new_name = NULL;
const char *uuid;
int priority;
+ int rename_conflict = 0;

/* Activate children first */
while ((child = dm_tree_next_child(&handle, dnode, 0))) {
@@ -1831,12 +1834,21 @@ int dm_tree_activate_children(struct
dm_tree_node *dnode,

/* Rename? */
if (child->props.new_name) {
- if (_rename_conflict_exists(dnode,
child, &resolvable_name_conflict) &&
- resolvable_name_conflict) {
- awaiting_peer_rename++;
- continue;
+ rename_conflict =
_rename_conflict_exists(dnode, child, &resolvable_name_conflict);
+ if (rename_conflict) {
+ if (resolvable_name_conflict) {
+ awaiting_peer_rename++;
+ continue;
+ } else {
+ /* assign a temporary
name for swapping names */
+ new_name =
dm_pool_zalloc(dnode->dtree->mem, strlen(child->props.new_name) + 5);
+
dm_snprintf((char*)new_name, strlen(child->props.new_name) + 5,
"%s_tmp", child->props.new_name);
+ awaiting_peer_rename++;
+ }
+ } else {
+ new_name = child->props.new_name;
}
- if (!_rename_node(name,
child->props.new_name, child->info.major,
+ if (!_rename_node(name, new_name,
child->info.major,
child->info.minor,
&child->dtree->cookie,
child->udev_flags)) {
log_error("Failed to rename %s
(%" PRIu32
@@ -1844,8 +1856,10 @@ int dm_tree_activate_children(struct dm_tree_node *dnode,
child->info.minor,
child->props.new_name);
return 0;
}
- child->name = child->props.new_name;
- child->props.new_name = NULL;
+ child->name = new_name;
+
+ if (!rename_conflict)
+ child->props.new_name = NULL;
}

if (!child->info.inactive_table &&
!child->info.suspended)


Thanks,
Ming-Hung Tsai

[Zdenek Kabelac]

Post by M.H. Tsai
Hi All,
I found that there's a potential bug in libdm when I try to swap sub
volume's name. By executing the following code, the function
dm_tree_activate_children() will go into infinite loop since that
_rename_conflict_exists() still reports "resolvable=1" in this case.
logical_volume *parent = ...;
logical_volume *child1 = seg_lv(parent, 0);
logical_volume *child2 = seg_lv(parent, 1);
char const *tmp = child1->name;
child1->name = child2->name;
child2->name = tmp;
resume_lv(cmd, parent);
Although this scenario might not happen through command-line
operation, some programmers might need this feature. I think that this
issue could be resolve in libdm by assigning a temporary name, as the
following patch does. Is it safe to do this?

Please provide sequence of 'ioctl' where you think there is a bug.
(or disclose your code using libdm).

There are many rules behind the work with subLV.
(e.g. you can't operate on subLV without operating through top level LV
(and yes some parts of lvm2 code are still buggy in this regard...))

Regards

Zdenek

[M.H. Tsai]

Post by Zdenek Kabelac
Please provide sequence of 'ioctl' where you think there is a bug.
(or disclose your code using libdm).

Assume that there's a top-level LV "lv1",
and two sub-LVs "lv1_child1" and "lv1_child2".
The table of the top-level LV is "0 1234 my-target-type 253:1 253:2",
as following:

# dmsetup ls --tree
vg1-lv1 (253:3)
├─ vg1-lv1_child1 (253:1)
└─ vg1-lv2_child2 (253:2)

I want to swap the ordering of subLVs, by using a dm-reload IOCTL:
dmsetup reload vg1-lv1 --table "0 1234 my-target-type 253:2 253:1"
dmsetup suspend vg1-lv1
dmsetup resume vg1-lv1

Then swap names of the two subLVs, to make the first child named "vg1_child1"
dmsetup rename vg1-lv1_child1 vg1-lv1_tmp
dmsetup rename vg1-lv1_child2 vg1-lv1_child1
dmsetup rename vg1-lv1_tmp vg1-lv1_child2


In LVM, the above action is:
logical_volume *parent = ...; // vg1-lv1
logical_volume *child1 = seg_lv(parent, 0);
logical_volume *child2 = seg_lv(parent, 1);
// swap the subLV ordering
struct lv_segment_area tmp = first_seg(parent)->areas[0];
first_seg(parent)->areas[0] = first_seg(parent)->areas[1];
first_seg(parent)->areas[1] = tmp;
// swap the subLV names
char const *tmp = child1->name;
child1->name = child2->name;
child2->name = tmp;
lv_update_and_reload(parent); // in lv_manip.c


Apply my previous patch on LVM2.2.02.105, the result IOCTLs are:
dm reload vg1-lv1 // swap child1 and child2
dm resume vg1-lv1
dm suspend vg1-lv1
dm suspend vg1-lv1_child1
dm suspend vg1-lv1_child2
dm rename vg1-lv1_child1 // temporarily renamed to vg1-lv1_child2_tmp,
but do not resume
dm rename vg1-lv1_child2 // renamed to vg1-lv1_child1
dm resume vg1-lv1_child1 // resume the renamed device
dm rename vg1-lv1_child2_tmp // renamed to the actual name "vg1-lv1_child2"
dm resume vg1-lv1_child2 // resume the actually renamed device

(There's might be an issue in my previous patch: I should not resume
the temporarily renamed device)
diff --git a/libdm/libdm-deptree.c b/libdm/libdm-deptree.c
index b0c5f13..3776200 100644
--- a/libdm/libdm-deptree.c
+++ b/libdm/libdm-deptree.c
@@ -1858,7 +1858,9 @@ int dm_tree_activate_children(struct dm_tree_node *dnode,
}
child->name = new_name;

- if (!rename_conflict)
+ if (rename_conflict)
+ continue;
+ else
child->props.new_name = NULL;
}


However, then current LVM cannot accomplish the above IOCTLs,
due to the infinite loop in dm_tree_activate_children(). Also, It doesn't
assign a temporary name to avoid naming conflict.

If libdm doesn't support this feature, then the client need to
invoke lv_update_and_reload() twice to swap names, which might not
be efficiency:

// invoke lv_update_and_reload() twice to swap the subLV names
char const *tmp = child1->name;
child1->name = child2->name;
child2->name = "a_temporary_name";
lv_update_and_reload(parent); // in lv_manip.c
child2->name = tmp;
lv_update_and_reload(parent); // in lv_manip.c

Post by Zdenek Kabelac

Post by M.H. Tsai
Hi All,
I found that there's a potential bug in libdm when I try to swap sub
volume's name. By executing the following code, the function
dm_tree_activate_children() will go into infinite loop since that
_rename_conflict_exists() still reports "resolvable=1" in this case.
logical_volume *parent = ...;
logical_volume *child1 = seg_lv(parent, 0);
logical_volume *child2 = seg_lv(parent, 1);
char const *tmp = child1->name;
child1->name = child2->name;
child2->name = tmp;
resume_lv(cmd, parent);
Although this scenario might not happen through command-line
operation, some programmers might need this feature. I think that this
issue could be resolve in libdm by assigning a temporary name, as the
following patch does. Is it safe to do this?

[Zdenek Kabelac]

Post by M.H. Tsai

Post by Zdenek Kabelac
Please provide sequence of 'ioctl' where you think there is a bug.
(or disclose your code using libdm).

Assume that there's a top-level LV "lv1",
and two sub-LVs "lv1_child1" and "lv1_child2".
The table of the top-level LV is "0 1234 my-target-type 253:1 253:2",
# dmsetup ls --tree
vg1-lv1 (253:3)
├─ vg1-lv1_child1 (253:1)
└─ vg1-lv2_child2 (253:2)

The problem with rename is -

you have device 'lv1' you rename it to 'lv2' - yet
those who opened device with the name 'lv1' still thinks
the 'lv1' device exits.

So for safety reason before you 'reuse' any existing name in-use,
there should be 'deactivating' such device first - so there is no 'race' in
name usage.

It's even possible we miss to track full history of active renamed device.

Since you get into strange scenarios when you start to count
with udev event handling and link generating here - it's getting nearly
impossible to synchronize this properly...

Zdenek

[M.H. Tsai]

Post by Zdenek Kabelac
The problem with rename is -
you have device 'lv1' you rename it to 'lv2' - yet
those who opened device with the name 'lv1' still thinks
the 'lv1' device exits.
So for safety reason before you 'reuse' any existing name in-use,
there should be 'deactivating' such device first - so there is no 'race' in
name usage.
It's even possible we miss to track full history of active renamed device.
Since you get into strange scenarios when you start to count
with udev event handling and link generating here - it's getting nearly
impossible to synchronize this properly...

Does that mean, if I can confirm that there's no program using the device name,
then it's safe to rename an active device? The devices I want to rename are
internal volumes. I think that there's no user space program using these names,
except LVM.

There's a typo error in my previous message.
The 2nd subLV is "vg1-lv1_child2", not "vg1-lv2_child2"

# dmsetup ls --tree
vg1-lv1 (253:3)
├─ vg1-lv1_child1 (253:1)
└─ vg1-lv1_child2 (253:2)

[Zdenek Kabelac]

Post by M.H. Tsai

Post by Zdenek Kabelac
The problem with rename is -
you have device 'lv1' you rename it to 'lv2' - yet
those who opened device with the name 'lv1' still thinks
the 'lv1' device exits.
So for safety reason before you 'reuse' any existing name in-use,
there should be 'deactivating' such device first - so there is no 'race' in
name usage.
It's even possible we miss to track full history of active renamed device.
Since you get into strange scenarios when you start to count
with udev event handling and link generating here - it's getting nearly
impossible to synchronize this properly...

Does that mean, if I can confirm that there's no program using the device name,

Hi

It doesn't really matter here what you could confirm here - there is a race
you can't avoid - i.e. udev is completely 'independent' and may execute
trigger udev rules at any random point in time or some other command may try
to open device in parallel (i.e. 'dd')

So the only way how to ensure there is no such race - is to deactivate such
device (which should be possible - since as you said - noone has it open)

Also remember - activation routine is 'separate' from command code - as it
could run on a completely different node - so you cannot 'validate' from
command code there is no user of a device on 'activation' node unless device
is locally active.

Post by M.H. Tsai
then it's safe to rename an active device? The devices I want to rename are
internal volumes. I think that there's no user space program using these names,
except LVM.

IMHO there is no point to 'optimize' this process - I do not expect anyone is
doing million swaps of internal LVs in a second.

Thus going through the proper sequence of steps and allowing udev to properly
synchronize (i.e. you should not 'mix' activation & deactivation under same
cookie) is clearly the best way how to achieve your desired goal.

Regards

Zdenek

[M.H. Tsai]

Post by Zdenek Kabelac
It doesn't really matter here what you could confirm here - there is a race
you can't avoid - i.e. udev is completely 'independent' and may execute
trigger udev rules at any random point in time or some other command may try
to open device in parallel (i.e. 'dd')
So the only way how to ensure there is no such race - is to deactivate such
device (which should be possible - since as you said - noone has it open)
Also remember - activation routine is 'separate' from command code - as it
could run on a completely different node - so you cannot 'validate' from
command code there is no user of a device on 'activation' node unless device
is locally active.
Thus going through the proper sequence of steps and allowing udev to
properly synchronize (i.e. you should not 'mix' activation & deactivation
under same cookie) is clearly the best way how to achieve your desired goal.

Hi,

Sorry for opening this question again. I could image the potential problem in
name swapping. But does that mean, It's even unrecommended to rename
an active device, although the target name are not used? The renaming path
in LVM is not suggested ?

## this is not recommended
# lvchange -ay vg1/lv1
# lvrename vg1/lv1 vg1/lv2

## do this instead
# lvchange -an vg1/lv1
# lvrename vg1/lv1 vg1/lv2


Thanks,
Ming-Hung Tsai

Post by Zdenek Kabelac

Post by Zdenek Kabelac
The problem with rename is -
you have device 'lv1' you rename it to 'lv2' - yet
those who opened device with the name 'lv1' still thinks
the 'lv1' device exits.
So for safety reason before you 'reuse' any existing name in-use,
there should be 'deactivating' such device first - so there is no 'race'
in name usage.

[Zdenek Kabelac]

Post by M.H. Tsai

Post by Zdenek Kabelac
It doesn't really matter here what you could confirm here - there is a race
you can't avoid - i.e. udev is completely 'independent' and may execute
trigger udev rules at any random point in time or some other command may try
to open device in parallel (i.e. 'dd')
So the only way how to ensure there is no such race - is to deactivate such
device (which should be possible - since as you said - noone has it open)
Also remember - activation routine is 'separate' from command code - as it
could run on a completely different node - so you cannot 'validate' from
command code there is no user of a device on 'activation' node unless device
is locally active.
Thus going through the proper sequence of steps and allowing udev to
properly synchronize (i.e. you should not 'mix' activation & deactivation
under same cookie) is clearly the best way how to achieve your desired goal.

Hi,
Sorry for opening this question again. I could image the potential problem in
name swapping. But does that mean, It's even unrecommended to rename
an active device, although the target name are not used? The renaming path
in LVM is not suggested ?
## this is not recommended
# lvchange -ay vg1/lv1
# lvrename vg1/lv1 vg1/lv2
## do this instead
# lvchange -an vg1/lv1
# lvrename vg1/lv1 vg1/lv2

Rename is fine as long as you are not reusing names while devices are still
active.

Check which devices names are reported by i.e. mount after you rename a device
- keep devices mounted and do rename like this:

lvcreate lv1
mkfs,mount lv1
lvrename lv1 -> lv2
lvcreate lv3
mkfs,mount lv3
lvrename lv3 -> lv1

Check 'mount' result or /proc/self/mountinfo

So doing such 'live' rename may lead to major confusion of system admin -
since those device names might be completely different from reality in your
/dev dir.

Zdenek