From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751235AbbJKEft (ORCPT ); Sun, 11 Oct 2015 00:35:49 -0400 Received: from ns.horizon.com ([71.41.210.147]:36806 "HELO ns.horizon.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1750712AbbJKEfs (ORCPT ); Sun, 11 Oct 2015 00:35:48 -0400 Date: 11 Oct 2015 00:35:46 -0400 Message-ID: <20151011043546.19633.qmail@ns.horizon.com> From: "George Spelvin" To: linux@horizon.com, tytso@mit.edu Subject: Re: Updated scalable urandom patchkit Cc: ahferroin7@gmail.com, andi@firstfloor.org, jepler@unpythonic.net, linux-kernel@vger.kernel.org, linux@rasmusvillemoes.dk In-Reply-To: <20151011023146.GA5341@thunk.org> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Damn, I bow before the master. That is a much neater solution than mine; I had assumed a lock was required for writing. While it's good enough for benchmarking, there are a few leftover problems I mention so they don't get missed. One is the final write back of add_ptr on the last line of _mix_pool_bytes. It actually writes i, which includes the per-cpu nonce, and will have it jumping all over without the steady progression that the mixing polynomial assumes. (There's a similar, lesser problem with input_rotate.) The second, less obvious, problem is that by calling _mix_pool_bytes completely lockless, there's the risk that it will race with and overwrite the addition of new seed material to the pool. The add-back is not critical, and races between two writers don't really do any harm. But seed entropy is valuable. And unfortunately, transferring 256 bits (32 bytes) to the output pool will try to write every word, so *any* concurrent add-back is risky; there's no "safe" part of the pool that can be accessed lockless. (The first crude hack that comes to mind is to double the size of the output pool, without increasing its nominal capacity, and do seeding and add-back to different halves. Hopefully there's something more elegant.) Two minor suggestions about is_nonblock: 1) Rather than using (r == &nonblocking_pool), how about !r->limit? 2) Would you mind making is_nonblock bool? I know back before the sacred text of the prophets Kernighan and Ritchie was corrupted by modern heresies, we used "int" for everything, and we liked it. 5 miles in the snow, uphill both ways, yadda yadda. But I like to document range restrictions as much as possible, and "bool" makes it clearer to both the compiler and the reader of the code.