Crypto digests and kmapping sg entries larger than a page, with [PATCH]

Crypto digests and kmapping sg entries larger than a page, with [PATCH]

[Clay Haapala]

[-- Attachment #1: Type: text/plain, Size: 553 bytes --]

N.C.Krishna Murthy asked me to forward this to the crypto maintainers.

While doing work on the iSCSI driver and 2.6.6 to use the crypto
CRC32C routines for digests, he saw that the scatterlist entries had
entries larger than one page.  We've had some discussion in
linux-iscsi about making sure we kmap() each page in such entries, and
Krishna saw that the crypto digest code may also need to do the same.

He supplies a patch, below.

If you agree with the implementation, I'll re-diff and test against a
recent BitKeeper extract and submit a patch.


[-- Attachment #2: Type: message/rfc822, Size: 4301 bytes --]

[-- Attachment #2.1.1: Type: text/plain, Size: 2010 bytes --]

Hi Clay,
	I was testing iSCSI driver code from 4.0 branch against 2.6.6 kernel.
I had the in kernel crypto CRC32 feature enabled. Initial testing resulted
in CRC errors. This was due to bugs in iSCSI driver and it has been reported
on sourceforge([965626 ] CRC errors seen when crypto APIs are used).

	I fixed the bugs in iSCSI driver and  ran some file I/O.
This resulted in an OOPs.
-----------------------------------------------------------------------------
---------- Process iscsi-tx (pid: 3344, threadinfo=f63e4000 task=f5c9f2a0)
Stack: f64d82b0 00000000 c0215af5 ffffffff fffe2000 00002000 fffe2000
 c0214c32 f64d82b0 fffe2000 00002000 f64d82b0 f63e4000 f63e4000 00002000
 f63e5e94 f7f609a0 f5c74000 f8b0dc80 f64d8284 f63e5e94 00000001 f63e5e78
 f63e5e7c Call Trace:
 [<c0215af5>] chksum_update+0x25/0x30
 [<c0214c32>] update+0x92/0x100
 [<f8b0dc80>] iscsi_xmit_data+0x460/0xb60 [iscsi_sfnet]
 [<c0119ff3>] scheduler_tick+0x43/0x630
 [<c036ea7f>] schedule+0x36f/0x6a0
 [<f8b0e52a>] iscsi_xmit_r2t_data+0xca/0x1b0 [iscsi_sfnet]
 [<f8af9025>] process_tx_requests+0x1f5/0x320 [iscsi_sfnet]
 [<c011a5e0>] default_wake_function+0x0/0x20
 [<f8af92e5>] iscsi_tx_thread+0x195/0x1d0 [iscsi_sfnet]
 [<c011a5e0>] default_wake_function+0x0/0x20
 [<f8af9150>] iscsi_tx_thread+0x0/0x1d0 [iscsi_sfnet]
 [<c0103f95>] kernel_thread_helper+0x5/0x10
-----------------------------------------------------------------------------
-----------

Upon looking into the crypto/digest.c, I found that "update()" function
does crypto_kmap() only the first page in the sg. A "struct scatterlist "
can point to a single page or physically contiguous pages.  The oops
seen is a result of "update()" not handling contiguous pages.
The patch I have attached solves the bug. I was able to test it and did
not see any oops after that. File I/O went through fine without any digest
errors.  Could you please forward the patch to the appropriate forum?

Thanx
N.C.Krishna Murthy

-------------------------------------------------------


[-- Attachment #2.1.2: digest.patch --]
[-- Type: text/x-diff, Size: 1014 bytes --]

--- /usr/src/linux-2.6.6/crypto/digest.c	2004-06-03 14:36:09.000000000 +0530
+++ /usr/src/linux-2.6.6/crypto.new/digest.c	2004-06-03 15:12:10.683261696 +0530
@@ -27,13 +27,28 @@
                    struct scatterlist *sg, unsigned int nsg)
 {
 	unsigned int i;
-	
+
 	for (i = 0; i < nsg; i++) {
-		char *p = crypto_kmap(sg[i].page, 0) + sg[i].offset;
-		tfm->__crt_alg->cra_digest.dia_update(crypto_tfm_ctx(tfm),
-		                                      p, sg[i].length);
-		crypto_kunmap(p, 0);
-		crypto_yield(tfm);
+
+		struct page *pg = sg[i].page;
+		unsigned int offset = sg[i].offset;
+		unsigned int l = sg[i].length;
+
+		do {
+			unsigned int bytes_from_page = min(l, ((unsigned int)
+							   (PAGE_SIZE)) - 
+							   offset);
+			char *p = crypto_kmap(pg, 0) + offset;
+
+			tfm->__crt_alg->cra_digest.dia_update
+					(crypto_tfm_ctx(tfm), p,
+					 bytes_from_page);
+			crypto_kunmap(p, 0);
+			crypto_yield(tfm);
+			offset = 0;
+			pg++;
+			l -= bytes_from_page;
+		} while (l > 0);
 	}
 }
 

[-- Attachment #3: Type: text/plain, Size: 250 bytes --]



-- 
Clay Haapala (chaapala@cisco.com) Cisco Systems SRBU +1 763-398-1056
   6450 Wedgwood Rd, Suite 130 Maple Grove MN 55311 PGP: C89240AD
"We're serious about this campaign now!  The training wheels are coming off!"
   -- a high White Horse Souse

Re: Crypto digests and kmapping sg entries larger than a page, with [PATCH]

[James Morris]

On Fri, 4 Jun 2004, Clay Haapala wrote:

> He supplies a patch, below.
> 
> If you agree with the implementation, I'll re-diff and test against a
> recent BitKeeper extract and submit a patch.

Thanks, please do so.


- James
-- 
James Morris
<jmorris@redhat.com>

[PATCH] Fix Crypto digest.c kmapping sg entries > page in length

[Clay Haapala]

Below is the patch, against 2.6.7-rc2, to fix crypto/digest.c to do
multiple kmap()/kunmap() for scatterlist entries which have a size
greater than a single page, originally found and fixed by
N.C.Krishna Murthy <krmurthy@cisco.com>.
-- 
Clay Haapala (chaapala@cisco.com) Cisco Systems SRBU +1 763-398-1056
   6450 Wedgwood Rd, Suite 130 Maple Grove MN 55311 PGP: C89240AD
"We're serious about this campaign now!  The training wheels are coming off!"
   - a high White Horse Souse

Signed-off-by: Clay Haapala <chaapala@cisco.com>

diff -uNr --exclude=SCCS --exclude '*.mod.c' --exclude='*.o' --exclude='*.ko' --exclude '.*' linux-2.6.6-bk.orig/crypto/digest.c linux-2.6.6-bk/crypto/digest.c
--- linux-2.6.6-bk.orig/crypto/digest.c	2004-06-07 10:58:36.000000000 -0500
+++ linux-2.6.6-bk/crypto/digest.c	2004-06-07 15:21:05.000000000 -0500
@@ -27,13 +27,28 @@
                    struct scatterlist *sg, unsigned int nsg)
 {
 	unsigned int i;
-	
+
 	for (i = 0; i < nsg; i++) {
-		char *p = crypto_kmap(sg[i].page, 0) + sg[i].offset;
-		tfm->__crt_alg->cra_digest.dia_update(crypto_tfm_ctx(tfm),
-		                                      p, sg[i].length);
-		crypto_kunmap(p, 0);
-		crypto_yield(tfm);
+
+		struct page *pg = sg[i].page;
+		unsigned int offset = sg[i].offset;
+		unsigned int l = sg[i].length;
+
+		do {
+			unsigned int bytes_from_page = min(l, ((unsigned int)
+							   (PAGE_SIZE)) - 
+							   offset);
+			char *p = crypto_kmap(pg, 0) + offset;
+
+			tfm->__crt_alg->cra_digest.dia_update
+					(crypto_tfm_ctx(tfm), p,
+					 bytes_from_page);
+			crypto_kunmap(p, 0);
+			crypto_yield(tfm);
+			offset = 0;
+			pg++;
+			l -= bytes_from_page;
+		} while (l > 0);
 	}
 }
 

Re: [PATCH] Fix Crypto digest.c kmapping sg entries > page in length

[David S. Miller]

On Mon, 07 Jun 2004 15:43:27 -0500
Clay Haapala <chaapala@cisco.com> wrote:

> Below is the patch, against 2.6.7-rc2, to fix crypto/digest.c to do
> multiple kmap()/kunmap() for scatterlist entries which have a size
> greater than a single page, originally found and fixed by
> N.C.Krishna Murthy <krmurthy@cisco.com>.

Applied, thanks a lot Clay.