Re: [RFC PATCH] crypto: Make the page handling of hash walk compatible to networking.

[Steffen Klassert]

On Mon, Apr 25, 2016 at 06:05:27PM +0800, Herbert Xu wrote:
> On Thu, Apr 21, 2016 at 09:14:51AM +0200, Steffen Klassert wrote:
> > The network layer tries to allocate high order pages for skb_buff
> > fragments, this leads to problems if we pass such a buffer to
> > crypto because crypto assumes to have always order null pages
> > in the scatterlists.
> 
> I don't understand.  AFAICS the crypto API assumes no such thing.
> Of course there might be a bug there since we probably don't get
> too many superpages coming in normally.

Maybe I misinterpreted the things I observed.

> 
> > Herbert, I could not find out why this PAGE_SIZE limit is in place.
> > So not sure if this is the right fix. Also, would it be ok to merge
> > this, or whatever is the right fix through the IPsec tree? We need
> > this before we can change esp to avoid linearization.
> 
> Your patch makes no sense.  

That's possible :)

> When you do a kmap you can only do
> one page at a time.  So if you have a "superpage" (an SG list
> entry with multiple contiguous pages) then you must walk it one
> page at a time.
> 
> That's why we cap it at PAGE_SIZE.
> 
> Is it not walking the superpage properly?

The problem was that if offset (in a superpage) equals
PAGE_SIZE in hash_walk_next(), nbytes becomes zero. So
we map the page, but we don't hash and unmap because we
exit the loop in shash_ahash_update() in this case.

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] crypto: Make the page handling of hash walk compatible to networking.

[Herbert Xu]

On Thu, Apr 21, 2016 at 09:14:51AM +0200, Steffen Klassert wrote:
> The network layer tries to allocate high order pages for skb_buff
> fragments, this leads to problems if we pass such a buffer to
> crypto because crypto assumes to have always order null pages
> in the scatterlists.

I don't understand.  AFAICS the crypto API assumes no such thing.
Of course there might be a bug there since we probably don't get
too many superpages coming in normally.

> Herbert, I could not find out why this PAGE_SIZE limit is in place.
> So not sure if this is the right fix. Also, would it be ok to merge
> this, or whatever is the right fix through the IPsec tree? We need
> this before we can change esp to avoid linearization.

Your patch makes no sense.  When you do a kmap you can only do
one page at a time.  So if you have a "superpage" (an SG list
entry with multiple contiguous pages) then you must walk it one
page at a time.

That's why we cap it at PAGE_SIZE.

Is it not walking the superpage properly?

Cheers,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH] crypto: Make the page handling of hash walk compatible to networking.

[Steffen Klassert]

The network layer tries to allocate high order pages for skb_buff
fragments, this leads to problems if we pass such a buffer to
crypto because crypto assumes to have always order null pages
in the scatterlists.

This was not a problem so far, because the network stack linearized
all buffers before passing them to crypto. If we try to avoid the
linearization with skb_cow_data in IPsec esp4/esp6 this incompatibility
becomes visible.

Signed-off-by: Steffen Klassert 
---

Herbert, I could not find out why this PAGE_SIZE limit is in place.
So not sure if this is the right fix. Also, would it be ok to merge
this, or whatever is the right fix through the IPsec tree? We need
this before we can change esp to avoid linearization.

The full IPsec patchset can be found here:

https://git.kernel.org/cgit/linux/kernel/git/klassert/linux-stk.git/log/?h=net-next-ipsec-offload-work

 crypto/ahash.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/crypto/ahash.c b/crypto/ahash.c
index 5fc1f17..ca92783 100644
--- a/crypto/ahash.c
+++ b/crypto/ahash.c
@@ -44,8 +44,7 @@ static int hash_walk_next(struct crypto_hash_walk *walk)
 {
unsigned int alignmask = walk->alignmask;
unsigned int offset = walk->offset;
-   unsigned int nbytes = min(walk->entrylen,
- ((unsigned int)(PAGE_SIZE)) - offset);
+   unsigned int nbytes = walk->entrylen;
 
if (walk->flags & CRYPTO_ALG_ASYNC)
walk->data = kmap(walk->pg);
@@ -91,8 +90,6 @@ int crypto_hash_walk_done(struct crypto_hash_walk *walk, int 
err)
walk->offset = ALIGN(walk->offset, alignmask + 1);
walk->data += walk->offset;
 
-   nbytes = min(nbytes,
-((unsigned int)(PAGE_SIZE)) - walk->offset);
walk->entrylen -= nbytes;
 
return nbytes;
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html