RFC(ry): breaking loop.c's IV calculation

RFC(ry): breaking loop.c's IV calculation

[Herbert Valerio Riedel]

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

On Sun, 2001-12-02 at 23:46, Andrea Arcangeli wrote:
> > >> ps: any chance to get a sector-based-IV calculation (instead of the
> > >> actual broken soft blocksize based one) into loop.c?!?
> > > I can extract all loop.c bug fixes from loop-AES, excluding AES cipher, if
> > > someone wants them. Well, I can include AES cipher too, but that would
> > > royally piss-off the cryptoapi people.
> > ..maybe :-))
> > 
> > > Does anyone want the bug fixes? Jens? Marcelo?
> > I hope jens & andrea still remember the motivation this IV thing... :-)

> Of course I remeber. I still vote for breaking the IV API and to avoid
> the compatibility cruft. Please post to l-k the patch to change the IV
> granularity from the softblocksize to 512 fixed describing our
> discussion, so if anybody really cares about the current IV API he will
> have a chance to complain before we post the patch for inclusion to
> Marcelo and Linus.
 
> > btw, I don't care, whether my backward-compatible (or 
> > 'toothpaste-back-into-tube'-approach as jari 
> > would call it ;) patch gets approved or whether a radical switch to sector 
> > based IV calculation as jari proposes gets accepted...
> > 
> > we just need a consistent IV metric, regardless of the underlying medium 
> > (/dev/cdrom,/dev/fd0,/dev/hda,...) or any involved layers (lvm, md, ...)
> 
> Indeed.

well, I've put one patch together (it still needs (constructive)
auditing though! jari?) here it is (it's against 2.4.16's loop.[ch])

(also available as /pub/linux/kernel/people/hvr/loop2-iv-2.4.16.patch)

