loop driver, device-mapper and crypto

loop driver, device-mapper and crypto

[Christophe Saout]

Hello Andrew,

again I've read that there are problems with the loop driver in the
kernel with block device backing.

In 2.6 we have a device-mapper which does such things in a much more
generic way. I've already talked to a bunch of people working on loop
and cryptoloop (also with Clemens Fruhwirth, the cryptoloop maintainer)
and they all agreed that device-mapper is probably the most correct way
to go, and would be happier if the loop driver was used for files only.

I've written a device-mapper target "dm-crypt" some month ago which uses
cryptoapi and is compatible with cryptoloop. I never got much feedback,
some people tested it and found it to work just fine while cryptoloop
didn't, back then, when the test-series started or so.

Unfortunately I never got much public support on LKML itself and was
mostly ignored. And I'm not a politician...

Well, I just talked to Joe Thornber (again, he helped me developing the
patch) and he wrote:

> I'm happy to take the dm-crypt stuff into my unstable tree.  I don't
> think you'll get it into the kernel unless you get the cryptoloop
> people to publically (ie. on lkml) support you.

Well, you see, I'm a sort of difficult situation here. I would really be
happy to hear what you think of it, if it's worth to invest more energy
or not.

The target is currently a single file and makes use of dm-daemon.c from
Joe's current device-mapper patchset where the snapshot, mirror and
multpath targets are being developed.

The two patches (dm-daemon and the actual dm-crypt) are following.

A short introduction how to use the target:
http://marc.theaimsgroup.com/?l=linux-kernel&m=105967481007242&w=2

--
Christophe Saout <christophe@saout.de>
Please avoid sending me Word or PowerPoint attachments.
See http://www.fsf.org/philosophy/no-word-attachments.html

[PATCH 1/2] Add dm-daemon

[Christophe Saout]

Hi.

The first patch adds dm-daemon.c/.h.

The code is from Joe Thornbers current unstable device-mapper patchset.


diff -Nur linux-2.6.0/drivers/md/dm-daemon.c linux-2.6.0~dm-daemon/drivers/md/dm-daemon.c
--- linux-2.6.0/drivers/md/dm-daemon.c	1970-01-01 01:00:00.000000000 +0100
+++ linux-2.6.0~dm-daemon/drivers/md/dm-daemon.c	2003-12-22 13:16:55.000000000 +0100
@@ -0,0 +1,103 @@
+/*
+ * Copyright (C) 2003 Sistina Software
+ *
+ * This file is released under the LGPL.
+ */
+
+#include "dm.h"
+#include "dm-daemon.h"
+
+#include <linux/module.h>
+#include <linux/sched.h>
+#include <linux/suspend.h>
+#include <linux/completion.h>
+
+static int daemon(void *arg)
+{
+	struct dm_daemon *dd = (struct dm_daemon *) arg;
+	DECLARE_WAITQUEUE(wq, current);
+
+	daemonize("%s", dd->name);
+
+	atomic_set(&dd->please_die, 0);
+
+	add_wait_queue(&dd->job_queue, &wq);
+
+	complete(&dd->start);
+
+	/*
+	 * dd->fn() could do anything, very likely it will
+	 * suspend.  So we can't set the state to
+	 * TASK_INTERRUPTIBLE before calling it.  In order to
+	 * prevent a race with a waking thread we do this little
+	 * dance with the dd->woken variable.
+	 */
+	while (1) {
+		if (atomic_read(&dd->please_die))
+			goto out;
+
+		if (current->flags & PF_FREEZE)
+			refrigerator(PF_IOTHREAD);
+
+		do {
+			set_current_state(TASK_RUNNING);
+			atomic_set(&dd->woken, 0);
+			dd->fn();
+			set_current_state(TASK_INTERRUPTIBLE);
+
+		} while (atomic_read(&dd->woken));
+
+		schedule();
+	}
+
+ out:
+	remove_wait_queue(&dd->job_queue, &wq);
+	complete_and_exit(&dd->run, 0);
+}
+
+int dm_daemon_start(struct dm_daemon *dd, const char *name, jiffy_t (*fn)(void))
+{
+	pid_t pid = 0;
+
+	/*
+	 * Initialise the dm_daemon.
+	 */
+	dd->fn = fn;
+	strncpy(dd->name, name, sizeof(dd->name) - 1);
+	init_completion(&dd->start);
+	init_completion(&dd->run);
+	init_waitqueue_head(&dd->job_queue);
+
+	/*
+	 * Start the new thread.
+	 */
+	pid = kernel_thread(daemon, dd, CLONE_KERNEL);
+	if (pid <= 0) {
+		DMERR("Failed to start %s thread", name);
+		return -EAGAIN;
+	}
+
+	/*
+	 * wait for the daemon to up this mutex.
+	 */
+	wait_for_completion(&dd->start);
+
+	return 0;
+}
+
+void dm_daemon_stop(struct dm_daemon *dd)
+{
+	atomic_set(&dd->please_die, 1);
+	dm_daemon_wake(dd);
+	wait_for_completion(&dd->run);
+}
+
+void dm_daemon_wake(struct dm_daemon *dd)
+{
+	atomic_set(&dd->woken, 1);
+	wake_up_interruptible(&dd->job_queue);
+}
+
+EXPORT_SYMBOL(dm_daemon_start);
+EXPORT_SYMBOL(dm_daemon_stop);
+EXPORT_SYMBOL(dm_daemon_wake);
diff -Nur linux-2.6.0/drivers/md/dm-daemon.h linux-2.6.0~dm-daemon/drivers/md/dm-daemon.h
--- linux-2.6.0/drivers/md/dm-daemon.h	1970-01-01 01:00:00.000000000 +0100
+++ linux-2.6.0~dm-daemon/drivers/md/dm-daemon.h	2003-12-22 13:16:56.000000000 +0100
@@ -0,0 +1,33 @@
+/*
+ * Copyright (C) 2003 Sistina Software
+ *
+ * This file is released under the LGPL.
+ */
+
+#ifndef DM_DAEMON_H
+#define DM_DAEMON_H
+
+#include <asm/atomic.h>
+#include <linux/completion.h>
+
+/*
+ * The daemons work function returns a *hint* as to when it
+ * should next be woken up.
+ */
+struct dm_daemon {
+	jiffy_t (*fn)(void);
+	char name[16];
+	atomic_t please_die;
+	struct completion start;
+	struct completion run;
+
+	atomic_t woken;
+	wait_queue_head_t job_queue;
+};
+
+int dm_daemon_start(struct dm_daemon *dd, const char *name, jiffy_t (*fn)(void));
+void dm_daemon_stop(struct dm_daemon *dd);
+void dm_daemon_wake(struct dm_daemon *dd);
+int dm_daemon_running(struct dm_daemon *dd);
+
+#endif
diff -Nur linux-2.6.0/drivers/md/Makefile linux-2.6.0~dm-daemon/drivers/md/Makefile
--- linux-2.6.0/drivers/md/Makefile	2003-11-24 02:32:03.000000000 +0100
+++ linux-2.6.0~dm-daemon/drivers/md/Makefile	2003-12-22 13:17:35.000000000 +0100
@@ -3,7 +3,7 @@
 #
 
 dm-mod-objs	:= dm.o dm-table.o dm-target.o dm-linear.o dm-stripe.o \
-		   dm-ioctl.o
+		   dm-ioctl.o dm-daemon.o
 
 # Note: link order is important.  All raid personalities
 # and xor.o must come before md.o, as they each initialise 
diff -Nur linux-2.6.0.orig/drivers/md/dm.h linux-2.6.0/drivers/md/dm.h
--- linux-2.6.0.orig/drivers/md/dm.h	2003-11-24 02:31:53.000000000 +0100
+++ linux-2.6.0/drivers/md/dm.h	2003-12-22 17:56:05.000000000 +0100
@@ -20,6 +20,12 @@
 #define DMINFO(f, x...) printk(KERN_INFO DM_NAME ": " f "\n" , ## x)
 
 /*
+ * FIXME: There must be a better place for this.
+ */
+typedef typeof(jiffies) jiffy_t;
+
+
+/*
  * FIXME: I think this should be with the definition of sector_t
  * in types.h.
  */

[PATCH 2/2][RFC] Add dm-crypt target

[Christophe Saout]

Hi,

this is the actual dm-crypt target. It uses cryptoapi to achive the same
goal as cryptoloop.

It uses mempools to ensure not to ever run out of memory and can split
large IOs into smaller ones under memory pressure.

Tested by some people, also works on a swap device.


diff -Nur linux-2.6.0/drivers/md/dm-crypt.c linux-2.6.0~dm-crypt/drivers/md/dm-crypt.c
--- linux-2.6.0/drivers/md/dm-crypt.c	1970-01-01 01:00:00.000000000 +0100
+++ linux-2.6.0~dm-crypt/drivers/md/dm-crypt.c	2003-12-22 15:14:24.903024384 +0100
@@ -0,0 +1,788 @@
+/*
+ * Copyright (C) 2003 Christophe Saout <christophe@saout.de>
+ *
+ * This file is released under the GPL.
+ */
+
+#include "dm.h"
+#include "dm-daemon.h"
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/bio.h>
+#include <linux/mempool.h>
+#include <linux/slab.h>
+#include <linux/crypto.h>
+#include <linux/spinlock.h>
+#include <asm/scatterlist.h>
+
+/*
+ * per bio private data
+ */
+struct crypt_io {
+	struct dm_target *target;
+	struct bio *bio;
+	struct bio *first_clone;
+	atomic_t pending;
+	int error;
+};
+
+/*
+ * context holding the current state of a multi-part conversion
+ */
+struct convert_context {
+	struct bio *bio_in;
+	struct bio *bio_out;
+	unsigned int offset_in;
+	unsigned int offset_out;
+	int idx_in;
+	int idx_out;
+	sector_t sector;
+	int write;
+};
+
+/*
+ * Crypt: maps a linear range of a block device
+ * and encrypts / decrypts at the same time.
+ */
+struct crypt_c {
+	struct dm_dev *dev;
+	sector_t start;
+
+	/*
+	 * pool for per bio private data and
+	 * for encryption buffer pages
+	 */
+	mempool_t *io_pool;
+	mempool_t *page_pool;
+
+	/*
+	 * crypto related data
+	 */
+	struct crypto_tfm *tfm;
+	sector_t iv_offset;
+	int iv_size;
+	int key_size;
+	u8 key[0];
+};
+
+#define MIN_IOS        256
+#define MIN_POOL_PAGES 16
+#define MIN_BIO_PAGES  8
+
+static kmem_cache_t *_io_cache;
+
+/*
+ * Mempool alloc and free functions for the page and io pool
+ */
+static void *mempool_alloc_page(int gfp_mask, void *data)
+{
+	return alloc_page(gfp_mask);
+}
+
+static void mempool_free_page(void *page, void *data)
+{
+	__free_page(page);
+}
+
+static inline struct page *crypt_alloc_page(struct crypt_c *cc, int gfp_mask)
+{
+	return mempool_alloc(cc->page_pool, gfp_mask);
+}
+
+static inline void crypt_free_page(struct crypt_c *cc, struct page *page)
+{
+	 mempool_free(page, cc->page_pool);
+}
+
+static inline struct crypt_io *crypt_alloc_io(struct crypt_c *cc)
+{
+	return mempool_alloc(cc->io_pool, GFP_NOIO);
+}
+
+static inline void crypt_free_io(struct crypt_c *cc, struct crypt_io *io)
+{
+	return mempool_free(io, cc->io_pool);
+}
+
+/*
+ * Encrypt / decrypt a single sector, source and destination buffers
+ * are stored in scatterlists. In CBC mode initialise the "previous
+ * block" with the sector number (it's not a real chaining because
+ * it would not allow to seek on the device...)
+ */
+static inline int
+crypt_convert_scatterlist(struct crypt_c *cc, struct scatterlist *out,
+                          struct scatterlist *in, unsigned int length,
+                          int write, sector_t sector)
+{
+	u8 iv[cc->iv_size];
+	int r;
+
+	if (cc->iv_size) {
+		*(u32 *)iv = cpu_to_le32(sector & 0xffffffff);
+		if (cc->iv_size > sizeof(u32) / sizeof(u8))
+			memset(iv + (sizeof(u32) / sizeof(u8)), 0,
+			       cc->iv_size - (sizeof(u32) / sizeof(u8)));
+
+		if (write)
+			r = crypto_cipher_encrypt_iv(cc->tfm, out, in, length, iv);
+		else
+			r = crypto_cipher_decrypt_iv(cc->tfm, out, in, length, iv);
+	} else {
+		if (write)
+			r = crypto_cipher_encrypt(cc->tfm, out, in, length);
+		else
+			r = crypto_cipher_decrypt(cc->tfm, out, in, length);
+	}
+
+	return r;
+}
+
+static void
+crypt_convert_init(struct crypt_c *cc, struct convert_context *ctx,
+                   struct bio *bio_out, struct bio *bio_in,
+                   sector_t sector, int write)
+{
+	ctx->bio_in = bio_in;
+	ctx->bio_out = bio_out;
+	ctx->offset_in = 0;
+	ctx->offset_out = 0;
+	ctx->idx_in = bio_in ? bio_in->bi_idx : 0;
+	ctx->idx_out = bio_out ? bio_out->bi_idx : 0;
+	ctx->sector = sector + cc->iv_offset;
+	ctx->write = write;
+}
+
+/*
+ * Encrypt / decrypt data from one bio to another one (may be the same)
+ */
+static int crypt_convert(struct crypt_c *cc,
+                         struct convert_context *ctx)
+{
+	int r = 0;
+
+	while(ctx->idx_in < ctx->bio_in->bi_vcnt &&
+	      ctx->idx_out < ctx->bio_out->bi_vcnt) {
+		struct bio_vec *bv_in = bio_iovec_idx(ctx->bio_in, ctx->idx_in);
+		struct bio_vec *bv_out = bio_iovec_idx(ctx->bio_out, ctx->idx_out);
+		struct scatterlist sg_in = {
+			.page = bv_in->bv_page,
+			.offset = bv_in->bv_offset + ctx->offset_in,
+			.length = 1 << SECTOR_SHIFT
+		};
+		struct scatterlist sg_out = {
+			.page = bv_out->bv_page,
+			.offset = bv_out->bv_offset + ctx->offset_out,
+			.length = 1 << SECTOR_SHIFT
+		};
+
+		ctx->offset_in += sg_in.length;
+		if (ctx->offset_in >= bv_in->bv_len) {
+			ctx->offset_in = 0;
+			ctx->idx_in++;
+		}
+
+		ctx->offset_out += sg_out.length;
+		if (ctx->offset_out >= bv_out->bv_len) {
+			ctx->offset_out = 0;
+			ctx->idx_out++;
+		}
+
+		r = crypt_convert_scatterlist(cc, &sg_out, &sg_in, sg_in.length,
+		                              ctx->write, ctx->sector);
+		if (r < 0)
+			break;
+
+		ctx->sector++;
+	}
+
+	return r;
+}
+
+/*
+ * Generate a new unfragmented bio with the given size
+ * This should never violate the device limitations
+ * May return a smaller bio when running out of pages
+ */
+static struct bio *
+crypt_alloc_buffer(struct crypt_c *cc, unsigned int size,
+                   struct bio *base_bio, int *bio_vec_idx)
+{
+	struct bio *bio;
+	int nr_iovecs = dm_div_up(size, PAGE_SIZE);
+	int gfp_mask = GFP_NOIO | __GFP_HIGHMEM;
+	int i;
+
+	if (base_bio)
+		bio = bio_clone(base_bio, GFP_NOIO);
+	else
+		bio = bio_alloc(GFP_NOIO, nr_iovecs);
+	if (!bio)
+		return NULL;
+
+	/* if the last bio was not complete, continue where that one ends */
+	bio->bi_idx = *bio_vec_idx;
+	bio->bi_vcnt = *bio_vec_idx;
+	bio->bi_size = 0;
+
+	/* bio->bi_idx pages have already been allocated */
+	size -= bio->bi_idx * PAGE_SIZE;
+
+	for(i = bio->bi_idx; i < nr_iovecs; i++) {
+		struct bio_vec *bv = bio_iovec_idx(bio, i);
+
+		bv->bv_page = crypt_alloc_page(cc, gfp_mask);
+		if (!bv->bv_page)
+			break;
+
+		/*
+		 * if additional pages cannot be allocated without waiting
+		 * return a partially allocated bio, the caller will then try
+		 * to allocate additional bios while submitting this partial bio
+		 */
+		if ((i - bio->bi_idx) == (MIN_BIO_PAGES - 1))
+			gfp_mask = (gfp_mask | __GFP_NOWARN) & ~__GFP_WAIT;
+
+		bv->bv_offset = 0;
+		if (size > PAGE_SIZE)
+			bv->bv_len = PAGE_SIZE;
+		else
+			bv->bv_len = size;
+
+		bio->bi_size += bv->bv_len;
+		bio->bi_vcnt++;
+		size -= bv->bv_len;
+	}
+
+	if (!bio->bi_size) {
+		bio_put(bio);
+		return NULL;
+	}
+
+	/*
+	 * remember the last bio_vec allocated to be able to correctly
+	 * continue after splitting caused by memory pressure
+	 */
+	*bio_vec_idx = bio->bi_vcnt;
+
+	return bio;
+}
+
+static void crypt_free_buffer_pages(struct crypt_c *cc, struct bio *bio,
+                                    unsigned int bytes)
+{
+	int i = bio->bi_idx;
+
+	while(bytes) {
+		struct bio_vec *bv = bio_iovec_idx(bio, i++);
+		crypt_free_page(cc, bv->bv_page);
+		bytes -= bv->bv_len;
+	}
+}
+
+/*
+ * One of the bios was finished. Check for completion of
+ * the whole request and correctly cleanup the buffer.
+ */
+static void dec_pending(struct crypt_io *io, int error)
+{
+	struct crypt_c *cc = (struct crypt_c *) io->target->private;
+
+	if (!atomic_dec_and_test(&io->pending))
+		return;
+
+	if (io->first_clone)
+		bio_put(io->first_clone);
+
+	if (error < 0)
+		io->error = error;
+
+	if (io->bio)
+		bio_endio(io->bio, io->bio->bi_size, io->error);
+
+	crypt_free_io(cc, io);
+}
+
+/*
+ * kcryptd:
+ *
+ * Needed because we can't decrypt when called in an
+ * interrupt context, so returning bios from read requests get
+ * queued here.
+ */
+static spinlock_t _kcryptd_lock = SPIN_LOCK_UNLOCKED;
+static struct bio *_bio_head;
+static struct bio *_bio_tail;
+
+static struct dm_daemon _kcryptd;
+
+/*
+ * Fetch a list of the complete bios.
+ */
+static struct bio *kcryptd_get_bios(void)
+{
+	struct bio *bio;
+
+	spin_lock_irq(&_kcryptd_lock);
+	bio = _bio_head;
+	if (bio)
+		_bio_head = _bio_tail = NULL;
+	spin_unlock_irq(&_kcryptd_lock);
+
+	return bio;
+}
+
+/*
+ * Append bio to work queue
+ */
+static void kcryptd_queue_bio(struct bio *bio)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&_kcryptd_lock, flags);
+	if (_bio_tail)
+		_bio_tail->bi_next = bio;
+	else
+		_bio_head = bio;
+	_bio_tail = bio;
+	spin_unlock_irqrestore(&_kcryptd_lock, flags);
+}
+
+static jiffy_t kcryptd(void)
+{
+	int r;
+	struct bio *bio;
+	struct bio *next_bio;
+	struct crypt_io *io;
+	struct crypt_c *cc;
+	struct convert_context ctx;
+
+	bio = kcryptd_get_bios();
+
+	while (bio) {
+		io = (struct crypt_io *) bio->bi_private;
+		cc = (struct crypt_c *) io->target->private;
+
+		crypt_convert_init(cc, &ctx, io->bio, io->bio,
+		                   io->bio->bi_sector - io->target->begin, 0);
+		r = crypt_convert(cc, &ctx);
+
+		next_bio = bio->bi_next;
+
+		bio->bi_next = NULL;
+		bio_put(bio);
+		dec_pending(io, r);
+
+		bio = next_bio;
+	}
+
+	return 0;
+}
+
+/*
+ * Decode key from its hex representation
+ */
+static int crypt_decode_key(u8 *key, char *hex, int size)
+{
+	int i;
+	for(i = 0; i < size; i++) {
+		int digits;
+		if (*hex >= 'a' && *hex <= 'f')
+			digits = *hex - ('a' - 10);
+		else if (*hex >= 'A' && *hex <= 'F')
+			digits = *hex - ('A' - 10);
+		else if (*hex >= '0' && *hex <= '9')
+			digits = *hex - '0';
+		else
+			return -EINVAL;
+
+		digits <<= 4;
+		hex++;
+
+		if (*hex >= 'a' && *hex <= 'f')
+			digits += *hex - ('a' - 10);
+		else if (*hex >= 'A' && *hex <= 'F')
+			digits += *hex - ('A' - 10);
+		else if (*hex >= '0' && *hex <= '9')
+			digits += *hex - '0';
+		else
+			return -EINVAL;
+
+		hex++;
+		key[i] = (u8)digits;
+	}
+
+	if (*hex != '\0')
+		return -EINVAL;
+
+	return 0;
+}
+
+/*
+ * Encode key into its hex representation
+ */
+static void crypt_encode_key(char *hex, u8 *key, int size)
+{
+	static char hex_digits[] = "0123456789abcdef";
+	int i;
+
+	for(i = 0; i < size; i++) {
+		*hex++ = hex_digits[*key >> 4];
+		*hex++ = hex_digits[*key & 0x0f];
+		key++;
+	}
+
+	*hex++ = '\0';
+}
+
+/*
+ * Construct an encryption mapping:
+ * <cipher> <key> <iv_offset> <dev_path> <start>
+ */
+static int crypt_ctr(struct dm_target *ti, unsigned int argc, char **argv)
+{
+	struct crypt_c *cc;
+	struct crypto_tfm *tfm;
+	char *tmp;
+	char *cipher;
+	char *mode;
+	int crypto_flags;
+	int iv_size;
+	int key_size;
+
+	if (argc != 5) {
+		ti->error = "dm-crypt: Not enough arguments";
+		return -EINVAL;
+	}
+
+	tmp = argv[0];
+	cipher = strsep(&tmp, "-");
+	mode = strsep(&tmp, "-");
+
+	if (tmp)
+		DMWARN("dm-crypt: Unexpected additional cipher options");
+
+	if (!mode || strcmp(mode, "cbc") == 0)
+		crypto_flags = CRYPTO_TFM_MODE_CBC;
+	else if (strcmp(mode, "ecb") == 0)
+		crypto_flags = CRYPTO_TFM_MODE_ECB;
+	else {
+		ti->error = "dm-crypt: Invalid chaining mode";
+		return -EINVAL;
+	}
+
+	tfm = crypto_alloc_tfm(cipher, crypto_flags);
+	if (!tfm) {
+		ti->error = "dm-crypt: Error allocating crypto tfm";
+		return -EINVAL;
+	}
+
+	key_size = strlen(argv[1]) >> 1;
+	if (tfm->crt_u.cipher.cit_decrypt_iv && tfm->crt_u.cipher.cit_encrypt_iv)
+		iv_size = max(crypto_tfm_alg_ivsize(tfm), sizeof(u32) / sizeof(u8));
+	else
+		iv_size = 0;
+
+	cc = kmalloc(sizeof(*cc) + key_size * sizeof(u8), GFP_KERNEL);
+	if (cc == NULL) {
+		ti->error =
+			"dm-crypt: Cannot allocate transparent encryption context";
+		crypto_free_tfm(tfm);
+		return -ENOMEM;
+	}
+
+	cc->io_pool = mempool_create(MIN_IOS, mempool_alloc_slab,
+				     mempool_free_slab, _io_cache);
+	if (!cc->io_pool) {
+		ti->error = "dm-crypt: Cannot allocate crypt io mempool";
+		goto bad1;
+	}
+
+	cc->page_pool = mempool_create(MIN_POOL_PAGES, mempool_alloc_page,
+				       mempool_free_page, NULL);
+	if (!cc->page_pool) {
+		ti->error = "dm-crypt: Cannot allocate page mempool";
+		goto bad2;
+	}
+
+	cc->tfm = tfm;
+	cc->iv_size = iv_size;
+	cc->key_size = key_size;
+	if ((key_size == 0 && strcmp(argv[1], "-") != 0)
+	    || crypt_decode_key(cc->key, argv[1], key_size) < 0) {
+		ti->error = "dm-crypt: Error decoding key";
+		goto bad3;
+	}
+
+	if (tfm->crt_u.cipher.cit_setkey(tfm, cc->key, key_size) < 0) {
+		ti->error = "dm-crypt: Error setting key";
+		goto bad3;
+	}
+
+	if (sscanf(argv[2], SECTOR_FORMAT, &cc->iv_offset) != 1) {
+		ti->error = "dm-crypt: Invalid iv_offset sector";
+		goto bad3;
+	}
+
+	if (sscanf(argv[4], SECTOR_FORMAT, &cc->start) != 1) {
+		ti->error = "dm-crypt: Invalid device sector";
+		goto bad3;
+	}
+
+	if (dm_get_device(ti, argv[3], cc->start, ti->len,
+	                  dm_table_get_mode(ti->table), &cc->dev)) {
+		ti->error = "dm-crypt: Device lookup failed";
+		goto bad3;
+	}
+
+	ti->private = cc;
+	return 0;
+
+bad3:
+	mempool_destroy(cc->page_pool);
+bad2:
+	mempool_destroy(cc->io_pool);
+bad1:
+	crypto_free_tfm(tfm);
+	kfree(cc);
+	return -EINVAL;
+}
+
+static void crypt_dtr(struct dm_target *ti)
+{
+	struct crypt_c *cc = (struct crypt_c *) ti->private;
+
+	mempool_destroy(cc->page_pool);
+	mempool_destroy(cc->io_pool);
+
+	crypto_free_tfm(cc->tfm);
+	dm_put_device(ti, cc->dev);
+	kfree(cc);
+}
+
+static int crypt_endio(struct bio *bio, unsigned int done, int error)
+{
+	struct crypt_io *io = (struct crypt_io *) bio->bi_private;
+	struct crypt_c *cc = (struct crypt_c *) io->target->private;
+
+	if (bio_rw(bio) == WRITE) {
+		/*
+		 * free the processed pages, even if
+		 * it's only a partially completed write
+		 */
+		crypt_free_buffer_pages(cc, bio, done);
+	}
+
+	if (bio->bi_size)
+		return 1;
+
+	/*
+	 * successful reads get decrypted by the worker thread
+	 * because we never want to decrypt in an irq context
+	 */
+	if ((bio_rw(bio) == READ || bio_rw(bio) == READA)
+	    && bio_flagged(bio, BIO_UPTODATE)) {
+		kcryptd_queue_bio(bio);
+		dm_daemon_wake(&_kcryptd);
+		return 0;
+	}
+
+	bio_put(bio);
+	dec_pending(io, error);
+
+	return error;
+}
+
+static int crypt_map(struct dm_target *ti, struct bio *bio)
+{
+	struct crypt_c *cc = (struct crypt_c *) ti->private;
+	struct crypt_io *io = crypt_alloc_io(cc);
+	struct bio *clone = NULL;
+	struct convert_context ctx;
+	unsigned int remaining = bio->bi_size;
+	sector_t sector = bio->bi_sector - ti->begin;
+	int bio_vec_idx = 0;
+	int r = 0;
+
+	io->target = ti;
+	io->bio = bio;
+	io->first_clone = NULL;
+	io->error = 0;
+	atomic_set(&io->pending, 1); /* hold a reference */
+
+	if (bio_rw(bio) == WRITE)
+		crypt_convert_init(cc, &ctx, NULL, bio, sector, 1);
+
+	/*
+	 * The allocated buffers can be smaller then the whole bio,
+	 * so repeat the whole process until all the data can be handled.
+	 */
+	while (remaining) {
+		if (bio_rw(bio) == WRITE) {
+			clone = crypt_alloc_buffer(cc, bio->bi_size,
+						   io->first_clone,
+			                           &bio_vec_idx);
+			if (clone) {
+				ctx.bio_out = clone;
+				r = crypt_convert(cc, &ctx);
+				if (r < 0) {
+					crypt_free_buffer_pages(cc, clone,
+								clone->bi_size);
+					bio_put(clone);
+					goto cleanup;
+				}
+			}
+		} else
+			clone = bio_clone(bio, GFP_NOIO);
+
+		if (!clone) {
+			r = -ENOMEM;
+			goto cleanup;
+		}
+
+		if (!io->first_clone) {
+			/*
+			 * hold a reference to the first clone, because it holds
+			 * the bio_vec array and that needs to be released only
+			 * after all other clones are released
+			 */
+			bio_get(clone);
+			io->first_clone = clone;
+		}
+		atomic_inc(&io->pending);
+
+		clone->bi_private = io;
+		clone->bi_end_io = crypt_endio;
+		clone->bi_bdev = cc->dev->bdev;
+		clone->bi_sector = cc->start + sector;
+		clone->bi_rw = bio->bi_rw;
+
+		remaining -= clone->bi_size;
+		sector += bio_sectors(clone);
+
+		generic_make_request(clone);
+	}
+
+	/* drop reference, clones could have returned before we reach this */
+	dec_pending(io, 0);
+	return 0;
+
+cleanup:
+	if (io->first_clone) {
+		dec_pending(io, r);
+		return 0;
+	}
+
+	/* if no bio has been dispatched yet, we can directly return the error */
+	crypt_free_io(cc, io);
+	return r;
+}
+
+static int crypt_status(struct dm_target *ti, status_type_t type,
+			char *result, unsigned int maxlen)
+{
+	struct crypt_c *cc = (struct crypt_c *) ti->private;
+	char buffer[32];
+	const char *cipher;
+	const char *mode = NULL;
+	int offset;
+
+	switch (type) {
+	case STATUSTYPE_INFO:
+		result[0] = '\0';
+		break;
+
+	case STATUSTYPE_TABLE:
+		cipher = crypto_tfm_alg_name(cc->tfm);
+
+		switch(cc->tfm->crt_u.cipher.cit_mode) {
+		case CRYPTO_TFM_MODE_CBC:
+			mode = "cbc";
+			break;
+		case CRYPTO_TFM_MODE_ECB:
+			mode = "ecb";
+			break;
+		default:
+			BUG();
+		}
+
+		snprintf(result, maxlen, "%s-%s ", cipher, mode);
+		offset = strlen(result);
+
+		if (cc->key_size > 0) {
+			if ((maxlen - offset) < ((cc->key_size << 1) + 1))
+				return -ENOMEM;
+
+			crypt_encode_key(result + offset, cc->key, cc->key_size);
+			offset += cc->key_size << 1;
+		} else {
+			if (offset >= maxlen)
+				return -ENOMEM;
+			result[offset++] = '-';
+		}
+
+		format_dev_t(buffer, cc->dev->bdev->bd_dev);
+		snprintf(result + offset, maxlen - offset, " " SECTOR_FORMAT
+		         " %s " SECTOR_FORMAT, cc->iv_offset,
+		         buffer, cc->start);
+		break;
+	}
+	return 0;
+}
+
+static struct target_type crypt_target = {
+	.name   = "crypt",
+	.module = THIS_MODULE,
+	.ctr    = crypt_ctr,
+	.dtr    = crypt_dtr,
+	.map    = crypt_map,
+	.status = crypt_status,
+};
+
+int __init dm_crypt_init(void)
+{
+	int r;
+
+	_io_cache = kmem_cache_create("dm-crypt_io", sizeof(struct crypt_io),
+	                              0, 0, NULL, NULL);
+	if (!_io_cache)
+		return -ENOMEM;
+
+	r = dm_daemon_start(&_kcryptd, "kcryptd", kcryptd);
+	if (r) {
+		DMERR("couldn't create kcryptd: %d", r);
+		kmem_cache_destroy(_io_cache);
+		return r;
+	}
+
+	r = dm_register_target(&crypt_target);
+	if (r < 0) {
+		DMERR("crypt: register failed %d", r);
+		dm_daemon_stop(&_kcryptd);
+		kmem_cache_destroy(_io_cache);
+	}
+
+	return r;
+}
+
+void __exit dm_crypt_exit(void)
+{
+	int r = dm_unregister_target(&crypt_target);
+
+	if (r < 0)
+		DMERR("crypt: unregister failed %d", r);
+
+	dm_daemon_stop(&_kcryptd);
+	kmem_cache_destroy(_io_cache);
+}
+
+/*
+ * module hooks
+ */
+module_init(dm_crypt_init)
+module_exit(dm_crypt_exit)
+
+MODULE_AUTHOR("Christophe Saout <christophe@saout.de>");
+MODULE_DESCRIPTION(DM_NAME " target for transparent encryption / decryption");
+MODULE_LICENSE("GPL");
diff -Nur linux-2.6.0/drivers/md/Kconfig linux-2.6.0~dm-crypt/drivers/md/Kconfig
--- linux-2.6.0/drivers/md/Kconfig	2003-11-24 02:31:11.000000000 +0100
+++ linux-2.6.0~dm-crypt/drivers/md/Kconfig	2003-12-22 13:41:02.000000000 +0100
@@ -142,5 +142,17 @@
 	  Recent tools use a new version of the ioctl interface, only
           select this option if you intend using such tools.
 
