[PATCH] Fix segfault when using vgsplit in stacked environment

[PATCH] Fix segfault when using vgsplit in stacked environment

[Peter Rajnoha]

Hi,

this is a small patch for segmentation fault in vgsplit while using 
stacked VGs (rh bz 481793). Segfault is caused by null dereference in 
'pv_uses_vg' function that is called to check if a PV uses VG somewhere 
in its construction. In stacked environment, when the LV/VG from lower 
layer is disabled, the PV constructed above does not exist as well and 
so the device value in PV's structure is set to NULL. We should check 
for this NULL value in 'pv_uses_vg' and return immediately if the 
situation occurs.

Peter


diff --git a/lib/activate/activate.c b/lib/activate/activate.c
index 2bc1db7..4ffd7bb 100644
--- a/lib/activate/activate.c
+++ b/lib/activate/activate.c
@@ -1105,6 +1105,9 @@ int pv_uses_vg(struct physical_volume *pv,
  	if (!activation())
  		return 0;

+	if (!pv->dev || !pv->dev->dev)
+		return 1;
+
  	if (!dm_is_dm_major(MAJOR(pv->dev->dev)))
  		return 0;

[PATCH] Fix segfault when using vgsplit in stacked environment

[Milan Broz]

Peter Rajnoha wrote:
> diff --git a/lib/activate/activate.c b/lib/activate/activate.c
> index 2bc1db7..4ffd7bb 100644
> --- a/lib/activate/activate.c
> +++ b/lib/activate/activate.c
> @@ -1105,6 +1105,9 @@ int pv_uses_vg(struct physical_volume *pv,
>   	if (!activation())
>   		return 0;
> 
> +	if (!pv->dev || !pv->dev->dev)
> +		return 1;
> +

(answering myself - why "return 1". the function name is misleading for me...)
/*
 * Does PV use VG somewhere in its construction?
 * Returns 1 on failure.
 */
int pv_uses_vg(struct physical_volume *pv,
	       struct volume_group *vg)



Please add something like this to cover it by testsuite
(best add it to already existing vgsplit tests):

. ./test-utils.sh

aux prepare_devs 3

pvcreate $devs
vgcreate $vg1 $dev1 $dev2
lvcreate -n $lv1 -l 100%FREE $vg1

#top VG
pvcreate $G_dev_/$vg1/$lv1
vgcreate $vg $G_dev_/$vg1/$lv1 $dev3

vgchange -a n $vg
vgchange -a n $vg1

# this should fail but not segfault
not vgsplit $vg $vg1 $dev3


Acked-by: Milan Broz <mbroz@redhat.com>
Tested-by: Milan Broz <mbroz@redhat.com>

Milan
--
mbroz at redhat.com

[PATCH] Fix segfault when using vgsplit in stacked environment

[Alasdair G Kergon]

On Mon, Feb 09, 2009 at 08:53:45AM +0100, Peter Rajnoha wrote:
> this is a small patch for segmentation fault in vgsplit while using 
> stacked VGs (rh bz 481793). Segfault is caused by null dereference in 
> 'pv_uses_vg' function that is called to check if a PV uses VG somewhere 
> in its construction. In stacked environment, when the LV/VG from lower 
> layer is disabled, the PV constructed above does not exist as well and 

I don't think the code should be getting as far as it does in that
situation.

It is meaningless to call pv_uses_vg() if the PV does not exist.
And the function currently returns 0 if the answer is unknown - it'll
get even more confusing if it returns 0 for some types of 'unknown'
and 1 for other types!

Alasdair
-- 
agk at redhat.com

[PATCH] Fix segfault when using vgsplit in stacked environment

[Peter Rajnoha]

Alasdair G Kergon wrote:
> It is meaningless to call pv_uses_vg() if the PV does not exist.

Well, it's some kind of an exception, so I thought it would be
better to check it right here than all the other places outside
the function.

> And the function currently returns 0 if the answer is unknown - it'll
> get even more confusing if it returns 0 for some types of 'unknown'
> and 1 for other types!

Hmm, isn't it the opposite way? Looking at the dev_manager_device_uses_vg
function that is called from pv_uses_vg and returs its value,
dev_manager_device_uses_vg should return 1 if uncertain. So I relied on
this at that time... I return 1 when checking for NULL value of pv->dev,
because we can't decide if PV uses VG in this situation, too.

Peter

[PATCH] Fix segfault when using vgsplit in stacked environment

[Petr Rockai]

Hi,

Alasdair G Kergon <agk@redhat.com> writes:
> I don't think the code should be getting as far as it does in that
> situation.

(explanation for the list) After an IRC discussion, we have agreed that a
different approach would work here, specifically, that we should not allow
tools to try tinkering with VGs that have PVs missing, unless they specifically
know what they are doing. There's probably half dozen other places where we
assume that `pv->dev` is valid. The attached patch changes the meaning of
`cmd->handles_missing_pvs` somewhat: If a tool now opens a VG *for writing* but
it does not set handles_missing_pvs, vg_read will fail.

This check was previously done only in vg_write, which led to situations like
the above bug, where a little less vigorous code path trips a NULL
pointer. This behaviour change of handles_missing_pvs affects these situations:

- lvchange -a, vgchange -a take a write lock, so they need to set, for the -a
  case, handles_missing_pvs. This is safe.
- vgreduce needs to set handles_missing_pvs, since it is supposed to work in
  that situation... it previously did not need the flag in general, since it
  most of the time writes out a VG that has no PVs missing in it; however,
  since it's now needed also for opening the VG for writing, this needs to be
  always set for --removemissing; there's a small risk of bugs associated, but
  it can be mitigated by adding appropriate check to the code...

Please note that this patch depends on the "vg_read" patchset found elsewhere
on this list.

Yours,
   Petr.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: lvm-vg_read-missing_pvs.diff
Type: text/x-diff
Size: 4043 bytes
Desc: lvm-vg_read-missing_pvs.diff
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20090216/47777170/attachment.bin>
-------------- next part --------------

PS: The patch passes the testsuite and also makes Milan's testcase pass (it is
included with the patch).

-- 
Peter Rockai | me()mornfall!net | prockai()redhat!com
 http://blog.mornfall.net | http://web.mornfall.net

"In My Egotistical Opinion, most people's C programs should be
 indented six feet downward and covered with dirt."
     -- Blair P. Houghton on the subject of C program indentation