Hello.
Today the build of an initrd from a 2.6.19-rc3 kernel failed.
fairlight:/boot# yaird -o /tmp/foo.initrd 2.6.19-rc3-fairlight
yaird error: Could not read output for /sbin/modprobe -v -n --show-depends --set-version 2.6.19-rc3-fairlight cbc(aes) (fatal)
dmsetup table has changed the report for luks cipher.
dmsetup table on 2.6.18 reports: aes-cbc-essiv:sha256
dmsetup table on 2.6.19-rc3 reports: cbc(aes)-cbc-essiv:sha256
The problem seems to be on the kernel side here. Herbert Xu changed
the output with d1806f6a97a536b043fe50e6d8a25b061755cf50
http://kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=d1806f6a97a536b043fe50e6d8a25b061755cf50
The question is if this change was intentional and yaird should be
fixed, or it's a kernel API breakage.
regards
Stefan Schmidt
On Mon, Oct 30, 2006 at 04:19:30PM +0100, Stefan Schmidt wrote:
> dmsetup table on 2.6.18 reports: aes-cbc-essiv:sha256
> dmsetup table on 2.6.19-rc3 reports: cbc(aes)-cbc-essiv:sha256
> The problem seems to be on the kernel side here. Herbert Xu changed
> the output with d1806f6a97a536b043fe50e6d8a25b061755cf50
> http://kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=d1806f6a97a536b043fe50e6d8a25b061755cf50
> The question is if this change was intentional and yaird should be
> fixed, or it's a kernel API breakage.
It cannot have been intentional as there was no mention of the change to the
userspace interface in the git changelog (and the interface version number
was not changed).
A new patch is needed to revert the part of the patch that changed the
userspace interface.
Please don't forget to copy in the appropriate maintainers when you send
messages like this one:
http://marc.theaimsgroup.com/?l=linux-netdev&m=115547174417490&w=2
so they can provide acks:-)
Alasdair
--
[email protected]
On Mon, 30 Oct 2006, Alasdair G Kergon wrote:
>
> It cannot have been intentional as there was no mention of the change to the
> userspace interface in the git changelog (and the interface version number
> was not changed).
>
> A new patch is needed to revert the part of the patch that changed the
> userspace interface.
>
> Please don't forget to copy in the appropriate maintainers when you send
> messages like this one:
> http://marc.theaimsgroup.com/?l=linux-netdev&m=115547174417490&w=2
> so they can provide acks:-)
Yeah.
Herbert, the breakage _seems_ to be due to the STATUSTYPE_TABLE case
change:
- cipher = crypto_tfm_alg_name(cc->tfm);
+ cipher = crypto_blkcipher_name(cc->tfm);
which effectively changes "aes" into "cbc(aes)", which is wrong, since we
show the chainmode separately.
Please, somebody who knows this area, send me a fix,
Linus
----
(maybe something like this trivial one? Totally untested, but it would
seem to be the sane approach)
diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index a625576..645e3ce 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -925,8 +925,7 @@ static int crypt_status(struct dm_target
break;
case STATUSTYPE_TABLE:
- cipher = crypto_blkcipher_name(cc->tfm);
-
+ cipher = cc->cipher;
chainmode = cc->chainmode;
if (cc->iv_mode)
On Mon, Oct 30, 2006 at 11:00:29AM -0800, Linus Torvalds wrote:
> (maybe something like this trivial one? Totally untested, but it would
> seem to be the sane approach)
> diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
> index a625576..645e3ce 100644
> --- a/drivers/md/dm-crypt.c
> +++ b/drivers/md/dm-crypt.c
> @@ -925,8 +925,7 @@ static int crypt_status(struct dm_target
> break;
>
> case STATUSTYPE_TABLE:
> - cipher = crypto_blkcipher_name(cc->tfm);
> -
> + cipher = cc->cipher;
> chainmode = cc->chainmode;
>
> if (cc->iv_mode)
>
>
Looks correct.
The point of STATUSTYPE_TABLE is to return (readable) output to userspace in
a format that the crypt_ctr() function would accept back in.
So crypt_ctr() now stores a private copy of cipher and chainmode for
crypt_status() to regurgitate when requested.
Alasdair
--
[email protected]
Am Montag, den 30.10.2006, 11:00 -0800 schrieb Linus Torvalds:
> On Mon, 30 Oct 2006, Alasdair G Kergon wrote:
> >
> > It cannot have been intentional as there was no mention of the change to the
> > userspace interface in the git changelog (and the interface version number
> > was not changed).
> >
> > A new patch is needed to revert the part of the patch that changed the
> > userspace interface.
> >
> > Please don't forget to copy in the appropriate maintainers when you send
> > messages like this one:
> > http://marc.theaimsgroup.com/?l=linux-netdev&m=115547174417490&w=2
> > so they can provide acks:-)
>
> Yeah.
>
> Herbert, the breakage _seems_ to be due to the STATUSTYPE_TABLE case
> change:
>
> - cipher = crypto_tfm_alg_name(cc->tfm);
> + cipher = crypto_blkcipher_name(cc->tfm);
>
> which effectively changes "aes" into "cbc(aes)", which is wrong, since we
> show the chainmode separately.
>
> Please, somebody who knows this area, send me a fix,
>
> (maybe something like this trivial one? Totally untested, but it would
> seem to be the sane approach)
Yes, this works just fine. It can also be cleaned up a little further as
the temporary variables are unnecessary at that point.
----
Fix dm-crypt after the block cipher API changes to correctly return the
backwards compatible cipher-chainmode[-ivmode] format for "dmsetup
table".
Signed-off-by: Christophe Saout <[email protected]>
diff linux-2.6.19-rc3.orig/drivers/md/dm-crypt.c linux-2.6.19-rc3/drivers/md/dm-crypt.c
--- linux-2.6.19-rc3.orig/drivers/md/dm-crypt.c 2006-10-26 13:17:58.000000000 +0200
+++ linux-2.6.19-rc3/drivers/md/dm-crypt.c 2006-10-30 20:26:37.000000000 +0100
@@ -915,8 +915,6 @@ static int crypt_status(struct dm_target
char *result, unsigned int maxlen)
{
struct crypt_config *cc = (struct crypt_config *) ti->private;
- const char *cipher;
- const char *chainmode = NULL;
unsigned int sz = 0;
switch (type) {
@@ -925,14 +923,11 @@ static int crypt_status(struct dm_target
break;
case STATUSTYPE_TABLE:
- cipher = crypto_blkcipher_name(cc->tfm);
-
- chainmode = cc->chainmode;
-
if (cc->iv_mode)
- DMEMIT("%s-%s-%s ", cipher, chainmode, cc->iv_mode);
+ DMEMIT("%s-%s-%s ", cc->cipher, cc->chainmode,
+ cc->iv_mode);
else
- DMEMIT("%s-%s ", cipher, chainmode);
+ DMEMIT("%s-%s ", cc->cipher, cc->chainmode);
if (cc->key_size > 0) {
if ((maxlen - sz) < ((cc->key_size << 1) + 1))
Hello.
On Mon, 2006-10-30 at 11:00, Linus Torvalds wrote:
>
> (maybe something like this trivial one? Totally untested, but it would
> seem to be the sane approach)
I just tested it. Works fine for me.
regards
Stefan Schmidt
On Mon, Oct 30, 2006 at 08:39:08PM +0100, Christophe Saout wrote:
>
> > Herbert, the breakage _seems_ to be due to the STATUSTYPE_TABLE case
> > change:
> >
> > - cipher = crypto_tfm_alg_name(cc->tfm);
> > + cipher = crypto_blkcipher_name(cc->tfm);
> >
> > which effectively changes "aes" into "cbc(aes)", which is wrong, since we
> > show the chainmode separately.
Yes that's wrong.
However, the bigger problem is that dmcrypt's algorithm specification
does not allow all algorithms to be specified. In particular, algorithms
with dashes in their names cannot be represented in an unambiguous way.
The separation of chain modes and algorithm names is in fact unnecessary
and only complicates matters.
For now it's no big deal since the algorithms people actually use
can be represented. But it would be nice to get this fixed.
> ----
>
> Fix dm-crypt after the block cipher API changes to correctly return the
> backwards compatible cipher-chainmode[-ivmode] format for "dmsetup
> table".
>
> Signed-off-by: Christophe Saout <[email protected]>
Looks good to me.
Acked-by: Herbert Xu <[email protected]>
Thanks,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt