Changeset View
Standalone View
sys/kern/uipc_shm.c
Context not available. | |||||
/* | /* | ||||
* Support for shared swap-backed anonymous memory objects via | * Support for shared swap-backed anonymous memory objects via | ||||
* shm_open(2) and shm_unlink(2). While most of the implementation is | * shm_open(2), shm_rename(2), and shm_unlink(2). | ||||
* here, vm_mmap.c contains mapping logic changes. | * While most of the implementation is here, vm_mmap.c contains | ||||
* mapping logic changes. | |||||
* | * | ||||
* posixshmcontrol(1) allows users to inspect the state of the memory | * posixshmcontrol(1) allows users to inspect the state of the memory | ||||
* objects. Per-uid swap resource limit controls total amount of | * objects. Per-uid swap resource limit controls total amount of | ||||
jilles: This adds some additional complication (boolean) to avoid two refcount operations in the rename… | |||||
Done Inline ActionsIt also avoids some cleanup like mtx_destroy that would invalidate the object's state. So: avoiding the shm_drop is best. matthew.bryan_isilon.com: It also avoids some cleanup like mtx_destroy that would invalidate the object's state. So… | |||||
Done Inline ActionsDerp; let's just shm_hold before shm_remove to prevent any invalidation. Fixed. matthew.bryan_isilon.com: Derp; let's just shm_hold before shm_remove to prevent any invalidation. Fixed. | |||||
Context not available. | |||||
} | } | ||||
int | int | ||||
sys_shm_rename(struct thread *td, struct shm_rename_args *uap) | |||||
Done Inline ActionsAre you planning on adding the instrumentation to audit the syscall arguments in a future revision? csjp: Are you planning on adding the instrumentation to audit the syscall arguments in a future… | |||||
Done Inline ActionsLooks like you found the OpenBSM PR :) Yes; my plan is:
matthew.bryan_isilon.com: Looks like you found the OpenBSM PR :)
https://github.com/openbsm/openbsm/pull/54
Yes; my plan… | |||||
{ | |||||
char *path_from = NULL, *path_to = NULL; | |||||
Fnv32_t fnv_from, fnv_to; | |||||
struct shmfd *fd_from; | |||||
struct shmfd *fd_to; | |||||
int error; | |||||
int flags; | |||||
flags = uap->flags; | |||||
Done Inline ActionsPlease add something like if ((flags & ~(SHM_RENAME_NOREPLACE | SHM_RENAME_EXCHANGE)) != 0) return (EINVAL); so new flags can be safely added later. jilles: Please add something like
```
if ((flags & ~(SHM_RENAME_NOREPLACE | SHM_RENAME_EXCHANGE)) != 0)… | |||||
path_from = malloc(MAXPATHLEN, M_SHMFD, M_WAITOK); | |||||
error = copyinstr(uap->path_from, path_from, MAXPATHLEN, NULL); | |||||
if (error) | |||||
goto out; | |||||
/* | |||||
* Malloc zone M_SHMFD, since this path may end up freed later from | |||||
* M_SHMFD. | |||||
*/ | |||||
Done Inline ActionsThis comment now applies to both paths. jilles: This comment now applies to both paths. | |||||
path_to = malloc(MAXPATHLEN, M_SHMFD, M_WAITOK); | |||||
error = copyinstr(uap->path_to, path_to, MAXPATHLEN, NULL); | |||||
if (error) | |||||
goto out; | |||||
/* Rename with from/to equal is a no-op */ | |||||
if (strncmp(path_from, path_to, MAXPATHLEN) == 0) | |||||
goto out; | |||||
fnv_from = fnv_32_str(path_from, FNV1_32_INIT); | |||||
fnv_to = fnv_32_str(path_to, FNV1_32_INIT); | |||||
sx_xlock(&shm_dict_lock); | |||||
fd_from = shm_lookup(path_from, fnv_from); | |||||
if (fd_from == NULL) { | |||||
sx_xunlock(&shm_dict_lock); | |||||
error = ENOENT; | |||||
goto out; | |||||
} | |||||
fd_to = shm_lookup(path_to, fnv_to); | |||||
if ((flags & SHM_RENAME_NOREPLACE) != 0 && fd_to != NULL) { | |||||
sx_xunlock(&shm_dict_lock); | |||||
error = EEXIST; | |||||
goto out; | |||||
} | |||||
/* | |||||
* Unconditionally prevents shm_remove from invalidating the 'from' | |||||
* shm's state. | |||||
*/ | |||||
shm_hold(fd_from); | |||||
error = shm_remove(path_from, fnv_from, td->td_ucred); | |||||
/* | |||||
* One of my assumptions failed if ENOENT (e.g. locking didn't | |||||
* protect us) | |||||
*/ | |||||
KASSERT(error != ENOENT, ("Our shm disappeared during shm_rename: %s", | |||||
Done Inline ActionsThis case is clearly possible since permissions to rename/unlink a shared memory object are determined by bits on the object itself and may differ between two objects. Since shm_dict_lock is not unlocked in the meantime, the temporary removal should not be visible to other threads. jilles: This case is clearly possible since permissions to rename/unlink a shared memory object are… | |||||
Done Inline ActionsSounds right. I'll clean up the comment. matthew.bryan_isilon.com: Sounds right. I'll clean up the comment. | |||||
path_from)); | |||||
if (error) { | |||||
shm_drop(fd_from); | |||||
sx_xunlock(&shm_dict_lock); | |||||
goto out; | |||||
} | |||||
/* | |||||
* If we are exchanging, we need to ensure the shm_remove below | |||||
* doesn't invalidate the dest shm's state. | |||||
*/ | |||||
if ((flags & SHM_RENAME_EXCHANGE) != 0 && fd_to != NULL) | |||||
shm_hold(fd_to); | |||||
/* | |||||
* NOTE: if path_to is not already in the hash, c'est la vie; | |||||
* it simply means we have nothing already at path_to to unlink. | |||||
* That is the ENOENT case. | |||||
* | |||||
* If we somehow don't have access to unlink this guy, but | |||||
* did for the shm at path_from, then relink the shm to path_from | |||||
* and abort with EACCES. | |||||
* | |||||
* All other errors: that is weird; let's relink and abort the | |||||
* operation. | |||||
*/ | |||||
error = shm_remove(path_to, fnv_to, td->td_ucred); | |||||
if (error && error != ENOENT) { | |||||
shm_insert(path_from, fnv_from, fd_from); | |||||
shm_drop(fd_from); | |||||
/* Don't free path_from now, since the hash references it */ | |||||
path_from = NULL; | |||||
sx_xunlock(&shm_dict_lock); | |||||
goto out; | |||||
} | |||||
shm_insert(path_to, fnv_to, fd_from); | |||||
/* Don't free path_to now, since the hash references it */ | |||||
path_to = NULL; | |||||
/* We kept a ref when we removed, and incremented again in insert */ | |||||
shm_drop(fd_from); | |||||
#ifdef DEBUG | |||||
KASSERT(fd_from->shm_refs > 0, ("Expected >0 refs; got: %d\n", | |||||
fd_from->shm_refs)); | |||||
#endif | |||||
if ((flags & SHM_RENAME_EXCHANGE) != 0 && fd_to != NULL) { | |||||
shm_insert(path_from, fnv_from, fd_to); | |||||
path_from = NULL; | |||||
shm_drop(fd_to); | |||||
#ifdef DEBUG | |||||
KASSERT(fd_to->shm_refs > 0, ("Expected >0 refs; got: %d\n", | |||||
fd_to->shm_refs)); | |||||
#endif | |||||
} | |||||
error = 0; | |||||
sx_xunlock(&shm_dict_lock); | |||||
out: | |||||
if (path_from != NULL) | |||||
free(path_from, M_SHMFD); | |||||
if (path_to != NULL) | |||||
free(path_to, M_SHMFD); | |||||
return(error); | |||||
} | |||||
int | |||||
shm_mmap(struct file *fp, vm_map_t map, vm_offset_t *addr, vm_size_t objsize, | shm_mmap(struct file *fp, vm_map_t map, vm_offset_t *addr, vm_size_t objsize, | ||||
vm_prot_t prot, vm_prot_t cap_maxprot, int flags, | vm_prot_t prot, vm_prot_t cap_maxprot, int flags, | ||||
vm_ooffset_t foff, struct thread *td) | vm_ooffset_t foff, struct thread *td) | ||||
Context not available. |
This adds some additional complication (boolean) to avoid two refcount operations in the rename path. I can't judge whether it's worth it.
It still does two hash lookups. Consider returning the shmfd via a struct shmfd ** parameter that can be NULL.