[lkml]   [2011]   [Dec]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [git pull] apparmor fix for __d_path() misuse
    On Wed, Dec 07, 2011 at 02:44:33PM +0900, Tetsuo Handa wrote:

    > Excuse me, what "once the race window is over" means?
    > Does
    > do {
    > pos = d_absolute_path(path, buffer, buflen - 1);
    > } while (pos == ERR_PTR(-EINVAL));
    > work (i.e. racing with "umount -l" is a temporary failure)?

    I said "what your original use of __d_path() would stabilize to". IOW,
    that's what you'd get after all ->mnt_parent in the chain are killed
    by release_mounts(). And no, since the moment that release_mounts()
    started there was *no* *absolute* *path* *at* *all*.

    From that moment on, the point you are looking at is not connected to any
    global root. Or to anything still mounted, of course. What changes is
    how little of what used to be the path to root remains; very shortly
    it's down to just path->mnt not connected to *anything*. __d_path() call
    as you have it in the current tree will report the remaining (shrinking)
    part of path, eventually settling to just the part from path->mnt->root
    to path->dentry. d_absolute_path() will be giving you ERR_PTR(-EINVAL)
    all along; the thing it is supposed to give you does not exist anymore.

    Racing with umount -l is temporary in a sense that as soon as a vfsmount
    detached by umount -l ceases being busy, it gets killed. If you stand
    there, holding a reference and looking for a path connecting it to something
    mounted, well... (a) such path won't appear and (b) vfsmount will remain
    busy for as long as you are holding that reference...

    The real question is what pathname do you _want_ in this situation. Define
    that and we'll be able to do something about it; if you really are asking
    for "whatever this code used to do, modulo races", then what you want is
    if (pos == ERR_PTR(-EINVAL)) {
    /* it got unmounted; just report what's left and be quiet */
    struct path root = {path->mnt, path->mnt->root};
    pos = __d_path(path, &root, buf, buflen - 1);
    if (!pos) /* it's really, *REALLY* screwed up somehow */
    return ERR_PTR(-EINVAL);
    and that's it. But at that point I would start seriously thinking about the
    usefulness of checks done on that tail of pathname, false negatives, etc.

    We could, I guess, make d_absolute_path() do just that on such paths as an
    automatic fallback [1]. apparmor's use of our_mnt(path->mnt) would've caught
    those, so we are not introducing false negatives there.

    However, I *really* wonder if that's the right thing to do in any sense.
    BTW, what your current code does if you have a file bound on another
    file, open it, umount -l it, let the dust settle and then do some operation
    that triggers tomoyo_get_absolute_path() on it? Because you'll be getting
    a vfsmount/dentry pair that has
    * dentry == vfsmount->mnt_root
    * vfsmount->mnt_parent == vfsmount
    * dentry->d_inode being a non-directory
    and there is nothing whatsoever in what remains of the pathname. Not a single
    component. IOW, you'll get "/" in buf. Might be good in a testsuite - is
    there any code in security/tomoyo that would be relying on assumption that
    only directory might have a name entirely without components?

     \ /
      Last update: 2011-12-07 07:57    [W:0.027 / U:11.732 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site