Add an shm_rename syscall

[dab]

Summary

Add an atomic shm rename operation, similar in spirit to a file rename. Atomically unlink an shm from a source path and link it to a destination path. If an existing shm is linked at the destination path, unlink it as part of the same atomic operation. The caller needs the same permissions as shm_unlink to the shm being renamed, and the same permissions for the shm at the destination which is being unlinked, if it exists. If those fail, EACCES is returned, as with the other shm_* syscalls.

I've included truss support, but not audit support yet. I have an email from Robert Watson about how to do that. I don't want to propose OpenBSM changes until I get some agreement that this syscall is even sane, which I'm doing with this review. If it seems fine, I'll do the audit bits in a subsequent review.

I've included the sysent-generated bits here for completeness, but will commit them as a separate rev, as recommended: https://wiki.freebsd.org/AddingSyscalls

Test Plan

Extend posixshm kyua tests to include the new syscall. This is done; see diff.

Manually tested truss.

[jilles]

This seems to fit the model.

lib/libc/sys/shm_open.2

46 ↗

(On Diff #61321)

I recommend adding an int flags here (for example, compare Linux's renameat2()). Since no flags are defined yet, any non-zero flags argument should cause the call to return error [EINVAL].

300 ↗

(On Diff #61321)

.Fa path_from

sys/kern/uipc_shm.c

691 ↗

(On Diff #61321)

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.

950–952 ↗

(On Diff #61321)

This 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.

[matthew.bryan_isilon.com]

lib/libc/sys/shm_open.2

46 ↗

(On Diff #61321)

Good idea! I'm under crunch time, so I won't do this right now, but it is absolutely worth a TODO in the code to remember the idea. Marking 'done', having added the TODO. Please let me know if you think this is crucial enough to attack now.

sys/kern/uipc_shm.c

691 ↗

(On Diff #61321)

It also avoids some cleanup like mtx_destroy that would invalidate the object's state. So: avoiding the shm_drop is best.

950–952 ↗

(On Diff #61321)

Sounds right. I'll clean up the comment.

[matthew.bryan_isilon.com]

Updates for CR feedback:

• Update man page
• Comment changes in uipc_shm.c

[matthew.bryan_isilon.com]

sys/kern/uipc_shm.c

691 ↗

(On Diff #61321)

Derp; let's just shm_hold before shm_remove to prevent any invalidation. Fixed.

[matthew.bryan_isilon.com]

Revert the shm_remove change; let's just take an extra ref instead. Makes an error path in rename more complex, but that is fine.

[jilles]

lib/libc/sys/shm_open.2

46 ↗

(On Diff #61321)

Adding this later is an incompatible ABI and API change, so it should be done soon and definitely before MFC or heavier use.

[matthew.bryan_isilon.com]

Add flags for alternatives to overwriting an existing sysctl.

[dab]

Looks good, Matthew; just a couple nits. It would be nice if you could upload a full-context diff with any further changes. When you think it is ready, I'll be happy to carry this through to commit.

lib/libc/sys/shm_open.2

125 ↗

(On Diff #61470)

The period (.) following shm_unlink should be replaced by a comma (,). Alternatively, just delete the period; a comma really isn't needed between the two clauses in this sentence.

310 ↗

(On Diff #61470)

s/exceeded/exceeds/

[matthew.bryan_isilon.com]

Updated for David's man page comments.

[matthew.bryan_isilon.com]

In D21423#467509, @dab wrote:

Looks good, Matthew; just a couple nits. It would be nice if you could upload a full-context diff with any further changes. When you think it is ready, I'll be happy to carry this through to commit.

I'd like one more review since I added the new flags. I'm okay to wait for Eric, or whoever.

[jilles]

lib/libc/sys/shm_open.2

46 ↗

(On Diff #61321)

I actually asked for the flags mechanism only, without the actual flags like renameat2(). However, having it will be useful.

sys/kern/uipc_shm.c

902 ↗

(On Diff #61486)

Please add something like

if ((flags & ~(SHM_RENAME_NOREPLACE | SHM_RENAME_EXCHANGE)) != 0)
    return (EINVAL);

so new flags can be safely added later.

909–912 ↗

(On Diff #61486)

This comment now applies to both paths.

[matthew.bryan_isilon.com]

Updated with extra protections for flags, and an extra test.

[matthew.bryan_isilon.com]

Audit support will come in a future review, after this PR to OpenBSM: https://github.com/openbsm/openbsm/pull/54

[csjp]

sys/kern/uipc_shm.c

893 ↗

(On Diff #61509)

Are you planning on adding the instrumentation to audit the syscall arguments in a future revision?

[matthew.bryan_isilon.com]

sys/kern/uipc_shm.c

893 ↗

(On Diff #61509)

Looks like you found the OpenBSM PR :)
https://github.com/openbsm/openbsm/pull/54

Yes; my plan is:

1. Get some agreement on the syscall here on the FreeBSD side (done)
2. Get a commit into OpenBSM so I know exactly what the audit event num is going to be (waiting for it to merge).
3. Make a second review on the FreeBSD side for the audit changes.

[dab]

Someone snuck in another syscall. I'll update and commit this.

[dab]

Updated to account for another syscall that was recently added.

[dab]

Updated to account for another syscall that was recently added.

[brueffer]

lib/libc/sys/shm_open.2

163 ↗

(On Diff #61843)

Please use the .Fx macro instead of FreeBSD.

367 ↗

(On Diff #61843)

Please use the .Fx macro instead of FreeBSD.

[dab]

Made requested man page changes.