Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 610f2de3 authored by Mikulas Patocka's avatar Mikulas Patocka Committed by Mike Snitzer
Browse files

dm crypt: fix cpu hotplug crash by removing per-cpu structure 



The DM crypt target used per-cpu structures to hold pointers to a
ablkcipher_request structure.  The code assumed that the work item keeps
executing on a single CPU, so it didn't use synchronization when
accessing this structure.

If a CPU is disabled by writing 0 to /sys/devices/system/cpu/cpu*/online,
the work item could be moved to another CPU.  This causes dm-crypt
crashes, like the following, because the code starts using an incorrect
ablkcipher_request:

 smpboot: CPU 7 is now offline
 BUG: unable to handle kernel NULL pointer dereference at 0000000000000130
 IP: [<ffffffffa1862b3d>] crypt_convert+0x12d/0x3c0 [dm_crypt]
 ...
 Call Trace:
  [<ffffffffa1864415>] ? kcryptd_crypt+0x305/0x470 [dm_crypt]
  [<ffffffff81062060>] ? finish_task_switch+0x40/0xc0
  [<ffffffff81052a28>] ? process_one_work+0x168/0x470
  [<ffffffff8105366b>] ? worker_thread+0x10b/0x390
  [<ffffffff81053560>] ? manage_workers.isra.26+0x290/0x290
  [<ffffffff81058d9f>] ? kthread+0xaf/0xc0
  [<ffffffff81058cf0>] ? kthread_create_on_node+0x120/0x120
  [<ffffffff813464ac>] ? ret_from_fork+0x7c/0xb0
  [<ffffffff81058cf0>] ? kthread_create_on_node+0x120/0x120

Fix this bug by removing the per-cpu definition.  The structure
ablkcipher_request is accessed via a pointer from convert_context.
Consequently, if the work item is rescheduled to a different CPU, the
thread still uses the same ablkcipher_request.

This change may undermine performance improvements intended by commit
c0297721 ("dm crypt: scale to multiple cpus") on select hardware.  In
practice no performance difference was observed on recent hardware.  But
regardless, correctness is more important than performance.

Signed-off-by: default avatarMikulas Patocka <mpatocka@redhat.com>
Signed-off-by: default avatarMike Snitzer <snitzer@redhat.com>
Cc: stable@vger.kernel.org
parent d6d211db

drivers/md/dm-crypt.c

+12 −49
Original line number Diff line number Diff line
@@ -19,7 +19,6 @@ 
#include <linux/crypto.h>

#include <linux/workqueue.h>

#include <linux/backing-dev.h>

#include <linux/percpu.h>

#include <linux/atomic.h>

#include <linux/scatterlist.h>

#include <asm/page.h>
@@ -43,6 +42,7 @@ struct convert_context { 
	struct bvec_iter iter_out;

	sector_t cc_sector;

	atomic_t cc_pending;

	struct ablkcipher_request *req;

};



/*
@@ -111,15 +111,7 @@ struct iv_tcw_private { 
enum flags { DM_CRYPT_SUSPENDED, DM_CRYPT_KEY_VALID };



/*

 * Duplicated per-CPU state for cipher.

 */

struct crypt_cpu {

	struct ablkcipher_request *req;

};



/*

 * The fields in here must be read only after initialization,

 * changing state should be in crypt_cpu.

 * The fields in here must be read only after initialization.

 */

struct crypt_config {

