[PATCHSET 0/2] Fix splice from random/urandom

[PATCHSET 0/2] Fix splice from random/urandom

[Jens Axboe]

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

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

[Jens Axboe]

This is a pre-requisite to writing up splice() again for the random
and urandom drivers.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 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

[PATCH 2/2] random: wire up fops->splice_read_iter()

[Jens Axboe]

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 <axboe@kernel.dk>
---
 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

Re: [PATCHSET 0/2] Fix splice from random/urandom

[Christoph Hellwig]

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")
> 
> which already has more than two handful of Fixes registered to its
> name...

Yes.  It was a hard break to get rid of set_fs and it's abuse.

Re: [PATCHSET 0/2] Fix splice from random/urandom

[Jens Axboe]

On 5/19/22 1:48 PM, Christoph Hellwig wrote:
> 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")
>>
>> which already has more than two handful of Fixes registered to its
>> name...
> 
> Yes.  It was a hard break to get rid of set_fs and it's abuse.

I'm a bit torn on this one, because I _really_ want us to get rid of
read/write and make everything use read_iter/write_iter. Firstly because
it's really stupid to have two interfaces, and secondly because even
basic things like "can we block here" doesn't work in the older
interface without fiddling with file flags which is a non-starter for
certain things.

However, it's also problematic that we end up breaking real applications
because of this change. Arguably we should've converted existing
read/write first and avoid this grey zone we are in now (best solution),
or provided splice read/write helpers that work with the non-iter based
read/write handlers.

-- 
Jens Axboe

Re: [PATCHSET 0/2] Fix splice from random/urandom

[Jason A. Donenfeld]

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

Re: [PATCHSET 0/2] Fix splice from random/urandom

[Jens Axboe]

On 5/19/22 2:05 PM, Jason A. Donenfeld wrote:
> 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:

Sounds good, thanks!

> 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.

No, it's just a wrapper around copying to the user memory pointed to by
the iov_iter. No extra buffering or anything like that. So I think it
should be fine in that respect, and it actually cleans up the code a bit
imho since the copy_to_iter() since the return value of "bytes copied"
is easier to work with than the "bytes not copied".

> 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

OK, I will rebase it on that branch, not a problem.

-- 
Jens Axboe

Re: [PATCHSET 0/2] Fix splice from random/urandom

[Jens Axboe]