+config DM_CRYPT
+	tristate "Crypt target support"
+	depends on BLK_DEV_DM && EXPERIMENTAL
+	select CRYPTO
+	---help---
+	  This device-mapper target allows you to create a device that
+	  transparently encrypts the data on it. You'll need to activate
+	  the required ciphers in the cryptoapi configuration in order to
+	  be able to use it.
+
+	  If unsure, say N.
+
 endmenu
 
diff -Nur linux-2.6.0/drivers/md/Makefile linux-2.6.0~dm-crypt/drivers/md/Makefile
--- linux-2.6.0/drivers/md/Makefile	2003-12-22 13:17:35.000000000 +0100
+++ linux-2.6.0~dm-crypt/drivers/md/Makefile	2003-12-22 13:43:11.000000000 +0100
@@ -17,3 +17,4 @@
 obj-$(CONFIG_MD_MULTIPATH)	+= multipath.o
 obj-$(CONFIG_BLK_DEV_MD)	+= md.o
 obj-$(CONFIG_BLK_DEV_DM)	+= dm-mod.o
+obj-$(CONFIG_DM_CRYPT)		+= dm-crypt.o

Re: loop driver, device-mapper and crypto

[Andrew Morton]

Christophe Saout <christophe@saout.de> wrote:
>
> In 2.6 we have a device-mapper which does such things in a much more
> generic way. I've already talked to a bunch of people working on loop
> and cryptoloop (also with Clemens Fruhwirth, the cryptoloop maintainer)
> and they all agreed that device-mapper is probably the most correct way
> to go, and would be happier if the loop driver was used for files only.

