* [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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox