[PATCH] crypto: scatterwalk_copychunks() fails to advance through scatterlist

[PATCH] crypto: scatterwalk_copychunks() fails to advance through scatterlist

[J. Bruce Fields]

In the loop in scatterwalk_copychunks(), if walk->offset is zero,
then scatterwalk_pagedone rounds that up to the nearest page boundary:

		walk->offset += PAGE_SIZE - 1;
		walk->offset &= PAGE_MASK;

which is a no-op in this case, so we don't advance to the next element
of the scatterlist array:

		if (walk->offset >= walk->sg->offset + walk->sg->length)
			scatterwalk_start(walk, sg_next(walk->sg));

and we end up copying the same data twice.

It appears that other callers of scatterwalk_{page}done first advance
walk->offset, so I believe that's the correct thing to do here.

This caused a bug in NFS when run with krb5p security, which would
cause some writes to fail with permissions errors--for example, writes
of less than 8 bytes (the des blocksize) at the start of a file.

A git-bisect shows the bug was originally introduced by
5c64097aa0f6dc4f27718ef47ca9a12538d62860, first in 2.6.19-rc1.

Signed-off-by: "J. Bruce Fields" <bfields@citi.umich.edu>
---
 crypto/scatterwalk.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/crypto/scatterwalk.c b/crypto/scatterwalk.c
index 35172d3..a664231 100644
--- a/crypto/scatterwalk.c
+++ b/crypto/scatterwalk.c
@@ -91,6 +91,8 @@ void scatterwalk_copychunks(void *buf, struct scatter_walk *walk,
 		memcpy_dir(buf, vaddr, len_this_page, out);
 		scatterwalk_unmap(vaddr, out);
 
+		scatterwalk_advance(walk, nbytes);
+
 		if (nbytes == len_this_page)
 			break;
 
@@ -99,7 +101,5 @@ void scatterwalk_copychunks(void *buf, struct scatter_walk *walk,
 
 		scatterwalk_pagedone(walk, out, 1);
 	}
-
-	scatterwalk_advance(walk, nbytes);
 }
 EXPORT_SYMBOL_GPL(scatterwalk_copychunks);
-- 
1.5.0.3.31.ge47c

Re: [PATCH] crypto: scatterwalk_copychunks() fails to advance through scatterlist

[Herbert Xu]

On Mon, Mar 19, 2007 at 07:55:08PM -0400, J. Bruce Fields wrote:
> In the loop in scatterwalk_copychunks(), if walk->offset is zero,
> then scatterwalk_pagedone rounds that up to the nearest page boundary:
> 
> 		walk->offset += PAGE_SIZE - 1;
> 		walk->offset &= PAGE_MASK;
> 
> which is a no-op in this case, so we don't advance to the next element
> of the scatterlist array:
> 
> 		if (walk->offset >= walk->sg->offset + walk->sg->length)
> 			scatterwalk_start(walk, sg_next(walk->sg));
> 
> and we end up copying the same data twice.

Thanks for the patch.  However I still have a question as to why
this is happening.

As far as I can see scatterwalk_copychunks is only called in two
places.  In both spots it only processes one block of data.  Since
we set the maximum block size to PAGE_SIZE/8 I don't see how you
can get an offset of zero and still roll over to the next page
in scatterwalk_copychunks.

So perhaps the bug is elsewhere in the original patch?

Your patch looks like a good clean-up anyway but I just wanted to
make sure that I understand what the problem is first.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: scatterwalk_copychunks() fails to advance through scatterlist

[J. Bruce Fields]

On Tue, Mar 20, 2007 at 04:16:56PM +1100, Herbert Xu wrote:
> Thanks for the patch.  However I still have a question as to why
> this is happening.
> 
> As far as I can see scatterwalk_copychunks is only called in two
> places.  In both spots it only processes one block of data.  Since
> we set the maximum block size to PAGE_SIZE/8 I don't see how you
> can get an offset of zero and still roll over to the next page
> in scatterwalk_copychunks.

Are the elements of the scatterlists assumed to always be full pages?  I
need to encrypt things that look like, for example:

	sg[0].page = page1
	sg[0].offset = 0
	sg[0].length = 5
	sg[1].page = page2
	sg[1].offset = 0
	sg[1].length = 37

and worse....  I could do this by hand if I had to, but the crypto code,
if it's not designed to handle this sort of thing, seems very close, so
I'd rather enhance it than duplicate a lot of this complicated
scatterlist-traversal stuff.