I'm not a crypto-loop user, so I am not in a position to judge whether
using dm for crypto-on-disk is feature-sufficient and adequate from an
operational point of view.

It is good that Joe-and-co are OK with it and are prepared to help support
it.  If the people who _do_ use crypto-loop like the look of the feature
set and the user interface then fine.  Certainly the loop driver has a long
history of not working very well.

So.  If those-in-the-know like it then go wild.  It would be useful to get
an opinion from the distro guys too.

However I suspect that there will be a migration issue, and that we should
continue to work to get crypto-loop functioning well, plan to remove it
from 2.8, yes?

Re: [PATCH 2/2][RFC] Add dm-crypt target

[Jeff Garzik]

Christophe Saout wrote:
> Hi,
> 
> this is the actual dm-crypt target. It uses cryptoapi to achive the same
> goal as cryptoloop.
> 
> It uses mempools to ensure not to ever run out of memory and can split
> large IOs into smaller ones under memory pressure.
> 
> Tested by some people, also works on a swap device.


I like this.   Nice and clean.

May I assume that this is -not- intended as a replacement for 
cryptoloop?  I hope so, as that would be my recommendation:  new driver, 
new on-disk format.

cryptoloop predates block layer stacking, and also support files, so I 
would prefer to emphasize their differences.  A replacement for 
cryptoloop means you must support cryptoloop's on-disk format.

	Jeff

