GNOME Bugzilla – Bug 747540
g_unix_mounts_get() ignores btrfs subvolumes
Last modified: 2018-05-24 17:44:49 UTC
Originally filed here: https://bugzilla.redhat.com/show_bug.cgi?id=1209989 g_unix_mounts_get() ignores btrfs subvolumes This causes some misbehaviors like nautilus or 'gvfs-ls trash://' fails to list trashed files. (e.g. http://askubuntu.com/questions/537055/ ) I think the culprit is the following portion of _g_get_unix_mounts() from gio/gunixmounts.c: | /* ignore any mnt_fsname that is repeated and begins with a '/' | * | * We do this to avoid being fooled by --bind mounts, since | * these have the same device as the location they bind to. | * It's not an ideal solution to the problem, but it's likely that | * the most important mountpoint is first and the --bind ones after | * that aren't as important. So it should work. | * | * The '/' is to handle procfs, tmpfs and other no device mounts. | */ | if (mntent->mnt_fsname != NULL && | mntent->mnt_fsname[0] == '/' && | g_hash_table_lookup (mounts_hash, mntent->mnt_fsname)) | continue; https://git.gnome.org/browse/glib/tree/gio/gunixmounts.c#n392 The code is not aware of the btrfs subvolumes.
I have similar problem with virtio block device, which is probably caused by the mentioned code. gvfsd-trash crashes on assert using gnome-continuous image, because GUnixMountPoint is missing for "/". /proc/mounts contains following entries: /dev/vda3 /sysroot /dev/vda3 / /dev/vda3 /usr /dev/vda3 /var however g_unix_mounts_get returns only the first one: /dev/vda3 /sysroot
The mentioned code should avoid bind mounts. Wouldn't be a solution to check for "bind" in mnttent->mnt_opts?
See also: https://mail.gnome.org/archives/gtk-devel-list/2016-June/msg00012.html for using libmount instead of our homegrown hack.
Yep, it seems that the recently added libmount support (Bug 522053) fixes this bug. Are there any plans setting libmount as default?
*** Bug 771384 has been marked as a duplicate of this bug. ***
It should be already fixed thanks to the libmount support, which is currently set as default.
*** Bug 765650 has been marked as a duplicate of this bug. ***
The idea that the libmount support fixes doesn't seem to be right - at least for the bind mounts - I have no idea about btrfs subvolumes. The code in _g_get_unix_mounts() ignores bind mounts in an inelegant way - it does: if (!mnt_table_is_fs_mounted (table, fs)) continue; But mnt_table_is_fs_mounted() is really only meant to be called on /etc/fstab entries and chokes on bind mounts. After all, anything found in /proc/self/mountinfo is by definition mounted! So questions are: * Do we actually want to return bind mounts out of _g_get_unix_mounts() - possibly breaking existing code that doesn't expect that. And if so, should g_get_unix_mount_points(), which explicitly ignores bind mounts, be modified? * If not, how should this be fixed - should there be new api to return a larger list of bind mounts? Should glib have a higher level function to do what gvfs is trying to do? (get the g_unix_mount_get_fs_type() result for a directory.) Short term fix for gvfs would be to assume that if the volume lookup logic fails, the file is *not* on NFS - instead of an assertion failure.
Hmm, I've not seen the libmount code in GLib in detail, however I see GUnixMountEntries for bind mounts and brtfs subvolumes in the output of g_unix_mounts_get with --enable-libmount and do not see them with --disable-libmount...
Created attachment 337388 [details] [review] trash: Do not crash if home mount isn't found Mount entry might not be found e.g. for bind mounts, btrfs subvolumes. Let's assume that the home trash is on filesystem with trusted notifications.
Created attachment 337431 [details] Test libmount behavior The testing I did yesterday to make sure my diagnosis in gdb was right was using the attached program. If you run it: $ mkdir /tmp/xxx $ sudo mount --bind /home/otaylor/ /tmp/m $ ./test-libmount | grep xxx /dev/mapper/fedora-home /tmp/xxx 0 The '0' there indicates mnt_table_is_fs_mounted() returned 0 for that entry. There are *some* circumstances where a bind mount will be returned - in particular, if you bind mount the root of a filesystem. (Try the above with / instead of $HOME)
Hmm, I've tried gnome-continuous image again and the result is still same as it is in Comment 1. You are right, some bind mounts are shown and some not... not sure what is right way to fix. It would be nice to have a comment from msanchez...
Review of attachment 337388 [details] [review]: ::: daemon/trashlib/trashwatcher.c @@ +73,3 @@ + */ + if (mount == NULL) + return is_home_trash ? TRASH_WATCHER_TRUSTED : TRASH_WATCHER_NO_WATCH; Do you think it makes sense to special case the homedir here? Does something horrible happens if we try to watch a read-only filesystem, or do we just waste a watch? @@ +126,3 @@ + { + pathname = g_file_get_path (file); + g_warning ("Mount entry was not found for %s", pathname); What action do you expect someone to take when seeing: *WARNING*: gvfsd-trash: Mount entry was not found for /home/otaylor in the journal? @@ +127,3 @@ + pathname = g_file_get_path (file); + g_warning ("Mount entry was not found for %s", pathname); + g_free (pathname); Double free of pathname
I remember there were some issues with bind mounts when I was working on this, but I don't remember the exact details right now, might need some time to wrap my head around this once again... That said, I do remember having the issue mentioned below downstream, although I'd swear using libmount got rid of the issue when I wrote that patch, but maybe I dreamed of it... (In reply to Ondrej Holy from comment #1) > I have similar problem with virtio block device, which is probably caused by > the mentioned code. gvfsd-trash crashes on assert using gnome-continuous > image, because GUnixMountPoint is missing for "/". > > /proc/mounts contains following entries: > /dev/vda3 /sysroot > /dev/vda3 / > /dev/vda3 /usr > /dev/vda3 /var > > however g_unix_mounts_get returns only the first one: > /dev/vda3 /sysroot The problem I experienced back in the day, if I recall correctly, was that the hack currently in place (see [1]) would never actually check "/" if "/sysroot" was checkd first, and then "/" would be reported as not-mounted, which was wrong. We applied locally (downstream) the following two patches (the first one had a bug, fixed in the second one): https://github.com/endlessm/glib/commit/674cba54461a311234af2f5996d6e667e4e834b7 https://github.com/endlessm/glib/commit/642934d77de4dbe94e9739ba9abdf169e58646f9 Perhaps we should apply these patches/hack upstream too, but I'm not 100% sure that would fix the issue reported. Ondrej, would you be able to apply those two patches locally and do a quick test run? [1] https://git.gnome.org/browse/glib/tree/gio/gunixmounts.c#n500
(In reply to Mario Sánchez Prada from comment #14) > The problem I experienced back in the day, if I recall correctly, was that > the hack currently in place (see [1]) would never actually check "/" if > "/sysroot" was checkd first, and then "/" would be reported as not-mounted, > which was wrong. > > We applied locally (downstream) the following two patches (the first one had > a bug, fixed in the second one): > > https://github.com/endlessm/glib/commit/ > 674cba54461a311234af2f5996d6e667e4e834b7 > https://github.com/endlessm/glib/commit/ > 642934d77de4dbe94e9739ba9abdf169e58646f9 > > Perhaps we should apply these patches/hack upstream too, but I'm not 100% > sure that would fix the issue reported. > > Ondrej, would you be able to apply those two patches locally and do a quick > test run? > > [1] https://git.gnome.org/browse/glib/tree/gio/gunixmounts.c#n500 These patches look like they are for the *non-libmount* getmntent code. glib now builds by default with the libmount code. I think in terms of effect, they will fix the crash by making the code return / and not /sysroot for ostree. And something similar could be done in the libmount case - removing the malfunctioning check for "mounted" and adding this sort of logic. But I really think that it's not fundamentally very sound. / => /dev/sda1 /foo => nfs,/ /home => nfs,/subdir (bind) will look like /bar is part of the root filesystem, which would confuse the gvfsd code. Note that it's perfectly possible, if unusual, to have: / => /dev/sda1 /foo => /dev/sda2,/subdir1 /bar => /dev/sda2,/subdir2 With / mounted nowhere, so I think the only sound thing to do in glib is to consider bind mounts mounts. I just don't know what that does to arbitrary current users. Would it confuse nautilus in some way? It would definitely have to be a devel-version only change.
(In reply to Owen Taylor from comment #15) > [...] > These patches look like they are for the *non-libmount* getmntent code. glib > now builds by default with the libmount code. You're right, those patches are for the non-libmount code, sorry for the confusion. > I think in terms of effect, > they will fix the crash by making the code return / and not /sysroot for > ostree. And something similar could be done in the libmount case - removing > the malfunctioning check for "mounted" and adding this sort of logic. > > But I really think that it's not fundamentally very sound. Yeah, I don't think either. It was a hack we took but never was meant to be a long term solution, that's why I actually worked on the libmount patches after all. > / => /dev/sda1 > /foo => nfs,/ > /home => nfs,/subdir (bind) > > will look like /bar is part of the root filesystem, which would confuse the > gvfsd code. > > Note that it's perfectly possible, if unusual, to have: > > / => /dev/sda1 > /foo => /dev/sda2,/subdir1 > /bar => /dev/sda2,/subdir2 > > With / mounted nowhere, so I think the only sound thing to do in glib is to > consider bind mounts mounts. I just don't know what that does to arbitrary > current users. Would it confuse nautilus in some way? It would definitely > have to be a devel-version only change. I think that's probably the best approach too. I have no idea either what will break by doing that but getting rid of the hack sounds like the best proposition to me too, so a devel version would probably make sense.
Thanks for the review! (In reply to Owen Taylor from comment #13) > Review of attachment 337388 [details] [review] [review]: > > ::: daemon/trashlib/trashwatcher.c > @@ +73,3 @@ > + */ > + if (mount == NULL) > + return is_home_trash ? TRASH_WATCHER_TRUSTED : TRASH_WATCHER_NO_WATCH; > > Do you think it makes sense to special case the homedir here? Does something > horrible happens if we try to watch a read-only filesystem, or do we just > waste a watch? I've just wanted to cover all cases, but it is true that the following condition should never happen: (mount == NULL && is_home_trash == FALSE)... (See Bug 522314 about monitoring read only filesystems...) > @@ +126,3 @@ > + { > + pathname = g_file_get_path (file); > + g_warning ("Mount entry was not found for %s", pathname); > > What action do you expect someone to take when seeing: > > *WARNING*: gvfsd-trash: Mount entry was not found for /home/otaylor > > in the journal? I expect that somebody will file a bug report, because this shouldn't really happen. It is not uncommon that home dir is on NFS, then it will be useful for debugging... This is a still workaround, we can't remove the assert and tell it is ok now... > @@ +127,3 @@ > + pathname = g_file_get_path (file); > + g_warning ("Mount entry was not found for %s", pathname); > + g_free (pathname); > > Double free of pathname I don't think so, there are two g_file_get_path and two g_free, or am I wrong?
(In reply to Ondrej Holy from comment #17) > Thanks for the review! > > (In reply to Owen Taylor from comment #13) > > Review of attachment 337388 [details] [review] [review] [review]: > > > > ::: daemon/trashlib/trashwatcher.c > > @@ +73,3 @@ > > + */ > > + if (mount == NULL) > > + return is_home_trash ? TRASH_WATCHER_TRUSTED : TRASH_WATCHER_NO_WATCH; > > > > Do you think it makes sense to special case the homedir here? Does something > > horrible happens if we try to watch a read-only filesystem, or do we just > > waste a watch? > > I've just wanted to cover all cases, but it is true that the following > condition should never happen: (mount == NULL && is_home_trash == FALSE)... Ah, OK, I agree it's impossible to have this condition, but then I'd expect the code to either a) just always return TRUSTED, or b) assert() - not c) return something different for the impossible case. Or maybe the workaround hould live in the caller, and decide_watch_type() should only be used for mount != NULL? Basically just style - whatever you think is appropriate. > (See Bug 522314 about monitoring read only filesystems...) > > @@ +126,3 @@ > > + { > > + pathname = g_file_get_path (file); > > + g_warning ("Mount entry was not found for %s", pathname); > > > > What action do you expect someone to take when seeing: > > > > *WARNING*: gvfsd-trash: Mount entry was not found for /home/otaylor > > > > in the journal? > > I expect that somebody will file a bug report, because this shouldn't really > happen. It is not uncommon that home dir is on NFS, then it will be useful > for debugging... > > This is a still workaround, we can't remove the assert and tell it is ok > now... But we don't want a bug report from everybody using an ostree system, do we? I think people are pretty conditioned to ignoring scary WARNING in their system logs, but that's not a good thing. We really shouldn't be putting out a warning unless we actually think that it's telling the user that something went wrong in a way they should know about or we should know about. (The absence of the warning doesn't mean that we got the mount point detection right - it just means that we found a mount point for /) Up to you. > > @@ +127,3 @@ > > + pathname = g_file_get_path (file); > > + g_warning ("Mount entry was not found for %s", pathname); > > + g_free (pathname); > > > > Double free of pathname > > I don't think so, there are two g_file_get_path and two g_free, or am I > wrong? Oops, yes, you are right.
Created attachment 339247 [details] [review] trash: Do not crash if home mount isn't found I have slightly modified the patch, however, I didn't remove the warning, because this is just a workaround and it has to be fixed in a different way. We can't simply remove the assert and tell it is ok...
Comment on attachment 339247 [details] [review] trash: Do not crash if home mount isn't found Attachment 339247 [details] pushed as 897f516 - trash: Do not crash if home mount isn't found
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/1024.