Update the crypto(4) and crypto(9) manpages.
ClosedPublic
Actions

Authored by jhb on Dec 12 2019, 11:02 PM.

Details

Reviewers

cem

Group Reviewers

manpages

Commits

rS355866: Update the crypto(4) and crypto(9) manpages.

Summary

There are probably bits that are still wrong, but this fixes some
things at least:

Add named arguments to the functions in crypto(9).
Add missing algorithms.
Don't mention arguments that don't exist in crypto_register.
Add CIOGSESSION2.
Remove CIOCNFSESSION.
Clarify some stale language that assumed an fd had only one sesson.
Note that you have to use CRIOGET and add a note in BUGS lamenting that one has to use CRIOGET.
Various other cleanups.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

jhb created this revision.Dec 12 2019, 11:02 PM

Herald added a reviewer: manpages. · View Herald TranscriptDec 12 2019, 11:02 PM

Herald added a subscriber: imp. · View Herald Transcript

Harbormaster completed remote builds in B28112: Diff 65567.Dec 12 2019, 11:02 PM

Thanks, this page is a mess and this is a good improvement. I've got some quibbles and nits below; some is subjective, feel free to reject proposals. Hopefully someone from manpages has some time to review this as well.

share/man/man4/crypto.4
96 ↗	(On Diff #65567)	nice.
116–119 ↗	(On Diff #65567)	I have two alternative suggestions. `.Xr open 2 \n the \n .Pa ... \n device.` (just make `.Xr open 2` the verb and remove the redundancy) or `Open ... device with \n .Xr open 2 .` (s/via/with/)
121 ↗	(On Diff #65567)	It's a little unclear what the relationship between session fds and sessions is, and I think the similar naming implies a relationship that doesn't exist. But I'm struggling to come up with a better name for the fd, and the ioctl name is already tied firmly to SESSION.
131 ↗	(On Diff #65567)	Also things like hashes and MACs, I think (which aren't symmetric crypto). The clearest description might be "if the cryptop ioctl of what you want to do includes a session identifier, create a session for it" but that isn't a great logical explanation.
148–150 ↗	(On Diff #65567)	It might be worth pointing out that this is unnecessary if you're closing the fd anyway. I'm not sure there's any real utility to end users of closing sessions at all (aside from preventing kernel memory leak until the process exits). Do we have a user resource limit on sessions?
206–207 ↗	(On Diff #65567)	Something slightly less obsolete for both examples while here? Also technically there should be a comma after `e.g.`.
214 ↗	(On Diff #65567)	C99 `uint32_t` while you're here?
263 ↗	(On Diff #65567)	session2_op
272 ↗	(On Diff #65567)	Ditto remarks re: earlier session_op
280 ↗	(On Diff #65567)	I think `s/via/with/` here as well.
281 ↗	(On Diff #65567)	I think we usually just say `.Fa crid .`. I'd also suggest documenting that `pad` must be zero. Otherwise it doesn't serve a lot of purpose as a future expansion option.
454–456 ↗	(On Diff #65567)	This sentence is a bit awkward. Maybe just "CRIOGET should not exist."?
share/man/man9/crypto.9
166 ↗	(On Diff #65567)	by -> be
250–251 ↗	(On Diff #65567)	Maybe: "For variable-size key algorithms, the length of the key, in bits."
253 ↗	(On Diff #65567)	Maybe "If non-zero, truncate the calculated hash to this many bytes."
256 ↗	(On Diff #65567)	s/ with the algorithm//
258 ↗	(On Diff #65567)	I'd drop "(IV)" given the field has "iv" in the name and if someone doesn't know that initialization vector and IV are synonymous, they're unlikely to be reading this manual page at all.
268 ↗	(On Diff #65567)	suggest: `s/multi-algorithm/dual-algorithm/`, since that's all drivers support in practice and we're trying to ditch the arbitrary linked list model soon. Also, comma before "such as."
269 ↗	(On Diff #65567)	Maybe just "a MAC."
274–279 ↗	(On Diff #65567)	Maybe "... will not be modified or referenced by the framework ...". (I'd keep the clarifying 2nd sentence even with that change, though.
307 ↗	(On Diff #65567)	wat
369 ↗	(On Diff #65567)	In general, sized buffers are expected to be contiguous (in the virtual memory context that this use means) in C. So I'd just drop that qualifying word. I tend to prefer leading with the conditionals, so I'd suggest "Unless ... IMBUF or ... IOV is specified in crp_flags, a pointer to the data."
375–377 ↗	(On Diff #65567)	Missing a word ("... is normally set crp_ilen bytes"), and missing a period at the end of the sentence.
468–470 ↗	(On Diff #65567)	The parentheses bit is awkward. I'd pull it out as its own sentence and rephrase a bit: "Two examples of this use are ipsec, in some configurations, and encrypted swap." If the latter means GELI, I'd just replace it with GELI. If it doesn't mean GELI, what does it mean?

jhb marked 12 inline comments as done.Dec 16 2019, 6:32 PM

jhb added inline comments.

share/man/man4/crypto.4
121 ↗	(On Diff #65567)	So you can have multiple sessions on a single fd, but it has to be an fd created via CRIOGET. I wanted to have some way to differentiate a /dev/crypto fd from a CRIOGET fd. Long term I'd like to kill CRIOGET, but until then, I was trying to use "session fd" to mean "fd returned from CRIOGET" vs /dev/crypto fd. Not sure of a better way to name these. Perhaps "cryptography fd". That's kind of what procstat(8) calls these.
131 ↗	(On Diff #65567)	I tried to borrow from the language in the para at line 105, but added digests explicitly.
148–150 ↗	(On Diff #65567)	We do not.
206–207 ↗	(On Diff #65567)	The comment is straight from cryptodev.h. I think in my old trees I need to rebase that remove these algorithms I do change them to something else, so I can do that here when I do it to the header.
214 ↗	(On Diff #65567)	So, I might do a pass over the crypto header to do that at some point, but for now I think it's best to match what the header uses.
280 ↗	(On Diff #65567)	With also reads a bit odd, so I've reworked the sentence.
281 ↗	(On Diff #65567)	I chose to the 'sesp->crid' to match the rest of the manpage which uses that style. Documenting pad is a good idea.
share/man/man9/crypto.9
307 ↗	(On Diff #65567)	sigh
369 ↗	(On Diff #65567)	For a definition list I like to start with the root definition first and then append qualifiers. I do think "unless" is a better wording than "when neither" though.
468–470 ↗	(On Diff #65567)	I think this sentence predates geli? I'm not sure, I might just axe it. I plan to rewrite this entire section for the refactor anyway.