Page MenuHomeFreeBSD

man9: Add ioctl implementation guide
Needs ReviewPublic

Authored by jfree on Jul 29 2024, 11:28 PM.
Referenced Files
F93099744: D46181.id141556.diff
Sat, Sep 7, 8:20 AM
Unknown Object (File)
Mon, Sep 2, 4:09 PM
Unknown Object (File)
Aug 8 2024, 10:44 AM
Unknown Object (File)
Aug 4 2024, 12:22 PM
Unknown Object (File)
Jul 31 2024, 5:49 PM
Unknown Object (File)
Jul 31 2024, 5:24 AM
Unknown Object (File)
Jul 30 2024, 5:06 PM
Unknown Object (File)
Jul 30 2024, 2:28 AM
Subscribers

Details

Reviewers
imp
markj
Summary

NetBSD added an ioctl implementation manual page back in 1999, but it
was never brought over to FreeBSD. Import NetBSD's page and make a few
minor changes for FreeBSD.

Sponsored by: NIKSUN, Inc.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 58882
Build 55769: arc lint + arc unit

Event Timeline

jfree created this revision.

The ioctl subsystem section was not modified from NetBSD's manual page, so it may be inaccurate for FreeBSD. Any help adding FreeBSD-specific subsystem character codes would be great.

markj added inline comments.
share/man/man9/ioctl.9
79

This list is rather outdated, as is the notion that each subsystem has a unique number. Note also that ioctls are routed based on the descriptor type (i.e., by having a fo_ioctl implementation), so the descriptor type also implicitly selects the supported ioctls.

I would limit this list to 'f' (used for FIONBIO and other common ioctls implemented in kern_ioctl() and vn_ioctl()) and perhaps 's' for sockets. The rest is not worth documenting IMO.

229

As I tried to express above, this terminology about subsystems is a bit vague. We should describe fo_ioctl somehow in this man page.

240

It would be nice to enumerate best practices for ioctl interfaces. I don't have a clear set of rules in mind, but things like, use uintptr_t for userspace pointers instead of a fixed-width type; be sure to zero out structures that are copied back to userspace to avoid kernel memory disclosures; if possible, avoid complex structures which require multiple copyin()s, and if necessary use nvlists instead.

Maybe @brooks would have some commentary about this based on experiences from CHERI.

260

These sentences seem confusing to me. I'd just point out that it's standard to return ENOTTY for unrecognized ioctls.

301

Maybe link cap_ioctls_limit(2) and cap_ioctls_get(2).

Maybe also link nv(9), since nvlists are useful for creating extensible interfaces for userspace. In the same vein it might be useful to link to netlink(4), which provides a more structured interface for passing data between userspace and the kernel.

Agree with most of what markj has said. The best practices for ioctl has definitely evolved since 1999...
The 'subsystem' mostly is to allow devices to handle multiple 'classes' of ioctls, but even that's somewhat not used.
+1 for nvlist. Anything more than a few items in a struct is about the cutoff for it.

(Please forgive my perhaps snarky comments in places. I'm not a huge fan of the style of the original document.)

The automatic copying in/zeroing and copying out performed in sys_ioctl needs to be documented. It's one of the things that most commonly trips people up with ioctl. @def recently fixed some bugs due to wrong types that are partially due to this code. There are also some weird edge cases in the implementation that probably should be discussed somewhere (e.g. size=0 and IOV_VOID means the third arguments is an integer, not a pointer).

Some assorted off-the-cuff advice:

  • Never use long in structs unless a standard mandates it
  • Do use fixed width types for integer values,
  • Try to avoid the pattern (common in Linux) of making pointers u64's. It simplifies 32/64-bit compat, but breaks with 128-bit pointer schemes (CHERI) and removes types.
  • For in/out requests, consider updating selectively and avoid updating userspace pointers (your interface is very, very weird if you need to change a userspace pointer so updating just introduces the opportunity for corruption)
  • Don't output kernel pointers except as addresses (use kvaddr_t)
  • Be very cautious of unions. If you must, make sure to add static asserts so they don't change size.
  • If things will need per-ABI compat, it's convenient if the type changes size, otherwise you have to due process ABI checks in the implementation. If the size differs and you can use _IOC_NEWTYPE to declare another case where you handle the difference.
share/man/man9/ioctl.9
36

It would be nice to pick a single name for the integer value declared by these macros and stick to it. the ioctl(2) manpage uses "request" as does wikipedia's rather random article. Actual implementations usually use com seemingly for command and Linux's manpage uses op.

40

Maybe give the variables more descriptive names?

42

I'd like to see _IOC_NEWTYPE documented.

48
50

These are macros not functions so it seems weird to say func here.

75–77

That's kind of an important typo...

79

Thinking about them in terms of "subsystems" isn't entirely wrong, but it's a bit weird. It's all down to what ever is at the bottom of a given file descriptor.

I'd definitely prune this list quite hard and focus on common things.

234

More accurately there can be only one (n, sizeof(t)) tuple for each group on a given descriptor, but good practice suggests making (g, n) unique.

236

following the declarations above.

238–240

This text is very informal. It might benefit from some more examples to make it more concrete.

250–254

This is just how a syscall works unless it's weird.