lvm problems on sparc64 - Trying to vfree() nonexistent vm area

All,

I'm seeing problems with lvm on sparc64. I have a reproducible test case using snapshots where I can reliably reproduce an error similar to

Trying to vfree() nonexistent vm area (0000000140072000)

Under some circumstances I can get this to lead to a panic where the kernel reports that it is unable to handle a paging request for the same request (the address below is different because it comes from a different attempt to reproduce it)

Unable to handle kernel paging request at virtual address

000000014007a000

First off the obligatory "what I'm using" statement.

I've seen this with debian/testing on sparc using the stock 2.4.26 kernel.
Using lvm1.0.8

pingu:~# apt-show-versions | grep lvm
lvm10/testing uptodate 1:1.0.8-4
lvm-common/testing uptodate 1.5.16

kernel-image-2.4.26-sparc64/testing uptodate 39

I've done a fair amount of detective work and am now fairly confident that the problem is somewhere in the sparc64 ioctl32 glue code in arch/sparc64/kernel/ioctl32.c

Adding printk tracing into the kernel and using strace I can see that the problem occurs on a call to

ioctl(4, LV_STATUS_BYNAME, 0xeffff9f8) = 0

The message only gets reported on second and subsequent calls to this ioctl. So I'm guessing that something gets freed wrongly during the first call.

I added some tracing into do_lvm_ioctl() in ioctl32.c. From this I can see that the problem gets reported after calling the "real" ioctl routine whilst transferring the result data back into the 32 bit structures.

Specifically the problem is occurring in put_lv_t(u.lv_req.lv) as called from the LV_CREATE/EXTEND/REDUCE case in the switch statement.

This means that the problem is related to the vfree of either l->lv_current_pe or l->lv_block_exception.

I cannot see anything obviously wrong there (and need some sleep!) so hopefully someone else will have some ideas as to what the problem is.

Thanks

Richard

P.S.

Forgot to mention. The steps to reproduct the problem are (assuming that you have a vg called test)

lvcreate --size 128M --name fsone /dev/test lvcreate --size 128M --name fstwo /dev/test lvcreate --snapshot --size 128M --name bakone /dev/test/fsone lvremove /dev/test/bakone

You get the problem even before you answer the y/n question in lvremove.

[email protected]

For once it's not sparc64's fault, it's a bug in the generic LVM ioctl handling :-)

It saves both pointers, clobbers the userspace copy, then only restores one of the two pointers correctly. Easy to fix, see below.

Marcelo, please apply, thanks.

# This is a BitKeeper generated diff -Nru style patch. #
# ChangeSet
# 2004/08/30 17:24:05-07:00 [email protected]
# [LVM]: Do not forget to restore both user pointers.
#

#   This in particular can make compatability layers
#   crash, and it is a bug for regular applications
#   too.
#   
#   Signed-off-by: David S. Miller <[email protected]>
# 
# drivers/md/lvm.c
#   2004/08/30 17:23:48-07:00 [email protected] +12 -0
#   [LVM]: Do not forget to restore both user pointers.
#   
#   This in particular can make compatability layers
#   crash, and it is a bug for regular applications
#   too.
#   
#   Signed-off-by: David S. Miller <[email protected]>
# 
diff -Nru a/drivers/md/lvm.c b/drivers/md/lvm.c
--- a/drivers/md/lvm.c	2004-08-30 17:24:13 -07:00
+++ b/drivers/md/lvm.c	2004-08-30 17:24:13 -07:00
@@ -2689,6 +2689,10 @@
 			    (&lv_status_byname_req.lv->lv_current_pe,
 			     &saved_ptr1, sizeof(void *)) != 0)
 				return -EFAULT;
+			if (copy_to_user
+			    (&lv_status_byname_req.lv->lv_block_exception,
+			     &saved_ptr2, sizeof(void *)) != 0)
+				return -EFAULT;
 			return 0;
 		}
 	}