	struct dm_dev *dev;
@@ -150,12 +142,6 @@ struct crypt_config { 
	sector_t iv_offset;

	unsigned int iv_size;



	/*

	 * Duplicated per cpu state. Access through

	 * per_cpu_ptr() only.

	 */

	struct crypt_cpu __percpu *cpu;



	/* ESSIV: struct crypto_cipher *essiv_tfm */

	void *iv_private;

	struct crypto_ablkcipher **tfms;
@@ -192,11 +178,6 @@ static void clone_init(struct dm_crypt_io *, struct bio *); 
static void kcryptd_queue_crypt(struct dm_crypt_io *io);

static u8 *iv_of_dmreq(struct crypt_config *cc, struct dm_crypt_request *dmreq);



static struct crypt_cpu *this_crypt_config(struct crypt_config *cc)

{

	return this_cpu_ptr(cc->cpu);

}



/*

 * Use this to access cipher attributes that are the same for each CPU.

 */
@@ -903,16 +884,15 @@ static void kcryptd_async_done(struct crypto_async_request *async_req, 
static void crypt_alloc_req(struct crypt_config *cc,

			    struct convert_context *ctx)

{

	struct crypt_cpu *this_cc = this_crypt_config(cc);

	unsigned key_index = ctx->cc_sector & (cc->tfms_count - 1);



	if (!this_cc->req)

		this_cc->req = mempool_alloc(cc->req_pool, GFP_NOIO);

	if (!ctx->req)

		ctx->req = mempool_alloc(cc->req_pool, GFP_NOIO);



	ablkcipher_request_set_tfm(this_cc->req, cc->tfms[key_index]);

	ablkcipher_request_set_callback(this_cc->req,

	ablkcipher_request_set_tfm(ctx->req, cc->tfms[key_index]);

	ablkcipher_request_set_callback(ctx->req,

	    CRYPTO_TFM_REQ_MAY_BACKLOG | CRYPTO_TFM_REQ_MAY_SLEEP,

	    kcryptd_async_done, dmreq_of_req(cc, this_cc->req));

	    kcryptd_async_done, dmreq_of_req(cc, ctx->req));

}



/*
@@ -921,7 +901,6 @@ static void crypt_alloc_req(struct crypt_config *cc, 
static int crypt_convert(struct crypt_config *cc,

			 struct convert_context *ctx)

{

	struct crypt_cpu *this_cc = this_crypt_config(cc);

	int r;



	atomic_set(&ctx->cc_pending, 1);
@@ -932,7 +911,7 @@ static int crypt_convert(struct crypt_config *cc, 


		atomic_inc(&ctx->cc_pending);



		r = crypt_convert_block(cc, ctx, this_cc->req);

		r = crypt_convert_block(cc, ctx, ctx->req);



		switch (r) {

		/* async */
@@ -941,7 +920,7 @@ static int crypt_convert(struct crypt_config *cc, 
			reinit_completion(&ctx->restart);

			/* fall through*/

		case -EINPROGRESS:

			this_cc->req = NULL;

			ctx->req = NULL;

			ctx->cc_sector++;

			continue;
@@ -1040,6 +1019,7 @@ static struct dm_crypt_io *crypt_io_alloc(struct crypt_config *cc, 
	io->sector = sector;

	io->error = 0;

	io->base_io = NULL;

	io->ctx.req = NULL;

	atomic_set(&io->io_pending, 0);



	return io;
@@ -1065,6 +1045,8 @@ static void crypt_dec_pending(struct dm_crypt_io *io) 
	if (!atomic_dec_and_test(&io->io_pending))

		return;



	if (io->ctx.req)

		mempool_free(io->ctx.req, cc->req_pool);

	mempool_free(io, cc->io_pool);



	if (likely(!base_io))
@@ -1492,8 +1474,6 @@ static int crypt_wipe_key(struct crypt_config *cc) 
static void crypt_dtr(struct dm_target *ti)

{

	struct crypt_config *cc = ti->private;

	struct crypt_cpu *cpu_cc;

	int cpu;



	ti->private = NULL;
@@ -1505,13 +1485,6 @@ static void crypt_dtr(struct dm_target *ti) 
	if (cc->crypt_queue)

		destroy_workqueue(cc->crypt_queue);



	if (cc->cpu)

		for_each_possible_cpu(cpu) {

			cpu_cc = per_cpu_ptr(cc->cpu, cpu);

			if (cpu_cc->req)

				mempool_free(cpu_cc->req, cc->req_pool);

		}



	crypt_free_tfms(cc);



	if (cc->bs)
@@ -1530,9 +1503,6 @@ static void crypt_dtr(struct dm_target *ti) 
	if (cc->dev)

		dm_put_device(ti, cc->dev);



	if (cc->cpu)

		free_percpu(cc->cpu);



	kzfree(cc->cipher);

	kzfree(cc->cipher_string);
@@ -1588,13 +1558,6 @@ static int crypt_ctr_cipher(struct dm_target *ti, 
	if (tmp)

		DMWARN("Ignoring unexpected additional cipher options");



	cc->cpu = __alloc_percpu(sizeof(*(cc->cpu)),

				 __alignof__(struct crypt_cpu));

	if (!cc->cpu) {

		ti->error = "Cannot allocate per cpu state";

		goto bad_mem;

	}



	/*

	 * For compatibility with the original dm-crypt mapping format, if

	 * only the cipher name is supplied, use cbc-plain.