Bug 22652

Created attachment 27449 [details]
debug output

This is the devkit-disks-daemon output when being d-bus activated through gvfs. The log shows that initially the internal hard disk partitions (sdaX) are added:

**** ADDING /sys/devices/pci0000:00/0000:00:1f.1/host0/target0:0:0/0:0:0:0/block/sda/sda3
**** UPDATING /sys/devices/pci0000:00/0000:00:1f.1/host0/target0:0:0/0:0:0:0/block/sda/sda3

This shouldn't emit events (device_add() gets called with emit_event==FALSE from devkit_disks_daemon_new()), but right after that I get change events as well:

**** CHANGING /sys/devices/pci0000:00/0000:00:1f.1/host0/target0:0:0/0:0:0:0/block/sda/sda3
**** UPDATING /sys/devices/pci0000:00/0000:00:1f.1/host0/target0:0:0/0:0:0:0/block/sda/sda3

"udevadm monitor --udev -e" doesn't show anything coming from udev

(In reply to comment #1)
> Created an attachment (id=27449) [details]
> debug output
> 
> This is the devkit-disks-daemon output when being d-bus activated through gvfs.
> The log shows that initially the internal hard disk partitions (sdaX) are
> added:
> 
> **** ADDING
> /sys/devices/pci0000:00/0000:00:1f.1/host0/target0:0:0/0:0:0:0/block/sda/sda3
> **** UPDATING
> /sys/devices/pci0000:00/0000:00:1f.1/host0/target0:0:0/0:0:0:0/block/sda/sda3
> 
> This shouldn't emit events (device_add() gets called with emit_event==FALSE
> from devkit_disks_daemon_new()), but right after that I get change events as
> well:
> 
> **** CHANGING
> /sys/devices/pci0000:00/0000:00:1f.1/host0/target0:0:0/0:0:0:0/block/sda/sda3
> **** UPDATING
> /sys/devices/pci0000:00/0000:00:1f.1/host0/target0:0:0/0:0:0:0/block/sda/sda3
> 

I don't get any add events from devkit-disks-daemon using 'devkit-disks --monitor', only some 'change' event which is correct since we read ATA SMART data async on startup. It would be a bug to get 'add' events on coldplug, sure.

Please verify it's the same for you (and state the version - if see the bug, please also try with HEAD).

FWIW, it's true, that the debug spew from the daemon is misleading - that's just because daemon startup involves coldplug and the same codepaths are reused.

Hi,

This is not really a DeviceKit-disks bug, it's a gvfs bug. Anyway, let me try and explain how this is supposed to work since this information applies to any environment attempting to enforce policy. When I've done that, I'll address your bug report and then I'll move the bug to the GNOME bugzilla.

First of all, automounting (and, for that matter, also: autoassemby of RAID arrays, auto unlocking LUKS devices, autoassembly of LVM logical volumes, establishing connections to preconfigured iSCSI connections) is a really tricky business (we don't yet handle LVM or iSCSI but that is on the road map).

This is especially true if you want the added geek-comfort of things working perfectly with the classic command line tools such as fdisk(8), parted(8), mkfs(8), mdadm(8) and so on. And, in GNOME at least, we do want that level of geek-comfort.

For example, if you use the low-level tools from gnome-terminal within a GNOME desktop session, we want the following user experience

 1. The desktop shell (e.g. file manager, Places menu in panel, Disk
    Mounter applet, file chooser dialogs) should always reflect the current
    state of affairs - for example, if you add/remove partitions with fdisk(8)
    or parted(8) then you want the desktop shell bits to be instantly updated.
    Ditto if you create file systems. Ditto if you create a new RAID device.
    and so on.

The way we implement (most of) 1. is through having udev listen on changes on block devices with inotify. E.g. every time the last opener of a O_RDRW file descriptor for a block device closes his fd, then udev will synthesize a 'change' uevent. 

Now, this 'change' event bubbles through the udev stack, triggers file system probing (60-persistent-storage-rules.udev), and then gets delivered to devkit-disks-daemon which acts on it (typically turning it into one or more D-Bus signals).

Eventually, the event from devkit-disks daemon reaches the GIO volume monitor implemetation (gvfs-gdu-volume-monitor) in your desktop session and it is turned into appropriate signals on GVolumeMonitor and GDrive/GVolume/GMount objects.

Finally, the desktop automounter (and, in the future, autoassembler) that (currently) resides in the Nautilus process acts on these events.

 2. We _only_ want the desktop automounter (and, in the future,
    autoassembler) to take action if a couple of conditions are true,
    typically if it looks like you've just inserted the device (for example
    a USB enclosure with non-removable media) or just inserted the media
    for which the block device in question in residing on (for example if
    you just inserted a CF card into a USB card reader).

    For example, if you create a new partition with fdisk(8) things might
    just happen to be in a state (depending on the phase of the moon) so
    the newly created partition is picked up (by fs probers invoked by udev)
    as a mountable file system. This is typically due to fdisk(8) not clearing
    filesystem signatures on the partition. FWIW, This happens more
    frequently than you'd expect since many users use same/similar sizes
    when repartioning media.

    (Of course, if you create partitions/filesystems via the
    devkit-disks-daemon interfaces we do clear filesystem signatures so
    this can't happen. But we want the old tools to keep working and it
    would be wrong for fdisk(8) to start messing around with your data.)

    Also, even if you create a filesystem via mkfs(8) then you don't want
    it to be automounted.