Index: drivers/block/loop.c
===================================================================
RCS file: /cvs/linux-2.4-xfs/linux/drivers/block/loop.c,v
retrieving revision 1.43
diff -u -r1.43 loop.c
--- drivers/block/loop.c	2001/11/20 18:59:02	1.43
+++ drivers/block/loop.c	2001/12/03 15:03:36
@@ -85,7 +85,7 @@
  * Transfer functions
  */
 static int transfer_none(struct loop_device *lo, int cmd, char *raw_buf,
-			 char *loop_buf, int size, int real_block)
+			 char *loop_buf, int size, loop_iv_t IV)
 {
 	if (raw_buf != loop_buf) {
 		if (cmd == READ)
@@ -98,7 +98,7 @@
 }
 
 static int transfer_xor(struct loop_device *lo, int cmd, char *raw_buf,
-			char *loop_buf, int size, int real_block)
+			char *loop_buf, int size, loop_iv_t IV)
 {
 	char	*in, *out, *key;
 	int	i, keysize;
@@ -186,7 +186,7 @@
 	len = bh->b_size;
 	data = bh->b_data;
 	while (len > 0) {
-		int IV = index * (PAGE_CACHE_SIZE/bsize) + offset/bsize;
+		const loop_iv_t IV = (index << (PAGE_CACHE_SHIFT - LOOP_IV_SECTOR_BITS)) + (offset >> LOOP_IV_SECTOR_BITS);
 		int transfer_result;
 
 		size = PAGE_CACHE_SIZE - offset;
@@ -244,7 +244,7 @@
 	unsigned long count = desc->count;
 	struct lo_read_data *p = (struct lo_read_data*)desc->buf;
 	struct loop_device *lo = p->lo;
-	int IV = page->index * (PAGE_CACHE_SIZE/p->bsize) + offset/p->bsize;
+	const loop_iv_t IV = (page->index << (PAGE_CACHE_SHIFT - LOOP_IV_SECTOR_BITS)) + (offset >> LOOP_IV_SECTOR_BITS);
 
 	if (size > count)
 		size = count;
@@ -296,20 +296,6 @@
 	return bs;
 }
 
-static inline unsigned long loop_get_iv(struct loop_device *lo,
-					unsigned long sector)
-{
-	int bs = loop_get_bs(lo);
-	unsigned long offset, IV;
-
-	IV = sector / (bs >> 9) + lo->lo_offset / bs;
-	offset = ((sector % (bs >> 9)) << 9) + lo->lo_offset % bs;
-	if (offset >= bs)
-		IV++;
-
-	return IV;
-}
-
 static int do_bh_filebacked(struct loop_device *lo, struct buffer_head *bh, int rw)
 {
 	loff_t pos;
@@ -455,7 +441,7 @@
 {
 	struct buffer_head *bh = NULL;
 	struct loop_device *lo;
-	unsigned long IV;
+	loop_iv_t IV;
 
 	if (!buffer_locked(rbh))
 		BUG();
@@ -502,7 +488,7 @@
 	 * piggy old buffer on original, and submit for I/O
 	 */
 	bh = loop_get_buffer(lo, rbh);
-	IV = loop_get_iv(lo, rbh->b_rsector);
+	IV = rbh->b_rsector + (lo->lo_offset >> LOOP_IV_SECTOR_BITS);
 	if (rw == WRITE) {
 		set_bit(BH_Dirty, &bh->b_state);
 		if (lo_do_transfer(lo, WRITE, bh->b_data, rbh->b_data,
@@ -539,7 +525,7 @@
 		bh->b_end_io(bh, !ret);
 	} else {
 		struct buffer_head *rbh = bh->b_private;
-		unsigned long IV = loop_get_iv(lo, rbh->b_rsector);
+		const loop_iv_t IV = rbh->b_rsector + (lo->lo_offset >> LOOP_IV_SECTOR_BITS);
 
 		ret = lo_do_transfer(lo, READ, bh->b_data, rbh->b_data,
 				     bh->b_size, IV);
Index: include/linux/loop.h
===================================================================
RCS file: /cvs/linux-2.4-xfs/linux/include/linux/loop.h,v
retrieving revision 1.5
diff -u -r1.5 loop.h
--- include/linux/loop.h	2001/09/21 16:28:50	1.5
+++ include/linux/loop.h	2001/12/03 15:03:36
@@ -17,6 +17,12 @@
 
 #ifdef __KERNEL__
 
+/* definitions for IV metric */
+#define LOOP_IV_SECTOR_BITS 9
+#define LOOP_IV_SECTOR_SIZE (1 << LO_IV_SECTOR_BITS)
+
+typedef unsigned long loop_iv_t;
+
 /* Possible states of device */
 enum {
 	Lo_unbound,
@@ -24,6 +30,12 @@
 	Lo_rundown,
 };
 
+struct loop_device;
+
+typedef	int (* transfer_proc_t)(struct loop_device *, int cmd,
+				char *raw_buf, char *loop_buf, int size,
+				loop_iv_t IV);
+
 struct loop_device {
 	int		lo_number;
 	int		lo_refcnt;
@@ -32,9 +44,7 @@
 	int		lo_encrypt_type;
 	int		lo_encrypt_key_size;
 	int		lo_flags;
-	int		(*transfer)(struct loop_device *, int cmd,
-				    char *raw_buf, char *loop_buf, int size,
-				    int real_block);
+	transfer_proc_t transfer;
 	char		lo_name[LO_NAME_SIZE];
 	char		lo_encrypt_key[LO_KEY_SIZE];
 	__u32           lo_init[2];
@@ -58,17 +68,13 @@
 	atomic_t		lo_pending;
 };
 
-typedef	int (* transfer_proc_t)(struct loop_device *, int cmd,
-				char *raw_buf, char *loop_buf, int size,
-				int real_block);
-
 static inline int lo_do_transfer(struct loop_device *lo, int cmd, char *rbuf,
-				 char *lbuf, int size, int rblock)
+				 char *lbuf, int size, loop_iv_t IV)
 {
 	if (!lo->transfer)
 		return 0;
 
-	return lo->transfer(lo, cmd, rbuf, lbuf, size, rblock);
+	return lo->transfer(lo, cmd, rbuf, lbuf, size, IV);
 }
 #endif /* __KERNEL__ */
 
@@ -122,6 +128,8 @@
 #define LO_CRYPT_IDEA     6
 #define LO_CRYPT_DUMMY    9
 #define LO_CRYPT_SKIPJACK 10
+#define LO_CRYPT_AES      16   /* loop-AES */
+#define LO_CRYPT_CRYPTOAPI 18  /* international crypto patch */
 #define MAX_LO_CRYPT	20
 
 #ifdef __KERNEL__
@@ -129,7 +137,7 @@
 struct loop_func_table {
 	int number; 	/* filter type */ 
 	int (*transfer)(struct loop_device *lo, int cmd, char *raw_buf,
-			char *loop_buf, int size, int real_block);
+			char *loop_buf, int size, loop_iv_t IV);
 	int (*init)(struct loop_device *, struct loop_info *); 
 	/* release is called from loop_unregister_transfer or clr_fd */
 	int (*release)(struct loop_device *); 

-- 
Herbert Valerio Riedel       /    Phone: (EUROPE) +43-1-58801-18840
Email: hvr@hvrlab.org       /    Finger hvr@gnu.org for GnuPG Public Key
GnuPG Key Fingerprint: 7BB9 2D6C D485 CE64 4748  5F65 4981 E064 883F
4142

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: RFC(ry): breaking loop.c's IV calculation

[Jari Ruusu]

Herbert Valerio Riedel wrote:
> well, I've put one patch together (it still needs (constructive)
> auditing though! jari?) here it is (it's against 2.4.16's loop.[ch])

1)  For 2.4 kernels, IV type must remain int, not loop_iv_t, ok?
    Make the type loop_iv_t for 2.5 kernels but not for 2.4.

2)  Get rid of the loop_get_bs() crap.

Regards,
Jari Ruusu <jari.ruusu@pp.inet.fi>

Re: RFC(ry): breaking loop.c's IV calculation

[Jari Ruusu]

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

Herbert Valerio Riedel wrote:
> well, I've put one patch together (it still needs (constructive)
> auditing though! jari?) here it is (it's against 2.4.16's loop.[ch])

I have attached my version of loop.c bug fixes. These are extracted from
loop-AES and are well tested.

- IV computed in 512 byte units.
- Make device backed loop work with swap by pre-allocating pages.
- External encryption module locking bug fixed (from Ingo Rohloff).
- Get rid of the loop_get_bs() crap.
- grab_cache_page() return value handled properly, avoids Oops.
- No more illegal messing with BH_Dirty flag.
- No more illegal sleeping in generic_make_request().
- Loops can be set-up properly when root partition is still mounted ro.
- Default soft block size is set properly for file backed loops.
- kmalloc() error case handled properly.

Regards,
Jari Ruusu <jari.ruusu@pp.inet.fi>

[-- Attachment #2: loop-fixes-2.4.17-pre2.diff.gz --]
[-- Type: application/x-gzip, Size: 7182 bytes --]

Re: RFC(ry): breaking loop.c's IV calculation

[Herbert Valerio Riedel]

hello!

On Mon, 2001-12-03 at 20:00, Jari Ruusu wrote:
> Herbert Valerio Riedel wrote:
>> well, I've put one patch together (it still needs (constructive)
>> auditing though! jari?) here it is (it's against 2.4.16's loop.[ch])
 
> 1)  For 2.4 kernels, IV type must remain int, not loop_iv_t, ok?
>     Make the type loop_iv_t for 2.5 kernels but not for 2.4.
no problem, the typedef can be changed to

