linux-kernel - Re: [PATCH] /dev/zero: also implement ->read

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8d430999-b155-dbfa-e7db-f414b48014b1@rasmusvillemoes.dk>
Date:   Mon, 7 Sep 2020 00:34:37 +0200
From:   Rasmus Villemoes <linux@...musvillemoes.dk>
To:     Christoph Hellwig <hch@....de>, arnd@...db.de,
        gregkh@...uxfoundation.org
Cc:     christophe.leroy@...roup.eu, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] /dev/zero: also implement ->read

On 03/09/2020 17.59, Christoph Hellwig wrote:
> Christophe reported a major speedup due to avoiding the iov_iter
> overhead, so just add this trivial function.  Note that /dev/zero
> already implements both an iter and non-iter writes so this just
> makes it more symmetric.
> 
> Christophe Leroy <christophe.leroy@...roup.eu>

?-by ?

> Signed-off-by: Christoph Hellwig <hch@....de>
> ---
>  drivers/char/mem.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/char/mem.c b/drivers/char/mem.c
> index abd4ffdc8cdebc..1dc99ab158457a 100644
> --- a/drivers/char/mem.c
> +++ b/drivers/char/mem.c
> @@ -726,6 +726,27 @@ static ssize_t read_iter_zero(struct kiocb *iocb, struct iov_iter *iter)
>  	return written;
>  }
>  
> +static ssize_t read_zero(struct file *file, char __user *buf,
> +			 size_t count, loff_t *ppos)
> +{
> +	size_t cleared = 0;
> +
> +	while (count) {
> +		size_t chunk = min_t(size_t, count, PAGE_SIZE);
> +
> +		if (clear_user(buf + cleared, chunk))
> +			return cleared ? cleared : -EFAULT;

Probably nobody really cares, but currently doing

read(fd, &unmapped_page - 5, 123);

returns 5, and those five bytes do get cleared; if I'm reading the above
right you'd return -EFAULT for that case.


> +		cleared += chunk;
> +		count -= chunk;
> +
> +		if (signal_pending(current))
> +			return cleared ? cleared : -ERESTARTSYS;

I can't see how we can get here without 'cleared' being positive, so
this can just be 'return cleared' (and if you fix the above EFAULT case
to more accurately track how much got cleared, there's probably no
longer any code to be symmetric with anyway).

Rasmus