Re: [PATCH 2/2][RFC] Add dm-crypt target

[Mike Fedyk]

On Mon, Dec 22, 2003 at 06:07:57PM -0500, Jeff Garzik wrote:
> would prefer to emphasize their differences.  A replacement for 
> cryptoloop means you must support cryptoloop's on-disk format.

Do you dislike the cryptoloop format?

What if you wanted to take a disk that was used with dm-crypt, and copy it
to a file on a larger filesystem?  Would the data now be inaccessable
because it's not in a format mountable by a loop driver?

Re: [PATCH 2/2][RFC] Add dm-crypt target

[Jeff Garzik]

Mike Fedyk wrote:
> On Mon, Dec 22, 2003 at 06:07:57PM -0500, Jeff Garzik wrote:
> 
>>would prefer to emphasize their differences.  A replacement for 
>>cryptoloop means you must support cryptoloop's on-disk format.
> 
> 
> Do you dislike the cryptoloop format?
> 
> What if you wanted to take a disk that was used with dm-crypt, and copy it
> to a file on a larger filesystem?  Would the data now be inaccessable
> because it's not in a format mountable by a loop driver?


Remember we are talking about two -totally different- drivers here.

I can't take my reiserfs data, copy it to a file on a larger filesystem, 
and then mount it with ext3.  And that's a good thing.

dm-crypt should not be constrained by cryptoloop, and vice versa.

	Jeff

Re: [PATCH 2/2][RFC] Add dm-crypt target

[Mike Fedyk]

On Mon, Dec 22, 2003 at 06:29:13PM -0500, Jeff Garzik wrote:
> Mike Fedyk wrote:
> >Do you dislike the cryptoloop format?
> >
> >What if you wanted to take a disk that was used with dm-crypt, and copy it
> >to a file on a larger filesystem?  Would the data now be inaccessable
> >because it's not in a format mountable by a loop driver?
> 
> Remember we are talking about two -totally different- drivers here.
> 
> I can't take my reiserfs data, copy it to a file on a larger filesystem, 
> and then mount it with ext3.  And that's a good thing.
> 

Aww, why not? ;)

> dm-crypt should not be constrained by cryptoloop, and vice versa.

It seems dm-crypt was meant to overcome the problems with loop against block
devices.  If it uses another format, it would loose that ability to be a
replacement, and unless there are shortcomings in the format, why should
there be a change?

Also, while cryptoloop on block devices may be bass ackwards to get
encryption (use a driver meant to turn files into block devices on another
block device since there is now crypto tied into it...), if there's another
format, won't that data become inaccessable unless it's in a block device,
or do you get the dm-crypt -> loop -> file in the case a dm-crypt image gets
copied to a file?

Re: [PATCH 2/2][RFC] Add dm-crypt target

[Christophe Saout]

Am Di, den 23.12.2003 schrieb Mike Fedyk um 00:50:

> > dm-crypt should not be constrained by cryptoloop, and vice versa.
> 
> It seems dm-crypt was meant to overcome the problems with loop against block
> devices.  If it uses another format, it would loose that ability to be a
> replacement, and unless there are shortcomings in the format, why should
> there be a change?

The target option line is quite flexible. It could be used to list the
required crypto features. If only an encryption cipher is selected, it
would be backward compatible with the cryptoloop on-disk format (like it
is now). But when additional options are given IV hashing could be
turned on, another option could turn on block shuffling or something.

I'm currently having a private conversation with the cryptoloop
maintainer. The possibilities in cryptoloop are mainly restricted by the
losetup interface, he would love to have more possibilities. There are a
lot of things that could and should be implemented.

> Also, while cryptoloop on block devices may be bass ackwards to get
> encryption (use a driver meant to turn files into block devices on another
> block device since there is now crypto tied into it...), if there's another
> format, won't that data become inaccessable unless it's in a block device,
> or do you get the dm-crypt -> loop -> file in the case a dm-crypt image gets
> copied to a file?

dm-crypt -> loop -> file should work. Just like LVM on top of a file,
I've already done that before.

--
Christophe Saout <christophe@saout.de>
Please avoid sending me Word or PowerPoint attachments.
See http://www.fsf.org/philosophy/no-word-attachments.html

Re: [PATCH 2/2][RFC] Add dm-crypt target

[Rusty Russell]

On Mon, 22 Dec 2003 22:52:36 +0100
Christophe Saout <christophe@saout.de> wrote:

> Hi,

Hi Christophe!

	One anal comment:
> +/*
> + * module hooks
> + */
> +module_init(dm_crypt_init)
> +module_exit(dm_crypt_exit)

Please use semicolons here.  It's nicer C-looking, and it means we don't have
to do silly things in the macros.

Thanks,
Rusty.
-- 
   there are those who do and those who hang on and you don't see too
   many doers quoting their contemporaries.  -- Larry McVoy

Re: loop driver, device-mapper and crypto

[Fruhwirth Clemens]

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

On Mon, Dec 22, 2003 at 02:03:05PM -0800, Andrew Morton wrote:
> Christophe Saout <christophe@saout.de> wrote:
> >
> > In 2.6 we have a device-mapper which does such things in a much more
> > generic way. I've already talked to a bunch of people working on loop
> > and cryptoloop (also with Clemens Fruhwirth, the cryptoloop maintainer)
> > and they all agreed that device-mapper is probably the most correct way
> > to go, and would be happier if the loop driver was used for files only.
> 
> I'm not a crypto-loop user, so I am not in a position to judge whether
> using dm for crypto-on-disk is feature-sufficient and adequate from an
> operational point of view.

First of all, eventhou I'm the maintainer of cryptoloop, when Christophe
posted the first time I immediately recognized that dm-crypt is vastly
superior to cryptoloop for a number of reasons:

.) It does not suffer from loop.c bugs (There are a lot, no maintainer)
.) dm-crypt does not depend on special user space tool (util-linux)
.) dm-crypt uses mempool, which makes it rock stable compared to cryptoloop. 

From an operational point of view, patching util-linux has been the most
troublesome point. Lot's of incompatible inofficial patches floating around
in the net, while the last release of util-linux provides a broken and IMHO
insecure why of setting up cryptokeys. Further: To fix the design flaw of
unencrypted/unhashed IV vectors one has to add another setup parameter.
That's where I really do like the flexible interface dm has.

> However I suspect that there will be a migration issue, and that we should
> continue to work to get crypto-loop functioning well, plan to remove it
> from 2.8, yes?

We should support cryptoloop. No new features, but working well. At the same
time we should declare it 'deprecated' and provide dm-crypt as alternative.
I'm happy to provide documentation on how to migrate.

I will review Christophe's patch as well as test compatiblity with
cryptoloop. Expect my results in a few days.

Regards, Clemens

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

Re: [PATCH 1/2] Add dm-daemon

[Christoph Hellwig]

On Mon, Dec 22, 2003 at 10:49:52PM +0100, Christophe Saout wrote:
> Hi.
> 
> The first patch adds dm-daemon.c/.h.
> 
> The code is from Joe Thornbers current unstable device-mapper patchset.

This should really be in generic code, not in DM.  I remember I did
something similar as kthread abstraction a while ago, but it didn't head
anywhere..

> diff -Nur linux-2.6.0.orig/drivers/md/dm.h linux-2.6.0/drivers/md/dm.h
> --- linux-2.6.0.orig/drivers/md/dm.h	2003-11-24 02:31:53.000000000 +0100
> +++ linux-2.6.0/drivers/md/dm.h	2003-12-22 17:56:05.000000000 +0100
> @@ -20,6 +20,12 @@
>  #define DMINFO(f, x...) printk(KERN_INFO DM_NAME ": " f "\n" , ## x)
>  
>  /*
> + * FIXME: There must be a better place for this.
> + */
> +typedef typeof(jiffies) jiffy_t;

Eeek..

Re: [PATCH 2/2][RFC] Add dm-crypt target

[Christoph Hellwig]

[Fruhwirth dropped from Cc list due to his braindead spam "filter"]

On Mon, Dec 22, 2003 at 10:52:36PM +0100, Christophe Saout wrote:
> +#include "dm.h"
> +#include "dm-daemon.h"

Please include driver-private headers after kernel headers.

> +/*
> + * Crypt: maps a linear range of a block device
> + * and encrypts / decrypts at the same time.
> + */
> +struct crypt_c {

crypt_context?  The current name isn't very descriptive..

> +static kmem_cache_t *_io_cache;

Again a rather strange variable name.  What about dm_crypt_pool?

> +
> +/*
> + * Mempool alloc and free functions for the page and io pool
> + */
> +static void *mempool_alloc_page(int gfp_mask, void *data)
> +{
> +	return alloc_page(gfp_mask);
> +}
> +
> +static void mempool_free_page(void *page, void *data)
> +{
> +	__free_page(page);
> +}

Should probably go into generic code one day, along with the slab wrappers.

> +
> +static inline struct page *crypt_alloc_page(struct crypt_c *cc, int gfp_mask)
> +{
> +	return mempool_alloc(cc->page_pool, gfp_mask);
> +}
> +
> +static inline void crypt_free_page(struct crypt_c *cc, struct page *page)
> +{
> +	 mempool_free(page, cc->page_pool);
> +}
> +
> +static inline struct crypt_io *crypt_alloc_io(struct crypt_c *cc)
> +{
> +	return mempool_alloc(cc->io_pool, GFP_NOIO);
> +}
> +
> +static inline void crypt_free_io(struct crypt_c *cc, struct crypt_io *io)
> +{
> +	return mempool_free(io, cc->io_pool);
> +}

Please kill these superflous wrappers.

> +static spinlock_t _kcryptd_lock = SPIN_LOCK_UNLOCKED;
> +static struct bio *_bio_head;
> +static struct bio *_bio_tail;
> +
> +static struct dm_daemon _kcryptd;

Again, rather strange naming..

> +int __init dm_crypt_init(void)

static?

> +void __exit dm_crypt_exit(void)

static?

> +/*
> + * module hooks
> + */

Comment looks slightly superflous :)

Else the code look great and definitly better han cryptoloop from the
conceptual point of view.

Re: [PATCH 2/2][RFC] Add dm-crypt target

[Christophe Saout]

Am Di, den 23.12.2003 schrieb Christoph Hellwig um 14:13:

> Please include driver-private headers after kernel headers.
>
> > +struct crypt_c {
> 
> crypt_context?  The current name isn't very descriptive..
> 
> > +static kmem_cache_t *_io_cache;
> 
> Again a rather strange variable name.  What about dm_crypt_pool?
>
> Please kill these superflous wrappers.
> 
> > +static spinlock_t _kcryptd_lock = SPIN_LOCK_UNLOCKED;
> > +static struct bio *_bio_head;
> > +static struct bio *_bio_tail;
> > +
> > +static struct dm_daemon _kcryptd;
> 
> Again, rather strange naming..

This was done to be consistent with the other device-mapper code. I can
change it though.

Joe?

--
Christophe Saout <christophe@saout.de>
Please avoid sending me Word or PowerPoint attachments.
See http://www.fsf.org/philosophy/no-word-attachments.html

Re: [PATCH 1/2] Add dm-daemon

[Joe Thornber]

On Tue, Dec 23, 2003 at 12:27:51PM +0000, Christoph Hellwig wrote:
> On Mon, Dec 22, 2003 at 10:49:52PM +0100, Christophe Saout wrote:
> > Hi.
> > 
> > The first patch adds dm-daemon.c/.h.
> > 
> > The code is from Joe Thornbers current unstable device-mapper patchset.
> 
> This should really be in generic code, not in DM.  I remember I did
> something similar as kthread abstraction a while ago, but it didn't head
> anywhere..

Agreed, but lets put it into drivers/md to start with and move it
later when it matures.  There are still a couple of interface changes
I want to make, and I'm not yet convinced that I've got it yielding
enough on 2.6.

- Joe

Re: [PATCH 2/2][RFC] Add dm-crypt target

[Joe Thornber]

On Tue, Dec 23, 2003 at 02:36:22PM +0100, Christophe Saout wrote:
> Am Di, den 23.12.2003 schrieb Christoph Hellwig um 14:13:
> 
> > Please include driver-private headers after kernel headers.

I tend to do things this way round to ensure that the private headers
have correctly included all that they need, rather than relying on the
accidental inclusion of a standard header.

> > > +static struct dm_daemon _kcryptd;
> > 
> > Again, rather strange naming..
> 
> This was done to be consistent with the other device-mapper code. I can
> change it though.

Kernel CRYPT Daemon

In the same way we have a kmirrord, kcopyd etc.  I'm happy with the
name.

- Joe

Re: [PATCH 2/2][RFC] Add dm-crypt target

[Jeff Garzik]

On Tue, Dec 23, 2003 at 03:15:45PM +0000, Joe Thornber wrote:
> On Tue, Dec 23, 2003 at 02:36:22PM +0100, Christophe Saout wrote:
> > Am Di, den 23.12.2003 schrieb Christoph Hellwig um 14:13:
> > 
> > > Please include driver-private headers after kernel headers.
> 
> I tend to do things this way round to ensure that the private headers
> have correctly included all that they need, rather than relying on the
> accidental inclusion of a standard header.

I agree w/ Christoph...  overly defensive programming like this just
creates a new class of programmer errors, doesn't really solve anything.
It's standard Linux kernel style, and making code look like all other
code has benefits in review and debugging.  Finally, the programmer
should be paying attention to what kernel APIs he/she uses, and add
headers accordingly.


> > > > +static struct dm_daemon _kcryptd;
> > > 
> > > Again, rather strange naming..
> > 
> > This was done to be consistent with the other device-mapper code. I can
> > change it though.
> 
> Kernel CRYPT Daemon
> 
> In the same way we have a kmirrord, kcopyd etc.  I'm happy with the
> name.

Not sure, but I think he meant the unneeded "_" prefix.

	Jeff

Re: [PATCH 2/2][RFC] Add dm-crypt target

[Joe Thornber]

On Tue, Dec 23, 2003 at 10:31:43AM -0500, Jeff Garzik wrote:
> I agree w/ Christoph...  overly defensive programming like this just
> creates a new class of programmer errors, doesn't really solve anything.
> It's standard Linux kernel style, and making code look like all other
> code has benefits in review and debugging.  Finally, the programmer
> should be paying attention to what kernel APIs he/she uses, and add
> headers accordingly.

ok, I don't feel strongly enough about it to argue, so we'll change it.

- Joe

Re: [PATCH 2/2][RFC] Add dm-crypt target

[Christophe Saout]

On Tue, Dec 23, 2003 at 03:43:25PM +0000, Joe Thornber wrote:

> On Tue, Dec 23, 2003 at 10:31:43AM -0500, Jeff Garzik wrote:
> > I agree w/ Christoph...  overly defensive programming like this just
> > creates a new class of programmer errors, doesn't really solve anything.
> > It's standard Linux kernel style, and making code look like all other
> > code has benefits in review and debugging.  Finally, the programmer
> > should be paying attention to what kernel APIs he/she uses, and add
> > headers accordingly.
> 
> ok, I don't feel strongly enough about it to argue, so we'll change it.

Ok, me too. :-)

So how does this look?

--- linux.orig/drivers/md/dm-crypt.c	2003-12-23 16:43:48.409077048 +0100
+++ linux/drivers/md/dm-crypt.c	2003-12-23 16:52:11.946527720 +0100
@@ -4,9 +4,6 @@
  * This file is released under the GPL.
  */
 
-#include "dm.h"
-#include "dm-daemon.h"
-
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/bio.h>
@@ -16,6 +13,9 @@
 #include <linux/spinlock.h>
 #include <asm/scatterlist.h>
 
+#include "dm.h"
+#include "dm-daemon.h"
+
 /*
  * per bio private data
  */
@@ -70,10 +70,10 @@
 #define MIN_POOL_PAGES 16
 #define MIN_BIO_PAGES  8
 
-static kmem_cache_t *_io_cache;
+static kmem_cache_t *crypt_io_pool;
 
 /*
- * Mempool alloc and free functions for the page and io pool
+ * Mempool alloc and free functions for the page
  */
 static void *mempool_alloc_page(int gfp_mask, void *data)
 {
@@ -85,26 +85,6 @@
 	__free_page(page);
 }
 