typedef int loop_iv_t;

that way the API does not change; it just looks more consistent...
 
IMHO having a typedef in loop.h instead of hardcoding it to an 'int' is
more flexible and less error-prone...

> 2)  Get rid of the loop_get_bs() crap.
btw, what's the motivation for it? I'd also like to know why it was used
in the first place at all... :-)

On Mon, 2001-12-03 at 23:22, Jari Ruusu wrote:
> I have attached my version of loop.c bug fixes. These are extracted from
> loop-AES and are well tested.
although I'm quite confident you patch is well tested, it goes well
beyond fixing only the IV calculation and as such represents a major
change to the loop.c driver -- I'm curious whether it will be accepted
for 2.4.x... :-) ; but don't get me wrong, I'll be quite happy if it
goes in nevertheless!

just a minor note regarding source 'aesthetics';

it would be more self-explaining IMNSHO if you used some macro constant
for the '9' shifting instead of hardcoding it...:

int IV = index * (PAGE_CACHE_SIZE >> 9) + (offset >> 9);

in your patch vs. in my patch:

/* loop.h */
#define LOOP_IV_SECTOR_BITS 9
#define LOOP_IV_SECTOR_SIZE (1 << LOOP_IV_SECTOR_BITS)

typedef int loop_iv_t;

/* loop.c */
const loop_iv_t IV =
    (index << (PAGE_CACHE_SHIFT - LOOP_IV_SECTOR_BITS))
  + (offset >> LOOP_IV_SECTOR_BITS);

it makes also life easier for filters which include loop.h, cause you
can check for the presence of the #defines above, whether a fixed loop.c
is available or not; and we are also prepared for the (even if unlikely)
event that those constants are changed some time...

> - IV computed in 512 byte units.
> - Make device backed loop work with swap by pre-allocating pages.
btw, just as a side note; IMHO this is a neat feature, but encrypted
swap shouldn't be done on top of a loop device but more like the bsd
people do it, as it allows for more interesting IV & key management
schemes...

> - External encryption module locking bug fixed (from Ingo Rohloff).
> - Get rid of the loop_get_bs() crap.
> - grab_cache_page() return value handled properly, avoids Oops.
> - No more illegal messing with BH_Dirty flag.
> - No more illegal sleeping in generic_make_request().
> - Loops can be set-up properly when root partition is still mounted ro.
> - Default soft block size is set properly for file backed loops.
> - kmalloc() error case handled properly.

regards,
-- 
Herbert Valerio Riedel       /    Phone: (EUROPE) +43-1-58801-18840
Email: hvr@hvrlab.org       /    Finger hvr@gnu.org for GnuPG Public Key
GnuPG Key Fingerprint: 7BB9 2D6C D485 CE64 4748  5F65 4981 E064 883F
4142