diff mbox series

[v4,1/4] lib/scatterlist: Fix mapping iterator when sg->offset is greater than PAGE_SIZE

Message ID f28c6b0e2f9510f42ca934f19c4315084e668c21.1560805614.git.christophe.leroy@c-s.fr (mailing list archive)
State Changes Requested
Delegated to: Herbert Xu
Headers show
Series Additional fixes on Talitos driver | expand

Commit Message

Christophe Leroy June 17, 2019, 9:15 p.m. UTC
All mapping iterator logic is based on the assumption that sg->offset
is always lower than PAGE_SIZE.

But there are situations where sg->offset is such that the SG item
is on the second page. In that case sg_copy_to_buffer() fails
properly copying the data into the buffer. One of the reason is
that the data will be outside the kmapped area used to access that
data.

This patch fixes the issue by adjusting the mapping iterator
offset and pgoffset fields such that offset is always lower than
PAGE_SIZE.

Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
Fixes: 4225fc8555a9 ("lib/scatterlist: use page iterator in the mapping iterator")
Cc: stable@vger.kernel.org
---
 lib/scatterlist.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Herbert Xu June 20, 2019, 6:02 a.m. UTC | #1
On Mon, Jun 17, 2019 at 09:15:02PM +0000, Christophe Leroy wrote:
> All mapping iterator logic is based on the assumption that sg->offset
> is always lower than PAGE_SIZE.
> 
> But there are situations where sg->offset is such that the SG item
> is on the second page. In that case sg_copy_to_buffer() fails
> properly copying the data into the buffer. One of the reason is
> that the data will be outside the kmapped area used to access that
> data.
> 
> This patch fixes the issue by adjusting the mapping iterator
> offset and pgoffset fields such that offset is always lower than
> PAGE_SIZE.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> Fixes: 4225fc8555a9 ("lib/scatterlist: use page iterator in the mapping iterator")
> Cc: stable@vger.kernel.org
> ---
>  lib/scatterlist.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)

Good catch.

> @@ -686,7 +686,12 @@ static bool sg_miter_get_next_page(struct sg_mapping_iter *miter)
>  		sg = miter->piter.sg;
>  		pgoffset = miter->piter.sg_pgoffset;
>  
> -		miter->__offset = pgoffset ? 0 : sg->offset;
> +		offset = pgoffset ? 0 : sg->offset;
> +		while (offset >= PAGE_SIZE) {
> +			miter->piter.sg_pgoffset = ++pgoffset;
> +			offset -= PAGE_SIZE;
> +		}

How about

	miter->piter.sg_pgoffset += offset >> PAGE_SHIFT;
	offset &= PAGE_SIZE - 1;

Thanks,
Imre Deak June 24, 2019, 5:35 p.m. UTC | #2
Hi,

On Thu, Jun 20, 2019 at 02:02:21PM +0800, Herbert Xu wrote:
> On Mon, Jun 17, 2019 at 09:15:02PM +0000, Christophe Leroy wrote:
> > All mapping iterator logic is based on the assumption that sg->offset
> > is always lower than PAGE_SIZE.
> > 
> > But there are situations where sg->offset is such that the SG item
> > is on the second page.

could you explain how sg->offset becomes >= PAGE_SIZE?

--Imre


> > In that case sg_copy_to_buffer() fails
> > properly copying the data into the buffer. One of the reason is
> > that the data will be outside the kmapped area used to access that
> > data.
> > 
> > This patch fixes the issue by adjusting the mapping iterator
> > offset and pgoffset fields such that offset is always lower than
> > PAGE_SIZE.
> > 
> > Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> > Fixes: 4225fc8555a9 ("lib/scatterlist: use page iterator in the mapping iterator")
> > Cc: stable@vger.kernel.org
> > ---
> >  lib/scatterlist.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> Good catch.
> 
> > @@ -686,7 +686,12 @@ static bool sg_miter_get_next_page(struct sg_mapping_iter *miter)
> >  		sg = miter->piter.sg;
> >  		pgoffset = miter->piter.sg_pgoffset;
> >  
> > -		miter->__offset = pgoffset ? 0 : sg->offset;
> > +		offset = pgoffset ? 0 : sg->offset;
> > +		while (offset >= PAGE_SIZE) {
> > +			miter->piter.sg_pgoffset = ++pgoffset;
> > +			offset -= PAGE_SIZE;
> > +		}
> 
> How about
> 
> 	miter->piter.sg_pgoffset += offset >> PAGE_SHIFT;
> 	offset &= PAGE_SIZE - 1;
> 
> Thanks,
> -- 
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Herbert Xu June 25, 2019, 1:20 a.m. UTC | #3
On Mon, Jun 24, 2019 at 08:35:33PM +0300, Imre Deak wrote:
> Hi,
> 
> On Thu, Jun 20, 2019 at 02:02:21PM +0800, Herbert Xu wrote:
> > On Mon, Jun 17, 2019 at 09:15:02PM +0000, Christophe Leroy wrote:
> > > All mapping iterator logic is based on the assumption that sg->offset
> > > is always lower than PAGE_SIZE.
> > > 
> > > But there are situations where sg->offset is such that the SG item
> > > is on the second page.
> 
> could you explain how sg->offset becomes >= PAGE_SIZE?

The network stack can produce SG list elements that are longer
than PAGE_SIZE.  If you the iterate over it one page at a time
then the offset can exceed PAGE_SIZE.

Cheers,
diff mbox series

Patch

diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index 739dc9fe2c55..39f00659898f 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -678,7 +678,7 @@  static bool sg_miter_get_next_page(struct sg_mapping_iter *miter)
 {
 	if (!miter->__remaining) {
 		struct scatterlist *sg;
-		unsigned long pgoffset;
+		unsigned long pgoffset, offset;
 
 		if (!__sg_page_iter_next(&miter->piter))
 			return false;
@@ -686,7 +686,12 @@  static bool sg_miter_get_next_page(struct sg_mapping_iter *miter)
 		sg = miter->piter.sg;
 		pgoffset = miter->piter.sg_pgoffset;
 
-		miter->__offset = pgoffset ? 0 : sg->offset;
+		offset = pgoffset ? 0 : sg->offset;
+		while (offset >= PAGE_SIZE) {
+			miter->piter.sg_pgoffset = ++pgoffset;
+			offset -= PAGE_SIZE;
+		}
+		miter->__offset = offset;
 		miter->__remaining = sg->offset + sg->length -
 				(pgoffset << PAGE_SHIFT) - miter->__offset;
 		miter->__remaining = min_t(unsigned long, miter->__remaining,