linux-kernel - Re: [PATCH v7 4/4] mm: vmalloc: convert vread() to vread

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 for Android: free password hash cracker in your pocket

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

Message-ID: <ZBu+2cPCQvvFF/FY@MiWiFi-R3L-srv>
Date:   Thu, 23 Mar 2023 10:52:09 +0800
From:   Baoquan He <bhe@...hat.com>
To:     Lorenzo Stoakes <lstoakes@...il.com>,
        David Hildenbrand <david@...hat.com>
Cc:     linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        linux-fsdevel@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Uladzislau Rezki <urezki@...il.com>,
        Matthew Wilcox <willy@...radead.org>,
        Liu Shixin <liushixin2@...wei.com>,
        Jiri Olsa <jolsa@...nel.org>, Jens Axboe <axboe@...nel.dk>,
        Alexander Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH v7 4/4] mm: vmalloc: convert vread() to vread_iter()

On 03/22/23 at 06:57pm, Lorenzo Stoakes wrote:
> Having previously laid the foundation for converting vread() to an iterator
> function, pull the trigger and do so.
> 
> This patch attempts to provide minimal refactoring and to reflect the
> existing logic as best we can, for example we continue to zero portions of
> memory not read, as before.
> 
> Overall, there should be no functional difference other than a performance
> improvement in /proc/kcore access to vmalloc regions.
> 
> Now we have eliminated the need for a bounce buffer in read_kcore_iter(),
> we dispense with it, and try to write to user memory optimistically but
> with faults disabled via copy_page_to_iter_nofault(). We already have
> preemption disabled by holding a spin lock. We continue faulting in until
> the operation is complete.

I don't understand the sentences here. In vread_iter(), the actual
content reading is done in aligned_vread_iter(), otherwise we zero
filling the region. In aligned_vread_iter(), we will use
vmalloc_to_page() to get the mapped page and read out, otherwise zero
fill. While in this patch, fault_in_iov_iter_writeable() fault in memory
of iter one time and will bail out if failed. I am wondering why we 
continue faulting in until the operation is complete, and how that is done. 

If we look into the failing point in vread_iter(), it's mainly coming
from copy_page_to_iter_nofault(), e.g page_copy_sane() checking failed,
i->data_source checking failed. If these conditional checking failed,
should we continue reading again and again? And this is not related to
memory faulting in. I saw your discussion with David, but I am still a
little lost. Hope I can learn it, thanks in advance.

......
> diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
> index 08b795fd80b4..25b44b303b35 100644
> --- a/fs/proc/kcore.c
> +++ b/fs/proc/kcore.c
......
> @@ -507,13 +503,30 @@ read_kcore_iter(struct kiocb *iocb, struct iov_iter *iter)
>  
>  		switch (m->type) {
>  		case KCORE_VMALLOC:
> -			vread(buf, (char *)start, tsz);
> -			/* we have to zero-fill user buffer even if no read */
> -			if (copy_to_iter(buf, tsz, iter) != tsz) {
> -				ret = -EFAULT;
> -				goto out;
> +		{
> +			const char *src = (char *)start;
> +			size_t read = 0, left = tsz;
> +
> +			/*
> +			 * vmalloc uses spinlocks, so we optimistically try to
> +			 * read memory. If this fails, fault pages in and try
> +			 * again until we are done.
> +			 */
> +			while (true) {
> +				read += vread_iter(iter, src, left);
> +				if (read == tsz)
> +					break;
> +
> +				src += read;
> +				left -= read;
> +
> +				if (fault_in_iov_iter_writeable(iter, left)) {
> +					ret = -EFAULT;
> +					goto out;
> +				}
>  			}
>  			break;
> +		}
>  		case KCORE_USER:
>  			/* User page is handled prior to normal kernel page: */
>  			if (copy_to_iter((char *)start, tsz, iter) != tsz) {