Date: Thu, 26 Sep 2019 20:53:42 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: David Bright <dab@FreeBSD.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r352747 - in head: lib/libc/sys sys/compat/freebsd32 sys/kern sys/sys tests/sys/posixshm usr.bin/truss Message-ID: <20190926175342.GI44691@kib.kiev.ua> In-Reply-To: <201909261532.x8QFWSDH049691@repo.freebsd.org> References: <201909261532.x8QFWSDH049691@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20190926175342.GI44691>