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

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

[Andi Kleen]

Christophe Saout <christophe@saout.de> writes:

> 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.

Is the IV argument compatible to block backed loop? 

If not it would not be possible to use existing block based
loop crypto file systems this way.

-Andi

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

[Christophe Saout]

Am Mo, den 22.12.2003 schrieb Andi Kleen um 23:29:

> > this is the actual dm-crypt target. It uses cryptoapi to achive the same
> > goal as cryptoloop.
>
> Is the IV argument compatible to block backed loop? 

Yes. You can mount filesystems created on cryptoloop devices with
dm-crypt.

There are plans to go beyond what cryptoloop can do (without giving up
compatibility) though, like being able to use a hash of the sector
number as iv using a digest algorithm for better security.

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

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 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: [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: [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 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

[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

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