The way we currently deal with this is by having devkit-disks-daemon export two properties

  :device-detection-time
  :device-media-detection-time

see

 http://hal.freedesktop.org/docs/DeviceKit-disks/Device.html#Device:device-detection-time
 http://hal.freedesktop.org/docs/DeviceKit-disks/Device.html#Device:device-media-detection-time

Having these properties around allows the desktop automounter/autoassembler to make informed decisions. In particular, the GIO volume monitor implementation (gvfs-gdu-volume-monitor) will set g_volume_should_automount() to either TRUE or FALSE, see

 http://library.gnome.org/devel/gio/unstable/GVolume.html#g-volume-should-automount

based on whether these values are (something like) five seconds from the current time.

Note that the way things work, we will automatically attempt to automount (and in the future autoassemble as well) all user-visible partitions (this includes system-internal partitions) when the GNOME desktop session starts. This is intended. And yes, it would be a bug (and, yes, as your report indicates) if we started popping up dialogs / error dialogs (either via GMountOperation or PolicyKit) if one or more of these operations fail.

Hope this clarifies how things are supposed to work - hope it's not too confusing.

(In reply to comment #0)
> When starting a GNOME session without dk-disks running already, gvfs D-BUS
> activates dk-disks, which causes an automount attempt on internal hard disks.
> This doesn't happen if dk-disks is already running.

The desktop automounter actually should attempt to automount even if when devkit-disks is running. This is probably due to the GIO volume monitor implementation (gvfs-gdu-volume-monitor) incorrectly setting the flag for g_volume_should_automount() based on the :device-detection-time and :device-media-detection-time properties.

Worth looking into whether the bug is in GVfs (interpreting the values wrong), devkit-disks-daemon (setting the values wrong) or both...

As noted in comment 4, however, there should be no user interaction during automounting at startup. This means

 1. No "LUKS Unlock Volume" password dialogs etc.
 2. No error dialogs if an operation invoked during start-up triggers
    an error.
 3. No dialogs from PolicyKit

The first two are handled by Nautilus' automounter by respectively passing in NULL for the GMountOperation (meaning "no user interaction" requested) and also avoiding showing error dialogs for these operations. I believe this is working mostly fine.

The latter is more tricky to fix and the reason it is not fixed is that it's not apparent to me what the best way to fix it is. Basically, with the move to the new PolicyKit version all authentication dialogs happen out-of-band so to speak. For some background see the last half of this mail

http://lists.freedesktop.org/archives/polkit-devel/2009-May/000124.html

So, basically, right now devkit-disks-daemon will always pass in the POLKIT_CHECK_AUTHORIZATION_FLAGS_ALLOW_USER_INTERACTION flag when checking authorizations. Meaning that any call to FilesystemMount() might trigger an authentication dialog. 

What happens on GNOME startup is that the desktop automounter attempts to mount all volumes. So if you have a system-internal volume the default configuration will require authentication to mount it. So you see the polkit authentication dialog at start-up. That's a bug.

There are two ways to fix this

 1. If GMountOperation is NULL (meaning no user interaction is requested)
    then the gdu volume monitor could pass a "no_auth_interaction" flag
    to the FilesystemMount() method on devkit-disks (and we'd honor that
    by not passing POLKIT_CHECK_AUTHORIZATION_FLAGS_ALLOW_USER_INTERACTION
    to polkit.

 2. Since gdu-gvfs-volume-monitor is a separate process, it could call into
    the polkit authority to signal that requests from this process should
    never trigger authentication dialogs even if the mechanism (devkit-disks)
    passes POLKIT_CHECK_AUTHORIZATION_FLAGS_ALLOW_USER_INTERACTION

I'm leaning more towards option 1. and also changing _all_ operations on devkit-disks to take an array of option strings (most of them do already, it's a great way to have some degree of extensibility) so you can always pass "no_auth_interaction" for any operation.

Thoughts?

FWIW, I've filed http://bugzilla.gnome.org/show_bug.cgi?id=587981 in the GNOME bugzila to track the GNOME side of this.

> This is quite a nasty usability issue on installed systems (since GNOME starts
> with a polkit auth dialog), 

Yep, the fact that we currently present an authentication dialog is a bug.

> and a potential data loss problem on the live
> system, since this auto-mounts all partitions without questions. This could
> potentially disrupt broken file systems, hibernated NTFS partitions, etc.

Oh, I don't think these are good reasons at all. By the same token all automounting / autoassembly is bad.

And if a file system driver corrupts your data, the solution is to fix the file system driver or fail the mount operation entirely (e.g. hibernated NTFS partition).

    David

> I don't get any add events from devkit-disks-daemon using 'devkit-disks
> --monitor', only some 'change' event which is correct since we read ATA SMART
> data async on startup. It would be a bug to get 'add' events on coldplug, sure.

This is a little tricky, I can't use --monitor, since this already autospawns devkit-disks-daemon and generates those add/change dk events (not udev events). The bug happens because/when gvfs gdu spawns dk-disks and thus seem to pick up these initial change events only.

If i kill -daemon and do "devkit-disks --monitor" I see nothing (also nothing in udevadm monitor).

> Please verify it's the same for you (and state the version - if see the bug, 
> please also try with HEAD).

