Page MenuHomeFreeBSD

Fix excessive reseeding in random(4).
AbandonedPublic

Authored by markm on Aug 23 2018, 1:16 PM.

Details

Reviewers
cem
delphij
Group Reviewers
secteam
Summary

Fix braino of mine where the reseeds would happen far too often, making the kernel process way too busy.

PR: 230808
Submitted by: Conrad Meyer <cem@FreeBSD.org>
Reported by: Danilo Egea Gondolfo <danilo@FreeBSD.org>

Test Plan

Make the PRNG work hard while doing something intensive like "make
world". To make the PRNG work hard, do e.g.
$ dd if=/dev/random of=/dev/null bs=$((1024*1024))

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 19093
Build 18721: arc lint + arc unit

Event Timeline

markm created this revision.Aug 23 2018, 1:16 PM
markm updated this revision to Diff 47165.Aug 23 2018, 1:21 PM

Add the differential revision to the commit message.

jmg added a subscriber: jmg.Aug 23 2018, 4:16 PM

Please commit these patches separately. They deal w/ different issues.

sys/dev/random/random_harvestq.c
222

this still results in excessive seeding. It caps the read rate to pools*8*8, or putting 64 bytes per pool in for every 4 bytes read. As most things will read a minimum of 16 bytes (128 bits), the smallest expected value for read_rate is 5 (the increment isn't a proper ceil function), or 40 bytes per pool for every read. With the number of pools, that's a lot of extra work still.

markm updated this revision to Diff 47178.Aug 23 2018, 4:27 PM

Split into two separate commits as per review request.

markm retitled this revision from Fix performance-damaging issues in random(4) to Fix excessive reseeding in random(4)..Aug 23 2018, 4:32 PM
markm edited the summary of this revision. (Show Details)
delphij accepted this revision.Aug 23 2018, 4:38 PM
delphij added a subscriber: delphij.

LGTM. cem@ or markm@, could you please commit this before code freeze?

This revision is now accepted and ready to land.Aug 23 2018, 4:38 PM
markm abandoned this revision.Aug 23 2018, 4:39 PM
cem added a comment.Aug 23 2018, 4:40 PM

(Approved — phab won't let me mark it as such because it is "closed.")

@jmg , @markm : Thanks, I also prefer the two independent pieces split into separate patches.

This one looks obviously correct to me. Go ahead and commit it. It fixes a security weakness in our implementation relative to spec Fortuna, so I'd be sure to tag the commit with "Security: yes."

cem added a comment.Aug 23 2018, 4:43 PM

Split into two separate commits as per review request.

Where did the other review end up? Thanks.