Page MenuHomeFreeBSD

Add an shm_rename syscall
ClosedPublic

Authored by dab on Aug 26 2019, 8:58 PM.

Details

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.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 26360
Build 24822: arc lint + arc unit

Event Timeline

jilles added a subscriber: jilles.Aug 27 2019, 12:56 PM

This seems to fit the model.

lib/libc/sys/shm_open.2
46

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

.Fa path_from

sys/kern/uipc_shm.c
691

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

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 marked 3 inline comments as done.Aug 28 2019, 6:23 PM
matthew.bryan_isilon.com added inline comments.
lib/libc/sys/shm_open.2
46

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

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

950–952

Sounds right. I'll clean up the comment.

matthew.bryan_isilon.com marked 2 inline comments as done.

Updates for CR feedback:

  • Update man page
  • Comment changes in uipc_shm.c
matthew.bryan_isilon.com marked an inline comment as done.Aug 28 2019, 8:04 PM
matthew.bryan_isilon.com added inline comments.
sys/kern/uipc_shm.c
691

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

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 added inline comments.Aug 28 2019, 8:29 PM
lib/libc/sys/shm_open.2
46

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 edited the test plan for this revision. (Show Details)

Add flags for alternatives to overwriting an existing sysctl.

dab added a comment.Aug 30 2019, 4:46 PM

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

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

s/exceeded/exceeds/

Updated for David's man page comments.

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 added inline comments.Aug 31 2019, 4:05 PM
lib/libc/sys/shm_open.2
46

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
901

Please add something like

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

so new flags can be safely added later.

908–911

This comment now applies to both paths.

matthew.bryan_isilon.com marked 2 inline comments as done.

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

jilles accepted this revision.Sep 1 2019, 10:01 PM
This revision is now accepted and ready to land.Sep 1 2019, 10:01 PM

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

csjp added a subscriber: csjp.Sep 3 2019, 11:31 PM
csjp added inline comments.
sys/kern/uipc_shm.c
892

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

matthew.bryan_isilon.com marked an inline comment as done.Sep 4 2019, 1:47 AM
matthew.bryan_isilon.com added inline comments.
sys/kern/uipc_shm.c
892

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 commandeered this revision.Sep 6 2019, 3:57 PM
dab edited reviewers, added: matthew.bryan_isilon.com; removed: dab.

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

dab updated this revision to Diff 61842.Sep 9 2019, 2:40 PM
dab marked an inline comment as done.
dab edited the summary of this revision. (Show Details)

Updated to account for another syscall that was recently added.

This revision now requires review to proceed.Sep 9 2019, 2:40 PM
dab updated this revision to Diff 61843.Sep 9 2019, 2:48 PM

Updated to account for another syscall that was recently added.

brueffer requested changes to this revision.Sep 19 2019, 8:52 AM
brueffer added a subscriber: brueffer.
brueffer added inline comments.
lib/libc/sys/shm_open.2
163

Please use the .Fx macro instead of FreeBSD.

367

Please use the .Fx macro instead of FreeBSD.

This revision now requires changes to proceed.Sep 19 2019, 8:52 AM
dab updated this revision to Diff 62470.Sep 23 2019, 3:09 PM

Made requested man page changes.

This revision was not accepted when it landed; it landed in state Needs Review.Sep 26 2019, 3:32 PM
Closed by commit rS352747: Add an shm_rename syscall (authored by dab). · Explain Why
This revision was automatically updated to reflect the committed changes.