I'm using version 005, can also try with HEAD. But I'll read your followup posts first, thank you for these details!

(In reply to comment #6)
> > I don't get any add events from devkit-disks-daemon using 'devkit-disks
> > --monitor', only some 'change' event which is correct since we read ATA SMART
> > data async on startup. It would be a bug to get 'add' events on coldplug, sure.
> 
> This is a little tricky, I can't use --monitor, since this already autospawns
> devkit-disks-daemon and generates those add/change dk events (not udev events).
> The bug happens because/when gvfs gdu spawns dk-disks and thus seem to pick up
> these initial change events only.
> 
> If i kill -daemon and do "devkit-disks --monitor" I see nothing (also nothing
> in udevadm monitor).

Running 'devkit-disks --monitor', then killing the daemon should work - e.g. as soon as the daemon comes up, then the 'devkit-disks --monitor' will start picking up events again (you can verify this by e.g. 'echo change > /sys/block/sda/uevent' as root and watching the monitor process print out the event).

So if you are not seeing anything, it seems like no wrong 'add' events are being emitted.

Btw, another way to verify this is to use 'dbus-monitor --system' as root.

> > Please verify it's the same for you (and state the version - if see the bug, 
> > please also try with HEAD).
> 
> I'm using version 005, can also try with HEAD. But I'll read your followup
> posts first, thank you for these details!

005 shouldn't be much different than HEAD in this respect. The major bug-fix that might have changed behavior was this one

http://cgit.freedesktop.org/DeviceKit/DeviceKit-disks/commit/?id=3f8f7c0983f2a081e194ca3620500ed023bb9088

>1. If GMountOperation is NULL (meaning no user interaction is requested)
>   then the gdu volume monitor could pass a "no_auth_interaction" flag
>   to the FilesystemMount() method on devkit-disks (and we'd honor that
>   by not passing POLKIT_CHECK_AUTHORIZATION_FLAGS_ALLOW_USER_INTERACTION
>   to polkit.

I don't have a firm opinion on this one, but it sounds sane to me.

This will still be a little inconsistent, since live CDs would always automount internal drives (which I still consider a bad thing [1]), while installed systems wouldn't. But I'll get used to it . :-)

Thanks! 

Martin

[1] Installers need to unmount them again, mounting takes time, and broken file systems really shouldn't be automounted r/w without your consent, it only makes things worse (and you might have booted a live system to repair exactly that). IMHO your huge internal disks should be automounted on demand when you need them, i. e. when you click on the drive icon. It shouldn't really be so much different from the user's perspective when the drive is mounted (at session startup or when you open it in nautilus), but it would avoid lengthy and expensive, and potential dangerous mount processes at session start.

