On Thu, Sep 26, 2019 at 03:32:28PM +0000, David Bright wrote:
> Author: dab
> Date: Thu Sep 26 15:32:28 2019
> New Revision: 352747
> URL: https://svnweb.freebsd.org/changeset/base/352747
>
> Log:
> Add an shm_rename syscall
> Modified: head/sys/kern/uipc_shm.c
> ==============================================================================
> --- head/sys/kern/uipc_shm.c Thu Sep 26 15:18:57 2019 (r352746)
> +++ head/sys/kern/uipc_shm.c Thu Sep 26 15:32:28 2019 (r352747)
> @@ -33,8 +33,9 @@
>
> /*
> * Support for shared swap-backed anonymous memory objects via
> - * shm_open(2) and shm_unlink(2). While most of the implementation is
> - * here, vm_mmap.c contains mapping logic changes.
> + * shm_open(2), shm_rename(2), and shm_unlink(2).
> + * 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
> * objects. Per-uid swap resource limit controls total amount of
> @@ -945,6 +946,158 @@ sys_shm_unlink(struct thread *td, struct shm_unlink_ar
> free(path, M_TEMP);
>
> return (error);
> +}
> +
> +int
> +sys_shm_rename(struct thread *td, struct shm_rename_args *uap)
> +{
> + 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;
> +
> + /*
> + * Make sure the user passed only valid flags.
> + * If you add a new flag, please add a new term here.
> + */
> + if ((flags & ~(
> + SHM_RENAME_NOREPLACE |
> + SHM_RENAME_EXCHANGE
> + )) != 0) {
> + error = EINVAL;
> + goto out;
> + }
> +
> + /*
> + * EXCHANGE and NOREPLACE don't quite make sense together. Let's
> + * force the user to choose one or the other.
> + */
> + if ((flags & SHM_RENAME_NOREPLACE) != 0 &&
> + (flags & SHM_RENAME_EXCHANGE) != 0) {
> + error = EINVAL;
> + goto out;
> + }
> +
> + /*
> + * Malloc zone M_SHMFD, since this path may end up freed later from
> + * M_SHMFD if we end up doing an insert.
> + */
I do not quite get the comment.
> + path_from = malloc(MAXPATHLEN, M_SHMFD, M_WAITOK);
> + error = copyinstr(uap->path_from, path_from, MAXPATHLEN, NULL);
> + if (error)
> + goto out;
> +
> + 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;
Is this needed for correctness, or just a micro-optimization ?
We require that all shm names started with '/'. I do not see a check
for that in your code, so it seems that you allow to create such entries.
Also look at the special handling for jailed callers in kern_shm_open().
> +
> + 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;
I think you can put an out_locked label just before final unlock
and jump to it instead of repeating sx_xunlock().
> + }
> +
> + fd_to = shm_lookup(path_to, fnv_to);
You added truss support, but not ktrace. I think it would be useful
to report looked up names, see the use of KTR_NAMEI tracepoints in
kern_shm_open().
> + 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",
> + 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
This is weird. Why did you put KASSERT under some custom define protection ?
> + 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;
I do not think this assignment does anything useful.
> + sx_xunlock(&shm_dict_lock);
> +
> +out:
> + if (path_from != NULL)
You do not need the checks, calls free() unconditionally.
> + free(path_from, M_SHMFD);
> + if (path_to != NULL)
> + free(path_to, M_SHMFD);
> + return(error);
There should be space before '('.
> }
>
> int
>
> Modified: head/sys/sys/mman.h
> ==============================================================================
> --- head/sys/sys/mman.h Thu Sep 26 15:18:57 2019 (r352746)
> +++ head/sys/sys/mman.h Thu Sep 26 15:32:28 2019 (r352747)
> @@ -134,6 +134,14 @@
> #define MAP_FAILED ((void *)-1)
>
> /*
> + * Flags provided to shm_rename
> + */
> +/* Don't overwrite dest, if it exists */
> +#define SHM_RENAME_NOREPLACE (1 << 0)
> +/* Atomically swap src and dest */
> +#define SHM_RENAME_EXCHANGE (1 << 1)
You already got a note about namespace.
> +
> +/*
> * msync() flags
> */
> #define MS_SYNC 0x0000 /* msync synchronously */
> @@ -313,6 +321,7 @@ int posix_madvise(void *, size_t, int);
> int mlockall(int);
> int munlockall(void);
> int shm_open(const char *, int, mode_t);
> +int shm_rename(const char *, const char *, int);
> int shm_unlink(const char *);
> #endif
> #if __BSD_VISIBLE
_______________________________________________
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