[-- Attachment #1: Type: text/plain, Size: 1860 bytes --]

On 5/19/22 2:49 PM, Jens Axboe wrote:
> On 5/19/22 2:05 PM, Jason A. Donenfeld wrote:
>> 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:
> 
> Sounds good, thanks!
> 
>> 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.
> 
> No, it's just a wrapper around copying to the user memory pointed to by
> the iov_iter. No extra buffering or anything like that. So I think it
> should be fine in that respect, and it actually cleans up the code a bit
> imho since the copy_to_iter() since the return value of "bytes copied"
> is easier to work with than the "bytes not copied".
> 
>> 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
> 
> OK, I will rebase it on that branch, not a problem.

Rebased patches attached, you can also find them here:

https://git.kernel.dk/cgit/linux-block/log/?h=random-splice

Did some basic sanity checking (and with splice too), and seems fine
rebased as well.

-- 
Jens Axboe

[-- Attachment #2: 0002-random-wire-up-fops-splice_read_iter.patch --]
[-- Type: text/x-patch, Size: 1128 bytes --]

From fd673dac9f7c905d95cbe334cc7519fb36ede678 Mon Sep 17 00:00:00 2001
From: Jens Axboe <axboe@kernel.dk>
Date: Thu, 19 May 2022 13:26:55 -0600
Subject: [PATCH 2/2] random: wire up fops->splice_read_iter()

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 <axboe@kernel.dk>
---
 drivers/char/random.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 147921d471d8..892513fb7479 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1415,6 +1415,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 = {
@@ -1424,6 +1425,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


[-- Attachment #3: 0001-random-convert-to-using-fops-read_iter.patch --]
[-- Type: text/x-patch, Size: 4254 bytes --]

From c65f0221b72173c19398884f3f49ef775e2c668f Mon Sep 17 00:00:00 2001
From: Jens Axboe <axboe@kernel.dk>
Date: Thu, 19 May 2022 14:55:37 -0600
Subject: [PATCH 1/2] random: convert to using fops->read_iter()

This is a pre-requisite to writing up splice() again for the random
and urandom drivers.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 drivers/char/random.c | 39 +++++++++++++++++++--------------------
 1 file changed, 19 insertions(+), 20 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 0958fa91a964..147921d471d8 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -397,11 +397,12 @@ void get_random_bytes(void *buf, size_t len)
 }
 EXPORT_SYMBOL(get_random_bytes);
 
-static ssize_t get_random_bytes_user(void __user *ubuf, size_t len)
+static ssize_t get_random_bytes_user(struct iov_iter *to)
 {
-	size_t block_len, left, ret = 0;
+	size_t block_len, ret = 0;
 	u32 chacha_state[CHACHA_STATE_WORDS];
 	u8 output[CHACHA_BLOCK_SIZE];
+	size_t len = iov_iter_count(to);
 
 	if (!len)
 		return 0;
@@ -418,7 +419,7 @@ static ssize_t get_random_bytes_user(void __user *ubuf, size_t len)
 	 * the user directly.
 	 */
 	if (len <= CHACHA_KEY_SIZE) {
-		ret = len - copy_to_user(ubuf, &chacha_state[4], len);
+		ret = copy_to_iter(&chacha_state[4], len, to);
 		goto out_zero_chacha;
 	}
 
@@ -428,17 +429,12 @@ static ssize_t get_random_bytes_user(void __user *ubuf, size_t len)
 			++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) {
@@ -1248,6 +1244,9 @@ static void __cold try_to_generate_entropy(void)
 
 SYSCALL_DEFINE3(getrandom, char __user *, ubuf, size_t, len, unsigned int, flags)
 {
+	struct iovec iov = { .iov_base = ubuf };
+	struct iov_iter iter;
+
 	if (flags & ~(GRND_NONBLOCK | GRND_RANDOM | GRND_INSECURE))
 		return -EINVAL;
 
@@ -1270,7 +1269,9 @@ SYSCALL_DEFINE3(getrandom, char __user *, ubuf, size_t, len, unsigned int, flags
 		if (unlikely(ret))
 			return ret;
 	}
-	return get_random_bytes_user(ubuf, len);
+	iov.iov_len = len;
+	iov_iter_init(&iter, READ, &iov, 1, len);
+	return get_random_bytes_user(&iter);
 }
 
 static __poll_t random_poll(struct file *file, poll_table *wait)
@@ -1314,8 +1315,7 @@ static ssize_t random_write(struct file *file, const char __user *ubuf,
 	return (ssize_t)len;
 }
 
-static ssize_t urandom_read(struct file *file, char __user *ubuf,
-			    size_t len, loff_t *ppos)
+static ssize_t urandom_read_iter(struct kiocb *kiocb, struct iov_iter *to)
 {
 	static int maxwarn = 10;
 
@@ -1332,22 +1332,21 @@ static ssize_t urandom_read(struct file *file, char __user *ubuf,
 		else if (ratelimit_disable || __ratelimit(&urandom_warning)) {
 			--maxwarn;
 			pr_notice("%s: uninitialized urandom read (%zd bytes read)\n",
-				  current->comm, len);
+				  current->comm, iov_iter_count(to));
 		}
 	}
 
-	return get_random_bytes_user(ubuf, len);
+	return get_random_bytes_user(to);
 }
 
-static ssize_t random_read(struct file *file, char __user *ubuf,
-			   size_t len, 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(ubuf, len);
+	return get_random_bytes_user(to);
 }
 
 static long random_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
@@ -1409,7 +1408,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,
@@ -1419,7 +1418,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

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

[Jason A. Donenfeld]

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.

Jason

Re: [PATCHSET 0/2] Fix splice from random/urandom

[Jason A. Donenfeld]

Hi Jens,

On Thu, May 19, 2022 at 02:49:13PM -0600, Jens Axboe wrote:
> > 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.
> 
> No, it's just a wrapper around copying to the user memory pointed to by
> the iov_iter. No extra buffering or anything like that. So I think it
> should be fine in that respect, and it actually cleans up the code a bit
> imho since the copy_to_iter() since the return value of "bytes copied"
> is easier to work with than the "bytes not copied".

Alright, that's good to hear. So even for kernel->kernel writes, the
argument is that what ever buffers are used in the process are the same
ones that the user would be hitting anyway by calling write() on the
destination if this roundtripped through userspace, so nothing changes?

Jason

Re: [PATCHSET 0/2] Fix splice from random/urandom

[Jason A. Donenfeld]

Hi Jens,

On Thu, May 19, 2022 at 03:02:28PM -0600, Jens Axboe wrote:
> Rebased patches attached, you can also find them here:
> 
> https://git.kernel.dk/cgit/linux-block/log/?h=random-splice
> 
> Did some basic sanity checking (and with splice too), and seems fine
> rebased as well.

Thanks. I left one comment about patch 1 in that subthread. The general
idea of this patchset seems fine, but: what about write_iter? Can't we
convert both of them? 1/3 - read_iter, 2/3 - write_iter, 3/3 - add the
generic splice helpers. I ask because it seems weird to keep around the
old thing (which sounds like is being gradually removed?) alongside the
new thing.

Jason

Re: [PATCHSET 0/2] Fix splice from random/urandom

[Jens Axboe]

On 5/19/22 5:13 PM, Jason A. Donenfeld wrote:
> Hi Jens,
> 
> On Thu, May 19, 2022 at 02:49:13PM -0600, Jens Axboe wrote:
>>> 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.
>>
>> No, it's just a wrapper around copying to the user memory pointed to by
>> the iov_iter. No extra buffering or anything like that. So I think it
>> should be fine in that respect, and it actually cleans up the code a bit
>> imho since the copy_to_iter() since the return value of "bytes copied"
>> is easier to work with than the "bytes not copied".
> 
> Alright, that's good to hear. So even for kernel->kernel writes, the
> argument is that what ever buffers are used in the process are the same
> ones that the user would be hitting anyway by calling write() on the
> destination if this roundtripped through userspace, so nothing changes?

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.

-- 
Jens Axboe

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

[Jason A. Donenfeld]

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;

		BUILD_BUG_ON(PAGE_SIZE % CHACHA_BLOCK_SIZE != 0);
		if (ret % PAGE_SIZE == 0) {
			if (signal_pending(current))
				break;
			cond_resched();
		}
	}

	memzero_explicit(output, sizeof(output));
out_zero_chacha:
	memzero_explicit(chacha_state, sizeof(chacha_state));
	return ret ? ret : -EFAULT;
}

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

[Jens Axboe]

On 5/19/22 5:12 PM, 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.

True, we could just make that:

copied = copy_to_iter(output, block_len, to);
if (copied != block_len)
	...

if that's important. Doesn't seem like it would, if you're passing in
invalid memory ranges. Maybe that ret check makes it so that it is
indeed important. I'll make the changes and send out a v2.

-- 
Jens Axboe

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

[Jens Axboe]

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

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@kernel.dk> 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

Re: [PATCHSET 0/2] Fix splice from random/urandom

[Jens Axboe]

On 5/19/22 5:15 PM, Jason A. Donenfeld wrote:
> Hi Jens,
> 
> On Thu, May 19, 2022 at 03:02:28PM -0600, Jens Axboe wrote:
>> Rebased patches attached, you can also find them here:
>>
>> https://git.kernel.dk/cgit/linux-block/log/?h=random-splice
>>
>> Did some basic sanity checking (and with splice too), and seems fine
>> rebased as well.
> 
> Thanks. I left one comment about patch 1 in that subthread. The general
> idea of this patchset seems fine, but: what about write_iter? Can't we
> convert both of them? 1/3 - read_iter, 2/3 - write_iter, 3/3 - add the
> generic splice helpers. I ask because it seems weird to keep around the
> old thing (which sounds like is being gradually removed?) alongside the
> new thing.

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.

In general, everyone using ->read and ->write should be converted so we
can kill these handlers. Would be great if someone would take that on...

-- 
Jens Axboe

Re: [PATCHSET 0/2] Fix splice from random/urandom

[Jason A. Donenfeld]

Hi Jens,

On Fri, May 20, 2022 at 1:19 AM Jens Axboe <axboe@kernel.dk> 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.

Jason

Re: [PATCHSET 0/2] Fix splice from random/urandom

[Jens Axboe]

On 5/19/22 5:23 PM, Jason A. Donenfeld wrote:
> Hi Jens,
> 
> On Fri, May 20, 2022 at 1:19 AM Jens Axboe <axboe@kernel.dk> 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

Re: [PATCHSET 0/2] Fix splice from random/urandom

[Jason A. Donenfeld]

Hi Jens,

On Fri, May 20, 2022 at 1:22 AM Jens Axboe <axboe@kernel.dk> 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.

Jason

Re: [PATCHSET 0/2] Fix splice from random/urandom

[Jason A. Donenfeld]

Hi Jens,

On Fri, May 20, 2022 at 1:25 AM Jens Axboe <axboe@kernel.dk> 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?

Jason

Re: [PATCHSET 0/2] Fix splice from random/urandom

[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 <axboe@kernel.dk> 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

Re: [PATCHSET 0/2] Fix splice from random/urandom

[Jason A. Donenfeld]

Hi Jens,

On Thu, May 19, 2022 at 05:33:01PM -0600, Jens Axboe wrote:
> On 5/19/22 5:25 PM, Jason A. Donenfeld wrote:
> > Hi Jens,
> > 
> > On Fri, May 20, 2022 at 1:22 AM Jens Axboe <axboe@kernel.dk> 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 :-)

Heh, noticed that too, thanks.

> 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.
 
No problem. Because of all the flux in random.c lately, I've been
preparing a massive backports branch, 2 branches actually, so I'll make
sure this is in there. Backport concern aside, though, I'll look for
your write_iter patch today. Thanks a bunch for doing this.

Jason

Re: [PATCHSET 0/2] Fix splice from random/urandom

[Jens Axboe]

On 5/19/22 5:39 PM, Jason A. Donenfeld wrote:
> Hi Jens,
> 
> On Thu, May 19, 2022 at 05:33:01PM -0600, Jens Axboe wrote:
>> On 5/19/22 5:25 PM, Jason A. Donenfeld wrote:
>>> Hi Jens,
>>>
>>> On Fri, May 20, 2022 at 1:22 AM Jens Axboe <axboe@kernel.dk> 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 :-)
> 
> Heh, noticed that too, thanks.

