GNOME Bugzilla – Bug 637800
Incorrect Trash behaviour for the whole file system if $HOME is on a bind mount
Last modified: 2018-09-21 17:07:13 UTC
I'm having problems with trashing files outside my home. The issue I experience is that I cannot trash any files in /storage/Music/, even though I have a /storage/.Trash/ directory with permissions 1777 (world writable + sticky bit, just like /tmp), which is exactly what the XDG Trash Specification prescribes. I investigated this a bit and seems that my /home being a bind mount is the culprit here. This seems strange at first, since the problem I have has to do with trashing files on a "regular" mount outside my home dir, i.e. not residing on the bind mount (and also outside $HOME). My mount setup is like this: /dev/mapper/vg-storage on /storage type ext3 (rw,user_xattr,acl) /storage/home on /home type none (rw,bind) /storage/srv on /srv type none (rw,bind) In other words: I have a large logical volume named "storage" (which is my shiny big disk) mounted at /storage. Inside /storage I have directories like /storage/Music and /storage/Backups (both quite large). Since this is a desktop machine with a single big disk and multiple user accounts, I don't want to have the "file storage" and the /home directory (and also /srv, but that doesn't matter) on different file systems, since that means I should tell beforehand how big those would be. The solution is elegant and obvious: mount the big disk at /storage and add a bind mount for /home pointing to the directory /storage/home (on the big disk). I hope you agree my setup is not "esoteric" or "totally unusual" - to the best of my knowledge this a clean and neat way to solve a standard "problem" for shared home machines. Back to the trash problem. This is what happens: $ cd /storage/Music /storage/Music $ touch empty /storage/Music $ gvfs-trash empty Error trashing file: Unable to move file to the wastebasket: Invalid cross-device link Strace(1) tells me gvfs-trash tries to rename(2) the file, which doesn't succeed because it returns EXDEV. The rename(2) man page states: EXDEV oldpath and newpath are not on the same mounted file system. (Linux permits a file system to be mounted at multiple points, but rename() does not work across different mount points, even if the same file system is mounted on both.) This is quite strange, so let's look at the strace(1) output: rename("/storage/Music/empty", "/home/uws/.local/share/Trash/files/empty.21") = -1 EXDEV (Invalid cross-device link) write(2, "Error trashing file: Unable to m"..., 87Error trashing file: Unable to move file to the wastebasket: Invalid cross-device link This is strange. Why does gio try to move the file inside /storage/Music into the trash directory inside my home directory while /storage/.Trash/ exists as per the XDG trash spec? Strace(1) also shows that /storage/.Trash/ is not even tried: no stat or open calls to be found in the output. I looked into the code for g_local_file_trash() in gio/glocalfile.c to discover this test that seems to cause the problem I'm seeing: if (file_stat.st_dev == home_stat.st_dev) { ... trashdir = g_build_filename (g_get_user_data_dir (), "Trash", NULL); ... } else { ... the $toplevel/.Trash/$uid and $toplevel/.Trash-$uid/ logic is here } The expected behaviour is that the file ends up in the /storage/.Trash/$uid/ directory. However, since my home dir is a bind mount inside /storage, and bind mounts and the "real" mount share the GStatBuf.st_dev value, GIO incorrectly concludes this file should be trashed inside the Trash in my home dir (consequently trying rename(2), which fails with EXDEV), even though a completely valid mount-local Trash directory exists! I'm not sure why things are the way they are right now. Is this really the intended behaviour? Or is the st_dev check purely intended to find out whether the file is inside the user's home directory (as opposed to on a removable disk, for instance)? In that case, something like if (g_file_has_prefix(file_to_trash, home_dir)) { ... } would be more appropriate, imo. This would mean only files inside the user's home dir end up in the home dir Trash, which seems intended behaviour to me. In normal setups users cannot delete files from random directories outside their home dir (e.g. from other users' home dirs), checking for the same st_dev (for e.g. the /home file system) seems a bit overkill to me. The device-specific .Trash/ support handles all other cases (storage mounts, removable disks, and so on) just fine. Oh, let me conclude with a final notion to be clear and complete. Nautilus shows the trash in /storage/.Trash/$uid just fine if I put some correct files in there by hand (data in files/ and metadata in the info/ directories), so this is purely a "trash this file" problem, not a "my trashed files are invisible" problem like as described in bug #604015. (Side note: the "gboolean is_homedir_trash" seems superfluous in g_local_file_trash() since it's only assigned to, but never used.)
(In reply to comment #0) > > I'm not sure why things are the way they are right now. Is this really the > intended behaviour? Or is the st_dev check purely intended to find out whether > the file is inside the user's home directory (as opposed to on a removable > disk, for instance)? In that case, something like > > if (g_file_has_prefix(file_to_trash, home_dir)) { ... } No, that's racy; any component of the pathname could change. I think the right fix is for gio here to, if rename() fails with -EXDEV, to fall back to copy/unlink. I'm not sure if there's a way to detect from userspace that bind mounts across the same disk break rename() or not.
Why not a symlink or actually have your home directory in /storage/? The wording in the trash specification is this: """ The “home trash” should function as the user's main trash directory. Files that the user trashes from the same file system (device/partition) should be stored here (see the next section for the storage details). """ Technically that means that Colin is right (although I'm not sure it's right in spirit). At the same time, we've refused in other bugs to do the rename-by-copy behaviour, so I think don't think we want to go that way. I don't understand the race with the home directory check. I also don't understand why the kernel can't just do the "obviously correct" thing here and allow the rename to proceed.
(In reply to comment #2) > Why not a symlink or actually have your home directory in /storage/? Because it's ugly and quite visible, and also because a) symlinks break "readlink -f" and give strange canonicalization results b) having a home directory outside /home is non-standard and can break things in unexpected ways, especially if existing config files/databases point to existing paths. Btw, from what I read on the interwebs, properly detecting bind mounts depends on parsing /proc/mounts and comparing path prefixes. Not sure that's the way to go here... Anyway, my homedir being a bind mount should not cause GIO trash support breakage on totally unrelated file paths (outside $HOME), which is the problem I'm experiencing.
(In reply to comment #2) > I also don't understand why the kernel can't just do the "obviously correct" > thing here and allow the rename to proceed. Yes, that would be the best I think. The kernel should be able to know that the path is on a bind mount, and transparently rename() from the "real" location to the new "real" location (both outside the bind mount) instead.
Created attachment 177613 [details] [review] Add file prefix checking in addition to /home filesystem device check This patch adds a file prefix check before deciding a file should be moved to the Trash directory inside the user's home directory (which fails for reasons described in the original report). The result after applying this patch is that trashing files as described in the original report works as expected for me.
(In reply to comment #5) > The result after applying this patch is that trashing files as described in the > original report works as expected for me. Forgot to add how: the file is trashed to the device-specific trash (/storage/.Trash/1000/... in my case). It subsequently shows up correctly in Nautilus, where it can be opened using e.g. gedit.
(In reply to comment #0) > (Side note: the "gboolean is_homedir_trash" seems superfluous in > g_local_file_trash() since it's only assigned to, but never used.) And I take back this side note - I overlooked a part of the code. The variable is used after all.
(In reply to comment #1) > No, that's racy; any component of the pathname could change. Btw, I don't see how. If a file is moved/renamed by another process while GIO is trying to trash the file, g_file_has_prefix() does not cause a race condition any more than an outdated GStatBuf for the file does, so I don't see a problem here.
Created attachment 177620 [details] [review] Add file prefix checking in addition to /home filesystem device check Same patch, but this time without an obvious memory leak.
Review of attachment 177620 [details] [review]: I still don't like this, and I think it's better to do the copy if rename() fails with -EXDEV for the same device, because I'm fairly sure the only way that could happen is Linux bind mounts, or possibly different process mount namespaces. I mean, we're going to be falling back to a copy anyways, right? ::: gio/glocalfile.c @@ +1816,3 @@ homedir = g_get_home_dir (); g_stat (homedir, &home_stat); + homedir_file = g_file_new_for_path(homedir); Space between identifier and paren (look at the rest of the code!) @@ +1820,3 @@ is_homedir_trash = FALSE; trashdir = NULL; + if (file_stat.st_dev == home_stat.st_dev && g_file_has_prefix(file, homedir_file)) Ditto. @@ +1938,3 @@ } + g_object_unref(homedir_file); Ditto.
(In reply to comment #10) > Review of attachment 177620 [details] [review]: I'll fix all issues you mentioned. > I still don't like this, and I think it's better to do the copy if rename() > fails with -EXDEV for the same device, because I'm fairly sure the only way > that could happen is Linux bind mounts, or possibly different process mount > namespaces. > > I mean, we're going to be falling back to a copy anyways, right? No, not at all! With my patch no copies are made, since the device-specific Trash is, by definition, on the same device (otherwise it wouldn't be the device-SPECIFIC Trash after all!). Look at this strace(1) output if you don't believe me: $ strace -erename gvfs-trash testfile rename("/storage/Music/testfile", "/storage/.Trash/1000/files/testfile.2") = 0 # (and another unrelated rename() for the metadata file by g_file_set_contents())
Created attachment 177625 [details] [review] Add file prefix checking in addition to /home filesystem device check Same patch as before, but with all style fixes applied (also "GFile *homedir_file" instead of "Gfile* homedir_file" - note the space).
Wouter: What if someone mounts another filesystem inside their home directory -- say, a large partition under ~/Music, for example? In my opinion, this is no more strange than what you are doing here, and your prefix-based check would fail in that case. Colin: I think we should never copy. Trashing must be fast.
(In reply to comment #13) > > Colin: I think we should never copy. Trashing must be fast. It's a well known cost of bind mounts. I don't see how we can work around it easily. The only way I can see making it work is adding a dedicated kernel interface for when the bind mount is internal to a filesystem, shuffle the data over directly.
I think the behaviour of my patch is misunderstood. My patch contains this line: + if (file_stat.st_dev == home_stat.st_dev && g_file_has_prefix (file, homedir_file)) The check is extended to check for *both* the same device (this succeeds for bind mounts - which is a problem) and the homedir prefix, and only proceeds to using ~/.local/share/.Trash/ if both conditions are satisfied. The else clause of this if statement contains all the "try to find a device-specific trash and try to use it" logic. Now, let me get back at Ryan's question. (In reply to comment #13) > Wouter: What if someone mounts another filesystem inside their home directory > -- say, a large partition under ~/Music, for example? Than file_stat.st_dev == home_stat.st_dev will be false, and no homedir trash will be used. Instead, ~/Music/.Trash/$uid will be tried (if that directory exists). If it works, it rename()s as usual to this device-specific trash. This seems correct to me. > In my opinion, this is no more strange than what you are doing here, and your > prefix-based check would fail in that case. I agree it's no more "strange" (if at all) than my case, but since the st_dev check has failed already no homedir prefix checking is done at all, so what you say seems incorrect to me: the check won't fail, since it's not performed in the first place. > Colin: I think we should never copy. Trashing must be fast. Agreed. With my patch Trashing still does a rename() whenever possible, and hence is fast. And to get back at Colin's remarks: (In reply to comment #14) > It's a well known cost of bind mounts. I don't see how we can work around it > easily. The only way I can see making it work is adding a dedicated kernel > interface for when the bind mount is internal to a filesystem, shuffle the data > over directly. In this case the toplevel mount point of the bind mount will be tried for a .Trash/ directory, and rename() will be used in that case. The kernel issue you raise is only applicable if one insists on using the homedir trash for other bind mounted file systems (e.g. rename() from /storage/Music to /home if the latter is a bind mount), but this is incorrect behaviour anyway since the mount-specific trash should be used in this case and rename() works here as well (e.g. the file should end up in /storage/.Trash/, not in ~/.local/share/Trash). In conclusion: to me all concerns raised so far seem invalid or addressed.
Colin, Ryan, any comments or questions regarding my previous explanation?
Review of attachment 177625 [details] [review]: The logic here looks indeed good to me -- as far as my opinion is valuable :) ::: gio/glocalfile.c @@ +1816,3 @@ homedir = g_get_home_dir (); g_stat (homedir, &home_stat); + homedir_file = g_file_new_for_path(homedir); missing space between identifier and parenthesis (habits are a though thing ;)
I'm not sure whether attachment 177625 [details] [review] is in line with the trash specification at https://www.freedesktop.org/wiki/Specifications/trash-spec/ saying "Files that the user trashes from the same file system (device/partition) SHOULD be stored [at home trash]": It is conceivable that files on the same partition do not belong to the home tree at ~/. That being said, it might be a good policy idea to trash only files of the home tree in the home trash, but probably this would need a prior change of specification... However, I believe the proposed mechanism would fail for links within the home directory (g_file_has_prefix works purely on names). If in the original situation ~/Music is a symbolic link to /storage/Music, trashing from ~/Music will probably still fail and not get diverted to /storage/.Trash/. A solution which I'd expect to detect this situation as well would be to compare the (apparent) mount points of the file to be deleted and homedir (or homedir/Trash) via _g_local_file_find_topdir_for (which is used later). It may not detect every possible situation involving bind mounts (which are hard to detect, see the implementation of libmount, mountpoint), but it would improve the situation. Perhaps the implementation of _g_local_file_has_trash_dir should also be adjusted accordingly? Finally, is this not an issue for glib/gio rather than gvfs/trash backend?
-- 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/gvfs/issues/159.