--b.

Re: [PATCH] crypto: scatterwalk_copychunks() fails to advance through scatterlist

[J. Bruce Fields]

On Tue, Mar 20, 2007 at 10:16:14AM -0400, J. Bruce Fields wrote:
> On Tue, Mar 20, 2007 at 04:16:56PM +1100, Herbert Xu wrote:
> > Thanks for the patch.  However I still have a question as to why
> > this is happening.
> > 
> > As far as I can see scatterwalk_copychunks is only called in two
> > places.  In both spots it only processes one block of data.  Since
> > we set the maximum block size to PAGE_SIZE/8 I don't see how you
> > can get an offset of zero and still roll over to the next page
> > in scatterwalk_copychunks.
> 
> Are the elements of the scatterlists assumed to always be full pages?  I
> need to encrypt things that look like, for example:
> 
> 	sg[0].page = page1
> 	sg[0].offset = 0
> 	sg[0].length = 5
> 	sg[1].page = page2
> 	sg[1].offset = 0
> 	sg[1].length = 37

(Erm, second should be 27--I did mean this to come out to a multiple of
8....)--b.

> 
> and worse....  I could do this by hand if I had to, but the crypto code,
> if it's not designed to handle this sort of thing, seems very close, so
> I'd rather enhance it than duplicate a lot of this complicated
> scatterlist-traversal stuff.
> 
> --b.
> _______________________________________________
> NFSv4 mailing list
> NFSv4@linux-nfs.org
> http://linux-nfs.org/cgi-bin/mailman/listinfo/nfsv4

Re: [PATCH] crypto: scatterwalk_copychunks() fails to advance through scatterlist

[Herbert Xu]

On Tue, Mar 20, 2007 at 10:16:14AM -0400, J. Bruce Fields wrote:
> 
> Are the elements of the scatterlists assumed to always be full pages?  I

Definitely not.

> need to encrypt things that look like, for example:
> 
> 	sg[0].page = page1
> 	sg[0].offset = 0
> 	sg[0].length = 5
> 	sg[1].page = page2
> 	sg[1].offset = 0
> 	sg[1].length = 37

OK I see what you mean now.  I'll apply your patch.

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: scatterwalk_copychunks() fails to advance through scatterlist

[J. Bruce Fields]

On Wed, Mar 21, 2007 at 07:54:21AM +1100, Herbert Xu wrote:
> On Tue, Mar 20, 2007 at 10:16:14AM -0400, J. Bruce Fields wrote:
> > 
> > Are the elements of the scatterlists assumed to always be full pages?  I
> 
> Definitely not.
> 
> > need to encrypt things that look like, for example:
> > 
> > 	sg[0].page = page1
> > 	sg[0].offset = 0
> > 	sg[0].length = 5
> > 	sg[1].page = page2
> > 	sg[1].offset = 0
> > 	sg[1].length = 37
> 
> OK I see what you mean now.  I'll apply your patch.

Thanks!

By the way, the commit that I believe introduced this regression did two
or three different things at once--I spent some time staring at it and
can't say I really understand the change.  That's probably just me!  But
would it be possible to split up these patches fine enough that people
inexperienced with this code would have a better chance of understanding
what's going on?

There's so many potential corner cases to handle here, I know I'd want
to tread very carefully....

(And did the crypto testing module use to have some tests for buffers
that were fragmented in odd ways, or was I imagining that?  I'd be happy
to help come up with some test cases if it'd be useful.)

--b.

Re: [PATCH] crypto: scatterwalk_copychunks() fails to advance through scatterlist

[Herbert Xu]

On Tue, Mar 20, 2007 at 05:17:51PM -0400, J. Bruce Fields wrote:
> 
> By the way, the commit that I believe introduced this regression did two
> or three different things at once--I spent some time staring at it and
> can't say I really understand the change.  That's probably just me!  But
> would it be possible to split up these patches fine enough that people
> inexperienced with this code would have a better chance of understanding
> what's going on?

Good point, I'll make sure they're more granular in future.

> (And did the crypto testing module use to have some tests for buffers
> that were fragmented in odd ways, or was I imagining that?  I'd be happy
> to help come up with some test cases if it'd be useful.)

It does test fragments but it doesn't do it in the way you
suggested.  In particular, the fragments probably won't be
starting off at a page boundary.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt