[PATCH]: Fix networking scatterlist regressions.

[PATCH]: Fix networking scatterlist regressions.

[David Miller]

I just checked the following bug fix into net-2.6

Rusty, have a quick look at virtio_net wrt. the changes I
made to skb_to_sgvec()'s behavior.  I think I might have
even fixed something :-)

Jens, please review my commentary wrt. sg_mark_end() and
it's nonintuitive behavior which led to these bugs.

Thanks.

[NET]: Fix incorrect sg_mark_end() calls.

This fixes scatterlist corruptions added by

	commit 68e3f5dd4db62619fdbe520d36c9ebf62e672256
	[CRYPTO] users: Fix up scatterlist conversion errors

The issue is that the code calls sg_mark_end() which clobbers the
sg_page() pointer of the final scatterlist entry.

The first part fo the fix makes skb_to_sgvec() do __sg_mark_end().

After considering all skb_to_sgvec() call sites the most correct
solution is to call __sg_mark_end() in skb_to_sgvec() since that is
what all of the callers would end up doing anyways.

I suspect this might have fixed some problems in virtio_net which is
the sole non-crypto user of skb_to_sgvec().

Other similar sg_mark_end() cases were converted over to
__sg_mark_end() as well.

Arguably sg_mark_end() is a poorly named function because it doesn't
just "mark", it clears out the page pointer as a side effect, which is
what led to these bugs in the first place.

The one remaining plain sg_mark_end() call is in scsi_alloc_sgtable()
and arguably it could be converted to __sg_mark_end() if only so that
we can delete this confusing interface from linux/scatterlist.h

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 net/core/skbuff.c                     |   16 +++++++++++++---
 net/ipv4/esp4.c                       |   12 +++++++-----
 net/ipv4/tcp_ipv4.c                   |    2 +-
 net/ipv6/esp6.c                       |   13 +++++++------
 net/ipv6/tcp_ipv6.c                   |    2 +-
 net/rxrpc/rxkad.c                     |    9 +++++----
 net/sunrpc/auth_gss/gss_krb5_crypto.c |    6 +++---
 7 files changed, 37 insertions(+), 23 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 573e172..64b50ff 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2028,8 +2028,8 @@ void __init skb_init(void)
  *	Fill the specified scatter-gather list with mappings/pointers into a
  *	region of the buffer space attached to a socket buffer.
  */
-int
-skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
+static int
+__skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
 {
 	int start = skb_headlen(skb);
 	int i, copy = start - offset;
@@ -2078,7 +2078,8 @@ skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
 			if ((copy = end - offset) > 0) {
 				if (copy > len)
 					copy = len;
-				elt += skb_to_sgvec(list, sg+elt, offset - start, copy);
+				elt += __skb_to_sgvec(list, sg+elt, offset - start,
+						      copy);
 				if ((len -= copy) == 0)
 					return elt;
 				offset += copy;
@@ -2090,6 +2091,15 @@ skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
 	return elt;
 }
 
+int skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
+{
+	int nsg = __skb_to_sgvec(skb, sg, offset, len);
+
+	__sg_mark_end(&sg[nsg - 1]);
+
+	return nsg;
+}
+
 /**
  *	skb_cow_data - Check that a socket buffer's data buffers are writable
  *	@skb: The socket buffer to check.
diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c
index cad4278..c31bccb 100644
--- a/net/ipv4/esp4.c
+++ b/net/ipv4/esp4.c
@@ -111,9 +111,10 @@ static int esp_output(struct xfrm_state *x, struct sk_buff *skb)
 				goto unlock;
 		}
 		sg_init_table(sg, nfrags);
-		sg_mark_end(sg, skb_to_sgvec(skb, sg, esph->enc_data +
-						      esp->conf.ivlen -
-						      skb->data, clen));
+		skb_to_sgvec(skb, sg,
+			     esph->enc_data +
+			     esp->conf.ivlen -
+			     skb->data, clen);
 		err = crypto_blkcipher_encrypt(&desc, sg, sg, clen);
 		if (unlikely(sg != &esp->sgbuf[0]))
 			kfree(sg);
@@ -205,8 +206,9 @@ static int esp_input(struct xfrm_state *x, struct sk_buff *skb)
 			goto out;
 	}
 	sg_init_table(sg, nfrags);
-	sg_mark_end(sg, skb_to_sgvec(skb, sg, sizeof(*esph) + esp->conf.ivlen,
-				     elen));
+	skb_to_sgvec(skb, sg,
+		     sizeof(*esph) + esp->conf.ivlen,
+		     elen);
 	err = crypto_blkcipher_decrypt(&desc, sg, sg, elen);
 	if (unlikely(sg != &esp->sgbuf[0]))
 		kfree(sg);
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index d3d8d5d..eec02b2 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1083,7 +1083,7 @@ static int tcp_v4_do_calc_md5_hash(char *md5_hash, struct tcp_md5sig_key *key,
 	sg_set_buf(&sg[block++], key->key, key->keylen);
 	nbytes += key->keylen;
 
-	sg_mark_end(sg, block);
+	__sg_mark_end(&sg[block - 1]);
 
 	/* Now store the Hash into the packet */
 	err = crypto_hash_init(desc);
diff --git a/net/ipv6/esp6.c b/net/ipv6/esp6.c
index ab17b5e..7db66f1 100644
--- a/net/ipv6/esp6.c
+++ b/net/ipv6/esp6.c
@@ -110,9 +110,10 @@ static int esp6_output(struct xfrm_state *x, struct sk_buff *skb)
 				goto unlock;
 		}
 		sg_init_table(sg, nfrags);
-		sg_mark_end(sg, skb_to_sgvec(skb, sg, esph->enc_data +
-						      esp->conf.ivlen -
-						      skb->data, clen));
+		skb_to_sgvec(skb, sg,
+			     esph->enc_data +
+			     esp->conf.ivlen -
+			     skb->data, clen);
 		err = crypto_blkcipher_encrypt(&desc, sg, sg, clen);
 		if (unlikely(sg != &esp->sgbuf[0]))
 			kfree(sg);
@@ -209,9 +210,9 @@ static int esp6_input(struct xfrm_state *x, struct sk_buff *skb)
 			}
 		}
 		sg_init_table(sg, nfrags);
-		sg_mark_end(sg, skb_to_sgvec(skb, sg,
-					     sizeof(*esph) + esp->conf.ivlen,
-					     elen));
+		skb_to_sgvec(skb, sg,
+			     sizeof(*esph) + esp->conf.ivlen,
+			     elen);
 		ret = crypto_blkcipher_decrypt(&desc, sg, sg, elen);
 		if (unlikely(sg != &esp->sgbuf[0]))
 			kfree(sg);
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index f1523b8..4b90328 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -781,7 +781,7 @@ static int tcp_v6_do_calc_md5_hash(char *md5_hash, struct tcp_md5sig_key *key,
 	sg_set_buf(&sg[block++], key->key, key->keylen);
 	nbytes += key->keylen;
 
-	sg_mark_end(sg, block);
+	__sg_mark_end(&sg[block - 1]);
 
 	/* Now store the hash into the packet */
 	err = crypto_hash_init(desc);
diff --git a/net/rxrpc/rxkad.c b/net/rxrpc/rxkad.c
index eebefb6..57458d4 100644
--- a/net/rxrpc/rxkad.c
+++ b/net/rxrpc/rxkad.c
@@ -237,7 +237,8 @@ static int rxkad_secure_packet_encrypt(const struct rxrpc_call *call,
 	len = data_size + call->conn->size_align - 1;
 	len &= ~(call->conn->size_align - 1);
 
-	sg_init_table(sg, skb_to_sgvec(skb, sg, 0, len));
+	sg_init_table(sg, nsg);
+	skb_to_sgvec(skb, sg, 0, len);
 	crypto_blkcipher_encrypt_iv(&desc, sg, sg, len);
 
 	_leave(" = 0");
@@ -344,7 +345,7 @@ static int rxkad_verify_packet_auth(const struct rxrpc_call *call,
 		goto nomem;
 
 	sg_init_table(sg, nsg);
-	sg_mark_end(sg, skb_to_sgvec(skb, sg, 0, 8));
+	skb_to_sgvec(skb, sg, 0, 8);
 
 	/* start the decryption afresh */
 	memset(&iv, 0, sizeof(iv));
@@ -426,7 +427,7 @@ static int rxkad_verify_packet_encrypt(const struct rxrpc_call *call,
 	}
 
 	sg_init_table(sg, nsg);
-	sg_mark_end(sg, skb_to_sgvec(skb, sg, 0, skb->len));
+	skb_to_sgvec(skb, sg, 0, skb->len);
 
 	/* decrypt from the session key */
 	payload = call->conn->key->payload.data;
@@ -701,7 +702,7 @@ static void rxkad_sg_set_buf2(struct scatterlist sg[2],
 		nsg++;
 	}
 
-	sg_mark_end(sg, nsg);
+	__sg_mark_end(sg, nsg);
 
 	ASSERTCMP(sg[0].length + sg[1].length, ==, buflen);
 }
diff --git a/net/sunrpc/auth_gss/gss_krb5_crypto.c b/net/sunrpc/auth_gss/gss_krb5_crypto.c
index 91cd8f0..4a8aa94 100644
--- a/net/sunrpc/auth_gss/gss_krb5_crypto.c
+++ b/net/sunrpc/auth_gss/gss_krb5_crypto.c
@@ -211,8 +211,8 @@ encryptor(struct scatterlist *sg, void *data)
 	if (thislen == 0)
 		return 0;
 
-	sg_mark_end(desc->infrags, desc->fragno);
-	sg_mark_end(desc->outfrags, desc->fragno);
+	__sg_mark_end(desc->infrags, desc->fragno);
+	__sg_mark_end(desc->outfrags, desc->fragno);
 
 	ret = crypto_blkcipher_encrypt_iv(&desc->desc, desc->outfrags,
 					  desc->infrags, thislen);
@@ -293,7 +293,7 @@ decryptor(struct scatterlist *sg, void *data)
 	if (thislen == 0)
 		return 0;
 
-	sg_mark_end(desc->frags, desc->fragno);
+	__sg_mark_end(desc->frags, desc->fragno);
 
 	ret = crypto_blkcipher_decrypt_iv(&desc->desc, desc->frags,
 					  desc->frags, thislen);
-- 
1.5.2.5

Re: [PATCH]: Fix networking scatterlist regressions.

[Rusty Russell]

On Wednesday 31 October 2007 14:40:02 David Miller wrote:
> I just checked the following bug fix into net-2.6
>
> Rusty, have a quick look at virtio_net wrt. the changes I
> made to skb_to_sgvec()'s behavior.  I think I might have
> even fixed something :-)

Grr, the scatterlist changes are really pissing me off.  We get ugly tricky 
code *and* transition pain.

I hate to suggest this, but I don't supposed there's a chance we can revert it 
for 2.6.24 and do something cleaner for 2.6.25?

Rusty.

Re: [PATCH]: Fix networking scatterlist regressions.

[David Miller]

From: Rusty Russell <rusty@rustcorp.com.au>
Date: Wed, 31 Oct 2007 17:06:32 +1100

> On Wednesday 31 October 2007 14:40:02 David Miller wrote:
> > I just checked the following bug fix into net-2.6
> >
> > Rusty, have a quick look at virtio_net wrt. the changes I
> > made to skb_to_sgvec()'s behavior.  I think I might have
> > even fixed something :-)
> 
> Grr, the scatterlist changes are really pissing me off.  We get ugly tricky 
> code *and* transition pain.
> 
> I hate to suggest this, but I don't supposed there's a chance we can revert it 
> for 2.6.24 and do something cleaner for 2.6.25?

I will hit you on the head with a hammer if this happens after all
the debugging and work I've put into fixing things so far. :-/

Just like this: Plonk!

Re: [PATCH]: Fix networking scatterlist regressions.

[Jens Axboe]

On Tue, Oct 30 2007, David Miller wrote:
> 
> I just checked the following bug fix into net-2.6
> 
> Rusty, have a quick look at virtio_net wrt. the changes I
> made to skb_to_sgvec()'s behavior.  I think I might have
> even fixed something :-)
> 
> Jens, please review my commentary wrt. sg_mark_end() and
> it's nonintuitive behavior which led to these bugs.

I fully agree, lets just change sg_mark_end() to NOT overwrite a stored
page there. The current interface isn't nice and can't be used after
filling the sg table, which is what users would want. I've added such a
patch to the sg repo.

>From 5a0347663f51850eb52b89c4dcf6a714ea8d3965 Mon Sep 17 00:00:00 2001
From: Jens Axboe <jens.axboe@oracle.com>
Date: Wed, 31 Oct 2007 08:31:23 +0100
Subject: [PATCH] [SG] Remove __sg_mark_end()

Make sg_mark_end() NOT overwrite the page link. Then it can be used
after filling the sg table, which is what users want. That means that
__sg_mark_end() is no longer useful, so kill it.

It's important the sg entries be initialized before using sg_mark_end(),
so also add a debug check to catch use-before-init.

Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
---
 block/ll_rw_blk.c           |    2 +-
 include/linux/scatterlist.h |   10 ++++------
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/block/ll_rw_blk.c b/block/ll_rw_blk.c
index e948407..fdc0707 100644
--- a/block/ll_rw_blk.c
+++ b/block/ll_rw_blk.c
@@ -1369,7 +1369,7 @@ new_segment:
 	} /* segments in rq */
 
 	if (sg)
-		__sg_mark_end(sg);
+		sg_mark_end(sg);
 
 	return nsegs;
 }
diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index d5e1876..aa97954 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -195,13 +195,11 @@ static inline void sg_chain(struct scatterlist *prv, unsigned int prv_nents,
  *   Marks the last entry as the termination point for sg_next()
  *
  **/
-static inline void sg_mark_end(struct scatterlist *sgl, unsigned int nents)
-{
-	sgl[nents - 1].page_link = 0x02;
-}
-
-static inline void __sg_mark_end(struct scatterlist *sg)
+static inline void sg_mark_end(struct scatterlist *sg)
 {
+#ifdef CONFIG_DEBUG_SG
+	BUG_ON(sg->sg_magic != SG_MAGIC);
+#endif
 	sg->page_link |= 0x02;
 }
 
-- 
1.5.3.GIT


-- 
Jens Axboe

Re: [PATCH]: Fix networking scatterlist regressions.

[David Miller]

From: Jens Axboe <jens.axboe@oracle.com>
Date: Wed, 31 Oct 2007 08:32:07 +0100

> [SG] Remove __sg_mark_end()
> 
> Make sg_mark_end() NOT overwrite the page link. Then it can be used
> after filling the sg table, which is what users want. That means that
> __sg_mark_end() is no longer useful, so kill it.
> 
> It's important the sg entries be initialized before using sg_mark_end(),
> so also add a debug check to catch use-before-init.
> 
> Signed-off-by: Jens Axboe <jens.axboe@oracle.com>

Ok, but I just pushed my changes to Linus and once those show up
you'll need to extend this patch to kill the '__' prefix from
all the rest of the calls which will be in the tree.

Thanks!

Re: [PATCH]: Fix networking scatterlist regressions.

[Jens Axboe]

On Tue, Oct 30 2007, David Miller wrote:
> diff --git a/net/sunrpc/auth_gss/gss_krb5_crypto.c b/net/sunrpc/auth_gss/gss_krb5_crypto.c
> index 91cd8f0..4a8aa94 100644
> --- a/net/sunrpc/auth_gss/gss_krb5_crypto.c
> +++ b/net/sunrpc/auth_gss/gss_krb5_crypto.c
> @@ -211,8 +211,8 @@ encryptor(struct scatterlist *sg, void *data)
>  	if (thislen == 0)
>  		return 0;
>  
> -	sg_mark_end(desc->infrags, desc->fragno);
> -	sg_mark_end(desc->outfrags, desc->fragno);
> +	__sg_mark_end(desc->infrags, desc->fragno);
> +	__sg_mark_end(desc->outfrags, desc->fragno);
>  
>  	ret = crypto_blkcipher_encrypt_iv(&desc->desc, desc->outfrags,
>  					  desc->infrags, thislen);
> @@ -293,7 +293,7 @@ decryptor(struct scatterlist *sg, void *data)
>  	if (thislen == 0)
>  		return 0;
>  
> -	sg_mark_end(desc->frags, desc->fragno);
> +	__sg_mark_end(desc->frags, desc->fragno);
>  
>  	ret = crypto_blkcipher_decrypt_iv(&desc->desc, desc->frags,
>  					  desc->frags, thislen);

Hmm? These don't seem right. It also has a weird code sequence:

...
        sg_mark_end(&desc->infrags[desc->fragno - 1]);
        sg_mark_end(&desc->outfrags[desc->fragno - 1]);

        ret = crypto_blkcipher_encrypt_iv(&desc->desc, desc->outfrags,
                                          desc->infrags, thislen);
        if (ret)
                return ret;

        sg_init_table(desc->infrags, 4);
        sg_init_table(desc->outfrags, 4);
...

Did something go wrong there?

-- 
Jens Axboe

Re: [PATCH]: Fix networking scatterlist regressions.

[Jens Axboe]

On Wed, Oct 31 2007, David Miller wrote:
> From: Jens Axboe <jens.axboe@oracle.com>
> Date: Wed, 31 Oct 2007 08:32:07 +0100
> 
> > [SG] Remove __sg_mark_end()
> > 
> > Make sg_mark_end() NOT overwrite the page link. Then it can be used
> > after filling the sg table, which is what users want. That means that
> > __sg_mark_end() is no longer useful, so kill it.
> > 
> > It's important the sg entries be initialized before using sg_mark_end(),
> > so also add a debug check to catch use-before-init.
> > 
> > Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
> 
> Ok, but I just pushed my changes to Linus and once those show up
> you'll need to extend this patch to kill the '__' prefix from
> all the rest of the calls which will be in the tree.
> 
> Thanks!

No problem, I'll base further sg_mark_end() updates on top of yours.
Just need to get that last email I sent out resolved, the
gss_krb5_crypto bits.

-- 
Jens Axboe

Re: [PATCH]: Fix networking scatterlist regressions.

[David Miller]

From: Jens Axboe <jens.axboe@oracle.com>
Date: Wed, 31 Oct 2007 08:46:21 +0100

> On Tue, Oct 30 2007, David Miller wrote:
> > @@ -293,7 +293,7 @@ decryptor(struct scatterlist *sg, void *data)
> >  	if (thislen == 0)
> >  		return 0;
> >  
> > -	sg_mark_end(desc->frags, desc->fragno);
> > +	__sg_mark_end(desc->frags, desc->fragno);
> >  
> >  	ret = crypto_blkcipher_decrypt_iv(&desc->desc, desc->frags,
> >  					  desc->frags, thislen);
> 
> Hmm? These don't seem right. It also has a weird code sequence:
 ...
> Did something go wrong there?

Yes, I fixed those up after doing some allmodconfig builds.

Here is the final patch I actually pushed to Linus:

>From 51c739d1f484b2562040a3e496dc8e1670d4e279 Mon Sep 17 00:00:00 2001
From: David S. Miller <davem@sunset.davemloft.net>
Date: Tue, 30 Oct 2007 21:29:29 -0700
Subject: [PATCH] [NET]: Fix incorrect sg_mark_end() calls.

This fixes scatterlist corruptions added by

	commit 68e3f5dd4db62619fdbe520d36c9ebf62e672256
	[CRYPTO] users: Fix up scatterlist conversion errors

The issue is that the code calls sg_mark_end() which clobbers the
sg_page() pointer of the final scatterlist entry.

The first part fo the fix makes skb_to_sgvec() do __sg_mark_end().

After considering all skb_to_sgvec() call sites the most correct
solution is to call __sg_mark_end() in skb_to_sgvec() since that is
what all of the callers would end up doing anyways.

I suspect this might have fixed some problems in virtio_net which is
the sole non-crypto user of skb_to_sgvec().

Other similar sg_mark_end() cases were converted over to
__sg_mark_end() as well.

Arguably sg_mark_end() is a poorly named function because it doesn't
just "mark", it clears out the page pointer as a side effect, which is
what led to these bugs in the first place.

The one remaining plain sg_mark_end() call is in scsi_alloc_sgtable()
and arguably it could be converted to __sg_mark_end() if only so that
we can delete this confusing interface from linux/scatterlist.h

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 net/core/skbuff.c                     |   16 +++++++++++++---
 net/ipv4/esp4.c                       |   12 +++++++-----
 net/ipv4/tcp_ipv4.c                   |    2 +-
 net/ipv6/esp6.c                       |   13 +++++++------
 net/ipv6/tcp_ipv6.c                   |    2 +-
 net/rxrpc/rxkad.c                     |    9 +++++----
 net/sunrpc/auth_gss/gss_krb5_crypto.c |    6 +++---
 7 files changed, 37 insertions(+), 23 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 573e172..64b50ff 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2028,8 +2028,8 @@ void __init skb_init(void)
  *	Fill the specified scatter-gather list with mappings/pointers into a
  *	region of the buffer space attached to a socket buffer.
  */
-int
-skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
+static int
+__skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
 {
 	int start = skb_headlen(skb);
 	int i, copy = start - offset;
@@ -2078,7 +2078,8 @@ skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
 			if ((copy = end - offset) > 0) {
 				if (copy > len)
 					copy = len;
-				elt += skb_to_sgvec(list, sg+elt, offset - start, copy);
+				elt += __skb_to_sgvec(list, sg+elt, offset - start,
+						      copy);
 				if ((len -= copy) == 0)
 					return elt;
 				offset += copy;
@@ -2090,6 +2091,15 @@ skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
 	return elt;
 }
 
+int skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
+{
+	int nsg = __skb_to_sgvec(skb, sg, offset, len);
+
+	__sg_mark_end(&sg[nsg - 1]);
+
+	return nsg;
+}
+
 /**
  *	skb_cow_data - Check that a socket buffer's data buffers are writable
  *	@skb: The socket buffer to check.
diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c
index cad4278..c31bccb 100644
--- a/net/ipv4/esp4.c
+++ b/net/ipv4/esp4.c
@@ -111,9 +111,10 @@ static int esp_output(struct xfrm_state *x, struct sk_buff *skb)
 				goto unlock;
 		}
 		sg_init_table(sg, nfrags);
-		sg_mark_end(sg, skb_to_sgvec(skb, sg, esph->enc_data +
-						      esp->conf.ivlen -
-						      skb->data, clen));
+		skb_to_sgvec(skb, sg,
+			     esph->enc_data +
+			     esp->conf.ivlen -
+			     skb->data, clen);
 		err = crypto_blkcipher_encrypt(&desc, sg, sg, clen);
 		if (unlikely(sg != &esp->sgbuf[0]))
 			kfree(sg);
@@ -205,8 +206,9 @@ static int esp_input(struct xfrm_state *x, struct sk_buff *skb)
 			goto out;
 	}
 	sg_init_table(sg, nfrags);
-	sg_mark_end(sg, skb_to_sgvec(skb, sg, sizeof(*esph) + esp->conf.ivlen,
-				     elen));
+	skb_to_sgvec(skb, sg,
+		     sizeof(*esph) + esp->conf.ivlen,
+		     elen);
 	err = crypto_blkcipher_decrypt(&desc, sg, sg, elen);
 	if (unlikely(sg != &esp->sgbuf[0]))
 		kfree(sg);
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index d3d8d5d..eec02b2 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1083,7 +1083,7 @@ static int tcp_v4_do_calc_md5_hash(char *md5_hash, struct tcp_md5sig_key *key,
 	sg_set_buf(&sg[block++], key->key, key->keylen);
 	nbytes += key->keylen;
 
-	sg_mark_end(sg, block);
+	__sg_mark_end(&sg[block - 1]);
 
 	/* Now store the Hash into the packet */
 	err = crypto_hash_init(desc);
diff --git a/net/ipv6/esp6.c b/net/ipv6/esp6.c
index ab17b5e..7db66f1 100644
--- a/net/ipv6/esp6.c
+++ b/net/ipv6/esp6.c
@@ -110,9 +110,10 @@ static int esp6_output(struct xfrm_state *x, struct sk_buff *skb)
 				goto unlock;
 		}
 		sg_init_table(sg, nfrags);
-		sg_mark_end(sg, skb_to_sgvec(skb, sg, esph->enc_data +
-						      esp->conf.ivlen -
-						      skb->data, clen));
+		skb_to_sgvec(skb, sg,
+			     esph->enc_data +
+			     esp->conf.ivlen -
+			     skb->data, clen);
 		err = crypto_blkcipher_encrypt(&desc, sg, sg, clen);
 		if (unlikely(sg != &esp->sgbuf[0]))
 			kfree(sg);
@@ -209,9 +210,9 @@ static int esp6_input(struct xfrm_state *x, struct sk_buff *skb)
 			}
 		}
 		sg_init_table(sg, nfrags);
-		sg_mark_end(sg, skb_to_sgvec(skb, sg,
-					     sizeof(*esph) + esp->conf.ivlen,
-					     elen));
+		skb_to_sgvec(skb, sg,
+			     sizeof(*esph) + esp->conf.ivlen,
+			     elen);
 		ret = crypto_blkcipher_decrypt(&desc, sg, sg, elen);
 		if (unlikely(sg != &esp->sgbuf[0]))
 			kfree(sg);
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index f1523b8..4b90328 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -781,7 +781,7 @@ static int tcp_v6_do_calc_md5_hash(char *md5_hash, struct tcp_md5sig_key *key,
 	sg_set_buf(&sg[block++], key->key, key->keylen);
 	nbytes += key->keylen;
 
-	sg_mark_end(sg, block);
+	__sg_mark_end(&sg[block - 1]);
 
 	/* Now store the hash into the packet */
 	err = crypto_hash_init(desc);
diff --git a/net/rxrpc/rxkad.c b/net/rxrpc/rxkad.c
index eebefb6..c387cf6 100644
--- a/net/rxrpc/rxkad.c
+++ b/net/rxrpc/rxkad.c
@@ -237,7 +237,8 @@ static int rxkad_secure_packet_encrypt(const struct rxrpc_call *call,
 	len = data_size + call->conn->size_align - 1;
 	len &= ~(call->conn->size_align - 1);
 
-	sg_init_table(sg, skb_to_sgvec(skb, sg, 0, len));
+	sg_init_table(sg, nsg);
+	skb_to_sgvec(skb, sg, 0, len);
 	crypto_blkcipher_encrypt_iv(&desc, sg, sg, len);
 
 	_leave(" = 0");
@@ -344,7 +345,7 @@ static int rxkad_verify_packet_auth(const struct rxrpc_call *call,
 		goto nomem;
 
 	sg_init_table(sg, nsg);
-	sg_mark_end(sg, skb_to_sgvec(skb, sg, 0, 8));
+	skb_to_sgvec(skb, sg, 0, 8);
 
 	/* start the decryption afresh */
 	memset(&iv, 0, sizeof(iv));
@@ -426,7 +427,7 @@ static int rxkad_verify_packet_encrypt(const struct rxrpc_call *call,
 	}
 
 	sg_init_table(sg, nsg);
-	sg_mark_end(sg, skb_to_sgvec(skb, sg, 0, skb->len));
+	skb_to_sgvec(skb, sg, 0, skb->len);
 
 	/* decrypt from the session key */
 	payload = call->conn->key->payload.data;
@@ -701,7 +702,7 @@ static void rxkad_sg_set_buf2(struct scatterlist sg[2],
 		nsg++;
 	}
 
-	sg_mark_end(sg, nsg);
+	__sg_mark_end(&sg[nsg - 1]);
 
 	ASSERTCMP(sg[0].length + sg[1].length, ==, buflen);
 }
diff --git a/net/sunrpc/auth_gss/gss_krb5_crypto.c b/net/sunrpc/auth_gss/gss_krb5_crypto.c
index 91cd8f0..ab7cbd6 100644
--- a/net/sunrpc/auth_gss/gss_krb5_crypto.c
+++ b/net/sunrpc/auth_gss/gss_krb5_crypto.c
@@ -211,8 +211,8 @@ encryptor(struct scatterlist *sg, void *data)
 	if (thislen == 0)
 		return 0;
 
-	sg_mark_end(desc->infrags, desc->fragno);
-	sg_mark_end(desc->outfrags, desc->fragno);
+	__sg_mark_end(&desc->infrags[desc->fragno - 1]);
+	__sg_mark_end(&desc->outfrags[desc->fragno - 1]);
 
 	ret = crypto_blkcipher_encrypt_iv(&desc->desc, desc->outfrags,
 					  desc->infrags, thislen);
@@ -293,7 +293,7 @@ decryptor(struct scatterlist *sg, void *data)
 	if (thislen == 0)
 		return 0;
 
-	sg_mark_end(desc->frags, desc->fragno);
+	__sg_mark_end(&desc->frags[desc->fragno - 1]);
 
 	ret = crypto_blkcipher_decrypt_iv(&desc->desc, desc->frags,
 					  desc->frags, thislen);
-- 
1.5.2.5

Re: [PATCH]: Fix networking scatterlist regressions.

[Jens Axboe]

On Wed, Oct 31 2007, David Miller wrote:
> From: Jens Axboe <jens.axboe@oracle.com>
> Date: Wed, 31 Oct 2007 08:46:21 +0100
> 
> > On Tue, Oct 30 2007, David Miller wrote:
> > > @@ -293,7 +293,7 @@ decryptor(struct scatterlist *sg, void *data)
> > >  	if (thislen == 0)
> > >  		return 0;
> > >  
> > > -	sg_mark_end(desc->frags, desc->fragno);
> > > +	__sg_mark_end(desc->frags, desc->fragno);
> > >  
> > >  	ret = crypto_blkcipher_decrypt_iv(&desc->desc, desc->frags,
> > >  					  desc->frags, thislen);
> > 
> > Hmm? These don't seem right. It also has a weird code sequence:
>  ...
> > Did something go wrong there?
> 
> Yes, I fixed those up after doing some allmodconfig builds.
> 
> Here is the final patch I actually pushed to Linus:

That fixes up the sg_mark_end() bit, but it's still calling
sg_init_table() just a few lines further down. Is that correct?


-- 
Jens Axboe

Re: [PATCH]: Fix networking scatterlist regressions.

[David Miller]

From: Jens Axboe <jens.axboe@oracle.com>
Date: Wed, 31 Oct 2007 09:01:43 +0100

> On Wed, Oct 31 2007, David Miller wrote:
> > From: Jens Axboe <jens.axboe@oracle.com>
> > Date: Wed, 31 Oct 2007 08:46:21 +0100
> > 
> > > On Tue, Oct 30 2007, David Miller wrote:
> > > > @@ -293,7 +293,7 @@ decryptor(struct scatterlist *sg, void *data)
> > > >  	if (thislen == 0)
> > > >  		return 0;
> > > >  
> > > > -	sg_mark_end(desc->frags, desc->fragno);
> > > > +	__sg_mark_end(desc->frags, desc->fragno);
> > > >  
> > > >  	ret = crypto_blkcipher_decrypt_iv(&desc->desc, desc->frags,
> > > >  					  desc->frags, thislen);
> > > 
> > > Hmm? These don't seem right. It also has a weird code sequence:
> >  ...
> > > Did something go wrong there?
> > 
> > Yes, I fixed those up after doing some allmodconfig builds.
> > 
> > Here is the final patch I actually pushed to Linus:
> 
> That fixes up the sg_mark_end() bit, but it's still calling
> sg_init_table() just a few lines further down. Is that correct?

Absolutely.  It initially using the scatterlist for this
crypto layer call:

	ret = crypto_blkcipher_decrypt_iv(&desc->desc, desc->frags,
					  desc->frags, thislen);
	if (ret)
		return ret;

then it reinits and sets the sglist to the values the caller
wants.

Re: [PATCH]: Fix networking scatterlist regressions.

[Jens Axboe]

On Wed, Oct 31 2007, David Miller wrote:
> From: Jens Axboe <jens.axboe@oracle.com>
> Date: Wed, 31 Oct 2007 09:01:43 +0100
> 
> > On Wed, Oct 31 2007, David Miller wrote:
> > > From: Jens Axboe <jens.axboe@oracle.com>
> > > Date: Wed, 31 Oct 2007 08:46:21 +0100
> > > 
> > > > On Tue, Oct 30 2007, David Miller wrote:
> > > > > @@ -293,7 +293,7 @@ decryptor(struct scatterlist *sg, void *data)
> > > > >  	if (thislen == 0)
> > > > >  		return 0;
> > > > >  
> > > > > -	sg_mark_end(desc->frags, desc->fragno);
> > > > > +	__sg_mark_end(desc->frags, desc->fragno);
> > > > >  
> > > > >  	ret = crypto_blkcipher_decrypt_iv(&desc->desc, desc->frags,
> > > > >  					  desc->frags, thislen);
> > > > 
> > > > Hmm? These don't seem right. It also has a weird code sequence:
> > >  ...
> > > > Did something go wrong there?
> > > 
> > > Yes, I fixed those up after doing some allmodconfig builds.
> > > 
> > > Here is the final patch I actually pushed to Linus:
> > 
> > That fixes up the sg_mark_end() bit, but it's still calling
> > sg_init_table() just a few lines further down. Is that correct?
> 
> Absolutely.  It initially using the scatterlist for this
> crypto layer call:
> 
> 	ret = crypto_blkcipher_decrypt_iv(&desc->desc, desc->frags,
> 					  desc->frags, thislen);
> 	if (ret)
> 		return ret;
> 
> then it reinits and sets the sglist to the values the caller
> wants.

Great, just wanted to double check that it was indeed correct!

-- 
Jens Axboe

Re: [PATCH]: Fix networking scatterlist regressions.

[Jens Axboe]

On Wed, Oct 31 2007, David Miller wrote:
> From: Jens Axboe <jens.axboe@oracle.com>
> Date: Wed, 31 Oct 2007 08:46:21 +0100
> 
> > On Tue, Oct 30 2007, David Miller wrote:
> > > @@ -293,7 +293,7 @@ decryptor(struct scatterlist *sg, void *data)
> > >  	if (thislen == 0)
> > >  		return 0;
> > >  
> > > -	sg_mark_end(desc->frags, desc->fragno);
> > > +	__sg_mark_end(desc->frags, desc->fragno);
> > >  
> > >  	ret = crypto_blkcipher_decrypt_iv(&desc->desc, desc->frags,
> > >  					  desc->frags, thislen);
> > 
> > Hmm? These don't seem right. It also has a weird code sequence:
>  ...
> > Did something go wrong there?
> 
> Yes, I fixed those up after doing some allmodconfig builds.
> 
> Here is the final patch I actually pushed to Linus:

Here's the sg_mark_end() patch on top of that.

>From 2f5371509d3d4d09269bf7a46868da2ac5c61d77 Mon Sep 17 00:00:00 2001
From: Jens Axboe <jens.axboe@oracle.com>
Date: Wed, 31 Oct 2007 09:11:10 +0100
Subject: [PATCH] [SG] Get rid of __sg_mark_end()

sg_mark_end() overwrites the page_link information, but all users want
__sg_mark_end() behaviour where we just set the end bit. That is the most
natural way to use the sg list, since you'll fill it in and then mark the
end point.

So change sg_mark_end() to only set the termination bit. Add a sg_magic
debug check as well, and clear a chain pointer if it is set.

Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
---
 block/ll_rw_blk.c                     |    2 +-
 drivers/scsi/scsi_lib.c               |    2 +-
 include/linux/scatterlist.h           |   22 ++++++++++++----------
 net/core/skbuff.c                     |    2 +-
 net/ipv4/tcp_ipv4.c                   |    2 +-
 net/ipv6/tcp_ipv6.c                   |    2 +-
 net/rxrpc/rxkad.c                     |    2 +-
 net/sunrpc/auth_gss/gss_krb5_crypto.c |    6 +++---
 8 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/block/ll_rw_blk.c b/block/ll_rw_blk.c
index e948407..fdc0707 100644
--- a/block/ll_rw_blk.c
+++ b/block/ll_rw_blk.c
@@ -1369,7 +1369,7 @@ new_segment:
 	} /* segments in rq */
 
 	if (sg)
-		__sg_mark_end(sg);
+		sg_mark_end(sg);
 
 	return nsegs;
 }
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 61fdaf0..88de771 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -785,7 +785,7 @@ struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd, gfp_t gfp_mask)
 		 * end-of-list
 		 */
 		if (!left)
-			sg_mark_end(sgl, this);
+			sg_mark_end(&sgl[this - 1]);
 
 		/*
 		 * don't allow subsequent mempool allocs to sleep, it would
diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index d5e1876..b2116a1 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -188,21 +188,23 @@ static inline void sg_chain(struct scatterlist *prv, unsigned int prv_nents,
 
 /**
  * sg_mark_end - Mark the end of the scatterlist
- * @sgl:	Scatterlist
- * @nents:	Number of entries in sgl
+ * @sg:		 SG entryScatterlist
  *
  * Description:
- *   Marks the last entry as the termination point for sg_next()
+ *   Marks the passed in sg entry as the termination point for the sg
+ *   table. A call to sg_next() on this entry will return NULL.
  *
  **/
-static inline void sg_mark_end(struct scatterlist *sgl, unsigned int nents)
-{
-	sgl[nents - 1].page_link = 0x02;
-}
-
-static inline void __sg_mark_end(struct scatterlist *sg)
+static inline void sg_mark_end(struct scatterlist *sg)
 {
+#ifdef CONFIG_DEBUG_SG
+	BUG_ON(sg->sg_magic != SG_MAGIC);
+#endif
+	/*
+	 * Set termination bit, clear potential chain bit
+	 */
 	sg->page_link |= 0x02;
+	sg->page_link &= ~0x01;
 }
 
 /**
@@ -218,7 +220,7 @@ static inline void __sg_mark_end(struct scatterlist *sg)
 static inline void sg_init_table(struct scatterlist *sgl, unsigned int nents)
 {
 	memset(sgl, 0, sizeof(*sgl) * nents);
-	sg_mark_end(sgl, nents);
+	sg_mark_end(&sgl[nents - 1]);
 #ifdef CONFIG_DEBUG_SG
 	{
 		unsigned int i;
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 64b50ff..32d5826 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2095,7 +2095,7 @@ int skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int le
 {
 	int nsg = __skb_to_sgvec(skb, sg, offset, len);
 
-	__sg_mark_end(&sg[nsg - 1]);
+	sg_mark_end(&sg[nsg - 1]);
 
 	return nsg;
 }
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index eec02b2..d438dfb 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1083,7 +1083,7 @@ static int tcp_v4_do_calc_md5_hash(char *md5_hash, struct tcp_md5sig_key *key,
 	sg_set_buf(&sg[block++], key->key, key->keylen);
 	nbytes += key->keylen;
 
-	__sg_mark_end(&sg[block - 1]);
+	sg_mark_end(&sg[block - 1]);
 
 	/* Now store the Hash into the packet */
 	err = crypto_hash_init(desc);
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 4b90328..06be2a1 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -781,7 +781,7 @@ static int tcp_v6_do_calc_md5_hash(char *md5_hash, struct tcp_md5sig_key *key,
 	sg_set_buf(&sg[block++], key->key, key->keylen);
 	nbytes += key->keylen;
 
-	__sg_mark_end(&sg[block - 1]);
+	sg_mark_end(&sg[block - 1]);
 
 	/* Now store the hash into the packet */
 	err = crypto_hash_init(desc);
diff --git a/net/rxrpc/rxkad.c b/net/rxrpc/rxkad.c
index c387cf6..e09a95a 100644
--- a/net/rxrpc/rxkad.c
+++ b/net/rxrpc/rxkad.c
@@ -702,7 +702,7 @@ static void rxkad_sg_set_buf2(struct scatterlist sg[2],
 		nsg++;
 	}
 
-	__sg_mark_end(&sg[nsg - 1]);
+	sg_mark_end(&sg[nsg - 1]);
 
 	ASSERTCMP(sg[0].length + sg[1].length, ==, buflen);
 }
diff --git a/net/sunrpc/auth_gss/gss_krb5_crypto.c b/net/sunrpc/auth_gss/gss_krb5_crypto.c
index ab7cbd6..0dd7923 100644
--- a/net/sunrpc/auth_gss/gss_krb5_crypto.c
+++ b/net/sunrpc/auth_gss/gss_krb5_crypto.c
@@ -211,8 +211,8 @@ encryptor(struct scatterlist *sg, void *data)
 	if (thislen == 0)
 		return 0;
 
-	__sg_mark_end(&desc->infrags[desc->fragno - 1]);
-	__sg_mark_end(&desc->outfrags[desc->fragno - 1]);
+	sg_mark_end(&desc->infrags[desc->fragno - 1]);
+	sg_mark_end(&desc->outfrags[desc->fragno - 1]);
 
 	ret = crypto_blkcipher_encrypt_iv(&desc->desc, desc->outfrags,
 					  desc->infrags, thislen);
@@ -293,7 +293,7 @@ decryptor(struct scatterlist *sg, void *data)
 	if (thislen == 0)
 		return 0;
 
-	__sg_mark_end(&desc->frags[desc->fragno - 1]);
+	sg_mark_end(&desc->frags[desc->fragno - 1]);
 
 	ret = crypto_blkcipher_decrypt_iv(&desc->desc, desc->frags,
 					  desc->frags, thislen);
