Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Alexander Egorenkov <egorenar@linux.ibm.com>
To:     Alexander Gordeev <agordeev@linux.ibm.com>,
        Heiko Carstens <hca@linux.ibm.com>,
        Vasily Gorbik <gor@linux.ibm.com>
Cc:     Matthew Wilcox <willy@infradead.org>, Baoquan He <bhe@redhat.com>,
        Christoph Hellwig <hch@lst.de>, linux-kernel@vger.kernel.org,
        linux-s390@vger.kernel.org
Subject: Re: [PATCH 1/1] s390/crash: allow multi-segment iterators
In-Reply-To: <3e713ce3865246766feca8397af2860cbe46854d.1657049033.git.agordeev@linux.ibm.com>
In-Reply-To: 
References: <cover.1657049033.git.agordeev@linux.ibm.com>
 <3e713ce3865246766feca8397af2860cbe46854d.1657049033.git.agordeev@linux.ibm.com>
Date:   Wed, 06 Jul 2022 10:18:15 +0200
Message-ID: <877d4q3b94.fsf@oc8242746057.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Hi Alexander,

Alexander Gordeev <agordeev@linux.ibm.com> writes:

> Rework copy_oldmem_page() to allow multi-segment iterators.
> Reuse existing iterate_iovec macro as is and only relevant
> bits from __iterate_and_advance macro.
>
> Fixes: 49b11524d648 ("s390/crash: add missing iterator advance in copy_oldmem_page())
> Signed-off-by: Alexander Gordeev <agordeev@linux.ibm.com>
> ---
>  arch/s390/kernel/crash_dump.c | 65 +++++++++++++++++++++++++++--------
>  1 file changed, 50 insertions(+), 15 deletions(-)
>
> diff --git a/arch/s390/kernel/crash_dump.c b/arch/s390/kernel/crash_dump.c
> index 28124d0fa1d5..ac873245d6f0 100644
> --- a/arch/s390/kernel/crash_dump.c
> +++ b/arch/s390/kernel/crash_dump.c
> @@ -210,6 +210,52 @@ static int copy_oldmem_user(void __user *dst, unsigned long src, size_t count)
>  	return 0;
>  }
>  
> +#define iterate_iovec(i, n, base, len, off, __p, STEP) {	\
> +	size_t off = 0;						\
> +	size_t skip = i->iov_offset;				\
> +	do {							\
> +		len = min(n, __p->iov_len - skip);		\
> +		if (likely(len)) {				\
> +			base = __p->iov_base + skip;		\
> +			len -= (STEP);				\
> +			off += len;				\
> +			skip += len;				\
> +			n -= len;				\
> +			if (skip < __p->iov_len)		\
> +				break;				\
> +		}						\
> +		__p++;						\
> +		skip = 0;					\
> +	} while (n);						\
> +	i->iov_offset = skip;					\
> +	n = off;						\
> +}
> +
> +#define __iterate_and_advance(i, n, base, len, off, I, K) {	\
> +	if (unlikely(i->count < n))				\
> +		n = i->count;					\
> +	if (likely(n)) {					\
> +		if (likely(iter_is_iovec(i))) {			\
> +			const struct iovec *iov = i->iov;	\
> +			void __user *base;			\
> +			size_t len;				\
> +			iterate_iovec(i, n, base, len, off,	\
> +						iov, (I))	\
> +			i->nr_segs -= iov - i->iov;		\
> +			i->iov = iov;				\
> +		} else if (iov_iter_is_kvec(i)) {		\
> +			const struct kvec *kvec = i->kvec;	\
> +			void *base;				\
> +			size_t len;				\
> +			iterate_iovec(i, n, base, len, off,	\
> +						kvec, (K))	\
> +			i->nr_segs -= kvec - i->kvec;		\
> +			i->kvec = kvec;				\
> +		}						\
> +		i->count -= n;					\
> +	}							\
> +}
> +
>  /*
>   * Copy one page from "oldmem"
>   */
> @@ -217,25 +263,14 @@ ssize_t copy_oldmem_page(struct iov_iter *iter, unsigned long pfn, size_t csize,
>  			 unsigned long offset)
>  {
>  	unsigned long src;
> -	int rc;
>  
>  	if (!(iter_is_iovec(iter) || iov_iter_is_kvec(iter)))
>  		return -EINVAL;
> -	/* Multi-segment iterators are not supported */
> -	if (iter->nr_segs > 1)
> -		return -EINVAL;
> -	if (!csize)
> -		return 0;
>  	src = pfn_to_phys(pfn) + offset;
> -
> -	/* XXX: pass the iov_iter down to a common function */
> -	if (iter_is_iovec(iter))
> -		rc = copy_oldmem_user(iter->iov->iov_base, src, csize);
> -	else
> -		rc = copy_oldmem_kernel(iter->kvec->iov_base, src, csize);
> -	if (rc < 0)
> -		return rc;
> -	iov_iter_advance(iter, csize);
> +	__iterate_and_advance(iter, csize, base, len, off,
> +		({ copy_oldmem_user(base, src + off, len) < 0 ? csize : 0; }),
> +		({ copy_oldmem_kernel(base, src + off, len) < 0 ? csize : 0; })

Question
--------
About return value of STEP in iterate_iovec().
We return "csize" in case copy_oldmem_*() fails.
If i'm not mistaken this could lead to an overflow in iterate_iovec():

  len -= (STEP);

Because len could be less than csize in case iovec consists of multiple
segments, one of which is less than csize.

Better to return len ?

({ copy_oldmem_user(base, src + off, len) < 0 ? len : 0; })

> +	)
>  	return csize;
>  }

Another thing is that now we never report any errors in contrast to
the version before. This is OK ?
Maybe set an error flag while iterating and then when the iteration is
done, check the flag and return an error ?


>  
> -- 
> 2.34.1


Otherwise, looks good to me.
Tested on LPAR and zVM , all our tela-kernel kdump tests in
tests/dump/kdump work now.

Reviewed-by: Alexander Egorenkov <egorenar@linux.ibm.com>
Tested-by: Alexander Egorenkov <egorenar@linux.ibm.com>

Regards
Alex