| Message ID | 20160421071451.GE3347@gauss.secunet.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Herbert Xu |
| Headers | show |
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,
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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 <steffen.klassert@secunet.com> --- 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(-)