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 | ||||
Context not available. | |||||
static void shm_init(void *arg); | static void shm_init(void *arg); | ||||
static void shm_insert(char *path, Fnv32_t fnv, struct shmfd *shmfd); | static void shm_insert(char *path, Fnv32_t fnv, struct shmfd *shmfd); | ||||
static struct shmfd *shm_lookup(char *path, Fnv32_t fnv); | static struct shmfd *shm_lookup(char *path, Fnv32_t fnv); | ||||
static int shm_remove(char *path, Fnv32_t fnv, struct ucred *ucred); | static int shm_remove(char *path, Fnv32_t fnv, struct ucred *ucred, | ||||
bool preserve); | |||||
static fo_rdwr_t shm_read; | static fo_rdwr_t shm_read; | ||||
static fo_rdwr_t shm_write; | static fo_rdwr_t shm_write; | ||||
Context not available. | |||||
} | } | ||||
static int | static int | ||||
shm_remove(char *path, Fnv32_t fnv, struct ucred *ucred) | shm_remove(char *path, Fnv32_t fnv, struct ucred *ucred, bool preserve) | ||||
{ | { | ||||
struct shm_mapping *map; | struct shm_mapping *map; | ||||
int error; | int error; | ||||
Context not available. | |||||
return (error); | return (error); | ||||
map->sm_shmfd->shm_path = NULL; | map->sm_shmfd->shm_path = NULL; | ||||
LIST_REMOVE(map, sm_link); | LIST_REMOVE(map, sm_link); | ||||
shm_drop(map->sm_shmfd); | if (!preserve) | ||||
shm_drop(map->sm_shmfd); | |||||
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. | |||||
free(map->sm_path, M_SHMFD); | free(map->sm_path, M_SHMFD); | ||||
free(map, M_SHMFD); | free(map, M_SHMFD); | ||||
return (0); | return (0); | ||||
Context not available. | |||||
AUDIT_ARG_UPATH1_CANON(path); | AUDIT_ARG_UPATH1_CANON(path); | ||||
fnv = fnv_32_str(path, FNV1_32_INIT); | fnv = fnv_32_str(path, FNV1_32_INIT); | ||||
sx_xlock(&shm_dict_lock); | sx_xlock(&shm_dict_lock); | ||||
error = shm_remove(path, fnv, td->td_ucred); | error = shm_remove(path, fnv, td->td_ucred, false); | ||||
sx_xunlock(&shm_dict_lock); | sx_xunlock(&shm_dict_lock); | ||||
free(path, M_TEMP); | free(path, M_TEMP); | ||||
Context not available. | |||||
return (error); | return (error); | ||||
} | } | ||||
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… | |||||
/* | |||||
* TODO: add a 'flags' argument, similar to the Linux renameat2. | |||||
* The flag states what should happen if there is an shm already at path_to. | |||||
* That way we have the option to e.g. return EEXIST instead of unlinking | |||||
* the shm at path_to. We could also have an option to swap the two shms. | |||||
*/ | |||||
int | int | ||||
sys_shm_rename(struct thread *td, struct shm_rename_args *uap) | |||||
{ | |||||
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)… | |||||
char *path_from = NULL, *path_to = NULL; | |||||
Fnv32_t fnv_from, fnv_to; | |||||
struct shmfd *fd_from; | |||||
int error; | |||||
path_from = malloc(MAXPATHLEN, M_TEMP, M_WAITOK); | |||||
error = copyinstr(uap->path_from, path_from, MAXPATHLEN, NULL); | |||||
if (error) | |||||
goto out; | |||||
Done Inline ActionsThis comment now applies to both paths. jilles: This comment now applies to both paths. | |||||
/* | |||||
* Malloc zone M_SHMFD, since this path may end up freed later from | |||||
* M_SHMFD. | |||||
*/ | |||||
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; | |||||
} | |||||
error = shm_remove(path_from, fnv_from, td->td_ucred, true); | |||||
/* | |||||
* One of my assumptions failed if ENOENT (e.g. locking didn't | |||||
* protect us) | |||||
*/ | |||||
KASSERT(error != ENOENT, ("Our shm disappeared during shm_rename: %s", | |||||
path_from)); | |||||
if (error) { | |||||
sx_xunlock(&shm_dict_lock); | |||||
goto out; | |||||
} | |||||
/* | |||||
* NOTE: if path_to is not already in the hash, c'est la vie; | |||||
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. | |||||
* 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. | |||||
*/ | |||||
error = shm_remove(path_to, fnv_to, td->td_ucred, false); | |||||
if (error == EACCES) { | |||||
shm_insert(path_from, fnv_from, fd_from); | |||||
/* Don't free path_from now, since the hash references it */ | |||||
path_from = NULL; | |||||
sx_xunlock(&shm_dict_lock); | |||||
goto out; | |||||
} | |||||
else if (error && error != ENOENT) { | |||||
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 | |||||
sx_xunlock(&shm_dict_lock); | |||||
out: | |||||
if (path_from != NULL) | |||||
free(path_from, M_TEMP); | |||||
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.