Changeset View
Standalone View
sys/amd64/vmm/vmm_dev.c
Show First 20 Lines • Show All 74 Lines • ▼ Show 20 Lines | struct devmem_softc { | ||||
struct cdev *cdev; | struct cdev *cdev; | ||||
struct vmmdev_softc *sc; | struct vmmdev_softc *sc; | ||||
SLIST_ENTRY(devmem_softc) link; | SLIST_ENTRY(devmem_softc) link; | ||||
}; | }; | ||||
struct vmmdev_softc { | struct vmmdev_softc { | ||||
struct vm *vm; /* vm instance cookie */ | struct vm *vm; /* vm instance cookie */ | ||||
struct cdev *cdev; | struct cdev *cdev; | ||||
struct ucred *ucred; | |||||
SLIST_ENTRY(vmmdev_softc) link; | SLIST_ENTRY(vmmdev_softc) link; | ||||
SLIST_HEAD(, devmem_softc) devmem; | SLIST_HEAD(, devmem_softc) devmem; | ||||
int flags; | int flags; | ||||
}; | }; | ||||
#define VSC_LINKED 0x01 | #define VSC_LINKED 0x01 | ||||
static SLIST_HEAD(, vmmdev_softc) head; | static SLIST_HEAD(, vmmdev_softc) head; | ||||
▲ Show 20 Lines • Show All 86 Lines • ▼ Show 20 Lines | #ifdef notyet /* XXX kernel is not compiled with invariants */ | ||||
mtx_assert(&vmmdev_mtx, MA_OWNED); | mtx_assert(&vmmdev_mtx, MA_OWNED); | ||||
#endif | #endif | ||||
SLIST_FOREACH(sc, &head, link) { | SLIST_FOREACH(sc, &head, link) { | ||||
if (strcmp(name, vm_name(sc->vm)) == 0) | if (strcmp(name, vm_name(sc->vm)) == 0) | ||||
break; | break; | ||||
} | } | ||||
if (sc == NULL) | |||||
return (NULL); | |||||
if (cr_cansee(curthread->td_ucred, sc->ucred)) | |||||
return (NULL); | |||||
return (sc); | return (sc); | ||||
jhb: This is ok, but another way you could have handled this would be:
```
SLIST_FOREACH(...) {… | |||||
} | } | ||||
static struct vmmdev_softc * | static struct vmmdev_softc * | ||||
vmmdev_lookup2(struct cdev *cdev) | vmmdev_lookup2(struct cdev *cdev) | ||||
{ | { | ||||
return (cdev->si_drv1); | return (cdev->si_drv1); | ||||
} | } | ||||
▲ Show 20 Lines • Show All 780 Lines • ▼ Show 20 Lines | vmmdev_destroy(void *arg) | ||||
} | } | ||||
if (sc->cdev != NULL) | if (sc->cdev != NULL) | ||||
destroy_dev(sc->cdev); | destroy_dev(sc->cdev); | ||||
if (sc->vm != NULL) | if (sc->vm != NULL) | ||||
vm_destroy(sc->vm); | vm_destroy(sc->vm); | ||||
if (sc->ucred != NULL) | |||||
crfree(sc->ucred); | |||||
if ((sc->flags & VSC_LINKED) != 0) { | if ((sc->flags & VSC_LINKED) != 0) { | ||||
mtx_lock(&vmmdev_mtx); | mtx_lock(&vmmdev_mtx); | ||||
SLIST_REMOVE(&head, sc, vmmdev_softc, link); | SLIST_REMOVE(&head, sc, vmmdev_softc, link); | ||||
mtx_unlock(&vmmdev_mtx); | mtx_unlock(&vmmdev_mtx); | ||||
} | } | ||||
free(sc, M_VMMDEV); | free(sc, M_VMMDEV); | ||||
} | } | ||||
Show All 20 Lines | sysctl_vmm_destroy(SYSCTL_HANDLER_ARGS) | ||||
mtx_lock(&vmmdev_mtx); | mtx_lock(&vmmdev_mtx); | ||||
sc = vmmdev_lookup(buf); | sc = vmmdev_lookup(buf); | ||||
if (sc == NULL || sc->cdev == NULL) { | if (sc == NULL || sc->cdev == NULL) { | ||||
mtx_unlock(&vmmdev_mtx); | mtx_unlock(&vmmdev_mtx); | ||||
error = EINVAL; | error = EINVAL; | ||||
goto out; | goto out; | ||||
} | } | ||||
Not Done Inline ActionsI think it's probably ok to borrow the cdev's credential reference like this but it'd be a bit nicer to maintain a separate reference (to the same credential) in the VM softc as well. The overhead of doing that is negligible, and it might avoid some tricky bugs that would arise if something tries to access the VM's credential after the cdev is destroyed. For example, this function sets sc->cdev = NULL before triggering asynchronous destruction of the VM (which may take a long time) by calling destroy_dev_sched_cb(). If someone naively modifies vmmdev_destroy() such that it tries to dereference sc->cdev->si_cred (perhaps through several layers of indirection), we'll get a null pointer dereference. Keeping a separate credential reference avoids coupling between VMM and devfs object lifecycles, so I'd tend to prefer that approach even though it requires a bit more code. markj: I think it's probably ok to borrow the cdev's credential reference like this but it'd be a bit… | |||||
/* | /* | ||||
* The 'cdev' will be destroyed asynchronously when 'si_threadcount' | * The 'cdev' will be destroyed asynchronously when 'si_threadcount' | ||||
Not Done Inline ActionsI think EPERM is a more suitable error number here. markj: I think EPERM is a more suitable error number here. | |||||
* goes down to 0 so we should not do it again in the callback. | * goes down to 0 so we should not do it again in the callback. | ||||
* | * | ||||
Not Done Inline ActionsI think that we should move this check into vmmdev_lookup() and have it return NULL if the check fails. Otherwise every caller of vmmdev_lookup() will have to be careful to check the credential. markj: I think that we should move this check into vmmdev_lookup() and have it return NULL if the… | |||||
* Setting 'sc->cdev' to NULL is also used to indicate that the VM | * Setting 'sc->cdev' to NULL is also used to indicate that the VM | ||||
* is scheduled for destruction. | * is scheduled for destruction. | ||||
*/ | */ | ||||
cdev = sc->cdev; | cdev = sc->cdev; | ||||
sc->cdev = NULL; | sc->cdev = NULL; | ||||
mtx_unlock(&vmmdev_mtx); | mtx_unlock(&vmmdev_mtx); | ||||
/* | /* | ||||
▲ Show 20 Lines • Show All 60 Lines • ▼ Show 20 Lines | if (sc != NULL) { | ||||
goto out; | goto out; | ||||
} | } | ||||
error = vm_create(buf, &vm); | error = vm_create(buf, &vm); | ||||
if (error != 0) | if (error != 0) | ||||
goto out; | goto out; | ||||
sc = malloc(sizeof(struct vmmdev_softc), M_VMMDEV, M_WAITOK | M_ZERO); | sc = malloc(sizeof(struct vmmdev_softc), M_VMMDEV, M_WAITOK | M_ZERO); | ||||
sc->ucred = crhold(curthread->td_ucred); | |||||
sc->vm = vm; | sc->vm = vm; | ||||
SLIST_INIT(&sc->devmem); | SLIST_INIT(&sc->devmem); | ||||
/* | /* | ||||
* Lookup the name again just in case somebody sneaked in when we | * Lookup the name again just in case somebody sneaked in when we | ||||
* dropped the lock. | * dropped the lock. | ||||
*/ | */ | ||||
mtx_lock(&vmmdev_mtx); | mtx_lock(&vmmdev_mtx); | ||||
sc2 = vmmdev_lookup(buf); | sc2 = vmmdev_lookup(buf); | ||||
if (sc2 == NULL) { | if (sc2 == NULL) { | ||||
SLIST_INSERT_HEAD(&head, sc, link); | SLIST_INSERT_HEAD(&head, sc, link); | ||||
sc->flags |= VSC_LINKED; | sc->flags |= VSC_LINKED; | ||||
} | } | ||||
mtx_unlock(&vmmdev_mtx); | mtx_unlock(&vmmdev_mtx); | ||||
if (sc2 != NULL) { | if (sc2 != NULL) { | ||||
vmmdev_destroy(sc); | vmmdev_destroy(sc); | ||||
error = EEXIST; | error = EEXIST; | ||||
goto out; | goto out; | ||||
} | } | ||||
error = make_dev_p(MAKEDEV_CHECKNAME, &cdev, &vmmdevsw, NULL, | error = make_dev_p(MAKEDEV_CHECKNAME, &cdev, &vmmdevsw, sc->ucred, | ||||
UID_ROOT, GID_WHEEL, 0600, "vmm/%s", buf); | UID_ROOT, GID_WHEEL, 0600, "vmm/%s", buf); | ||||
if (error != 0) { | if (error != 0) { | ||||
vmmdev_destroy(sc); | vmmdev_destroy(sc); | ||||
goto out; | goto out; | ||||
} | } | ||||
mtx_lock(&vmmdev_mtx); | mtx_lock(&vmmdev_mtx); | ||||
sc->cdev = cdev; | sc->cdev = cdev; | ||||
sc->cdev->si_drv1 = sc; | sc->cdev->si_drv1 = sc; | ||||
▲ Show 20 Lines • Show All 126 Lines • Show Last 20 Lines |
This is ok, but another way you could have handled this would be:
Layering the checks like that would make the new code a bit smaller (you are just adding a new check, not changing the existing loop). It also avoids deep nesting if we wanted to add additional checks in the future.