[PATCH] random: convert to using fops->write_iter()

[PATCH] random: convert to using fops->write_iter()

[Jens Axboe]

Now that the read side has been converted to fix a regression with
splice, convert the write side as well to have some symmetry in the
interface used (and help deprecate ->write()).

Signed-off-by: Jens Axboe <axboe@kernel.dk>

---

Jason, this has only been booted. I did verify that it seems to take a
write just fine, but I would appreciate if you could vet this one with
your testing. Thanks!

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 41ca5966aa4f..3da04068c225 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1283,20 +1283,20 @@ static __poll_t random_poll(struct file *file, poll_table *wait)
 	return crng_ready() ? EPOLLIN | EPOLLRDNORM : EPOLLOUT | EPOLLWRNORM;
 }
 
-static int write_pool(const char __user *ubuf, size_t len)
+static size_t write_pool(struct iov_iter *iter)
 {
 	size_t block_len;
 	int ret = 0;
 	u8 block[BLAKE2S_BLOCK_SIZE];
 
-	while (len) {
-		block_len = min(len, sizeof(block));
-		if (copy_from_user(block, ubuf, block_len)) {
-			ret = -EFAULT;
+	while (iov_iter_count(iter)) {
+		block_len = min(iov_iter_count(iter), sizeof(block));
+		if (!copy_from_iter(block, block_len, iter)) {
+			if (!ret)
+				ret = -EFAULT;
 			goto out;
 		}
-		len -= block_len;
-		ubuf += block_len;
+		ret += block_len;
 		mix_pool_bytes(block, block_len);
 		cond_resched();
 	}
@@ -1306,16 +1306,9 @@ static int write_pool(const char __user *ubuf, size_t len)
 	return ret;
 }
 
-static ssize_t random_write(struct file *file, const char __user *ubuf,
-			    size_t len, loff_t *ppos)
+static ssize_t random_write_iter(struct kiocb *kiocb, struct iov_iter *from)
 {
-	int ret;
-
-	ret = write_pool(ubuf, len);
-	if (ret)
-		return ret;
-
-	return (ssize_t)len;
+	return write_pool(from);
 }
 
 static ssize_t urandom_read_iter(struct kiocb *kiocb, struct iov_iter *to)
@@ -1373,7 +1366,10 @@ static long random_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
 			return -EINVAL;
 		credit_init_bits(ent_count);
 		return 0;
-	case RNDADDENTROPY:
+	case RNDADDENTROPY: {
+		struct iov_iter iter;
+		struct iovec iov;
+
 		if (!capable(CAP_SYS_ADMIN))
 			return -EPERM;
 		if (get_user(ent_count, p++))
@@ -1382,11 +1378,16 @@ static long random_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
 			return -EINVAL;
 		if (get_user(size, p++))
 			return -EFAULT;
-		retval = write_pool((const char __user *)p, size);
+
+		iov.iov_base = p;
+		iov.iov_len = size;
+		iov_iter_init(&iter, WRITE, &iov, 1, size);
+		retval = write_pool(&iter);
 		if (retval < 0)
 			return retval;
 		credit_init_bits(ent_count);
 		return 0;
+		}
 	case RNDZAPENTCNT:
 	case RNDCLEARPOOL:
 		/* No longer has any effect. */
@@ -1412,7 +1413,7 @@ static int random_fasync(int fd, struct file *filp, int on)
 
 const struct file_operations random_fops = {
 	.read_iter = random_read_iter,
-	.write = random_write,
+	.write_iter = random_write_iter,
 	.poll = random_poll,
 	.unlocked_ioctl = random_ioctl,
 	.compat_ioctl = compat_ptr_ioctl,
@@ -1423,7 +1424,7 @@ const struct file_operations random_fops = {
 
 const struct file_operations urandom_fops = {
 	.read_iter = urandom_read_iter,
-	.write = random_write,
+	.write_iter = random_write_iter,
 	.unlocked_ioctl = random_ioctl,
 	.compat_ioctl = compat_ptr_ioctl,
 	.fasync = random_fasync,

-- 
Jens Axboe

Re: [PATCH] random: convert to using fops->write_iter()

[Jens Axboe]

On 5/19/22 5:43 PM, Jens Axboe wrote:
> Now that the read side has been converted to fix a regression with
> splice, convert the write side as well to have some symmetry in the
> interface used (and help deprecate ->write()).

Oh, and similarly to the other one, you can then add ->splice_write
as well with using iter_file_splice_write for that. If you want to add
that, please do.

-- 
Jens Axboe

Re: [PATCH] random: convert to using fops->write_iter()

[Jason A. Donenfeld]

Hi Jens,

On Thu, May 19, 2022 at 05:43:15PM -0600, Jens Axboe wrote:
> -static int write_pool(const char __user *ubuf, size_t len)
> +static size_t write_pool(struct iov_iter *iter)
>  {
>  	size_t block_len;
>  	int ret = 0;

Changing the return value to size_t isn't quite right, as this can
return -EFAULT. So at the very minimum, it should return a ssize_t.

Jason

Re: [PATCH] random: convert to using fops->write_iter()

[Jens Axboe]

On 5/19/22 6:08 PM, Jason A. Donenfeld wrote:
> Hi Jens,
> 
> On Thu, May 19, 2022 at 05:43:15PM -0600, Jens Axboe wrote:
>> -static int write_pool(const char __user *ubuf, size_t len)
>> +static size_t write_pool(struct iov_iter *iter)
>>  {
>>  	size_t block_len;
>>  	int ret = 0;
> 
> Changing the return value to size_t isn't quite right, as this can
> return -EFAULT. So at the very minimum, it should return a ssize_t.

Good catch, yes let's make that a ssize_t instead. Can you do that while
testing/applying?

-- 
Jens Axboe

Re: [PATCH] random: convert to using fops->write_iter()

[Jason A. Donenfeld]

Hi Jens,

On Thu, May 19, 2022 at 05:43:15PM -0600, Jens Axboe wrote:
> +	while (iov_iter_count(iter)) {
> +		block_len = min(iov_iter_count(iter), sizeof(block));

Whereas get_random_bytes_user() used a separate len param, it looks like
this function is using iov's len. Maybe that's not such a bad idea? I'll
give a shot at fixing that up in both places, and you can tell me what
you think.

Jason

Re: [PATCH] random: convert to using fops->write_iter()

[Jens Axboe]

On 5/19/22 6:18 PM, Jason A. Donenfeld wrote:
> Hi Jens,
> 
> On Thu, May 19, 2022 at 05:43:15PM -0600, Jens Axboe wrote:
>> +	while (iov_iter_count(iter)) {
>> +		block_len = min(iov_iter_count(iter), sizeof(block));
> 
> Whereas get_random_bytes_user() used a separate len param, it looks like
> this function is using iov's len. Maybe that's not such a bad idea? I'll
> give a shot at fixing that up in both places, and you can tell me what
> you think.

Yes that'd be fine, it's just reading iter->count anyway. Agree that
it'd be nicer to just kill the separate 'len' as the copy advances the
iter anyway.

-- 
Jens Axboe

Re: [PATCH] random: convert to using fops->write_iter()

[Al Viro]

On Thu, May 19, 2022 at 05:43:15PM -0600, Jens Axboe wrote:

> -static int write_pool(const char __user *ubuf, size_t len)
> +static size_t write_pool(struct iov_iter *iter)
>  {
>  	size_t block_len;
>  	int ret = 0;
>  
> -	while (len) {
> -		block_len = min(len, sizeof(block));
> -		if (copy_from_user(block, ubuf, block_len)) {
> -			ret = -EFAULT;
> +	while (iov_iter_count(iter)) {
> +		block_len = min(iov_iter_count(iter), sizeof(block));
> +		if (!copy_from_iter(block, block_len, iter)) {
> +			if (!ret)
> +				ret = -EFAULT;
>  			goto out;
>  		}

Feed it a buffer with only 1 byte mapped, watch it'll pass to mix_pool_bytes().
And see how much of 'block' has been used uninitialized...

And why bother with that min thing, anyway?

	ssize_t ret = 0;

	while (iov_iter_count(iter)) {
	  	u8 block[BLAKE2S_BLOCK_SIZE];
		size_t copied = copy_from_iter(block, sizeof(block), iter);
		if (!copied) {
			if (!ret)
				ret = -EFAULT;
			break;
		}
		mix_pool_bytes(block, copied);
		ret += copied;
	}
	return ret;

and be done with that...

> @@ -1382,11 +1378,16 @@ static long random_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
>  			return -EINVAL;
>  		if (get_user(size, p++))
>  			return -EFAULT;
> -		retval = write_pool((const char __user *)p, size);
> +
> +		iov.iov_base = p;
> +		iov.iov_len = size;
> +		iov_iter_init(&iter, WRITE, &iov, 1, size);

That'd be
		import_single_range(WRITE, p, size, &iov, &iter);

Re: [PATCH] random: convert to using fops->write_iter()

[Jens Axboe]

On 5/19/22 9:01 PM, Al Viro wrote:
> On Thu, May 19, 2022 at 05:43:15PM -0600, Jens Axboe wrote:
> 
>> -static int write_pool(const char __user *ubuf, size_t len)
>> +static size_t write_pool(struct iov_iter *iter)
>>  {
>>  	size_t block_len;
>>  	int ret = 0;
>>  
>> -	while (len) {
>> -		block_len = min(len, sizeof(block));
>> -		if (copy_from_user(block, ubuf, block_len)) {
>> -			ret = -EFAULT;
>> +	while (iov_iter_count(iter)) {
>> +		block_len = min(iov_iter_count(iter), sizeof(block));
>> +		if (!copy_from_iter(block, block_len, iter)) {
>> +			if (!ret)
>> +				ret = -EFAULT;
>>  			goto out;
>>  		}
> 
> Feed it a buffer with only 1 byte mapped, watch it'll pass to mix_pool_bytes().
> And see how much of 'block' has been used uninitialized...

I don't follow? Buffer with 1 byte, iter setup with 1 byte. We copy 1 byte,
and we pass 1 byte to mix_pool_bytes(). What am I missing?

> And why bother with that min thing, anyway?
> 
> 	ssize_t ret = 0;
> 
> 	while (iov_iter_count(iter)) {
> 	  	u8 block[BLAKE2S_BLOCK_SIZE];
> 		size_t copied = copy_from_iter(block, sizeof(block), iter);
> 		if (!copied) {
> 			if (!ret)
> 				ret = -EFAULT;
> 			break;
> 		}
> 		mix_pool_bytes(block, copied);
> 		ret += copied;
> 	}
> 	return ret;
> 
> and be done with that...

Agree, that does look better, the min() part could've been killed with
the conversion indeed.

>> @@ -1382,11 +1378,16 @@ static long random_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
>>  			return -EINVAL;
>>  		if (get_user(size, p++))
>>  			return -EFAULT;
>> -		retval = write_pool((const char __user *)p, size);
>> +
>> +		iov.iov_base = p;
>> +		iov.iov_len = size;
>> +		iov_iter_init(&iter, WRITE, &iov, 1, size);
> 
> That'd be
> 		import_single_range(WRITE, p, size, &iov, &iter);

Yep that'd be a simpler equivalent.

-- 
Jens Axboe

Re: [PATCH] random: convert to using fops->write_iter()

[Al Viro]

On Thu, May 19, 2022 at 09:25:30PM -0600, Jens Axboe wrote:

> > Feed it a buffer with only 1 byte mapped, watch it'll pass to mix_pool_bytes().
> > And see how much of 'block' has been used uninitialized...
> 
> I don't follow? Buffer with 1 byte, iter setup with 1 byte. We copy 1 byte,
> and we pass 1 byte to mix_pool_bytes(). What am I missing?

"only 1 byte mapped" != "len is 1"...  Anonymous mmap() of 2*PAGE_SIZE, munmap()
the second half, ask that sucker to write PAGE_SIZE from buffer + PAGE_SIZE - 1...