diff mbox series

[v4,1/9] iov_iter: Handle compound highmem pages in copy_page_from_iter_atomic()

Message ID 20230710130253.3484695-2-willy@infradead.org (mailing list archive)
State Deferred, archived
Headers show
Series Create large folios in iomap buffered write path | expand

Commit Message

Matthew Wilcox July 10, 2023, 1:02 p.m. UTC
copy_page_from_iter_atomic() already handles !highmem compound
pages correctly, but if we are passed a highmem compound page,
each base page needs to be mapped & unmapped individually.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 lib/iov_iter.c | 43 ++++++++++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 15 deletions(-)

Comments

Luis Chamberlain July 10, 2023, 11:43 p.m. UTC | #1
On Mon, Jul 10, 2023 at 02:02:45PM +0100, Matthew Wilcox (Oracle) wrote:
> copy_page_from_iter_atomic() already handles !highmem compound
> pages correctly, but if we are passed a highmem compound page,
> each base page needs to be mapped & unmapped individually.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  lib/iov_iter.c | 43 ++++++++++++++++++++++++++++---------------
>  1 file changed, 28 insertions(+), 15 deletions(-)
> 
> diff --git a/lib/iov_iter.c b/lib/iov_iter.c
> index b667b1e2f688..c728b6e4fb18 100644
> --- a/lib/iov_iter.c
> +++ b/lib/iov_iter.c
> @@ -566,24 +566,37 @@ size_t iov_iter_zero(size_t bytes, struct iov_iter *i)
>  }
>  EXPORT_SYMBOL(iov_iter_zero);
>  
> -size_t copy_page_from_iter_atomic(struct page *page, unsigned offset, size_t bytes,
> -				  struct iov_iter *i)
> +size_t copy_page_from_iter_atomic(struct page *page, unsigned offset,
> +		size_t bytes, struct iov_iter *i)
>  {
> -	char *kaddr = kmap_atomic(page), *p = kaddr + offset;
> -	if (!page_copy_sane(page, offset, bytes)) {
> -		kunmap_atomic(kaddr);
> +	size_t n, copied = 0;
> +
> +	if (!page_copy_sane(page, offset, bytes))
>  		return 0;
> -	}
> -	if (WARN_ON_ONCE(!i->data_source)) {
> -		kunmap_atomic(kaddr);
> +	if (WARN_ON_ONCE(!i->data_source))
>  		return 0;

To make it easier to review the split of the kmap_atomic() until later
and the saving of the unwinding would be nice as separate patches.

> -	}
> -	iterate_and_advance(i, bytes, base, len, off,
> -		copyin(p + off, base, len),
> -		memcpy_from_iter(i, p + off, base, len)
> -	)
> -	kunmap_atomic(kaddr);
> -	return bytes;
> +
> +	do {
> +		char *p;
> +
> +		n = bytes - copied;
> +		if (PageHighMem(page)) {
> +			page += offset / PAGE_SIZE;

I don't quite follow here how before the page was not modified
to get to the first kmap_atomic(page) and now immediately if we're
on a PageHighMem(page) we're doing some arithmetic to the page
address to get the first kmap_atomic(). The only thing I could
think of is seems like an implicit assumption here that if its a compound
highmem page then we always start off with offset with a value of
0, is that right? But that seems to not be correct either.

  Luis
Matthew Wilcox July 11, 2023, 12:03 a.m. UTC | #2
On Mon, Jul 10, 2023 at 04:43:35PM -0700, Luis Chamberlain wrote:
> > -	}
> > -	iterate_and_advance(i, bytes, base, len, off,
> > -		copyin(p + off, base, len),
> > -		memcpy_from_iter(i, p + off, base, len)
> > -	)
> > -	kunmap_atomic(kaddr);
> > -	return bytes;
> > +
> > +	do {
> > +		char *p;
> > +
> > +		n = bytes - copied;
> > +		if (PageHighMem(page)) {
> > +			page += offset / PAGE_SIZE;
> 
> I don't quite follow here how before the page was not modified
> to get to the first kmap_atomic(page) and now immediately if we're
> on a PageHighMem(page) we're doing some arithmetic to the page
> address to get the first kmap_atomic(). The only thing I could
> think of is seems like an implicit assumption here that if its a compound
> highmem page then we always start off with offset with a value of
> 0, is that right? But that seems to not be correct either.

The important thing to know is that it was buggy before.  If you called
copy_page_from_iter_atomic() with an offset larger than PAGE_SIZE, it
corrupted random memory!  I can only assume that nobody was doing that.
Christoph Hellwig July 11, 2023, 6:30 a.m. UTC | #3
On Mon, Jul 10, 2023 at 02:02:45PM +0100, Matthew Wilcox (Oracle) wrote:
> +size_t copy_page_from_iter_atomic(struct page *page, unsigned offset,
> +		size_t bytes, struct iov_iter *i)
>  {
> -	char *kaddr = kmap_atomic(page), *p = kaddr + offset;
> -	if (!page_copy_sane(page, offset, bytes)) {
> -		kunmap_atomic(kaddr);
> +	size_t n, copied = 0;
> +
> +	if (!page_copy_sane(page, offset, bytes))
>  		return 0;
> -	}
> -	if (WARN_ON_ONCE(!i->data_source)) {
> -		kunmap_atomic(kaddr);
> +	if (WARN_ON_ONCE(!i->data_source))
>  		return 0;
> -	iterate_and_advance(i, bytes, base, len, off,
> -		copyin(p + off, base, len),
> -		memcpy_from_iter(i, p + off, base, len)
> -	)
> -	kunmap_atomic(kaddr);

I have to agree with Luis that just moving the odd early kmap in
the existing code is probably worth it's own patch just to keep
the thing readable.  I'm not going to ask for a resend just because
of that, but if we need a respin it would be nice to split it out.

> +
> +	do {
> +		char *p;
> +
> +		n = bytes - copied;
> +		if (PageHighMem(page)) {
> +			page += offset / PAGE_SIZE;
> +			offset %= PAGE_SIZE;
> +			n = min_t(size_t, n, PAGE_SIZE - offset);
> +		}
> +
> +		p = kmap_atomic(page) + offset;
> +		iterate_and_advance(i, n, base, len, off,
> +			copyin(p + off, base, len),
> +			memcpy_from_iter(i, p + off, base, len)
> +		)
> +		kunmap_atomic(p);
> +		copied += n;
> +		offset += n;
> +	} while (PageHighMem(page) && copied != bytes && n > 0);

This first read a little odd to me, but doing arithmetics on the page
should never move it from highmem to non-highmem so I think it's fine.
Would be a lot more readable if it was passed a folio, but I guess we
can do that later.

Otherwise looks good to me:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Kent Overstreet July 11, 2023, 8:05 p.m. UTC | #4
On Mon, Jul 10, 2023 at 02:02:45PM +0100, Matthew Wilcox (Oracle) wrote:
> copy_page_from_iter_atomic() already handles !highmem compound
> pages correctly, but if we are passed a highmem compound page,
> each base page needs to be mapped & unmapped individually.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Reviewed-by: Kent Overstreet <kent.overstreet@linux.dev>

I just pulled this into the bcachefs tree; in a week or two if you
haven't heard anything you can add my Tested-by:
Darrick J. Wong July 13, 2023, 4:42 a.m. UTC | #5
On Mon, Jul 10, 2023 at 02:02:45PM +0100, Matthew Wilcox (Oracle) wrote:
> copy_page_from_iter_atomic() already handles !highmem compound
> pages correctly, but if we are passed a highmem compound page,
> each base page needs to be mapped & unmapped individually.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Yikes.  Does this want a fixes tag?

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  lib/iov_iter.c | 43 ++++++++++++++++++++++++++++---------------
>  1 file changed, 28 insertions(+), 15 deletions(-)
> 
> diff --git a/lib/iov_iter.c b/lib/iov_iter.c
> index b667b1e2f688..c728b6e4fb18 100644
> --- a/lib/iov_iter.c
> +++ b/lib/iov_iter.c
> @@ -566,24 +566,37 @@ size_t iov_iter_zero(size_t bytes, struct iov_iter *i)
>  }
>  EXPORT_SYMBOL(iov_iter_zero);
>  
> -size_t copy_page_from_iter_atomic(struct page *page, unsigned offset, size_t bytes,
> -				  struct iov_iter *i)
> +size_t copy_page_from_iter_atomic(struct page *page, unsigned offset,
> +		size_t bytes, struct iov_iter *i)
>  {
> -	char *kaddr = kmap_atomic(page), *p = kaddr + offset;
> -	if (!page_copy_sane(page, offset, bytes)) {
> -		kunmap_atomic(kaddr);
> +	size_t n, copied = 0;
> +
> +	if (!page_copy_sane(page, offset, bytes))
>  		return 0;
> -	}
> -	if (WARN_ON_ONCE(!i->data_source)) {
> -		kunmap_atomic(kaddr);
> +	if (WARN_ON_ONCE(!i->data_source))
>  		return 0;
> -	}
> -	iterate_and_advance(i, bytes, base, len, off,
> -		copyin(p + off, base, len),
> -		memcpy_from_iter(i, p + off, base, len)
> -	)
> -	kunmap_atomic(kaddr);
> -	return bytes;
> +
> +	do {
> +		char *p;
> +
> +		n = bytes - copied;
> +		if (PageHighMem(page)) {
> +			page += offset / PAGE_SIZE;
> +			offset %= PAGE_SIZE;
> +			n = min_t(size_t, n, PAGE_SIZE - offset);
> +		}
> +
> +		p = kmap_atomic(page) + offset;
> +		iterate_and_advance(i, n, base, len, off,
> +			copyin(p + off, base, len),
> +			memcpy_from_iter(i, p + off, base, len)
> +		)
> +		kunmap_atomic(p);
> +		copied += n;
> +		offset += n;
> +	} while (PageHighMem(page) && copied != bytes && n > 0);
> +
> +	return copied;
>  }
>  EXPORT_SYMBOL(copy_page_from_iter_atomic);
>  
> -- 
> 2.39.2
>
Matthew Wilcox July 13, 2023, 1:46 p.m. UTC | #6
On Wed, Jul 12, 2023 at 09:42:07PM -0700, Darrick J. Wong wrote:
> On Mon, Jul 10, 2023 at 02:02:45PM +0100, Matthew Wilcox (Oracle) wrote:
> > copy_page_from_iter_atomic() already handles !highmem compound
> > pages correctly, but if we are passed a highmem compound page,
> > each base page needs to be mapped & unmapped individually.
> > 
> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> 
> Yikes.  Does this want a fixes tag?

I was wondering the same thing.  I think the bug is merely latent,
and all existing users limit themselves to being within the page
that they're passing in.
diff mbox series

Patch

diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index b667b1e2f688..c728b6e4fb18 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -566,24 +566,37 @@  size_t iov_iter_zero(size_t bytes, struct iov_iter *i)
 }
 EXPORT_SYMBOL(iov_iter_zero);
 
-size_t copy_page_from_iter_atomic(struct page *page, unsigned offset, size_t bytes,
-				  struct iov_iter *i)
+size_t copy_page_from_iter_atomic(struct page *page, unsigned offset,
+		size_t bytes, struct iov_iter *i)
 {
-	char *kaddr = kmap_atomic(page), *p = kaddr + offset;
-	if (!page_copy_sane(page, offset, bytes)) {
-		kunmap_atomic(kaddr);
+	size_t n, copied = 0;
+
+	if (!page_copy_sane(page, offset, bytes))
 		return 0;
-	}
-	if (WARN_ON_ONCE(!i->data_source)) {
-		kunmap_atomic(kaddr);
+	if (WARN_ON_ONCE(!i->data_source))
 		return 0;
-	}
-	iterate_and_advance(i, bytes, base, len, off,
-		copyin(p + off, base, len),
-		memcpy_from_iter(i, p + off, base, len)
-	)
-	kunmap_atomic(kaddr);
-	return bytes;
+
+	do {
+		char *p;
+
+		n = bytes - copied;
+		if (PageHighMem(page)) {
+			page += offset / PAGE_SIZE;
+			offset %= PAGE_SIZE;
+			n = min_t(size_t, n, PAGE_SIZE - offset);
+		}
+
+		p = kmap_atomic(page) + offset;
+		iterate_and_advance(i, n, base, len, off,
+			copyin(p + off, base, len),
+			memcpy_from_iter(i, p + off, base, len)
+		)
+		kunmap_atomic(p);
+		copied += n;
+		offset += n;
+	} while (PageHighMem(page) && copied != bytes && n > 0);
+
+	return copied;
 }
 EXPORT_SYMBOL(copy_page_from_iter_atomic);