svn commit: r352747 - in head: lib/libc/sys sys/compat/freebsd32 sys/kern sys/sys tests/sys/posixshm usr.bin/truss

[Konstantin Belousov]

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