Re: [AppArmor 31/41] Fix __d_path() for lazy unmounts and make it unambiguous; exclude unreachable mount points from /proc/mounts
> For /proc/mounts, one could argue that the admin might want to see > everything, > but then that's not actually true even today because /proc/mounts doesn't > show lazyily unmounted stuff or mounts from other namespaces, so that > everything is quite relative. The current state is not good either > The getcwd() case is even stronger as the "see everything" argument makes > even > less sense there. I really can't see why the kernel should return processes > fake pathnames. The process is explicitly asking for the current pathname to > the working directory, it doesn't want to know what the pathname was at some > previous point in time. Can you prove no existing application on the planet relies on the existing behaviour ? Actually more limited but sane as a test would be "Can you prove that the glibc behaviour visible to applications does not change" > Actually, no. We could live fine with leaving getcwd() and /proc/mounts as > ambiguous / weird / broken as they are right now. All it would take would be > to reambiguate the result of the unambiguous __d_path(), which is really > easy. Everything that cares about real pathnames would use the unambiguous > version while the legacy interfaces would use the ambiguous version. But that > really wouldn't make sense. I disagree - firstly because of not breaking stuff, and secondly because it separates two discussions - merging AppArmor being one of them , and the correct behaviour for getcwd & /proc/mounts being the other. > > Ok, providing the "real" root sees them all it isn't so bad, but to > > assume you can filter based upon what the task can see is dodgy as an > > assumption. > > Why? Because the viewing apparatus on the other side of the monitor is not operating in current directories/contexts. Alan - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [AppArmor 31/41] Fix __d_path() for lazy unmounts and make it unambiguous; exclude unreachable mount points from /proc/mounts
On Mon, April 16, 2007 23:57, Alan Cox wrote: >> > That is a fairly significant and sudden change to the existing >> > kernel/user interface. >> >> Well, this is not meant for 2.6.21. I hope it is possible to change it >> in >> early 2.6.22; otherwise if we can't fix mistakes from the past we are >> pretty >> doomed. > > I don't believe the existing behaviour _IS_ a mistake. > >> > This is untrue. The process can get there (via fd passing with another >> > task) >> >> Process can access file descriptors which are unreachable via path name >> just >> fine indeed, but those fds still don't have a valid path in the context >> of >> that process. > > Which while problematic to your name based security is just fine to > everything else. The realisation that I feel needs to be made is that fd passing is a legitimate transfer of authority, and attempting to block the usage of this authority (or block the transfer of this authority) would be extremely bad. One thing however that does need some major attention is the amounth of and the scope of the authority that is being transfered with directory file handles. - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [AppArmor 31/41] Fix __d_path() for lazy unmounts and make it unambiguous; exclude unreachable mount points from /proc/mounts
On Monday 16 April 2007 23:57, Alan Cox wrote: > I don't believe the existing behaviour _IS_ a mistake. So what would be the arguments why this behavior makes sense, other than legacy? For /proc/mounts, one could argue that the admin might want to see everything, but then that's not actually true even today because /proc/mounts doesn't show lazyily unmounted stuff or mounts from other namespaces, so that everything is quite relative. Along that line of argumentation, I would at least expect unambiguous output, to be able to tell which mountpoints are actually meaningful to the requesting process. It's not only human operators looking at /proc/mounts; applications care as well. But after thinking about this issue quite a while, I really can't see what that should be good for. The current /proc/mounts interface is obviously broken; the chroot example should have demonstrated that. There are also unnecessary special cases because of that, such as having to filter out the rootfs entry when trying to figure out what's really mounted on /, and having to guess what's there and what's not in a particular context. The more complex mount scenarios will get, the more obviously broken the current /proc/mounts interface will become. The getcwd() case is even stronger as the "see everything" argument makes even less sense there. I really can't see why the kernel should return processes fake pathnames. The process is explicitly asking for the current pathname to the working directory, it doesn't want to know what the pathname was at some previous point in time. > > Process can access file descriptors which are unreachable via path name > > just fine indeed, but those fds still don't have a valid path in the > > context of that process. > > Which while problematic to your name based security is just fine to > everything else. Actually, no. We could live fine with leaving getcwd() and /proc/mounts as ambiguous / weird / broken as they are right now. All it would take would be to reambiguate the result of the unambiguous __d_path(), which is really easy. Everything that cares about real pathnames would use the unambiguous version while the legacy interfaces would use the ambiguous version. But that really wouldn't make sense. > Ok, providing the "real" root sees them all it isn't so bad, but to > assume you can filter based upon what the task can see is dodgy as an > assumption. Why? Thanks, Andreas - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [AppArmor 31/41] Fix __d_path() for lazy unmounts and make it unambiguous; exclude unreachable mount points from /proc/mounts
> > That is a fairly significant and sudden change to the existing > > kernel/user interface. > > Well, this is not meant for 2.6.21. I hope it is possible to change it in > early 2.6.22; otherwise if we can't fix mistakes from the past we are pretty > doomed. I don't believe the existing behaviour _IS_ a mistake. > > This is untrue. The process can get there (via fd passing with another > > task) > > Process can access file descriptors which are unreachable via path name just > fine indeed, but those fds still don't have a valid path in the context of > that process. Which while problematic to your name based security is just fine to everything else. > We are only talking about mount points unreachable by a particular process; > this does not mean that the mount point isn't reachable by other processes. > Human operators can choose the context from which they are looking > at /proc/mounts. If they are looking form the "real" root, the will see all > mounts that any process can reach (in that namespace). Ok, providing the "real" root sees them all it isn't so bad, but to assume you can filter based upon what the task can see is dodgy as an assumption. - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [AppArmor 31/41] Fix __d_path() for lazy unmounts and make it unambiguous; exclude unreachable mount points from /proc/mounts
On Thursday 12 April 2007 11:58, Alan Cox wrote: > > Third, sys_getcwd() shouldn't return disconnected paths. The patch checks for > > that, and makes it fail with -ENOENT in that case > > That is a fairly significant and sudden change to the existing > kernel/user interface. Well, this is not meant for 2.6.21. I hope it is possible to change it in early 2.6.22; otherwise if we can't fix mistakes from the past we are pretty doomed. The problem with unreachable paths is that they are meaningless to the process -- the unreachable path does not really work as a path anymore, it could at best serve some informational value -- at worst it could lead to hard to track down misbehavior. ENOENT is a documented error code for ``directory has been unlinked''. The man page does not mention unreachable paths, but the documentation can easily be changed. What's an unreachable path to one process may be a reachable path for another, or may not be reachable at all (such as the rootfs). > > Fourth, this now allows us to tell unreachable mount points from reachable > > ones when generating the /proc/mounts and /proc/$pid/mountstats files. > > Unreachable mount points are not interesting to processes (they can't get > > there, anyway), so we hide unreachable mounts. In particular, ordinary > > This is untrue. The process can get there (via fd passing with another > task) Process can access file descriptors which are unreachable via path name just fine indeed, but those fds still don't have a valid path in the context of that process. Mount points in /proc/mounts as well as paths returned by getcwd() are relative to the chroot of the querying process, and listing a mount point that is unreachable by that process just doesn't help anybody -- that particular process cannot do anything with them anyway. > and the process can be producing output for the human operators, who most > definitely need to know and see this stuff. We are only talking about mount points unreachable by a particular process; this does not mean that the mount point isn't reachable by other processes. Human operators can choose the context from which they are looking at /proc/mounts. If they are looking form the "real" root, the will see all mounts that any process can reach (in that namespace). The rootfs is an example of a mount point that is not reachable by any process (after the initrd init process): listing it in /proc/mounts is totally pointless, for example -- the rootfs has no meaning to any of those processes. (From the point of view of the initrd init process the rootfs is reachable of course, and so it also shows up in /proc/mounts.) Another context in which the current /proc/mounts doesn't make sense is chroot environments: right now if you have proc mounted on /proc as usual as well as in a chroot, from the point of view of the "real" root, /proc/mounts will contain something like this (omitting all other mounts): proc /proc proc rw 0 0 proc /chroot/proc proc rw 0 0 >From the point of view of the chroot, we get: proc /proc proc rw 0 0 proc /proc proc rw 0 0 So there is no way to tell what's really mounted where (and how) from within the chroot. That's really quite broken. Now it would be possible to use some other syntax to disambiguate unreachable mounts in /proc/mounts, like letting such paths start with "//", or removing leading slashes in those cases. At least removing leading slashes is quite likely to break applications though, and such a hack wouldn't really help anybody. Whatever syntax we could come up with, those paths would be meaningless in the given context. They may be meaningful in other context -- exactly if the paths are reachable. > I don't think this is fit to apply in current form. The hiding of mounts > and mountstats is the wrong approach. The changes to getcwd behaviour > bother me too as we are changing user space behaviour without warning. Which kind of warning do you have in mind? Thanks, Andreas - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [AppArmor 31/41] Fix __d_path() for lazy unmounts and make it unambiguous; exclude unreachable mount points from /proc/mounts
> Third, sys_getcwd() shouldn't return disconnected paths. The patch checks for > that, and makes it fail with -ENOENT in that case That is a fairly significant and sudden change to the existing kernel/user interface. > Fourth, this now allows us to tell unreachable mount points from reachable > ones when generating the /proc/mounts and /proc/$pid/mountstats files. > Unreachable mount points are not interesting to processes (they can't get > there, anyway), so we hide unreachable mounts. In particular, ordinary This is untrue. The process can get there (via fd passing with another task) and the process can be producing output for the human operators, who most definitely need to know and see this stuff. > Signed-off-by: Andreas Gruenbacher <[EMAIL PROTECTED]> > Reviewed-by: NeilBrown <[EMAIL PROTECTED]> > Signed-off-by: Andrew Morton <[EMAIL PROTECTED]> I don't think this is fit to apply in current form. The hiding of mounts and mountstats is the wrong approach. The changes to getcwd behaviour bother me too as we are changing user space behaviour without warning. The general idea of pushing some of the fail detect logic into __d_path() seems good though. Alan - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[AppArmor 31/41] Fix __d_path() for lazy unmounts and make it unambiguous; exclude unreachable mount points from /proc/mounts
First, when d_path() hits a lazily unmounted mount point, it tries to prepend
the name of the lazily unmounted dentry to the path name. It gets this wrong,
and also overwrites the slash that separates the name from the following
pathname component.
Second, it isn't always possible to tell from the __d_path result whether the
specified root and rootmnt (i.e., the chroot) was reached: lazy unmounts of
bind mounts will produce a path that does start with a non-slash so we can
tell from that, but other lazy unmounts will produce a path that starts with a
slash, just like "ordinary" paths.
Third, sys_getcwd() shouldn't return disconnected paths. The patch checks for
that, and makes it fail with -ENOENT in that case.
Fourth, this now allows us to tell unreachable mount points from reachable
ones when generating the /proc/mounts and /proc/$pid/mountstats files.
Unreachable mount points are not interesting to processes (they can't get
there, anyway), so we hide unreachable mounts. In particular, ordinary
processes also will no longer see the rootfs mount (it is unreachable, after
all). The rootfs mount point will still be reachable to processes like the
initial initrd init process, and so those processes will continue to see this
mount point.
The attached patch cleans up __d_path() to fix the bug with overlapping
pathname components. It also adds a @fail_deleted argument, which allows to
get rid of some of the mess in sys_getcwd(). We make sure that paths will
only start with a slash if the path leads all the way up to the root. If the
resulting path would otherwise be empty, we return "." instead so that some
users of seq_path for files in /proc won't break.
The @fail_deleted argument allows sys_getcwd() to be simplified. Grabbing the
dcache_lock can be moved into __d_path().
The @fail_deleted argument could be added to d_path() as well: this would
allow callers to recognize deleted files without having to resort to the
ambiguous check for the " (deleted)" string at the end of the pathnames. This
is not currently done, but it might be worthwhile.
This patch also removes some code duplication between mounts_open() and
mountstats_open().
Signed-off-by: Andreas Gruenbacher <[EMAIL PROTECTED]>
Reviewed-by: NeilBrown <[EMAIL PROTECTED]>
Signed-off-by: Andrew Morton <[EMAIL PROTECTED]>
---
fs/dcache.c| 157 +++--
fs/namespace.c | 23 +++-
fs/proc/base.c | 52 +++---
3 files changed, 131 insertions(+), 101 deletions(-)
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1732,52 +1732,51 @@ shouldnt_be_hashed:
}
/**
- * d_path - return the path of a dentry
+ * __d_path - return the path of a dentry
* @dentry: dentry to report
* @vfsmnt: vfsmnt to which the dentry belongs
* @root: root dentry
* @rootmnt: vfsmnt to which the root dentry belongs
* @buffer: buffer to return value in
* @buflen: buffer length
+ * @fail_deleted: what to return for deleted files
*
- * Convert a dentry into an ASCII path name. If the entry has been deleted
+ * Convert a dentry into an ASCII path name. If the entry has been deleted,
+ * then if @fail_deleted is true, ERR_PTR(-ENOENT) is returned. Otherwise,
* the string " (deleted)" is appended. Note that this is ambiguous.
*
- * Returns the buffer or an error code if the path was too long.
+ * If @dentry is not connected to @root, the path returned will be relative
+ * (i.e., it will not start with a slash).
*
- * "buflen" should be positive. Caller holds the dcache_lock.
+ * Returns the buffer or an error code.
*/
-static char * __d_path( struct dentry *dentry, struct vfsmount *vfsmnt,
- struct dentry *root, struct vfsmount *rootmnt,
- char *buffer, int buflen)
-{
- char * end = buffer+buflen;
- char * retval;
- int namelen;
+static char *__d_path(struct dentry *dentry, struct vfsmount *vfsmnt,
+ struct dentry *root, struct vfsmount *rootmnt,
+ char *buffer, int buflen, int fail_deleted)
+{
+ int namelen, is_slash;
+
+ if (buflen < 2)
+ return ERR_PTR(-ENAMETOOLONG);
+ buffer += --buflen;
+ *buffer = '\0';
- *--end = '\0';
- buflen--;
+ spin_lock(&dcache_lock);
if (!IS_ROOT(dentry) && d_unhashed(dentry)) {
- buflen -= 10;
- end -= 10;
- if (buflen < 0)
+ if (fail_deleted) {
+ buffer = ERR_PTR(-ENOENT);
+ goto out;
+ }
+ if (buflen < 10)
goto Elong;
- memcpy(end, " (deleted)", 10);
+ buflen -= 10;
+ buffer -= 10;
+ memcpy(buffer, " (deleted)", 10);
}
-
- if (buflen < 1)
- goto Elong;
- /* Get '/' right */
- retval = end-1;
- *retval = '/';
-
- for (;;) {
+ whil
