GNOME Bugzilla – Bug 522053
GUnixMountMonitor needs to use /proc/self/mountinfo on recent Linux kernels
Last modified: 2016-07-27 19:03:38 UTC
Pasting from IRC. <davidz> alexl: btw.. we probably need to monitor /proc/mounts on Linux in GUnixMountMonitor at some point... with user mounts landing there are some issues with having /etc/mtab around <davidz> since.. obviously <davidz> unprivileged mounters can't update /etc/mtab <davidz> so /etc/mtab will most likely be a symlink to /proc/self/mounts <davidz> however <davidz> the way you monitor that file for changes <davidz> is currently via poll(2) <davidz> no inotify support on procfs <davidz> meaning <davidz> that using inotify on /etc/mtab will break <davidz> so we should fix this at some point * davidz will file a bug on b.g.o About usermounts see: http://lkml.org/lkml/2008/1/16/103 There's one complication; /proc/mounts only became poll'able in a late Linux 2.6 version.. think around 2.6.12.. will need to check. So probably need to check kernel version at runtime.
Monitoring was added here: Date: Mon Nov 7 17:15:49 2005 -0500 Subject: [PATCH] make /proc/mounts pollable Precedes: v2.6.15-rc1 We use this interface in HAL since then. Unfortunately, this is currently the only reliable interface for monitoring mount changes on Linux. We need to poll() a /proc/mounts file descriptor, all mount tree changes will wake up poll() with POLLERR. It is planned to kill /etc/mtab. See here: http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=f84e3f521e1449300e0fdc314b7b43b418a66dc3 There have many filesystem patches merged recently, so this may happen in the not too far future.
If we want to have this fixed in 2.18, we need to have a patch sometime soon.
Will do. Taking ownership.
Actually we should use /proc/self/mountinfo instead of /proc/mounts; see https://bugzilla.redhat.com/show_bug.cgi?id=491924 for details. Note that this requires Linux >= 2.6.26 but I think we can check runtime and use /etc/mtab or /proc/self/mountinfo depending.
Created attachment 193313 [details] [review] Use /proc/self/mountinfo to parse the list of mounts I’m attaching a draft patch that implements retrieving the list of mounts from /proc/self/mountinfo instead of /proc/mounts where available. The patch is very likely far from perfect as I am neither familiar with this specific code nor with glib’s coding style conventions. Reviews and comments are welcome, I’m particularly interested in knowing whether my approach has flaws I haven’t thought of. A bit of background on what motivated me to write the patch in the first place: I’m working on a project that runs in a chroot. The root partition (/) on the host is read-only. The guest root is located on another partition, and is read-write. Therefore there are two entries in /proc/mounts for '/', and they look like this: rootfs / rootfs ro,relatime 0 0 none / aufs rw,relatime 0 0 When querying the filesystem using gio in the chroot, find_mountpoint_for(…) will return the first one (that of the host), and consequently reports that the filesystem in the chroot is read-only, which is incorrect. Using /proc/self/mountinfo fixes the issue: only the mounts relevant to the chroot are exposed there.
Review of attachment 193313 [details] [review]: Your patch should be generated by "git format-patch". ::: gio/gunixmounts.c @@ +189,3 @@ +// for documentation on /proc/self/mountinfo, +// see http://www.kernel.org/doc/Documentation/filesystems/proc.txt No C++ comments in .c @@ +190,3 @@ +// for documentation on /proc/self/mountinfo, +// see http://www.kernel.org/doc/Documentation/filesystems/proc.txt +#define MOUNTINFO "/proc/self/mountinfo" Call this variable PROC_MOUNTINFO_PATH maybe for clarity? @@ +197,3 @@ + struct stat buf; + return (stat (MOUNTINFO, &buf) == 0) ? TRUE : FALSE; +} I think it'd be cleaner to have this function return a const char * with the path. Call it linux_get_mountinfo_path (). @@ +360,3 @@ +{ + const char *delim = ","; + char *token = strtok (mount_options, delim); No use of strtok in GLib please (it's a legacy broken non-threadsafe function). Just use g_strsplit() instead. @@ +398,3 @@ + char fstype[PATH_MAX]; + char mount_source[PATH_MAX]; + char super_options[PATH_MAX]; PATH_MAX on Linux is 4096 bytes - that's a decent amount of stack space you're allocating here. Granted the Linux default is 8MB, but still. Maybe consider dynamic allocation? If you use GRegex you can ignore this. @@ +404,3 @@ + have_mountinfo = mountinfo_exists (); + if (have_mountinfo) + file = fopen(read_file, "r"); Missing space between identifier and paren. @@ +415,3 @@ + + if (have_mountinfo) + { Ugh. Actually I think this whole function should be split in two probably if we have to handle /proc/self/mountinfo differently from the old /proc/mounts. @@ +418,3 @@ + while (fscanf (file, "%d %d %d:%d %s %s %s%*[^-]- %s %s %s%*[^\n]", + &mountid, &parentid, &major, &minor, root, mount_point, + mount_options, fstype, mount_source, super_options) == 10) Not really a big fan of fscanf. How about a GRegex instead?
https://www.kernel.org/pub/linux/utils/util-linux/v2.21/libmount-docs/ is now also a thing.
This issue has bitten us recently in endless and, as a temporary measure, we are testing a patch that slightly improves the current hack, which is here: https://github.com/endlessm/glib/pull/12/commits/d3cb94b547f07cc75e379782a50f698876eb67cb However, using /proc/self/mountinfo (or whatever is the right thing now) sounds like a much appealing option than keeping hacking the hack, so I'm adding myself to CC, hopefully will be able to complete the patch here at some point soon. Any advice (thanks Colin for the link, btw), more than welcome.
I've been working a bit this week on this and I have a branch with the changes that I think might help fixing this bug by using libmount (when available). Check the WIP branch out here: https://git.gnome.org/browse/glib/log/?h=wip/msanchez/libmount The patches are obviously not ready for committing yet (starting with the git log messages), but I think it could be a good point to start the discussion before converting in actual patches that can be reviewed here. Please let me know what you think, thanks.
Just a quick ping on this before I leave on holidays (off the next week). Any thoughts?
Created attachment 328489 [details] [review] Added autotools support for libmount Check whether libmount is available at configuration time and provide an option to explicitly enable or disable it, similar to libelf.
Created attachment 328492 [details] [review] Added placeholders for the libmount-based implementation
Created attachment 328495 [details] [review] Added placeholders for the libmount-based implementation
Created attachment 328496 [details] [review] Use libmount to find the path of the fstab file
Created attachment 328497 [details] [review] Implemented _g_get_unix_mounts() based on libmount
Created attachment 328498 [details] [review] Refactor common code into create_unix_mount_entry ()
Created attachment 328499 [details] [review] Implemented _g_get_unix_mount_points() based on libmount
Created attachment 328500 [details] [review] Refactor common code into create_unix_mount_point ()
Created attachment 328501 [details] [review] Monitor /proc/self/mountinfo when using libmount
Review of attachment 328498 [details] [review]: ::: gio/gunixmounts.c @@ +645,3 @@ + mount_entry = create_unix_mount_entry (vmt2dataptr (vmount_info, VMT_OBJECT), + vmt2dataptr (vmount_info, VMT_STUB), + (const char *) filesystem_type, calling g_strdup() above only to g_strdup() it again inside the helper, then free the dup seems unnecessary.
I only scanned this, but it looks quite reasonable to me.
Thanks for the feedback, Colin. You're right about the extra g_strdup, is totally unnecesary, but I'll wait a bit more in case someone else has more comments before updating the patches.
Created attachment 329543 [details] [review] Added placeholders for the libmount-based implementation
Created attachment 329544 [details] [review] Use libmount to find the path of the fstab file
Created attachment 329545 [details] [review] Implemented _g_get_unix_mounts() based on libmount
Created attachment 329546 [details] [review] Refactor common code into create_unix_mount_entry ()
Created attachment 329547 [details] [review] Implemented _g_get_unix_mount_points() based on libmount
Created attachment 329548 [details] [review] Refactor common code into create_unix_mount_point ()
Created attachment 329549 [details] [review] Monitor /proc/self/mountinfo when using libmount
Just pushed a new set of patches after removing the unnecessary g_strdup() and a really tiny re-organization of the first patches so that I make sure glib builds at any point (previously failed in the middle due to a missing reference to get_fstab_file()). Please review, thanks!
(In reply to Mario Sánchez Prada from comment #30) > Just pushed a new set of patches[...] ^^^^^^ Attached, not pushed!
Any more comments on this? Adding Alex Larsson (who I thought was already added) to CC in case he wants to comment on this too.
Review of attachment 328489 [details] [review]: sure
Review of attachment 329543 [details] [review]: This should be squashed with the following commits; we don't need to have a separate commit with a bunch of TODOs in it.
Review of attachment 329545 [details] [review]: ::: gio/gunixmounts.c @@ +363,3 @@ + + fs_source = mnt_fs_get_source(fs); + fs_target = mnt_fs_get_target(fs); Missing space before ( here (and in a couple of other places) @@ +390,3 @@ + } + mnt_free_context(ctxt); + I think you may need to mnt_free_iter here
Review of attachment 329544 [details] [review]: sure
Review of attachment 329546 [details] [review]: looks good
Review of attachment 329547 [details] [review]: ::: gio/gunixmounts.c @@ +868,3 @@ + gboolean is_loopback = FALSE; + + mount_path = mnt_fs_get_target(fs); Missing space before ( here and in a bunch of other places. @@ +887,3 @@ + if (mount_flags & MS_BIND) + continue; + Looks like you're leaking mount_options if the continue is taken. @@ +889,3 @@ + + is_read_only = (mount_flags & MS_RDONLY) ? TRUE : FALSE; + is_loopback = (userspace_flags & MNT_MS_LOOP) ? TRUE : FALSE; I would write these as var = (flags & FLAG) != 0 @@ +919,3 @@ + } + mnt_free_context(ctxt); + Missing mnt_free_iter here
Review of attachment 329548 [details] [review]: Looks like create_unix_mount_point was already contained in the previous patch
Review of attachment 329549 [details] [review]: ok
Review of attachment 329545 [details] [review]: ::: gio/gunixmounts.c @@ +363,3 @@ + + fs_source = mnt_fs_get_source(fs); + fs_target = mnt_fs_get_target(fs); Sorry, will fix those @@ +390,3 @@ + } + mnt_free_context(ctxt); + Oops! Good catch, thanks
Review of attachment 329547 [details] [review]: ::: gio/gunixmounts.c @@ +868,3 @@ + gboolean is_loopback = FALSE; + + mount_path = mnt_fs_get_target(fs); Argh.. sorry again about that. I guess I'm still too used to WebKit's coding style and have a hard time noticing these ones (even though I never liked not having the space). Will fix them all. @@ +887,3 @@ + if (mount_flags & MS_BIND) + continue; + Good catch again. You're right, I need to free that memory before skipping into the next iteration of the loop @@ +889,3 @@ + + is_read_only = (mount_flags & MS_RDONLY) ? TRUE : FALSE; + is_loopback = (userspace_flags & MNT_MS_LOOP) ? TRUE : FALSE; Ok, will change it @@ +919,3 @@ + } + mnt_free_context(ctxt); + True, will add it
(In reply to Matthias Clasen from comment #39) > Review of attachment 329548 [details] [review] [review]: > > Looks like create_unix_mount_point was already contained in the previous > patch Yes, sorry for the inconsistency with previous patches for g_get_unix_mounts. The point of this patch here is simply to re-write the rest of the file in terms of create_unix_mount_point(), as the newly added code already uses it, but this should be done in a consistent way, so I'll rework this while squashing.
(In reply to Matthias Clasen from comment #34) > Review of attachment 329543 [details] [review] [review]: > > This should be squashed with the following commits; we don't need to have a > separate commit with a bunch of TODOs in it. Sure. I've uploaded the patches like that so that they were easier to review, but I'll squash thise TODOs into their right places now.
Created attachment 331764 [details] [review] Use libmount to find the path of the fstab file
Created attachment 331765 [details] [review] Implemented _g_get_unix_mounts() based on libmount
Created attachment 331766 [details] [review] Refactor common code into create_unix_mount_entry ()
Created attachment 331767 [details] [review] Implemented _g_get_unix_mount_points() based on libmount
Created attachment 331768 [details] [review] Refactor common code into create_unix_mount_point ()
Created attachment 331769 [details] [review] Monitor /proc/self/mountinfo when using libmount
Comment on attachment 329543 [details] [review] Added placeholders for the libmount-based implementation Obsoleting this patch as the changes in here have been now squashed into the newer version of the other patches
Thanks for the review, Matthias. I think I have addressed all your comments in the new set of patches, but please take a look first if you can and let me know what you think before committing.
Review of attachment 331764 [details] [review]: ::: gio/gunixmounts.c @@ +745,3 @@ return "/etc/fstab"; #endif +#endif // HAVE_LIBMOUNT We generally use /* */ instead of //
Review of attachment 331765 [details] [review]: ok
Review of attachment 331766 [details] [review]: ok
Review of attachment 331767 [details] [review]: ok
Review of attachment 331768 [details] [review]: ok
Review of attachment 331769 [details] [review]: ok
Created attachment 331863 [details] [review] Use libmount to find the path of the fstab file
Created attachment 331864 [details] [review] Implemented _g_get_unix_mounts() based on libmount
Created attachment 331865 [details] [review] Implemented _g_get_unix_mount_points() based on libmount
(In reply to Matthias Clasen from comment #53) > Review of attachment 331764 [details] [review] [review]: > > ::: gio/gunixmounts.c > @@ +745,3 @@ > return "/etc/fstab"; > #endif > +#endif // HAVE_LIBMOUNT > > We generally use /* */ instead of // Argh! My C++ past haunting me again... I've changed that in the 3 patches affected and replaced the old ones. Will push later today if that's ok
Forgot to push the other day, pushed now to master after rebasing and testing: From https://git.gnome.org/browse/glib/commit/?id=4f9cddaeb8fc1b236ca49f08c1efa3fc9e75ecbf to https://git.gnome.org/browse/glib/commit/?id=f885c4dd0de984ec5dd4a498cc7819070129f9b5 Resolving as fixed
Hello. This is causing a leak as described in bug 769238. There is a valgrind log in that bug: https://bugzilla.gnome.org/attachment.cgi?id=332233
Sorry to have caused the trouble, I'll look into bug 769238 right away. Thanks