Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <8251e0d7-d506-16ea-7bc8-4c8bf098deea@kernel.dk>
Date:   Thu, 19 May 2022 17:21:32 -0600
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux aarch64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.1
Subject: Re: [PATCH 1/2] random: convert to using fops->read_iter()
Content-Language: en-US
To:     "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc:     tytso@mit.edu, hch@lst.de, linux-kernel@vger.kernel.org
References: <20220519193133.194138-1-axboe@kernel.dk>
 <20220519193133.194138-2-axboe@kernel.dk> <YobOxLKnRDyYChXS@zx2c4.com>
 <YobQt1Fbae0KtEFw@zx2c4.com>
From:   Jens Axboe <axboe@kernel.dk>
In-Reply-To: <YobQt1Fbae0KtEFw@zx2c4.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 5/19/22 5:20 PM, Jason A. Donenfeld wrote:
> On Fri, May 20, 2022 at 01:12:04AM +0200, Jason A. Donenfeld wrote:
>> Hi Jens,
>>
>> On Thu, May 19, 2022 at 01:31:32PM -0600, Jens Axboe wrote:
>>>  	for (;;) {
>>>  		chacha20_block(chacha_state, output);
>>>  		if (unlikely(chacha_state[12] == 0))
>>>  			++chacha_state[13];
>>>  
>>>  		block_len = min_t(size_t, len, CHACHA_BLOCK_SIZE);
>>> -		left = copy_to_user(ubuf, output, block_len);
>>> -		if (left) {
>>> -			ret += block_len - left;
>>> +		block_len = copy_to_iter(output, block_len, to);
>>> +		if (!block_len)
>>>  			break;
>>> -		}
>>>  
>>> -		ubuf += block_len;
>>>  		ret += block_len;
>>>  		len -= block_len;
>>> -		if (!len)
>>> -			break;
>>>  
>>>  		BUILD_BUG_ON(PAGE_SIZE % CHACHA_BLOCK_SIZE != 0);
>>>  		if (ret % PAGE_SIZE == 0) {
>>>  			if (signal_pending(current))
>>>  				break;
>>>  			cond_resched();
>>>  		}
>>>  	}
>>
>> This isn't quite the same, is it? Before, it would immediately break out
>> of the loop on any short copy. Now, it will only break out on a zero
>> copy, which means it's possible that ret % PAGE_SIZE == 0, and there'll
>> be an unnecessary cond_resched() before copy_to_iter() runs again and
>> then breaks.
> 
> Maybe something like the below would do the trick?
> 
> 
> static ssize_t get_random_bytes_user(struct iov_iter *to)
> {
> 	size_t block_len, copied, ret = 0, len = iov_iter_count(to);
> 	u32 chacha_state[CHACHA_STATE_WORDS];
> 	u8 output[CHACHA_BLOCK_SIZE];
> 
> 	if (!len)
> 		return 0;
> 
> 	/*
> 	 * Immediately overwrite the ChaCha key at index 4 with random
> 	 * bytes, in case userspace causes copy_to_user() below to sleep
> 	 * forever, so that we still retain forward secrecy in that case.
> 	 */
> 	crng_make_state(chacha_state, (u8 *)&chacha_state[4], CHACHA_KEY_SIZE);
> 	/*
> 	 * However, if we're doing a read of len <= 32, we don't need to
> 	 * use chacha_state after, so we can simply return those bytes to
> 	 * the user directly.
> 	 */
> 	if (len <= CHACHA_KEY_SIZE) {
> 		ret = copy_to_iter(&chacha_state[4], len, to);
> 		goto out_zero_chacha;
> 	}
> 
> 	for (;;) {
> 		chacha20_block(chacha_state, output);
> 		if (unlikely(chacha_state[12] == 0))
> 			++chacha_state[13];
> 
> 		block_len = min_t(size_t, len, CHACHA_BLOCK_SIZE);
> 		copied = copy_to_iter(output, block_len, to);
> 		ret += copied;
> 		if (block_len != copied)
> 			break;
> 		len -= copied;

Yep, that looks good! Do you still want a v2?

-- 
Jens Axboe