-static inline struct page *crypt_alloc_page(struct crypt_c *cc, int gfp_mask)
-{
-	return mempool_alloc(cc->page_pool, gfp_mask);
-}
-
-static inline void crypt_free_page(struct crypt_c *cc, struct page *page)
-{
-	 mempool_free(page, cc->page_pool);
-}
-
-static inline struct crypt_io *crypt_alloc_io(struct crypt_c *cc)
-{
-	return mempool_alloc(cc->io_pool, GFP_NOIO);
-}
-
-static inline void crypt_free_io(struct crypt_c *cc, struct crypt_io *io)
-{
-	return mempool_free(io, cc->io_pool);
-}
-
 /*
  * Encrypt / decrypt a single sector, source and destination buffers
  * are stored in scatterlists. In CBC mode initialise the "previous
@@ -232,7 +212,7 @@
 	for(i = bio->bi_idx; i < nr_iovecs; i++) {
 		struct bio_vec *bv = bio_iovec_idx(bio, i);
 
-		bv->bv_page = crypt_alloc_page(cc, gfp_mask);
+		bv->bv_page = mempool_alloc(cc->page_pool, gfp_mask);
 		if (!bv->bv_page)
 			break;
 
@@ -276,7 +256,7 @@
 
 	while(bytes) {
 		struct bio_vec *bv = bio_iovec_idx(bio, i++);
-		crypt_free_page(cc, bv->bv_page);
+		mempool_free(bv->bv_page, cc->page_pool);
 		bytes -= bv->bv_len;
 	}
 }
@@ -301,7 +281,7 @@
 	if (io->bio)
 		bio_endio(io->bio, io->bio->bi_size, io->error);
 
-	crypt_free_io(cc, io);
+	mempool_free(io, cc->io_pool);
 }
 
 /*
@@ -311,11 +291,11 @@
  * interrupt context, so returning bios from read requests get
  * queued here.
  */
-static spinlock_t _kcryptd_lock = SPIN_LOCK_UNLOCKED;
-static struct bio *_bio_head;
-static struct bio *_bio_tail;
+static spinlock_t kcryptd_lock = SPIN_LOCK_UNLOCKED;
+static struct bio *kcryptd_bio_head;
+static struct bio *kcryptd_bio_tail;
 
-static struct dm_daemon _kcryptd;
+static struct dm_daemon kcryptd;
 
 /*
  * Fetch a list of the complete bios.
@@ -324,11 +304,11 @@
 {
 	struct bio *bio;
 
-	spin_lock_irq(&_kcryptd_lock);
-	bio = _bio_head;
+	spin_lock_irq(&kcryptd_lock);
+	bio = kcryptd_bio_head;
 	if (bio)
-		_bio_head = _bio_tail = NULL;
-	spin_unlock_irq(&_kcryptd_lock);
+		kcryptd_bio_head = kcryptd_bio_tail = NULL;
+	spin_unlock_irq(&kcryptd_lock);
 
 	return bio;
 }
@@ -340,16 +320,16 @@
 {
 	unsigned long flags;
 
-	spin_lock_irqsave(&_kcryptd_lock, flags);
-	if (_bio_tail)
-		_bio_tail->bi_next = bio;
+	spin_lock_irqsave(&kcryptd_lock, flags);
+	if (kcryptd_bio_tail)
+		kcryptd_bio_tail->bi_next = bio;
 	else
-		_bio_head = bio;
-	_bio_tail = bio;
-	spin_unlock_irqrestore(&_kcryptd_lock, flags);
+		kcryptd_bio_head = bio;
+	kcryptd_bio_tail = bio;
+	spin_unlock_irqrestore(&kcryptd_lock, flags);
 }
 
-static jiffy_t kcryptd(void)
+static jiffy_t kcryptd_do_work(void)
 {
 	int r;
 	struct bio *bio;
@@ -493,7 +473,7 @@
 	}
 
 	cc->io_pool = mempool_create(MIN_IOS, mempool_alloc_slab,
-				     mempool_free_slab, _io_cache);
+				     mempool_free_slab, crypt_io_pool);
 	if (!cc->io_pool) {
 		ti->error = "dm-crypt: Cannot allocate crypt io mempool";
 		goto bad1;
@@ -584,7 +564,7 @@
 	if ((bio_rw(bio) == READ || bio_rw(bio) == READA)
 	    && bio_flagged(bio, BIO_UPTODATE)) {
 		kcryptd_queue_bio(bio);
-		dm_daemon_wake(&_kcryptd);
+		dm_daemon_wake(&kcryptd);
 		return 0;
 	}
 
@@ -597,7 +577,7 @@
 static int crypt_map(struct dm_target *ti, struct bio *bio)
 {
 	struct crypt_c *cc = (struct crypt_c *) ti->private;
-	struct crypt_io *io = crypt_alloc_io(cc);
+	struct crypt_io *io = mempool_alloc(cc->io_pool, GFP_NOIO);
 	struct bio *clone = NULL;
 	struct convert_context ctx;
 	unsigned int remaining = bio->bi_size;
@@ -675,7 +655,7 @@
 	}
 
 	/* if no bio has been dispatched yet, we can directly return the error */
-	crypt_free_io(cc, io);
+	mempool_free(io, cc->io_pool);
 	return r;
 }
 
@@ -740,48 +720,45 @@
 	.status = crypt_status,
 };
 
-int __init dm_crypt_init(void)
+static int __init dm_crypt_init(void)
 {
 	int r;
 
-	_io_cache = kmem_cache_create("dm-crypt_io", sizeof(struct crypt_io),
-	                              0, 0, NULL, NULL);
-	if (!_io_cache)
+	crypt_io_pool = kmem_cache_create("dm-crypt_io", sizeof(struct crypt_io),
+	                                  0, 0, NULL, NULL);
+	if (!crypt_io_pool)
 		return -ENOMEM;
 
-	r = dm_daemon_start(&_kcryptd, "kcryptd", kcryptd);
+	r = dm_daemon_start(&kcryptd, "kcryptd", kcryptd_do_work);
 	if (r) {
 		DMERR("couldn't create kcryptd: %d", r);
-		kmem_cache_destroy(_io_cache);
+		kmem_cache_destroy(crypt_io_pool);
 		return r;
 	}
 
 	r = dm_register_target(&crypt_target);
 	if (r < 0) {
 		DMERR("crypt: register failed %d", r);
-		dm_daemon_stop(&_kcryptd);
-		kmem_cache_destroy(_io_cache);
+		dm_daemon_stop(&kcryptd);
+		kmem_cache_destroy(crypt_io_pool);
 	}
 
 	return r;
 }
 
-void __exit dm_crypt_exit(void)
+static void __exit dm_crypt_exit(void)
 {
 	int r = dm_unregister_target(&crypt_target);
 
 	if (r < 0)
 		DMERR("crypt: unregister failed %d", r);
 
-	dm_daemon_stop(&_kcryptd);
-	kmem_cache_destroy(_io_cache);
+	dm_daemon_stop(&kcryptd);
+	kmem_cache_destroy(crypt_io_pool);
 }
 
-/*
- * module hooks
- */
-module_init(dm_crypt_init)
-module_exit(dm_crypt_exit)
+module_init(dm_crypt_init);
+module_exit(dm_crypt_exit);
 
 MODULE_AUTHOR("Christophe Saout <christophe@saout.de>");
 MODULE_DESCRIPTION(DM_NAME " target for transparent encryption / decryption");

Re: [PATCH 2/2][RFC] Add dm-crypt target

[Jeff Garzik]

On Tue, Dec 23, 2003 at 05:15:41PM +0000, Joe Thornber wrote:
> On Tue, Dec 23, 2003 at 06:01:18PM +0100, Christophe Saout wrote:
> > So how does this look?
> 
> <snip>
> 
> > -static struct dm_daemon _kcryptd;
> > +static struct dm_daemon kcryptd;
> 
> Please keep the leading underscore, it indicates that it is static
> data; a convention that is used throughout dm.  hch was ok about it
> last time he noticed it, so he can't really complain now ;)

ewwww...  The programmer knows what is static data without needing a
leading underscore.

This practice is wrong for the same reason that Hungarian notation is
wrong :)

	Jeff

Re: [PATCH 2/2][RFC] Add dm-crypt target

[Joe Thornber]

On Tue, Dec 23, 2003 at 06:01:18PM +0100, Christophe Saout wrote:
> So how does this look?

<snip>

> -static struct dm_daemon _kcryptd;
> +static struct dm_daemon kcryptd;

Please keep the leading underscore, it indicates that it is static
data; a convention that is used throughout dm.  hch was ok about it
last time he noticed it, so he can't really complain now ;)

- Joe