-- 
1.5.3.GIT



-- 
Jens Axboe

Re: [PATCH]: Fix networking scatterlist regressions.

[David Miller]

From: Jens Axboe <jens.axboe@oracle.com>
Date: Wed, 31 Oct 2007 09:14:28 +0100

> Subject: [PATCH] [SG] Get rid of __sg_mark_end()
> 
> sg_mark_end() overwrites the page_link information, but all users want
> __sg_mark_end() behaviour where we just set the end bit. That is the most
> natural way to use the sg list, since you'll fill it in and then mark the
> end point.
> 
> So change sg_mark_end() to only set the termination bit. Add a sg_magic
> debug check as well, and clear a chain pointer if it is set.
> 
> Signed-off-by: Jens Axboe <jens.axboe@oracle.com>

It doesn't build.  I suspect there is something else in your tree
that is necessary for this patch to work on it's own.

davem@sunset:~/src/GIT/net-2.6$ patch -p1 <diff
patching file block/ll_rw_blk.c
patching file drivers/scsi/scsi_lib.c
patching file include/linux/scatterlist.h
Hunk #2 succeeded at 242 (offset 22 lines).
patching file net/core/skbuff.c
patching file net/ipv4/tcp_ipv4.c
patching file net/ipv6/tcp_ipv6.c
patching file net/rxrpc/rxkad.c
patching file net/sunrpc/auth_gss/gss_krb5_crypto.c
 ...
  CC      init/main.o
In file included from include/asm/dma-mapping.h:4,
                 from include/linux/dma-mapping.h:52,
                 from include/asm/sbus.h:9,
                 from include/asm/dma.h:14,
                 from include/linux/bootmem.h:8,
                 from init/main.c:26:
include/linux/scatterlist.h: In function 'sg_init_one':
include/linux/scatterlist.h:228: error: too many arguments to function 'sg_mark_end'
make[1]: *** [init/main.o] Error 1
make: *** [init] Error 2

Re: [PATCH]: Fix networking scatterlist regressions.

[Jens Axboe]

On Wed, Oct 31 2007, David Miller wrote:
> From: Jens Axboe <jens.axboe@oracle.com>
> Date: Wed, 31 Oct 2007 09:14:28 +0100
> 
> > Subject: [PATCH] [SG] Get rid of __sg_mark_end()
> > 
> > sg_mark_end() overwrites the page_link information, but all users want
> > __sg_mark_end() behaviour where we just set the end bit. That is the most
> > natural way to use the sg list, since you'll fill it in and then mark the
> > end point.
> > 
> > So change sg_mark_end() to only set the termination bit. Add a sg_magic
> > debug check as well, and clear a chain pointer if it is set.
> > 
> > Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
> 
> It doesn't build.  I suspect there is something else in your tree
> that is necessary for this patch to work on it's own.

Builds here. But yes, it's on top of other patches, it was merely for
demonstration purposes that I posted it. Locally sg_init_one() uses
sg_init_table() here, it doesn't open code the init:

static inline void sg_init_one(struct scatterlist *sg, const void *buf,
                               unsigned int buflen)
{
        sg_init_table(sg, 1);
        sg_set_buf(sg, buf, buflen);
}


-- 
Jens Axboe

Re: [PATCH]: Fix networking scatterlist regressions.

[Herbert Xu]

On Tue, Oct 30, 2007 at 08:40:02PM -0700, David Miller wrote:
> 
> I just checked the following bug fix into net-2.6

Thanks for getting to the bottom of this Dave! I seem to have
mistaken the = for a |= in sg_mark_end :)

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]: Fix networking scatterlist regressions.

[Jens Axboe]

On Wed, Oct 31 2007, Herbert Xu wrote:
> On Tue, Oct 30, 2007 at 08:40:02PM -0700, David Miller wrote:
> > 
> > I just checked the following bug fix into net-2.6
> 
> Thanks for getting to the bottom of this Dave! I seem to have
> mistaken the = for a |= in sg_mark_end :)

I don't blame you, that function was definitely non-intuitive!

-- 
Jens Axboe