:-)

>> 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.
>  
> No problem. Because of all the flux in random.c lately, I've been
> preparing a massive backports branch, 2 branches actually, so I'll make
> sure this is in there. Backport concern aside, though, I'll look for
> your write_iter patch today. Thanks a bunch for doing this.

Sounds great, thanks - write patch has been sent out too.

-- 
Jens Axboe

Re: [PATCHSET 0/2] Fix splice from random/urandom

[Jens Axboe]

On 5/19/22 5:27 PM, Jason A. Donenfeld wrote:
> Hi Jens,
> 
> On Fri, May 20, 2022 at 1:25 AM Jens Axboe <axboe@kernel.dk> 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.

-- 
Jens Axboe

Re: [PATCHSET 0/2] Fix splice from random/urandom

[Jason A. Donenfeld]

Hi Jens,

On Fri, May 20, 2022 at 1:57 AM Jens Axboe <axboe@kernel.dk> 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 <axboe@kernel.dk> 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.

get_random_bytes() itself doesn't need to become iovec'd, as that has
no IO callers.

Jason

Re: [PATCHSET 0/2] Fix splice from random/urandom

[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 <axboe@kernel.dk> 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 <axboe@kernel.dk> 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

Re: [PATCHSET 0/2] Fix splice from random/urandom

[Jason A. Donenfeld]

Hi Jens,

On Thu, May 19, 2022 at 06:02:55PM -0600, Jens Axboe wrote:
> On 5/19/22 6:00 PM, Jason A. Donenfeld wrote:
> > Hi Jens,
> > 
> > On Fri, May 20, 2022 at 1:57 AM Jens Axboe <axboe@kernel.dk> 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 <axboe@kernel.dk> 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?

sendfile() returns -EINVAL even with your patches. Only splicing to pipes
seems to work.

Jason

Re: [PATCHSET 0/2] Fix splice from random/urandom

[Jens Axboe]

On 5/19/22 6:48 PM, Jason A. Donenfeld wrote:
> Hi Jens,
> 
> On Thu, May 19, 2022 at 06:02:55PM -0600, Jens Axboe wrote:
>> On 5/19/22 6:00 PM, Jason A. Donenfeld wrote:
>>> Hi Jens,
>>>
>>> On Fri, May 20, 2022 at 1:57 AM Jens Axboe <axboe@kernel.dk> 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 <axboe@kernel.dk> 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?
> 
> 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.

-- 
Jens Axboe

Re: [PATCHSET 0/2] Fix splice from random/urandom

[Jason A. Donenfeld]

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.

Jason

Re: [PATCHSET 0/2] Fix splice from random/urandom

[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

Re: [PATCHSET 0/2] Fix splice from random/urandom

[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

Re: [PATCHSET 0/2] Fix splice from random/urandom

[Christoph Hellwig]

On Thu, May 19, 2022 at 01:55:26PM -0600, Jens Axboe wrote:
> I'm a bit torn on this one, because I _really_ want us to get rid of
> read/write and make everything use read_iter/write_iter. Firstly because
> it's really stupid to have two interfaces, and secondly because even
> basic things like "can we block here" doesn't work in the older
> interface without fiddling with file flags which is a non-starter for
> certain things.

Converting everything was my initial plan, but Linus said no and just
fix whatever breaks.  And compared to my initial fears the fallout
actually isn't that bad.

Re: [PATCHSET 0/2] Fix splice from random/urandom

[Jason A. Donenfeld]

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/20220520095747.123748-1-Jason@zx2c4.com/ to
fix it if you'd like to review.

Jason

Re: [PATCHSET 0/2] Fix splice from random/urandom

[Jens Axboe]

On 5/20/22 6:43 AM, Jason A. Donenfeld wrote:
> 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!

Yep I would not be surprised if that is the case!

> I posted
> https://lore.kernel.org/lkml/20220520095747.123748-1-Jason@zx2c4.com/ to
> fix it if you'd like to review.

Not in my inbox, but you also used an email that hasn't been valid in 16
years :-)

But looks fine to me, we can open this up to character devices, don't
see an issue with that.

-- 
Jens Axboe

Re: [PATCHSET 0/2] Fix splice from random/urandom

[Jens Axboe]

On 5/20/22 12:02 AM, Christoph Hellwig wrote:
> On Thu, May 19, 2022 at 01:55:26PM -0600, Jens Axboe wrote:
>> I'm a bit torn on this one, because I _really_ want us to get rid of
>> read/write and make everything use read_iter/write_iter. Firstly because
>> it's really stupid to have two interfaces, and secondly because even
>> basic things like "can we block here" doesn't work in the older
>> interface without fiddling with file flags which is a non-starter for
>> certain things.
> 
> Converting everything was my initial plan, but Linus said no and just
> fix whatever breaks.  And compared to my initial fears the fallout
> actually isn't that bad.

That's a real shame, and very unlike Linus to advocate for just breaking
applications willy nilly. It would be a considerable amount of work
though, but it would really be great to not have ->read/->write anymore
and solely use the iter variants.

Not sure if I agree with "that bad" though looking at the number of
fixups, and we'll keep finding these going forward as well...

-- 
Jens Axboe

Re: [PATCHSET 0/2] Fix splice from random/urandom

[Jason A. Donenfeld]

Hi Jens,

On Fri, May 20, 2022 at 06:49:45AM -0600, Jens Axboe wrote:
> > I posted
> > https://lore.kernel.org/lkml/20220520095747.123748-1-Jason@zx2c4.com/ to
> > fix it if you'd like to review.
> 
> Not in my inbox, but you also used an email that hasn't been valid in 16
> years :-)
> 
> But looks fine to me, we can open this up to character devices, don't
> see an issue with that.

Whoops! I'll bounce it to you so you can provide a Reviewed-by for Al
over there.

Jason