Bug#545032: devicekit-disks - Handles add actions of dm de..

Bastian Blank · External Since: Nov 21, 2004 Posts: 775

Package: devicekit-disks
Severity: critical

devicekit-disks adds its own udev rules that likes to handle add actions
of device-mapper devices. This will break arbitrary usage, for example
cryptsetup. So this is not allowed for anything except the device mapper
core with special precaution. Also it overwrites data already setup by
device mapper.

Bastian

--
Humans do claim a great deal for that particular emotion (love).
-- Spock, "The Lights of Zetar", stardate 5725.6

--
To UNSUBSCRIBE, email to debian-bugs-dist-REQUEST.TakeThisOut@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster.TakeThisOut@lists.debian.org

Bastian Blank · External Since: Nov 21, 2004 Posts: 775

On Wed, Sep 23, 2009 at 04:36:54PM +0200, Michael Biebl wrote:
> Right, but not with this severity. So downgrading to important.

Sure with this. You break dmsetup. Anyway, will add a conflict.

Bastian

--
No one may kill a man. Not for any purpose. It cannot be condoned.
-- Kirk, "Spock's Brain", stardate 5431.6

--
To UNSUBSCRIBE, email to debian-bugs-dist-REQUEST RemoveThis @lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster RemoveThis @lists.debian.org

Bastian Blank · External Since: Nov 21, 2004 Posts: 775

On Wed, Sep 23, 2009 at 05:06:00PM +0200, Michael Biebl wrote:
> Bastian Blank wrote:
> > On Wed, Sep 23, 2009 at 04:36:54PM +0200, Michael Biebl wrote:
> >> Right, but not with this severity. So downgrading to important.
> > Sure with this. You break dmsetup. Anyway, will add a conflict.
> Adding a conflict is certainly not the way I want it to go, as dk-disks is meant
> to work with dm devices.

As long as it doesn't follow the rules, a conflict is correct.

> What do you suggest instead?

Remove it. dmsetup does the complete setup in favour of lvm and the
other devmapper using tools.

Bastian

--
Where there's no emotion, there's no motive for violence.
-- Spock, "Dagger of the Mind", stardate 2715.1

--
To UNSUBSCRIBE, email to debian-bugs-dist-REQUEST RemoveThis @lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster RemoveThis @lists.debian.org

Bastian Blank · External Since: Nov 21, 2004 Posts: 775

On Wed, Sep 23, 2009 at 04:36:54PM +0200, Michael Biebl wrote:
> > But anyway, the duplication of the rules already warants this bug.
> Right, but not with this severity. So downgrading to important.

The rules are set by the kernel and userspace part of device-mapper to
make several of its users work. So the next step is to enforce them from
the kernel.

The rules provided by dmsetup provide the information you need in
DM_NAME, DM_UUID and maybe DM_SUSPENDED.

Bastian

--
Lots of people drink from the wrong bottle sometimes.
-- Edith Keeler, "The City on the Edge of Forever",
stardate unknown

--
To UNSUBSCRIBE, email to debian-bugs-dist-REQUEST DeleteThis @lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster DeleteThis @lists.debian.org

Andreas Barth · External Since: Jul 31, 2005 Posts: 877

* Michael Biebl (biebl@debian.org) [091011 18:58]:
> Where exactly in the rules does it overwrite data setup by device mapper?
> Which part exactly in the rules file does break other apps?
> What do you suggest instead?

I think this are the core questions, and we should try to get to an
"at least" working configuration soon.

As what I read from the bug report, Bastian prefers to have all udev
rules in dmsetup. Would that work for you? If not, why not? Or: What
do you need from udev what is not in dmsetup?

Bastian, same questions to you: Is there any reason why we can't at
all have a working set of udev rules in devicekit-disks? (I'm not
necessarily speaking about the current rules, but why it is impossible
to have any udev rules in the package at all.)

I'd like to push towards an resolution of this issue that works, and
makes our users happy. I don't have an strict opinion how the
resolutions looks otherwise, and would like to make a decision
together how we can reach that best.

Cheers,
Andi

