On 2019-07-12, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
On Sun, Jul 07, 2019 at 12:57:32AM +1000, Aleksa Sarai wrote:@@ -1442,8 +1464,11 @@ static int follow_dotdot_rcu(struct nameidata *nd) struct inode *inode = nd->inode; while (1) { - if (path_equal(&nd->path, &nd->root)) + if (path_equal(&nd->path, &nd->root)) { + if (unlikely(nd->flags & LOOKUP_BENEATH)) + return -EXDEV;@@ -1468,6 +1493,8 @@ static int follow_dotdot_rcu(struct nameidata *nd) return -ECHILD; if (&mparent->mnt == nd->path.mnt) break; + if (unlikely(nd->flags & LOOKUP_XDEV)) + return -EXDEV; /* we know that mountpoint was pinned */ nd->path.dentry = mountpoint; nd->path.mnt = &mparent->mnt; @@ -1482,6 +1509,8 @@ static int follow_dotdot_rcu(struct nameidata *nd) return -ECHILD; if (!mounted) break; + if (unlikely(nd->flags & LOOKUP_XDEV)) + return -EXDEV;Are you sure these failure exits in follow_dotdot_rcu() won't give suprious hard errors?
I could switch to -ECHILD for the *_rcu() checks if you'd prefer that. Though, I'd have (probably naively) thought that you'd have already gotten -ECHILD from the seqlock checks if there was a race during ".." handling.
+ if (unlikely(nd->flags & LOOKUP_BENEATH)) { + error = dirfd_path_init(nd); + if (unlikely(error)) + return ERR_PTR(error); + nd->root = nd->path; + if (!(nd->flags & LOOKUP_RCU)) + path_get(&nd->root); + } if (*s == '/') { if (likely(!nd->root.mnt)) set_root(nd); @@ -2350,9 +2400,11 @@ static const char *path_init(struct nameidata *nd, unsigned flags) s = ERR_PTR(error); return s; } - error = dirfd_path_init(nd); - if (unlikely(error)) - return ERR_PTR(error); + if (likely(!nd->path.mnt)) {Is that a weird way of saying "if we hadn't already called dirfd_path_init()"?
Yes. I did it to be more consistent with the other "have we got the root" checks elsewhere. Is there another way you'd prefer I do it? -- Aleksa Sarai Senior Software Engineer (Containers) SUSE Linux GmbH <https://www.cyphar.com/>
Attachment:
signature.asc
Description: PGP signature