> Running 'devkit-disks --monitor', then killing the daemon should work

confirmed. No events at all when gvfs starts up, so it seems to be a bug in gvfs.

(In reply to comment #8)
> [1] Installers need to unmount them again, mounting takes time, and broken file
> systems really shouldn't be automounted r/w without your consent, it only makes
> things worse (and you might have booted a live system to repair exactly that).

So maybe live systems should just turn off automounting. Or have a special option in the bootloader to do that. And if people are doing "forensic live-cds" they definitely want to do that. And they can.

Either way, the presumption that automounting stuff "makes things worse" is in my view just a bad excuse for broken file system drivers. In fact, it's better to expose and fix these drivers than try to avoid not triggering bugs. 

If OS vendors don't want this behavior for certain versions of their OSes (like Red Hat's RHEL, SUSE's SLES or Canonical's Ubuntu LTS relases) it's relatively straightforward to patch things the way you want them. I just don't think it's something we want in the upstream sources where we are supposed to be moving things forward, not ensuring maximum compatibility.

(And, my gods, yes, some file systems are somewhat broken. When mounting, say, ext3 in read-only mode it plays back the journal triggering writes. You can argue both ways about this but my view is that it's mostly wrong to do this by default.)

(Btw, another thing is that the desktop bits should probably run fsck (via devkit-disks) before mounting anything. But that's a separate issue and should be filed agains gvfs in the gnome bugzilla.)

> IMHO your huge internal disks should be automounted on demand when you need
> them, i. e. when you click on the drive icon. It shouldn't really be so much
> different from the user's perspective when the drive is mounted (at session
> startup or when you open it in nautilus), 

We can't really do this since it will break autorun and other things like that. Also, you are only referring to the UI, keep in mind the APIs are stateful and has a notion of mounted/unmounted. So it won't work.

Do note that the file manager, file chooser and panel Places UI does work mostly like this though. E.g. you click an icon and the experience is the same whether the the underlying mount is mounted or not.

> but it would avoid lengthy and
> expensive, 

Mounting happens asynchronously so this "lengthy process" [sic] isn't really visible to anyone nor anything to worry about.

> and potential dangerous mount processes at session start.

See above.

OK, you convinced me wrt. automount. :-)

So, to summarize:

 - gvfs needs to get fixed to always automount when starting the gdu monitor, not just when it triggers DK-disks. This is gnome #587981

 - DK-disks needs to grow a no_auth_interaction flag (this bug)

 - gvfs needs to pass that flag on startup (also above gnome bug?)

Thanks!

(In reply to comment #11)
> OK, you convinced me wrt. automount. :-)

Cool, I'm glad about that. Automounting / policy stuff is really tricky and hard to get right so am happy the current solution works for more than me!

> So, to summarize:
> 
>  - gvfs needs to get fixed to always automount when starting the gdu monitor,
> not just when it triggers DK-disks. This is gnome #587981

Right.

>  - DK-disks needs to grow a no_auth_interaction flag (this bug)

Yeah, I'll add that soon.

>  - gvfs needs to pass that flag on startup (also above gnome bug?)

Yup, let's handle it here. 

FWIW, I'm currently waiting for alexl to review a huge invasive GVfs patch (bgo #587484) (this is also what is blocking getting your hal->gudev patches in). Then I'll fix up the gdu bits of gvfs (there are some other bugs that needs fixing too) to work according to what is outlined in this bug.

It will require new DKD and gdu releases - but should be able to get this done this or next week. Then after all that hopefully most desktop storage bugs, at least those bugs requiring GLib, DKD or gdu changes, should be fixed and we can move to bugfix mode for the gvfs-gdu bits after that. Which is probably a good thing as this is a major change...

So, yeah, lots of stuff happening, sorry if it's all a bit confusing!

Cheers,
David

> >  - gvfs needs to pass that flag on startup (also above gnome bug?)
>
> Yup, let's handle it here. 

Argh, I meant "there", not "here". Sorry about that.

It looks as if the DK-D side of this is fixed in 006?

     Make FilesystemMount() accept an 'auth_no_user_interaction' option

(In reply to comment #14)
> It looks as if the DK-D side of this is fixed in 006?
> 
>      Make FilesystemMount() accept an 'auth_no_user_interaction' option
> 

Yup, and the gvfs bit is here:

http://git.gnome.org/cgit/gvfs/commit/?id=3c9e828938625a88f71969c129a0972276b8c99e