@@ -2743,6 +2747,10 @@
 	    (&lv_status_byindex_req.lv->lv_current_pe, &saved_ptr1,
 	     sizeof(void *)) != 0)
 		return -EFAULT;
+	if (copy_to_user
+	    (&lv_status_byindex_req.lv->lv_block_exception, &saved_ptr2,
+	     sizeof(void *)) != 0)
+		return -EFAULT;

return 0;
} /* lvm_do_lv_status_byindex() */
@@ -2799,6 +2807,10 @@
/* Restore usermode pointers */
if (copy_to_user
(&lv_status_bydev_req.lv->lv_current_pe, &saved_ptr1,
+ sizeof(void *)) != 0)
+ return -EFAULT;
+ if (copy_to_user
+ (&lv_status_bydev_req.lv->lv_block_exception, &saved_ptr2,
sizeof(void *)) != 0)
return -EFAULT;

Thanks. Fixes the problem nicely.

Whilst I remember I forgot to include the patch below last night. When I was looking at the ioctl32.c code I noticed a few inconsistencies in the structures used in the copy to/from user calls. I don't think that the original versions were incorrect but it seems best to make things consistent.

How do they look

Richard

--- arch/sparc64/kernel/ioctl32.c.orig 2004-08-29 00:12:09.000000000
+0100
+++ arch/sparc64/kernel/ioctl32.c 2004-08-31 22:06:23.000000000 +0100
@@ -2949,7 +2949,7 @@
case LV_REMOVE:
case LV_RENAME:
case LV_STATUS_BYNAME:
- err = copy_from_user(&u.pv_status, arg,
sizeof(u.pv_status.pv_name));
+ err = copy_from_user(&u.lv_req, arg,
sizeof(u.lv_req.lv_name));
if (err)
return -EFAULT;
if (cmd != LV_REMOVE) {
@@ -2992,7 +2992,7 @@

case PV_CHANGE:
case PV_STATUS:
- err = copy_from_user(&u.pv_status, arg, sizeof(u.lv_req.lv_name));
+ err = copy_from_user(&u.pv_status, arg, sizeof(u.pv_status.pv_name));
if (err)
return -EFAULT;
err = __get_user(ptr, &((pv_status_req32_t *)arg)->pv);
@@ -3064,7 +3064,7 @@
if (u.lv_bydev.lv) {
if (!err)
err = copy_lv_t(ptr, u.lv_bydev.lv);
- put_lv_t(u.lv_byindex.lv);
+ put_lv_t(u.lv_bydev.lv);
}
break;

[email protected]

Please resend your patch without all the line breaks created by your email client. It looks fine otherwise :)

Bah. Clients are just getting too clever these days.

Hopefully this should be ok.

Richard

@@ -2949,7 +2949,7 @@
 	case LV_REMOVE:
 	case LV_RENAME:
 	case LV_STATUS_BYNAME:
-	        err = copy_from_user(&u.pv_status, arg, sizeof(u.pv_status.pv_name));
+	        err = copy_from_user(&u.lv_req, arg, sizeof(u.lv_req.lv_name));
 		if (err)
 			return -EFAULT;
 		if (cmd != LV_REMOVE) {

@@ -2992,7 +2992,7 @@
 
 	case PV_CHANGE:
 	case PV_STATUS:
-		err = copy_from_user(&u.pv_status, arg, sizeof(u.lv_req.lv_name));
+		err = copy_from_user(&u.pv_status, arg, sizeof(u.pv_status.pv_name));
 		if (err)
 			return -EFAULT;
 		err = __get_user(ptr, &((pv_status_req32_t *)arg)->pv);

@@ -3064,7 +3064,7 @@
 	        if (u.lv_bydev.lv) {
 			if (!err)
 				err = copy_lv_t(ptr, u.lv_bydev.lv);
-			put_lv_t(u.lv_byindex.lv);
+			put_lv_t(u.lv_bydev.lv);
 	        }
 	        break;
 

[email protected]

Thanks a lot Richard, applied.