Re: [PATCH 1/2] random: convert to using fops->read_iter()

[Jason A. Donenfeld]

On Fri, May 20, 2022 at 1:21 AM Jens Axboe <axboe@xxxxxxxxx> wrote:
>
> 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?

Yes please, thanks.

Jason