Hi Clemens & all,
recently was in cryptsetup introduced udevadm (udevsettle) call.
That solved some problems, but introduced many others:
- if cryptsetup is started through udev rules, it deadlocks (waiting
for itself) and need full timeout to recover (180 or 30 seconds)
- if udevd is not running yet (early installation, recovery, etc)
it waits for this timeout too
- it waits for other devices changes unnecessarily.
why it should wait for all disc to settle?
- using system() call in software like cryptsetup should be avoided
IMHO the udev command introduction is the not good idea.
I tried to install Fedora recently, using 3 encrypted logical volumes.
Every volume, two udevsettle calls, 6 x 180 timeout. Unusable without
manually kill the udevsettle process!
What's the problem - udev triggers activation of temporary-cryptsetup-$PID
device, keeping it open. So meaning of udevsettle is to ensure that
all udev scanning is done and no process has this temporary device open.
(So it is problem of wrong udev rules in the first case!)
But we can use device mapper and remap open device, similar way how
"dmsetup -f remove works" instead of waiting. See attached patch.
Please could we submit it upstream? Better ideas welcome:)
Patch description:
- It adds force flag to device-mapper backend remove call.
If force flag is specified, temporary mapped device is replaced by
read only error segment (all IO will return -EIO now).
It basically finish ongoing IO transaction and
all following IOs should finish immediately with error.
So the udev scanning should be finished very quickly now :-)
Force flag must not be set for real mapping, if real (not temporary)
mapped device is still open, there can be mounted fs or so - so it
is correct to fail here.
- After replacing with error device, It tries to remove temporary-device
completely - RETRY_COUNT times. In the worst case it fails (someone
still blocking temporary device), but underlying crypt device is
now not used in mapping, so we can continue safely.
(IOW: in the worst case it keeps temporary error mapped device in system.
If this happens, the problem is for sure in some system cfg, nobody
should try to open temporary cryptsetup devices. But it is not blocking
cryptsetup from operation now. It is workaround till udev rules are fixed in all
distros to not touch temporary cryptsetup devices...)
- It removes udev calls completely. Not needed now.
- It uses dm_task_update_nodes() instead of dm library reinitialization
workaround.
Patch is based on upstream SVN + my previous zero fd patch in ctrl+c handler.
Milan
--
mbroz@xxxxxxxxxx
p.s. any feedback appreciated, I would like to submit this patch to Fedora
cryptsetup ASAP...
Use remapping to error target instead of calling udevsettle.
---
lib/internal.h | 2
lib/libdevmapper.c | 143 ++++++++++++++++++++++++++++++++-------------------
lib/setup.c | 2
luks/keyencryption.c | 26 ++++-----
4 files changed, 104 insertions(+), 69 deletions(-)
Index: cryptsetup.my/lib/libdevmapper.c
===================================================================
--- cryptsetup.my.orig/lib/libdevmapper.c 2008-09-04 14:17:33.000000000 +0200
+++ cryptsetup.my/lib/libdevmapper.c 2008-09-04 14:21:36.000000000 +0200
@@ -17,20 +17,7 @@
#define DEVICE_DIR "/dev"
#define CRYPT_TARGET "crypt"
-
-#define UDEVSETTLE "/sbin/udevsettle"
-#define UDEVADM "/sbin/udevadm"
-#define UDEVADM_ARGS "settle --timeout=30"
-
-static void run_udevsettle(void)
-{
-#ifndef SKIP_UDEVSETTLE
- if (!access(UDEVADM, X_OK))
- system(UDEVADM " " UDEVADM_ARGS);
- else if (!access(UDEVSETTLE, X_OK))
- system(UDEVSETTLE);
-#endif
-}
+#define RETRY_COUNT 5
static void set_dm_error(int level, const char *file, int line,
const char *f, ...)
@@ -57,16 +44,6 @@ static void dm_exit(void)
dm_lib_release();
}
-static void flush_dm_workqueue(void)
-{
- /*
- * Unfortunately this is the only way to trigger libdevmapper's
- * update_nodes function
- */
- dm_exit();
- dm_init();
-}
-
static char *__lookup_dev(char *path, dev_t dev)
{
struct dirent *entry;
@@ -159,6 +136,89 @@ out:
return params;
}
+/* DM helpers */
+static int _dm_simple(int task, const char *name)
+{
+ int r = 0;
+ struct dm_task *dmt;
+
+ if (!(dmt = dm_task_create(task)))
+ return 0;
+
+ if (!dm_task_set_name(dmt, name))
+ goto out;
+
+ r = dm_task_run(dmt);
+
+ out:
+ dm_task_destroy(dmt);
+ return r;
+}
+
+static int _error_device(struct crypt_options *options)
+{
+ struct dm_task *dmt;
+ int r = 0;
+
+ if (!(dmt = dm_task_create(DM_DEVICE_RELOAD)))
+ return 0;
+
+ if (!dm_task_set_name(dmt, options->name))
+ goto error;
+
+ if (!dm_task_add_target(dmt, UINT64_C(0), options->size, "error", ""))
+ goto error;
+
+ if (!dm_task_set_ro(dmt))
+ goto error;
+
+ if (!dm_task_no_open_count(dmt))
+ goto error;
+
+ if (!dm_task_run(dmt))
+ goto error;
+
+ if (!_dm_simple(DM_DEVICE_RESUME, options->name)) {
+ _dm_simple(DM_DEVICE_CLEAR, options->name);
+ goto error;
+ }
+
+ r = 1;
+
+error:
+ dm_task_destroy(dmt);
+ return r;
+}
+
+static int _dm_remove(struct crypt_options *options, int force)
+{
+ int r = -EINVAL;
+ int retries = force ? RETRY_COUNT : 1;
+
+ /* If force flag is set, replace device with error, read-only target.
+ * it should stop processes from reading it and also removed underlying
+ * device from mapping, so it is usable again.
+ * Force flag should be used only for temporary devices, which are
+ * intended to work inside cryptsetup only!
+ * Anyway, if some process try to read temporary cryptsetup device,
+ * it is bug - no other process should try touch it (e.g. udev).
+ */
+ if (force) {
+ _error_device(options);
+ retries = RETRY_COUNT;
+ }
+
+ do {
+ r = _dm_simple(DM_DEVICE_REMOVE, options->name) ? 0 : -EINVAL;
+ if (--retries)
+ sleep(1);
+ } while (r == -EINVAL && retries);
+
+ dm_task_update_nodes();
+
+ return r;
+}
+
static int dm_create_device(int reload, struct crypt_options *options,
const char *key)
{
@@ -198,24 +258,14 @@ static int dm_create_device(int reload,
if (dmi.read_only)
options->flags |= CRYPT_FLAG_READONLY;
- /* run udevsettle to avoid a race in libdevmapper causing busy dm devices */
- run_udevsettle();
-
r = 0;
-
out:
if (r < 0 && !reload) {
char *error = (char *)get_error();
if (error)
error = strdup(error);
- if (dmt)
- dm_task_destroy(dmt);
- if (!(dmt = dm_task_create(DM_DEVICE_REMOVE)))
- goto out_restore_error;
- if (!dm_task_set_name(dmt, options->name))
- goto out_restore_error;
- if (!dm_task_run(dmt))
+ if (!_dm_remove(options, 0))
goto out_restore_error;
out_restore_error:
@@ -231,7 +281,7 @@ out_no_removal:
dm_task_destroy(dmt);
if(dmt_query)
dm_task_destroy(dmt_query);
- flush_dm_workqueue();
+ dm_task_update_nodes();
return r;
}
@@ -359,25 +409,12 @@ out:
return r;
}
-static int dm_remove_device(struct crypt_options *options)
+static int dm_remove_device(int force, struct crypt_options *options)
{
- struct dm_task *dmt;
- int r = -EINVAL;
+ if (!options || !options->name)
+ return -EINVAL;
- if (!(dmt = dm_task_create(DM_DEVICE_REMOVE)))
- goto out;
- if (!dm_task_set_name(dmt, options->name))
- goto out;
- if (!dm_task_run(dmt))
- goto out;
-
- r = 0;
-
-out:
- if (dmt)
- dm_task_destroy(dmt);
- flush_dm_workqueue();
- return r;
+ return _dm_remove(options, force);;
}
Index: cryptsetup.my/lib/internal.h
===================================================================
--- cryptsetup.my.orig/lib/internal.h 2008-09-04 14:17:33.000000000 +0200
+++ cryptsetup.my/lib/internal.h 2008-09-04 14:17:36.000000000 +0200
@@ -40,7 +40,7 @@ struct setup_backend {
const char *key);
int (*status)(int details, struct crypt_options *options,
char **key);
- int (*remove)(struct crypt_options *options);
+ int (*remove)(int force, struct crypt_options *options);
const char * (*dir)(void);
};
Index: cryptsetup.my/lib/setup.c
===================================================================
--- cryptsetup.my.orig/lib/setup.c 2008-09-04 14:17:33.000000000 +0200
+++ cryptsetup.my/lib/setup.c 2008-09-04 14:17:36.000000000 +0200
@@ -369,7 +369,7 @@ static int __crypt_remove_device(int arg
return -EBUSY;
}
- return backend->remove(options);
+ return backend->remove(0, options);
}
static int __crypt_luks_format(int arg, struct setup_backend *backend, struct crypt_options *options)
Index: cryptsetup.my/luks/keyencryption.c
===================================================================
--- cryptsetup.my.orig/luks/keyencryption.c 2008-09-04 14:17:33.000000000 +0200
+++ cryptsetup.my/luks/keyencryption.c 2008-09-04 14:17:36.000000000 +0200
@@ -50,10 +50,8 @@ static int setup_mapping(const char *cip
const char *key, size_t keyLength,
unsigned int sector, size_t srcLength,
struct setup_backend *backend,
- int mode)
+ int mode, struct crypt_options *options)
{
- struct crypt_options k;
- struct crypt_options *options = &k;
int device_sector_size = sector_size_for_device(device);
int r;
@@ -87,16 +85,14 @@ static int setup_mapping(const char *cip
return r;
}
-static int clear_mapping(const char *name, struct setup_backend *backend)
+static int clear_mapping(struct crypt_options *options, struct setup_backend *backend)
{
- struct crypt_options options;
- options.name=name;
- return backend->remove(&options);
+ return backend->remove(1, options);
}
/* I miss closures in C! */
static struct setup_backend *cleaner_backend=NULL;
-static const char *cleaner_name=NULL;
+static struct crypt_options *cleaner_options = NULL;
static int devfd=-1;
static void sigint_handler(int sig)
@@ -104,8 +100,8 @@ static void sigint_handler(int sig)
if(devfd >= 0)
close(devfd);
devfd = -1;
- if(cleaner_backend && cleaner_name)
- clear_mapping(cleaner_name, cleaner_backend);
+ if(cleaner_backend && cleaner_options)
+ clear_mapping(cleaner_options, cleaner_backend);
signal(SIGINT, SIG_DFL);
kill(getpid(), SIGINT);
}
@@ -120,6 +116,7 @@ static int LUKS_endec_template(char *src
ssize_t (*func)(int, void *, size_t),
int mode)
{
+ struct crypt_options options = {0};
char *name = NULL;
char *fullpath = NULL;
char *dmCipherSpec = NULL;
@@ -138,10 +135,11 @@ static int LUKS_endec_template(char *src
}
signal(SIGINT, sigint_handler);
- cleaner_name = name;
+ options.name = name;
+ cleaner_options = &options;
cleaner_backend = backend;
- r = setup_mapping(dmCipherSpec,name,device,hdr->payloadOffset,key,keyLength,sector,srcLength,backend,mode);
+ r = setup_mapping(dmCipherSpec,name,device,hdr->payloadOffset,key,keyLength,sector,srcLength,backend,mode,&options);
if(r < 0) {
if(!get_error())
set_error("Failed to setup dm-crypt key mapping.\nCheck kernel for support for the %s cipher spec and verify that %s contains at least %d sectors",
@@ -163,10 +161,10 @@ static int LUKS_endec_template(char *src
close(devfd);
devfd = -1;
out2:
- clear_mapping(name,backend);
+ clear_mapping(&options,backend);
out1:
signal(SIGINT, SIG_DFL);
- cleaner_name = NULL;
+ cleaner_options = NULL;
cleaner_backend = NULL;
free(dmCipherSpec);
free(fullpath);
---------------------------------------------------------------------
dm-crypt mailing list - http://www.saout.de/misc/dm-crypt/
To unsubscribe, e-mail: dm-crypt-unsubscribe@xxxxxxxx
For additional commands, e-mail: dm-crypt-help@xxxxxxxx
