Hello,
I've already posted this at the e2fsprogs sourceforge site, but our e2fsprogs maintainer
suggested that I should post it here as well for a review:
Attached is a patch to add cryptsetup-luks UUID detection in libblkid.so
This is required when we want to use UUIDs instead of hardcoded device names for
encrypted partitions.
Regards
Karsten Hopp
--
Karsten Hopp | Mail: [email protected]
Red Hat Deutschland | Tel: +49-711-96437-0
Hauptstaetterstr.58 | Fax: +49-711-613590
D-70178 Stuttgart | http://www.redhat.de
Karsten Hopp wrote:
> Hello,
>
> I've already posted this at the e2fsprogs sourceforge site, but our
> e2fsprogs maintainer
> suggested that I should post it here as well for a review:
>
>
> Attached is a patch to add cryptsetup-luks UUID detection in libblkid.so
> This is required when we want to use UUIDs instead of hardcoded device
> names for
> encrypted partitions.
Hi Karsten, in the patch you posted, need to remove the
__BLKID_ATTR((unused)) from:
+static int probe_luks(struct blkid_probe *probe,
+ struct blkid_magic *id __BLKID_ATTR((unused)),
+ unsigned char *buf)
+{
since id is being used now.
-Eric
On Jun 05, 2007 13:23 +0200, Karsten Hopp wrote:
> Attached is a patch to add cryptsetup-luks UUID detection in libblkid.so
> This is required when we want to use UUIDs instead of hardcoded device
> names for
> encrypted partitions.
>
>
> +/* check it manually as using LUKS_read_phdr from libcryptsetup
> + * prints too many warnings if it isn't a luks partition and would add a
> + * dependency on the lib */
This is true of all superblock probing, so I'm not sure it deserves a
comment.
> +static int probe_luks(struct blkid_probe *probe,
> + struct blkid_magic *id __BLKID_ATTR((unused)),
> + unsigned char *buf)
> +{
> + const char *luks_magic = id->bim_magic;
> + unsigned char *p_buf = buf;
> + unsigned char uuid[40];
> + if(strncmp(buf, luks_magic, strlen(luks_magic)) == 0) /* ID matches, continue */
> + {
> + /* 168 is the offset to the 40 character uuid: http://luks.endorphin.org/LUKS-on-disk-format.pdf */
> + p_buf += 168;
> + strncpy(uuid, p_buf, 40);
> + blkid_set_tag(probe->dev, "UUID", uuid, sizeof(uuid));
> + blkid_set_tag(probe->dev, "SEC_TYPE", "crypt_LUKS", sizeof("crypt_LUKS"));
Please wrap lines at 80 columns.
Also, why use SEC_TYPE here? That is "secondary type" and not (you
might think) "security type". That should only be used if there are
two filesystem types that could mount the filesystem, and the SEC_TYPE
is the less-preferred one.
> @@ -775,6 +796,7 @@ static struct blkid_magic type_array[] =
> { "ocfs2", 2, 0, 6, "OCFSV2", probe_ocfs2 },
> { "ocfs2", 4, 0, 6, "OCFSV2", probe_ocfs2 },
> { "ocfs2", 8, 0, 6, "OCFSV2", probe_ocfs2 },
> + { "crypt_LUKS", 0, 0, 6, "LUKS\xba\xbe", probe_luks },
Please fix the alignment here.
Cheers, Andreas
--
Andreas Dilger
Principal Software Engineer
Cluster File Systems, Inc.
In addition to the comments already posted:
> +/* check it manually as using LUKS_read_phdr from libcryptsetup
> + * prints too many warnings if it isn't a luks partition and would add a
> + * dependency on the lib */
> +static int probe_luks(struct blkid_probe *probe,
> + struct blkid_magic *id __BLKID_ATTR((unused)),
> + unsigned char *buf)
> +{
> + const char *luks_magic = id->bim_magic;
> + unsigned char *p_buf = buf;
> + unsigned char uuid[40];
> + if(strncmp(buf, luks_magic, strlen(luks_magic)) == 0) /* ID matches, continue */
There's no point in doing this check, since it's replicating a check
already done in the generic code. The probe function won't be called
if the bim_magic didn't match the specified offset.
- Ted
Theodore Tso schrieb:
> In addition to the comments already posted:
>
>> +/* check it manually as using LUKS_read_phdr from libcryptsetup
>> + * prints too many warnings if it isn't a luks partition and would add a
>> + * dependency on the lib */
>> +static int probe_luks(struct blkid_probe *probe,
>> + struct blkid_magic *id __BLKID_ATTR((unused)),
>> + unsigned char *buf)
>> +{
>> + const char *luks_magic = id->bim_magic;
>> + unsigned char *p_buf = buf;
>> + unsigned char uuid[40];
>> + if(strncmp(buf, luks_magic, strlen(luks_magic)) == 0) /* ID matches, continue */
>
> There's no point in doing this check, since it's replicating a check
> already done in the generic code. The probe function won't be called
> if the bim_magic didn't match the specified offset.
>
> - Ted
Thanks everyone for the replys.
I'll attach a new patch with the suggested fixes.
Karsten
--
Karsten Hopp | Mail: [email protected]
Red Hat Deutschland | Tel: +49-711-96437-0
Hauptstaetterstr.58 | Fax: +49-711-613590
D-70178 Stuttgart | http://www.redhat.de
On Mon, Jun 11, 2007 at 01:51:24PM +0200, Karsten Hopp wrote:
> +static int probe_luks(struct blkid_probe *probe,
> + struct blkid_magic *id __BLKID_ATTR((unused)),
> + unsigned char *buf)
> +{
> + unsigned char *p_buf = buf;
> + unsigned char uuid[40];
> + /* 168 is the offset to the 40 character uuid:
> + * http://luks.endorphin.org/LUKS-on-disk-format.pdf */
> + p_buf += 168;
> + strncpy(uuid, p_buf, 40);
Why bother with p_buf? It would actually be shorter and sweeter to
do:
strncpy(uuid, buf+168, 40);
And remove the lines dealing with p_buf above.
> + { "crypt_LUKS",0, 0, 6, "LUKS\xba\xbe", probe_luks },
Any particular reason to use "crypt_LUKS" instead of just "LUKS"? In
your documentation you generally just refer to it as LUKS.
- Ted
Theodore Tso schrieb:
> On Mon, Jun 11, 2007 at 01:51:24PM +0200, Karsten Hopp wrote:
>> +static int probe_luks(struct blkid_probe *probe,
>> + struct blkid_magic *id __BLKID_ATTR((unused)),
>> + unsigned char *buf)
>> +{
>> + unsigned char *p_buf = buf;
>> + unsigned char uuid[40];
>> + /* 168 is the offset to the 40 character uuid:
>> + * http://luks.endorphin.org/LUKS-on-disk-format.pdf */
>> + p_buf += 168;
>> + strncpy(uuid, p_buf, 40);
>
> Why bother with p_buf? It would actually be shorter and sweeter to
> do:
>
> strncpy(uuid, buf+168, 40);
>
> And remove the lines dealing with p_buf above.
>
>> + { "crypt_LUKS",0, 0, 6, "LUKS\xba\xbe", probe_luks },
>
> Any particular reason to use "crypt_LUKS" instead of just "LUKS"? In
> your documentation you generally just refer to it as LUKS.
>
> - Ted
I've used 'luks' in my first patch, but changed it to 'crypt_LUKS' when
Karel Zak pointed out that libvolume_id from udev already uses 'crypt_LUKS'
for this.
My first patch also did some other (unnecessary) stuff with p_buf and I just
didn't bother to remove it.
I'll attach a new patch without p_buf.
Karsten
--
Karsten Hopp | Mail: [email protected]
Red Hat Deutschland | Tel: +49-711-96437-0
Hauptstaetterstr.58 | Fax: +49-711-613590
D-70178 Stuttgart | http://www.redhat.de
Thanks, I've applied the blkid LUKS patch to the e2fsprogs SCM (modulo
a minor whitespace breakage which I fixed up).
BTW, there appears to be a problem here in udev regarding LUKS
identification:
https://bugs.launchpad.net/ubuntu/+source/e2fsprogs/+bug/93921
The problem is that udev sets its magic string in the first 512 bytes
of the partition. This is dangerous and error-prone, because other
things like boot sectors and BSD disk labels tend to live in the first
512 byte sector. Hence, many programs are very careful before zapping
the first 512 byte sector. LUKS was added to the vol_id program very
high in the list of filesystems to be probed, ahead of ext2/ext3, so a
filesystem previously contained a LUKS setup, but then was later
mke2fs'ed to be used as a normal ext2/3 filesyste, may get
misidentified by udev's vol_id as still being a LUKS filesystem if
other fields in the first 512 byte sector cause mke2fs to mistakenly
think there was a BSD disklabel in the sector and thus refuse to zero
it out.
This won't be a problem with blkid, since LUKS is placed *after*
ext3/4. However, it would be a good idea to check and make sure that
whever is setting up a LUKS partition clears the first and last 32k of
a filesystem, to avoid potential confusion by other in-band filesystem
type checkers. It would probably be a good idea to (after you make
sure this is done) to submit a patch to the uev folks changing the
probing order of vol_id so that it probes for the LUK filesystem after
ext2/3.
I am currently consider adding specific kludges to mke2fs that checks
the first couple of bytes of the 512 byte sector for the problematic
filesystem types (NTFS and LUKS), explicitly clearing just those bytes
to avoid future confusion. But that won't help the existing
filesystems that are out there....
- Ted
Theodore Tso schrieb:
> Thanks, I've applied the blkid LUKS patch to the e2fsprogs SCM (modulo
> a minor whitespace breakage which I fixed up).
>
> BTW, there appears to be a problem here in udev regarding LUKS
> identification:
>
> https://bugs.launchpad.net/ubuntu/+source/e2fsprogs/+bug/93921
>
...
Thanks for applying the small patch !
FYI:
I've opened a bugzilla about your udev concerns to get some comments from our
and the upstream maintainer at
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=245717
Kay Sievers from the udev team responded:
#> That's nothing new. Guessing filesystems by magic bytes on the disk is by
#> definition unsafe.
#>
#> _All_ filesystem formatters are required to wipe out _all_ existing signatures
#> before applying a new signature. Even then, it's not entirely safe to probe, but
#> we obviously have no alternatives.
#>
#> Shuffling the probing order around will only switch the systems where such
#> problems occur. From my standpoint, you can close this "bug". Thanks!
So, no help from this side....
Karsten
--
Karsten Hopp | Mail: [email protected]
Red Hat Deutschland | Tel: +49-711-96437-0
Hauptstaetterstr.58 | Fax: +49-711-613590
D-70178 Stuttgart | http://www.redhat.de
Karsten Hopp wrote:
> Thanks for applying the small patch !
>
> FYI:
> I've opened a bugzilla about your udev concerns to get some comments from our
> and the upstream maintainer at
> https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=245717
>
> Kay Sievers from the udev team responded:
> #> That's nothing new. Guessing filesystems by magic bytes on the disk is by
> #> definition unsafe.
> #>
> #> _All_ filesystem formatters are required to wipe out _all_ existing signatures
> #> before applying a new signature. Even then, it's not entirely safe to probe, but
> #> we obviously have no alternatives.
We fought this for a while in xfs, trying to be sure we wipe out old fs
signatures at mkfs time. It'd be nice to have a library to do such
things...
-Eric
Theodore Tso schrieb:
> Thanks, I've applied the blkid LUKS patch to the e2fsprogs SCM (modulo
> a minor whitespace breakage which I fixed up).
>
I've encountered a problem with this patch, consider the following
setup:
in /etc/fstab:
UUID=<some LUKS uuid> / ext3 defaults 0 0
During bootup the init scripts run 'fsck -A', fsck reads in /etc/fstab and
converts the LUKS-uuid to a device name. It then uses this devicename to
populate filesys_info and sets the type to ext3 (from fstab). It then tries to check
the underlying device instead of the /dev/mapper/ LUKS device.
I'm not sure how to work around this, adding crypt_luks to ignored_types won't work
because fsck thinks the device has an ext3 FS.
Is create_fs_device() the right place to add a check if this is a LUKS device or would it be better
to do that in parse_fstab_line() around the blkid_get_devname() call ?
Karsten
--
Karsten Hopp | Mail: [email protected]
Red Hat Deutschland | Tel: +49-711-96437-0
Hauptstaetterstr.58 | Fax: +49-711-613590
D-70178 Stuttgart | http://www.redhat.de
On Mon, Jul 23, 2007 at 05:00:02PM +0200, Karsten Hopp wrote:
>
> I've encountered a problem with this patch, consider the following
> setup:
>
> in /etc/fstab:
> UUID=<some LUKS uuid> / ext3 defaults 0 0
Well, the problem is that what you should be putting in /etc/fstab is:
UUID=<uuid of the ext3 filesystem> / ext3 defaults 0 0
If you put "UUID=<some LUKS uuid>", then of course you will get the
underlying device; that's what you asked for --- what device has the
UUID set to <some LUKS uuid> --- and the computer gave you what you
asked for, which is what not what you wanted. :-)
If you put the UUID of the ext3 filesystem, then the blkid library
will search through the /dev/mapper devices and find the correct
/dev/mapper device for the underlying filesystem, which is as it should be.
This of course assumes that someone has correctly activated the LUKS
filesystem (and presumably passed LUKS the correct crypto information)
so that it appears as one of the /dev/mapper filesystems. If that is
why you put the UUID of LUKS partition in /etc/fstab, then we have a
fundamental conflict of what /etc/fstab is for. /etc/fstab's job is
to identify the actual devices for mounting a filesystem.
- Ted