--
To UNSUBSCRIBE, email to debian-bugs-dist-REQUEST.TakeThisOut@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster.TakeThisOut@lists.debian.org

Bastian Blank · External Since: Nov 21, 2004 Posts: 775

On Sun, Oct 11, 2009 at 09:04:09PM +0200, Andreas Barth wrote:
> As what I read from the bug report, Bastian prefers to have all udev
> rules in dmsetup.

Not in dmsetup, but in the package that controls that sort of device. So
dmsetup for the core rules, lvm2 for the lvm specific rules, cryptsetup
for their rules (if necessary, I don't know). Because they control there
own devices, they may also break it.

> Bastian, same questions to you: Is there any reason why we can't at
> all have a working set of udev rules in devicekit-disks? (I'm not
> necessarily speaking about the current rules, but why it is impossible
> to have any udev rules in the package at all.)

As long as the rules only change the behaviour of the tools in this
package, but not just copy other rules and also follow the other rules
about introspection, I don't see a general problem.

However, the current rules file does the following.
- Probe for partitions, aka open, on all block devices with a (small)
blacklist. This can be only allowed with a whitelist, as several
device types may not be queried from add events or not at all, like
several dm subtypes.
- Try to get informations about dm devices from add events.
- Use blkid on dm devices even if this are hidden devices that must not
show up via filesystem infos. Also done from add events, something
that can never be allowed.
- Examine linux raid devices with mdadm. This is the sole responsibility
of the mdadm package.
- Examine SMART state, only set informations.
- Tag devices without known prefix and with an undocumented list of
devices.

The first point needs to be changed to a whitelist. The second only
provides information used by them but it is undetermined if it will
work and newer kernels change the way to actually get the information in
this state. The third one needs to go. The forth is not my problem but I don't
like it. The fifth is okay. The informations set in the sixth should get
a known prefix.

Bastian

--
Power is danger.
-- The Centurion, "Balance of Terror", stardate 1709.2

--
To UNSUBSCRIBE, email to debian-bugs-dist-REQUEST.TakeThisOut@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster.TakeThisOut@lists.debian.org

Bastian Blank · External Since: Nov 21, 2004 Posts: 775

On Thu, Oct 15, 2009 at 08:15:22PM +0200, Bastian Blank wrote:
> On Thu, Oct 15, 2009 at 02:44:23PM +0200, Michael Biebl wrote:
> > If not, since what version of cryptsetup/dmsetup is this naming interface stable?
> 1.1, which is supposed to be
> released shortly, will have CRYPT-PLAIN-* and CRYPT-LUKS-*.

cryptsetup 1.1(~rc2) reached unstable today. Also it is CRYPT-LUKS1-*
(note the 1).

> - Destinguish between luks crypted and anything else.

Some additional looks only allow this variant to stay and the original
check is not able to do that.

Bastian

--
Suffocating together ... would create heroic camaraderie.
-- Khan Noonian Singh, "Space Seed", stardate 3142.8

--
To UNSUBSCRIBE, email to debian-bugs-dist-REQUEST.RemoveThis@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster.RemoveThis@lists.debian.org

Bastian Blank · External Since: Nov 21, 2004 Posts: 775

On Tue, Oct 13, 2009 at 05:45:27PM +0200, Michael Biebl wrote:
> What we can not remove currently, is the call to devkit-disks-dm-export,
> as it makes additional information available like (DKM_)DM_TARGET_TYPES,
> which is used inside the dk-disks code.

Please explain which highlevel information you need.

DM_TARGET_TYPES is only used to find the string "crypt" in it and I
already explained several times that DM_UUID==CRYPT-* is a quite better
indicator, completely functional in the near future with cryptsetup 1.1.

Bastian

--
Four thousand throats may be cut in one night by a running man.
-- Klingon Soldier, "Day of the Dove", stardate unknown

--
To UNSUBSCRIBE, email to debian-bugs-dist-REQUEST DeleteThis @lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster DeleteThis @lists.debian.org

Bastian Blank · External Since: Nov 21, 2004 Posts: 775

[ Triming mdadm maintainers from CC. ]

On Thu, Oct 15, 2009 at 02:44:23PM +0200, Michael Biebl wrote:
> Is the DM_UUID=CRYPT-* naming scheme documented somewhere? I'd like to send this
> patch upstream and would need to know if it is a Debian specific feature.

I forgot to cite the information I got from the cryptsetup maintainer.
The current state is documented in #548988.

> If not, since what version of cryptsetup/dmsetup is this naming interface stable?

1.0.7 have CRYPT-$UUID for luks devices. 1.1, which is supposed to be
released shortly, will have CRYPT-PLAIN-* and CRYPT-LUKS-*.

I'm still not fully aware what you are really trying to detect. The code
is so full of comments in the relevant section, this makes it not easier
to give comments.

I see three possibilities:
- Destinguish between uncrypted and crypted devices (the luks
description would be wrong then).
- Destinguish between plain crypted and luks crypted devices.
- Destinguish between luks crypted and anything else.

The last looks like the correct one according to the names and checks.
The correct check for this one would be:
CRYPT-[0-9a-f]* || CRYPT-LUKS-*
The check for temporary-cryptsetup-* can go then. This will catch
anything from cryptsetup 1.0.7 up.

It makes no sense to comment on the actual check if I don't know what it
should do.

Bastian

--
There is an order of things in this universe.
-- Apollo, "Who Mourns for Adonais?" stardate 3468.1

--
To UNSUBSCRIBE, email to debian-bugs-dist-REQUEST.DeleteThis@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster.DeleteThis@lists.debian.org

Bastian Blank · External Since: Nov 21, 2004 Posts: 775

On Fri, Oct 16, 2009 at 04:37:53AM +0200, Michael Biebl wrote:
> + (g_str_has_prefix (dm_uuid, "CRYPT-LUKS1") ||
"CRYPT-LUKS1-", maybe there will be a CRYPT-LUKS10 someday.

> + g_regex_match_simple ("^CRYPT-[a-f0-9-]+$", dm_uuid, 0, 0)) &&
"^CRYPT-[0-9a-f]", no need to have an anchor at the end.

> - if (g_str_has_prefix (dkd_dm_name, "temporary-cryptsetup-")) {
> + /* Debian already hides temporary cryptsetup devices, so this check could probably be removed? */
> + if (g_str_has_prefix (dm_name, "temporary-cryptsetup-")) {
This check can go. This temporary devices either have no uuid
(cryptsetup < 1.1) or start with CRYPT-TEMP- (cryptsetup >= 1.1).

Bastian

--
Sometimes a feeling is all we humans have to go on.
-- Kirk, "A Taste of Armageddon", stardate 3193.9

--
To UNSUBSCRIBE, email to debian-bugs-dist-REQUEST.DeleteThis@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster.DeleteThis@lists.debian.org

Bastian Blank · External Since: Nov 21, 2004 Posts: 775

On Fri, Oct 16, 2009 at 11:43:52PM +0200, Bastian Blank wrote:
> On Fri, Oct 16, 2009 at 10:45:25PM +0200, Michael Biebl wrote:
> > I adressed the issues you mentioned [1] and just uploaded 007-3 to unstable.
> Okay. What about point 1 of my list? This still probes device-mapper
> devices.

This patch should use an apropriate whitelist.

--- a/data/95-devkit-disks.rules
+++ b/data/95-devkit-disks.rules
@@ -12,7 +12,7 @@
#

# skip rules for inappropriate block devices
-KERNEL=="fd*|mtd*|nbd*|gnbd*|btibm*", GOTO="probe_parttable_end"
+KERNEL!="hd*|sd*|sr*|cciss*|dasd*", GOTO="probe_parttable_end"

# never access non-cdrom removable ide devices, the drivers are causing event loops on open()
KERNEL=="hd*[!0-9]", ATTR{removable}=="1", SUBSYSTEMS=="ide", ATTRS{media}=="disk|floppy", GOTO="probe_parttable_end"

Bastian

--
Is truth not truth for all?
-- Natira, "For the World is Hollow and I have Touched
the Sky", stardate 5476.4.

--
To UNSUBSCRIBE, email to debian-bugs-dist-REQUEST.RemoveThis@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster.RemoveThis@lists.debian.org

Bastian Blank · External Since: Nov 21, 2004 Posts: 775

On Fri, Oct 16, 2009 at 10:45:25PM +0200, Michael Biebl wrote:
> I adressed the issues you mentioned [1] and just uploaded 007-3 to unstable.

Okay. What about point 1 of my list? This still probes device-mapper
devices.

Bastian

--
Lots of people drink from the wrong bottle sometimes.
-- Edith Keeler, "The City on the Edge of Forever",
stardate unknown

--
To UNSUBSCRIBE, email to debian-bugs-dist-REQUEST RemoveThis @lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster RemoveThis @lists.debian.org

David Zeuthen · External Since: Oct 24, 2006 Posts: 5

On Thu, 2009-10-15 at 14:44 +0200, Michael Biebl wrote:
> Bastian Blank wrote:
> > On Tue, Oct 13, 2009 at 05:45:27PM +0200, Michael Biebl wrote:
> >> What we can not remove currently, is the call to devkit-disks-dm-export,
> >> as it makes additional information available like (DKM_)DM_TARGET_TYPES,
> >> which is used inside the dk-disks code.
> >
> > Please explain which highlevel information you need.
> >
> > DM_TARGET_TYPES is only used to find the string "crypt" in it and I
> > already explained several times that DM_UUID==CRYPT-* is a quite better
> > indicator, completely functional in the near future with cryptsetup 1.1.
>
> Finally some useful information, thanks.
>
> Attached is a patch which makes use of DM_UUID making it possible to completely
> remove devkit-disks-dm-export, and it seems to work nicely.
>
> Is the DM_UUID=CRYPT-* naming scheme documented somewhere? I'd like to send this
> patch upstream and would need to know if it is a Debian specific feature.
> If not, since what version of cryptsetup/dmsetup is this naming interface stable?

Yeah, I'd like to know exactly how much of this can be expected to be in
other distros. FWIW, I've been telling the LVM/device-mapper guys for a
long time to include udev rules etc. in the stock upstream release. I
know they are planning to do that but I'm not sure it's there yet. If
there is, please point me to it.

Anyway, that's the reason why I have my own prober, udev rules and
name-spaced properties. This will have to stay in the upstream
distribution of DKD until there's an upstream release of
LVM/device-mapper with what I need.

Thanks,
David

--
To UNSUBSCRIBE, email to debian-bugs-dist-REQUEST.RemoveThis@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster.RemoveThis@lists.debian.org

Bastian Blank · External Since: Nov 21, 2004 Posts: 775

found 545032 008-1
thanks

On Sat, Oct 17, 2009 at 12:02:19AM +0200, Michael Biebl wrote:
> Bastian Blank wrote:
> > On Fri, Oct 16, 2009 at 10:45:25PM +0200, Michael Biebl wrote:
> >> I adressed the issues you mentioned [1] and just uploaded 007-3 to unstable.
> > Okay. What about point 1 of my list? This still probes device-mapper
> > devices.
> What? Where?

| KERNEL=="fd*|mtd*|nbd*|gnbd*|btibm*", GOTO="probe_parttable_end"
| IMPORT{program}="devkit-disks-part-id $tempnode"
| LABEL="probe_parttable_end"

devkit-disks-part-id opens the temporary device node to read data from
it.

Bastian

--
You! What PLANET is this!
-- McCoy, "The City on the Edge of Forever", stardate 3134.0

--
To UNSUBSCRIBE, email to debian-bugs-dist-REQUEST DeleteThis @lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster DeleteThis @lists.debian.org

Bastian Blank · External Since: Nov 21, 2004 Posts: 775

On Fri, Oct 16, 2009 at 06:20:48PM -0400, David Zeuthen wrote:
> Yeah, I'd like to know exactly how much of this can be expected to be in
> other distros.

All of them.

> FWIW, I've been telling the LVM/device-mapper guys for a
> long time to include udev rules etc. in the stock upstream release. I
> know they are planning to do that but I'm not sure it's there yet. If
> there is, please point me to it.

Can you please explain why you don't check that yourself against an
uptodate rawhide system? If seen LVM2 2.02.52, the first version
shipping the rules, in the package list several weeks ago.

> Anyway, that's the reason why I have my own prober, udev rules and
> name-spaced properties. This will have to stay in the upstream
> distribution of DKD until there's an upstream release of
> LVM/device-mapper with what I need.

Well, your turn to break device-mapper.

Bastian

--
Spock: We suffered 23 casualties in that attack, Captain.

--
To UNSUBSCRIBE, email to debian-bugs-dist-REQUEST.DeleteThis@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster.DeleteThis@lists.debian.org

Bastian Blank · External Since: Nov 21, 2004 Posts: 775

On Fri, Oct 16, 2009 at 11:55:55PM +0200, Bastian Blank wrote:
> On Fri, Oct 16, 2009 at 11:43:52PM +0200, Bastian Blank wrote:
> > Okay. What about point 1 of my list? This still probes device-mapper
> > devices.

| - Probe for partitions, aka open, on all block devices with a (small)
| blacklist. This can be only allowed with a whitelist, as several
| device types may not be queried from add events or not at all, like
| several dm subtypes.

> This patch should use an apropriate whitelist.

Actually it lacks some safe device types.

--- a/data/95-devkit-disks.rules
+++ b/data/95-devkit-disks.rules
@@ -12,7 +12,7 @@
#

# skip rules for inappropriate block devices
-KERNEL=="fd*|mtd*|nbd*|gnbd*|btibm*", GOTO="probe_parttable_end"
+KERNEL!="hd*|sd*|sr*|cciss*|dasd*|mmcblk*|mspblk*", GOTO="probe_parttable_end"

# never access non-cdrom removable ide devices, the drivers are causing event loops on open()
KERNEL=="hd*[!0-9]", ATTR{removable}=="1", SUBSYSTEMS=="ide", ATTRS{media}=="disk|floppy", GOTO="probe_parttable_end"

And while you are at it, add dasd*|cciss* to the presentation list.

Bastian

--
You! What PLANET is this!
-- McCoy, "The City on the Edge of Forever", stardate 3134.0

--
To UNSUBSCRIBE, email to debian-bugs-dist-REQUEST.TakeThisOut@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster.TakeThisOut@lists.debian.org

Bastian Blank · External Since: Nov 21, 2004 Posts: 775

On Tue, Nov 03, 2009 at 09:41:24PM +0100, Michael Biebl wrote:
> Discussing with upstream, 95-devkit-disks.rules was changed to only act on
> "change" events (and not "add|change")

The rules may still be called with the device suspended and then block
udev to handle further rules and events, including the callback to the
devmapper tools. This is a classic race condition and I have no recipe
for this yet.

> Hopefully this addresses your remaining complaints in a sufficient manner so the
> conflicts in dmsetup can be removed again.

I accept that for now. But if it starts to break things I'll come back.

Bastian

--
War is never imperative.
-- McCoy, "Balance of Terror", stardate 1709.2

--
To UNSUBSCRIBE, email to debian-bugs-dist-REQUEST DeleteThis @lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster DeleteThis @lists.debian.org

Raphael Hertzog · External Since: May 28, 2005 Posts: 534

On Wed, 04 Nov 2009, Michael Biebl wrote:
> > Michael or Martin, can you integrate those 2 patches and upload?
>
> Raphael, please see my last email why those changes were not uploaded yet and
> how it has been approached differently.

Oh I missed that mail, thanks for the update. I'm not subscribed to the
bug, just annoyed by the conflict and wanted to help resolve the issue
(I did not intend to upload any NMU, it just's a practical way to provide
something ready for the maintainer).

Cheers,
--
Raphaël Hertzog

--
To UNSUBSCRIBE, email to debian-bugs-dist-REQUEST RemoveThis @lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster RemoveThis @lists.debian.org