* [PATCH 0/1] s390/crash: allow multi-segment iterators
@ 2022-07-06 5:59 Alexander Gordeev
2022-07-06 5:59 ` [PATCH 1/1] " Alexander Gordeev
0 siblings, 1 reply; 4+ messages in thread
From: Alexander Gordeev @ 2022-07-06 5:59 UTC (permalink / raw)
To: Alexander Egorenkov, Heiko Carstens, Vasily Gorbik
Cc: Matthew Wilcox, Baoquan He, Christoph Hellwig, linux-kernel, linux-s390
Unlike other architectures s390 can not use copyout() and memcopy() for
accessing memory and thus using copy_to_iter() now is not possible. But
a fix is needed, since 'cp' routine as 'core_collector' for kdump service
initiates multi-segment iterator.
The reason iterate_iovec() and __iterate_and_advance() macros copied from
lib/iov_iter.c (thus introducing redundancy) is to avoid custom iterator-
treating in s390 code. I doubt these macros could be turned public (i.e
with a follow-up patch), so the intention is to do it like _copy_to_iter()
does.
Alexander Gordeev (1):
s390/crash: allow multi-segment iterators
arch/s390/kernel/crash_dump.c | 65 +++++++++++++++++++++++++++--------
1 file changed, 50 insertions(+), 15 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 1/1] s390/crash: allow multi-segment iterators
2022-07-06 5:59 [PATCH 0/1] s390/crash: allow multi-segment iterators Alexander Gordeev
@ 2022-07-06 5:59 ` Alexander Gordeev
2022-07-06 8:18 ` Alexander Egorenkov
0 siblings, 1 reply; 4+ messages in thread
From: Alexander Gordeev @ 2022-07-06 5:59 UTC (permalink / raw)
To: Alexander Egorenkov, Heiko Carstens, Vasily Gorbik
Cc: Matthew Wilcox, Baoquan He, Christoph Hellwig, linux-kernel, linux-s390
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; })
+ )
return csize;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1] s390/crash: allow multi-segment iterators
2022-07-06 5:59 ` [PATCH 1/1] " Alexander Gordeev
@ 2022-07-06 8:18 ` Alexander Egorenkov
2022-07-07 5:21 ` Alexander Gordeev
0 siblings, 1 reply; 4+ messages in thread
From: Alexander Egorenkov @ 2022-07-06 8:18 UTC (permalink / raw)
To: Alexander Gordeev, Heiko Carstens, Vasily Gorbik
Cc: Matthew Wilcox, Baoquan He, Christoph Hellwig, linux-kernel, linux-s390
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1] s390/crash: allow multi-segment iterators
2022-07-06 8:18 ` Alexander Egorenkov
@ 2022-07-07 5:21 ` Alexander Gordeev
0 siblings, 0 replies; 4+ messages in thread
From: Alexander Gordeev @ 2022-07-07 5:21 UTC (permalink / raw)
To: Alexander Egorenkov
Cc: Heiko Carstens, Vasily Gorbik, Matthew Wilcox, Baoquan He,
Christoph Hellwig, linux-kernel, linux-s390
On Wed, Jul 06, 2022 at 10:18:15AM +0200, Alexander Egorenkov wrote:
> > + __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 ?
It certainly better. I'll post the fixed version.
> ({ 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 ?
I think that is fine. It is consistent with other iterate_and_advance()
uses and actually converted by read_from_oldmem() to -EFAULT in case of
incomplete copy.
Thank you!
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-07-07 5:22 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-06 5:59 [PATCH 0/1] s390/crash: allow multi-segment iterators Alexander Gordeev
2022-07-06 5:59 ` [PATCH 1/1] " Alexander Gordeev
2022-07-06 8:18 ` Alexander Egorenkov
2022-07-07 5:21 ` Alexander Gordeev
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.