Implement the CloudABI random_get() system call.

[ed]

Summary

The random_get() system call works similar to getentropy()/getrandom() on OpenBSD/Linux. It fills a buffer with random data.

This change introduces a new function, read_random_uio(), that is used to implement read() on the random devices. We can call into this function from within the CloudABI compatibility layer.

[markm]

Please always supply complete files - it means reviewers get to look in one place only to see all related code - thanks!

Looks good. SO@ may need to OK it (Add delphij as reviewer).

sys/compat/cloudabi/cloudabi_random.c

36 ↗

(On Diff #6859)

At what sort of rate do you anticipate this being called? If the answer is "high", then I'd like to work with you on potentially seeking some optimisations.

sys/dev/random/randomdev.c

142 ↗

(On Diff #6859)

This may need an entry in random(9)? I intend to produce FreeBSD getentropy()/getrandom() syscalls at some point soonish, and this is a useful step!

[ed]

Add manual page entry for random_read_uio(9).

[ed]

In D3053#60262, @markm wrote:

Please always supply complete files - it means reviewers get to look in one place only to see all related code - thanks!

Sorry about that. Turns out that's a disadvantage of using Phabricator without Arcanist. Just installed Arcanist, so all should be good now.

Looks good. SO@ may need to OK it (Add delphij as reviewer).

Thanks!

[ed]

sys/compat/cloudabi/cloudabi_random.c

36 ↗

(On Diff #6871)

Userspace programs never call into this system call directly. It is only called by the arc4random() library function, so performance should be good enough (for now).

sys/dev/random/randomdev.c

142 ↗

(On Diff #6871)

Done! Be sure to let me know whether you think the phrasing is all right.

[markm]

Almost there!

share/man/man9/random.9

125 ↗

(On Diff #6871)

"identically"

132 ↗

(On Diff #6871)

"will block"

Please break the sentence at the commas.

(For long lines break them into convenient
chunks anyway as this makes any later diffs smaller).

sys/dev/random/randomdev.c

142 ↗

(On Diff #6871)

Phrasing is very nearly there, thanks!

[wblock]

share/man/man9/random.9

132 ↗

(On Diff #6871)

When possible, try to avoid if/then sentences which have a pause in the middle. For example, this can be rewritten:

This function will block unless the
.Fa flags
argument is equal to
.Dv O_NONBLOCK .

(Should it be "is equal to"? Or should it be "is set to"?)

140 ↗

(On Diff #6871)

Add a serial comma (https://www.freebsd.org/doc/en_US.ISO8859-1/books/fdp-primer/writing-style-guidelines.html)

.Fn read_random ,

190 ↗

(On Diff #6871)

This can be simpler and avoid the if/then/pause. "The blahblah function" is redundant, so just:

.Fn read_random
returns zero when successful.

Or it can include the error:

.Fn read_random
returns zero when successful, or an
error number on failure.

("error number" sounds wrong here. Maybe it should refer elsewhere (like .Xr errno) to be specific about what is meant.)

[markm]

With wblock's changes and my line break requests I'm ready to accept this.

[ed]

Process feedback on the manpage.

[ed]

share/man/man9/random.9

132 ↗

(On Diff #6871)

What do you think of this?

190 ↗

(On Diff #6871)

Done! I looked at some other man pages (e.g., VOP_READ(9)) and it looks like they use "otherwise an error code is returned." Decided to use that instead.

[wblock]

share/man/man9/random.9

132 ↗

(On Diff #6871)

Negative logic is kind of confusing. Maybe reverse it? (And remove "the ... argument"):

This function only proceeds if the random device is seeded or
.Fa flags
is set to
.Dv O_NONBLOCK .
Otherwise, it blocks.

(Um, not great, but just an example.)

[ed]

Even more wordsmithing.

[ed]

share/man/man9/random.9

132 ↗

(On Diff #6879)

What about this version?

[jmg]

Please make it clear how the function behaves. Too much is unspecified.

share/man/man9/random.9

131 ↗

(On Diff #6881)

will be stored.

136 ↗

(On Diff #6881)

Is the correct logic that the function will return a short read if the O_NONBLOCK?

A different name for the flag O_NONBLOCK should be used since this function doesn't deal w/ files.

Is it an error when O_NONBLOCK is set and it's not seeded? or do you get zero return in that case too?

194 ↗

(On Diff #6881)

What error codes are allowed to be returned?

[wblock]

share/man/man9/random.9

136 ↗

(On Diff #6881)

Maybe:

.Nm
blocks when the random device is unseeded.
To avoid blocking, set
.Dv O_NONBLOCK
in
.Fa flags ,
and
.Nm
will return (whatever it returns, an error or a short read or pi to 38 places, or 18 random digits from e) instead of blocking.

[ed]

Use a boolean argument instead of passing flags. Also improve the man page.

share/man/man9/random.9

136 ↗

(On Diff #6881)

This should be solved by adding the ERRORS section.

I've also replaced the flags argument by a boolean option.

194 ↗

(On Diff #6881)

Added an ERRORS section to the man page.

[ed]

Use false instead of 0, as the argument is boolean.

[jmg]

Other than the set/true change, the changes to the manpage are great.

Thanks.

share/man/man9/random.9

136 ↗

(On Diff #6913)

as nonblock is now a bool, not a flag, instead of saying "is set", say "is true".

[markm]

I'm happy.

[ed]

Hi Mark,

In D3053#60867, @markm wrote:

I'm happy.

Is this also an approval on behalf of secteam@? Looks like I need that in order to commit it. If not, who would be the best person to pull in?

[delphij]

The /sys/dev/random portion looks fine. You don't need our permission for the cloudabi portion of change.

One thing to note: getentropy() in OpenBSD doesn't block nor error out but instead returns some arc4random bytes. I'm not yet convinced which one in the several possible approaches would be better as application developers might forget to test results, etc., perhaps we should have some mechanisms to notify the user that this happened?

[markm]

In D3053#60878, @ed wrote:

Hi Mark,

In D3053#60867, @markm wrote:

I'm happy.

Is this also an approval on behalf of secteam@? Looks like I need that in order to commit it. If not, who would be the best person to pull in?

Hi Ed

I (possibly wrongly, I'm waiting for a LART) added myself to secteam@ on Phabricator. This doesn't imply that I have any authority, but puts me in a position where I can see security-related things happening.

Cheers,

M

[wblock]

This looks good, but there is a double "the" in the man page on line 132. textproc/igor will find a lot of these types of errors: igor -R random.9 | less -RS. It's also good to run changes through mandoc -tlint random.9. Thanks!

[eadler]

In D3053#60898, @markm wrote:

I (possibly wrongly, I'm waiting for a LART) added myself to secteam@ on Phabricator. This doesn't imply that I have any authority, but puts me in a position where I can see security-related things happening.

a bit off topic of the review but note that security is not the same as secteam. The former is an umbrella group for people interested in security. The latter is a team that has a hat. :)

[markm]

In D3053#61030, @eadler wrote:

In D3053#60898, @markm wrote:

I (possibly wrongly, I'm waiting for a LART) added myself to secteam@ on Phabricator. This doesn't imply that I have any authority, but puts me in a position where I can see security-related things happening.

a bit off topic of the review but note that security is not the same as secteam. The former is an umbrella group for people interested in security. The latter is a team that has a hat. :)

Duly noted (including my mistake - I meant security not secteam), thanks!