Hi,
We recently had a failure on a kernel upgrade because splice no longer
works on random/urandom. This is due to:
6e2c7421f02 ("fs: don't allow splice read/write without explicit ops")
which already has more than two handful of Fixes registered to its
name...
Wire up read_iter handling and then hook up splice_read for both of
them as well.
--
Jens Axboe
On 5/19/22 6:00 PM, Jason A. Donenfeld wrote:
> Hi Jens,
>
> On Fri, May 20, 2022 at 1:57 AM Jens Axboe <[email protected]> wrote:
>>
>> On 5/19/22 5:27 PM, Jason A. Donenfeld wrote:
>>> Hi Jens,
>>>
>>> On Fri, May 20, 2022 at 1:25 AM Jens Axboe <[email protected]> wrote:
>>>> I'll leave that to you :-)
>>>
>>> Alright, I'll have a look. Which one do I want here? This series adds
>>> splice_read/splice_write. The new thing would be sendpage? Or
>>> copy_file_range? Or something else?
>>
>> For copying kernel memory? It's really the same thing, you just
>> initialize the iter as an ITER_KVEC instead. The nice thing about the
>> iov_iter iterator that it then hides that for you, call the same copy
>> in/out helpers for it.
>
> Err, maybe we're talking about different things? I was thinking about
> the plumbing to make splice/sendfile work on non-pipes.
Ah I see. sendfile() just uses splice() internally, so that'll work
(again) with my changes. splice(), by definition, either moves to and
from a pipe. Hence one of the fds must be a pipe. Does that answer the
question?
> get_random_bytes() itself doesn't need to become iovec'd, as that has
> no IO callers.
I was a little puzzled, but didn't look deeper and see if it would make
sense to do so.
--
Jens Axboe
On 5/19/22 7:00 PM, Jason A. Donenfeld wrote:
> Hi Jens,
>
> On Thu, May 19, 2022 at 06:56:12PM -0600, Jens Axboe wrote:
>> On 5/19/22 6:48 PM, Jason A. Donenfeld wrote:
>>> sendfile() returns -EINVAL even with your patches. Only splicing to pipes
>>> seems to work.
>>
>> Huh, that really should work. Are you trying to sendfile() to random? If
>> so, you need that last write_iter patch too, and add the splice_write as
>> I mentioned.
>
> No, I've only tried the read side so far. I made a little program:
>
> #include <sys/sendfile.h>
> #include <stdio.h>
>
> int main(int argc, char *argv[])
> {
> ssize_t s = sendfile(1, 0, NULL, 0xffff);
> fprintf(stderr, "ret: %zd\n", s);
> return 0;
> }
>
> Then I ran `./a.out < /dev/urandom > /dev/null`. Fails. OTOH, if I
> replace /dev/urandom with an ordinary file, it succeeds.
Yeah I reproduced the same, doing it the other way works fine. Curious
enough to see what it is, looking into it.
--
Jens Axboe
On 5/19/22 5:25 PM, Jason A. Donenfeld wrote:
> Hi Jens,
>
> On Fri, May 20, 2022 at 1:22 AM Jens Axboe <[email protected]> wrote:
>> I can certainly do the write side too. To fix this regression, I just
>> valued doing read_iter first and I'd hate to hold that up to do the
>> write side too. I'll do the write side later today, but let's keep them
>> separate.
>
> Excellent, thanks. I plan to queue these up all in a row.
Built and tested v2, just sent it out. Note that it deviates from your
proposal a bit since with that we lost the
if (!len)
break;
check, which is kind of important if you ever want to be done :-)
I'll do the write_iter side, but as mentioned, I'd prefer to keep it
separate from this patchset as this one fixes a real regression that we
need to get backported too.
--
Jens Axboe
On 5/19/22 7:00 PM, Jason A. Donenfeld wrote:
> Hi Jens,
>
> On Thu, May 19, 2022 at 06:56:12PM -0600, Jens Axboe wrote:
>> On 5/19/22 6:48 PM, Jason A. Donenfeld wrote:
>>> sendfile() returns -EINVAL even with your patches. Only splicing to pipes
>>> seems to work.
>>
>> Huh, that really should work. Are you trying to sendfile() to random? If
>> so, you need that last write_iter patch too, and add the splice_write as
>> I mentioned.
>
> No, I've only tried the read side so far. I made a little program:
>
> #include <sys/sendfile.h>
> #include <stdio.h>
>
> int main(int argc, char *argv[])
> {
> ssize_t s = sendfile(1, 0, NULL, 0xffff);
> fprintf(stderr, "ret: %zd\n", s);
> return 0;
> }
>
> Then I ran `./a.out < /dev/urandom > /dev/null`. Fails. OTOH, if I
> replace /dev/urandom with an ordinary file, it succeeds.
Here's why, it's limited to regular files or block devices:
if (unlikely(!S_ISREG(i_mode) && !S_ISBLK(i_mode)))
return -EINVAL;
in splice_direct_to_actor().
--
Jens Axboe
Hi Jens,
On Thu, May 19, 2022 at 07:10:56PM -0600, Jens Axboe wrote:
> On 5/19/22 7:00 PM, Jason A. Donenfeld wrote:
> > Hi Jens,
> >
> > On Thu, May 19, 2022 at 06:56:12PM -0600, Jens Axboe wrote:
> >> On 5/19/22 6:48 PM, Jason A. Donenfeld wrote:
> >>> sendfile() returns -EINVAL even with your patches. Only splicing to pipes
> >>> seems to work.
> >>
> >> Huh, that really should work. Are you trying to sendfile() to random? If
> >> so, you need that last write_iter patch too, and add the splice_write as
> >> I mentioned.
> >
> > No, I've only tried the read side so far. I made a little program:
> >
> > #include <sys/sendfile.h>
> > #include <stdio.h>
> >
> > int main(int argc, char *argv[])
> > {
> > ssize_t s = sendfile(1, 0, NULL, 0xffff);
> > fprintf(stderr, "ret: %zd\n", s);
> > return 0;
> > }
> >
> > Then I ran `./a.out < /dev/urandom > /dev/null`. Fails. OTOH, if I
> > replace /dev/urandom with an ordinary file, it succeeds.
>
> Here's why, it's limited to regular files or block devices:
>
> if (unlikely(!S_ISREG(i_mode) && !S_ISBLK(i_mode)))
> return -EINVAL;
>
> in splice_direct_to_actor().
Indeed. Looks like that was your code from long long ago!
I posted
https://lore.kernel.org/lkml/[email protected]/ to
fix it if you'd like to review.
Jason
On 5/19/22 5:23 PM, Jason A. Donenfeld wrote:
> Hi Jens,
>
> On Fri, May 20, 2022 at 1:19 AM Jens Axboe <[email protected]> wrote:
>> The source and destination for the copies are exactly the same with the
>> change as before, so no changes there. The non-user copy is a different
>> helper.
>
> Oh, okay. Maybe a silly question but: should we wire up that helper
> too? (If I'm understanding your meaning right.) Seems like it'd be a
> good idea to just wire up all the things while we're at it.
I'll leave that to you :-)
I'll do the write_iter though, just for completeness.
--
Jens Axboe
This is a pre-requisite to writing up splice() again for the random
and urandom drivers.
Signed-off-by: Jens Axboe <[email protected]>
---
drivers/char/random.c | 37 +++++++++++++++++++------------------
1 file changed, 19 insertions(+), 18 deletions(-)
diff --git a/drivers/char/random.c b/drivers/char/random.c
index 4c9adb4f3d5d..529afd31d549 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -528,11 +528,12 @@ void get_random_bytes(void *buf, size_t nbytes)
}
EXPORT_SYMBOL(get_random_bytes);
-static ssize_t get_random_bytes_user(void __user *buf, size_t nbytes)
+static ssize_t get_random_bytes_user(struct iov_iter *to)
{
- size_t len, left, ret = 0;
+ size_t len, ret = 0;
u32 chacha_state[CHACHA_STATE_WORDS];
u8 output[CHACHA_BLOCK_SIZE];
+ size_t nbytes = iov_iter_count(to);
if (!nbytes)
return 0;
@@ -549,7 +550,7 @@ static ssize_t get_random_bytes_user(void __user *buf, size_t nbytes)
* the user directly.
*/
if (nbytes <= CHACHA_KEY_SIZE) {
- ret = nbytes - copy_to_user(buf, &chacha_state[4], nbytes);
+ ret = copy_to_iter(&chacha_state[4], nbytes, to);
goto out_zero_chacha;
}
@@ -559,13 +560,10 @@ static ssize_t get_random_bytes_user(void __user *buf, size_t nbytes)
++chacha_state[13];
len = min_t(size_t, nbytes, CHACHA_BLOCK_SIZE);
- left = copy_to_user(buf, output, len);
- if (left) {
- ret += len - left;
+ len = copy_to_iter(output, len, to);
+ if (!len)
break;
- }
- buf += len;
ret += len;
nbytes -= len;
if (!nbytes)
@@ -1466,6 +1464,9 @@ static void try_to_generate_entropy(void)
SYSCALL_DEFINE3(getrandom, char __user *, buf, size_t, count, unsigned int,
flags)
{
+ struct iovec iov = { .iov_base = buf };
+ struct iov_iter iter;
+
if (flags & ~(GRND_NONBLOCK | GRND_RANDOM | GRND_INSECURE))
return -EINVAL;
@@ -1488,7 +1489,9 @@ SYSCALL_DEFINE3(getrandom, char __user *, buf, size_t, count, unsigned int,
if (unlikely(ret))
return ret;
}
- return get_random_bytes_user(buf, count);
+ iov.iov_len = count;
+ iov_iter_init(&iter, READ, &iov, 1, count);
+ return get_random_bytes_user(&iter);
}
static __poll_t random_poll(struct file *file, poll_table *wait)
@@ -1540,8 +1543,7 @@ static ssize_t random_write(struct file *file, const char __user *buffer,
return (ssize_t)count;
}
-static ssize_t urandom_read(struct file *file, char __user *buf, size_t nbytes,
- loff_t *ppos)
+static ssize_t urandom_read_iter(struct kiocb *kiocb, struct iov_iter *to)
{
static int maxwarn = 10;
@@ -1556,21 +1558,20 @@ static ssize_t urandom_read(struct file *file, char __user *buf, size_t nbytes,
maxwarn--;
if (__ratelimit(&urandom_warning))
pr_notice("%s: uninitialized urandom read (%zd bytes read)\n",
- current->comm, nbytes);
+ current->comm, iov_iter_count(to));
}
- return get_random_bytes_user(buf, nbytes);
+ return get_random_bytes_user(to);
}
-static ssize_t random_read(struct file *file, char __user *buf, size_t nbytes,
- loff_t *ppos)
+static ssize_t random_read_iter(struct kiocb *kiocb, struct iov_iter *to)
{
int ret;
ret = wait_for_random_bytes();
if (ret != 0)
return ret;
- return get_random_bytes_user(buf, nbytes);
+ return get_random_bytes_user(to);
}
static long random_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
@@ -1639,7 +1640,7 @@ static int random_fasync(int fd, struct file *filp, int on)
}
const struct file_operations random_fops = {
- .read = random_read,
+ .read_iter = random_read_iter,
.write = random_write,
.poll = random_poll,
.unlocked_ioctl = random_ioctl,
@@ -1649,7 +1650,7 @@ const struct file_operations random_fops = {
};
const struct file_operations urandom_fops = {
- .read = urandom_read,
+ .read_iter = urandom_read_iter,
.write = random_write,
.unlocked_ioctl = random_ioctl,
.compat_ioctl = compat_ptr_ioctl,
--
2.35.1
Hi Jens,
On Thu, May 19, 2022 at 01:31:31PM -0600, Jens Axboe wrote:
> Hi,
>
> We recently had a failure on a kernel upgrade because splice no longer
> works on random/urandom. This is due to:
>
> 6e2c7421f02 ("fs: don't allow splice read/write without explicit ops")
Thanks for this. I'd noticed this a few months ago and assumed it has
just always been that way, and hadn't gotten to looking at what was up.
I'll take a look at these patches in detail when I'm home in a few
hours, but one thing maybe you can answer more easily than my digging
is:
There's a lot of attention in random.c devoted to not leaving any output
around on the stack or in stray buffers. The explicit use of
copy_to_user() makes it clear that the output isn't being copied
anywhere other than what's the user's responsibility to cleanup. I'm
wondering if the switch to copy_to_iter() introduces any buffering or
gotchas that you might be aware of.
Also you may need to rebase this on the random.git tree at
https://git.kernel.org/pub/scm/linux/kernel/git/crng/random.git
Regards,
Jason
Now that random/urandom is using read_iter, we can wire it up to using
the generic splice read handler.
Fixes: 36e2c7421f02 ("fs: don't allow splice read/write without explicit ops")
Signed-off-by: Jens Axboe <[email protected]>
---
drivers/char/random.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/char/random.c b/drivers/char/random.c
index 529afd31d549..6da8f1441815 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1647,6 +1647,7 @@ const struct file_operations random_fops = {
.compat_ioctl = compat_ptr_ioctl,
.fasync = random_fasync,
.llseek = noop_llseek,
+ .splice_read = generic_file_splice_read,
};
const struct file_operations urandom_fops = {
@@ -1656,6 +1657,7 @@ const struct file_operations urandom_fops = {
.compat_ioctl = compat_ptr_ioctl,
.fasync = random_fasync,
.llseek = noop_llseek,
+ .splice_read = generic_file_splice_read,
};
--
2.35.1