Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

setns: Check the file descriptor type #1402

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rusty-snake
Copy link
Contributor

… before passing it to setns

TODO: testing

… before passing it to setns
@sunfishcode
Copy link
Member

Could you say more about why it should be rustix's responsibility to do these checks?

@rusty-snake
Copy link
Contributor Author

I'm coming a bit from the other direction. Why do we have two functions for setns (with somewhat unsightly names IMHO).
The only reason I can imagen is type-safety and readability. But if I can pass a pidfd to move_into_link_name_space, what's the point of two functions.

@sunfishcode
Copy link
Member

In general, rustix tries to avoid making multiple syscalls per function, to be less surprising to users who know the syscalls, and to avoid the extra overhead for users that don't need it.

I agree it's a little awkward that there are two functions here. Perhaps we should add a setns function, which takes a bitflags argument. If you pass it multiple flags with a /proc/pid/ns/ link, Linux returns Errno::INVAL, and we can just document that.

@rusty-snake
Copy link
Contributor Author

Ok, I broaden this discussion a bit more. I also plan to add ioctl_ns_get_userns, ioctl_ns_get_parent, ioctl_ns_get_nstype, ioctl_ns_get_owner_uid functions.¹ There I need a type for CLONE_NEW* too.
Right now rustix has three types for CLONE_NEW*:

  • UnshareFlags, which has more CLONE_* than just CLONE_NEW*.
  • ThreadNameSpaceType bitflags struct, consumed by move_into_thread_name_spaces.
  • LinkNameSpaceType exhaustive enum, consumed by move_into_link_name_space.

Unifying the later two into a new NamespaceType which can the be used by

  • setns(pidfd, NamespaceType), takes already an bitflags type.
  • setns(nsfs, NamespaceType), fails with EINVAL on more than one bit.
  • ioctl_ns_get_nstype, return just one bit.

¹ Where should the be places?

  1. fs because the operate on file-descriptors.
  2. io because they are ioctls.
  3. thread because unshare/setns sit there.
  4. process because ?.
  5. namespaces/ns/... because the are worth a new top-level module.

@sunfishcode
Copy link
Member

¹ Where should the be places?

1. `fs` because the operate on file-descriptors.

fs is for things that work with actual files and directories.

2. `io` because they are `ioctl`s.

io is enabled by default so we ideally want to keep it minimal.

3. `thread` because `unshare`/`setns` sit there.

This could work.

4. `process` because ?.

I agree, it doesn't really fit in process.

5. `namespaces`/`ns`/... because the are worth a new top-level module.

This could work too. Are there any other functions that would belong in an ns module?

@rusty-snake
Copy link
Contributor Author

This could work too. Are there any other functions that would belong in an ns module?

Linux namespaces API:

  • clone/clone3 -> not_implemented::libc_internals
  • unshare
  • setns
  • ioctl_nsfs
  • ---
  • CLONE_NEW*
  • ---
  • regular filesystem calls on nsfs-files (namely readlinkat and stat*), nothing special for rustix IMHO
  • regular filesystem operations on various /proc/PTID/* files

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants