Page MenuHomeFreeBSD

Add an shm_rename syscall

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



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:

Test Plan

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

Manually tested truss.

Diff Detail

rS FreeBSD src repository - subversion
Lint Not Applicable
Tests Not Applicable

Event Timeline

This seems to fit the model.

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

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. added inline comments.
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.

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. marked 2 inline comments as done.

Updates for CR feedback:

  • Update man page
  • Comment changes in uipc_shm.c added inline comments.
691 ↗(On Diff #61321)

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.

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

Add flags for alternatives to overwriting an existing sysctl.

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.

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)


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.

46 ↗(On Diff #61321)

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

902 ↗(On Diff #61486)

Please add something like

    return (EINVAL);

so new flags can be safely added later.

909–912 ↗(On Diff #61486)

This comment now applies to both paths. marked 2 inline comments as done.

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

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:

csjp added inline comments.
893 ↗(On Diff #61509)

Are you planning on adding the instrumentation to audit the syscall arguments in a future revision? added inline comments.
893 ↗(On Diff #61509)

Looks like you found the OpenBSM PR :)

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 edited reviewers, added:; removed: dab.

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

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

Updated to account for another syscall that was recently added.

brueffer added a subscriber: brueffer.
brueffer added inline comments.
163 ↗(On Diff #61843)

Please use the .Fx macro instead of FreeBSD.

367 ↗(On Diff #61843)

Please use the .Fx macro instead of FreeBSD.

This revision now requires changes to proceed.Sep 19 2019, 8:52 AM

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.