Re: CVS commit: src/sys/arch/xen/xen

[Maxime Villard <max%M00nBSD.net@localhost>]

Le 21/09/2014 20:22, Christos Zoulas a écrit :
> On Sep 21,  7:57pm, max%M00nBSD.net@localhost (Maxime Villard) wrote:
> -- Subject: Re: CVS commit: src/sys/arch/xen/xen
>
> | Did you test this change? Verily I'm not sure it's a proper bug, but I
> | kept it in my list so that someone (bouyer@?) could investigate.
>
> This is why I committed the fix in two pieces. The first one is obviously
> right. The second one is probably correct too, and if you notice there is
> also another if statement below that does not free either, but returns
> EINVAL. Didn't your code checker flag that?
>
> The only change I did not apply was a false positive
> (jumping inside a loop!?!?) which was disgusting code.
>
> christos

--------------------------------- 1 ---------------------------------

 			error  = privcmd_map_obj(vmm, va, maddr,
 			    mentry.npages, mcmd->dom);
-			if (error)
+			if (error) {
+				kmem_free(maddr,
+				    sizeof(paddr_t) * mentry.npages);
 				return error;
+			}
 		}


That's a fix I don't feel comfortable with: in privcmd_map_obj(), there
are two

		kmem_free(maddr, sizeof(paddr_t) * npages);
		return EINVAL;

One however returns an error without freeing:

	if (newstart != start) {
		printf("uvm_map didn't give us back our vm space\n");
		return EINVAL;
	}

I think this one is the real bug; isn't it?



--------------------------------- 2 ---------------------------------
I see you committed this:

 #include <sys/param.h>
 #include <sys/systm.h>
@@ -556,7 +556,7 @@ privcmd_map_obj(struct vm_map *map, vadd
 	/* remove current entries */
 	uvm_unmap1(map, start, start + size, 0);
 
-	obj = kmem_alloc(sizeof(struct privcmd_object), KM_SLEEP);
+	obj = kmem_alloc(sizeof(*obj), KM_SLEEP);
 	if (obj == NULL) {
 		kmem_free(maddr, sizeof(paddr_t) * npages);
 		return ENOMEM;
@@ -576,6 +576,8 @@ privcmd_map_obj(struct vm_map *map, vadd
 	if (error) {
 		if (obj)
 			obj->uobj.pgops->pgo_detach(&obj->uobj);
+		kmem_free(maddr, sizeof(paddr_t) * npages);
+		kmem_free(obj, sizeof(*obj));
 		return error;
 	}


But if I understand correctly, pgo_detach is linked to privpgop_detach
which already frees 'obj' and 'maddr'. Doesn't it?