| Message ID | 20160603223700.GY14480@ZenIV.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Jun 3, 2016, at 6:37 PM, Al Viro wrote: > On Fri, Jun 03, 2016 at 11:23:55PM +0100, Al Viro wrote: > >> It's not that. It's explicit put_link() in do_last(), followed by >> ESTALEOPEN and subsequent misbegotten "retry the last step on ESTALEOPEN" >> looking at now-freed nd->last.name. IOW, the bug predates delayed_call >> stuff. > > EOPENSTALE, that is... Oleg, could you check if the following works? Ok, I'll try this one too. The other one with goto stale_open also was appearing to work. We'll see what else is going to show up next... > diff --git a/fs/namei.c b/fs/namei.c > index 4c4f95a..3d9511e 100644 > --- a/fs/namei.c > +++ b/fs/namei.c > @@ -3166,9 +3166,7 @@ static int do_last(struct nameidata *nd, > int acc_mode = op->acc_mode; > unsigned seq; > struct inode *inode; > - struct path save_parent = { .dentry = NULL, .mnt = NULL }; > struct path path; > - bool retried = false; > int error; > > nd->flags &= ~LOOKUP_PARENT; > @@ -3211,7 +3209,6 @@ static int do_last(struct nameidata *nd, > return -EISDIR; > } > > -retry_lookup: > if (open_flag & (O_CREAT | O_TRUNC | O_WRONLY | O_RDWR)) { > error = mnt_want_write(nd->path.mnt); > if (!error) > @@ -3292,23 +3289,14 @@ finish_lookup: > if (unlikely(error)) > return error; > > - if ((nd->flags & LOOKUP_RCU) || nd->path.mnt != path.mnt) { > - path_to_nameidata(&path, nd); > - } else { > - save_parent.dentry = nd->path.dentry; > - save_parent.mnt = mntget(path.mnt); > - nd->path.dentry = path.dentry; > - > - } > + path_to_nameidata(&path, nd); > nd->inode = inode; > nd->seq = seq; > /* Why this, you ask? _Now_ we might have grown LOOKUP_JUMPED... */ > finish_open: > error = complete_walk(nd); > - if (error) { > - path_put(&save_parent); > + if (error) > return error; > - } > audit_inode(nd->name, nd->path.dentry, 0); > error = -EISDIR; > if ((open_flag & O_CREAT) && d_is_dir(nd->path.dentry)) > @@ -3331,13 +3319,9 @@ finish_open_created: > goto out; > BUG_ON(*opened & FILE_OPENED); /* once it's opened, it's opened */ > error = vfs_open(&nd->path, file, current_cred()); > - if (!error) { > - *opened |= FILE_OPENED; > - } else { > - if (error == -EOPENSTALE) > - goto stale_open; > + if (error) > goto out; > - } > + *opened |= FILE_OPENED; > opened: > error = open_check_o_direct(file); > if (!error) > @@ -3353,26 +3337,7 @@ out: > } > if (got_write) > mnt_drop_write(nd->path.mnt); > - path_put(&save_parent); > return error; > - > -stale_open: > - /* If no saved parent or already retried then can't retry */ > - if (!save_parent.dentry || retried) > - goto out; > - > - BUG_ON(save_parent.dentry != dir); > - path_put(&nd->path); > - nd->path = save_parent; > - nd->inode = dir->d_inode; > - save_parent.mnt = NULL; > - save_parent.dentry = NULL; > - if (got_write) { > - mnt_drop_write(nd->path.mnt); > - got_write = false; > - } > - retried = true; > - goto retry_lookup; > } > > static int do_tmpfile(struct nameidata *nd, unsigned flags, -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Jun 3, 2016, at 6:37 PM, Al Viro wrote: > On Fri, Jun 03, 2016 at 11:23:55PM +0100, Al Viro wrote: > >> It's not that. It's explicit put_link() in do_last(), followed by >> ESTALEOPEN and subsequent misbegotten "retry the last step on ESTALEOPEN" >> looking at now-freed nd->last.name. IOW, the bug predates delayed_call >> stuff. > > EOPENSTALE, that is... Oleg, could you check if the following works? Yes, this one lasted for an hour with no crashing, so it must be good. Thanks. (note, I am not equipped to verify correctness of NFS operations, though). > diff --git a/fs/namei.c b/fs/namei.c > index 4c4f95a..3d9511e 100644 > --- a/fs/namei.c > +++ b/fs/namei.c > @@ -3166,9 +3166,7 @@ static int do_last(struct nameidata *nd, > int acc_mode = op->acc_mode; > unsigned seq; > struct inode *inode; > - struct path save_parent = { .dentry = NULL, .mnt = NULL }; > struct path path; > - bool retried = false; > int error; > > nd->flags &= ~LOOKUP_PARENT; > @@ -3211,7 +3209,6 @@ static int do_last(struct nameidata *nd, > return -EISDIR; > } > > -retry_lookup: > if (open_flag & (O_CREAT | O_TRUNC | O_WRONLY | O_RDWR)) { > error = mnt_want_write(nd->path.mnt); > if (!error) > @@ -3292,23 +3289,14 @@ finish_lookup: > if (unlikely(error)) > return error; > > - if ((nd->flags & LOOKUP_RCU) || nd->path.mnt != path.mnt) { > - path_to_nameidata(&path, nd); > - } else { > - save_parent.dentry = nd->path.dentry; > - save_parent.mnt = mntget(path.mnt); > - nd->path.dentry = path.dentry; > - > - } > + path_to_nameidata(&path, nd); > nd->inode = inode; > nd->seq = seq; > /* Why this, you ask? _Now_ we might have grown LOOKUP_JUMPED... */ > finish_open: > error = complete_walk(nd); > - if (error) { > - path_put(&save_parent); > + if (error) > return error; > - } > audit_inode(nd->name, nd->path.dentry, 0); > error = -EISDIR; > if ((open_flag & O_CREAT) && d_is_dir(nd->path.dentry)) > @@ -3331,13 +3319,9 @@ finish_open_created: > goto out; > BUG_ON(*opened & FILE_OPENED); /* once it's opened, it's opened */ > error = vfs_open(&nd->path, file, current_cred()); > - if (!error) { > - *opened |= FILE_OPENED; > - } else { > - if (error == -EOPENSTALE) > - goto stale_open; > + if (error) > goto out; > - } > + *opened |= FILE_OPENED; > opened: > error = open_check_o_direct(file); > if (!error) > @@ -3353,26 +3337,7 @@ out: > } > if (got_write) > mnt_drop_write(nd->path.mnt); > - path_put(&save_parent); > return error; > - > -stale_open: > - /* If no saved parent or already retried then can't retry */ > - if (!save_parent.dentry || retried) > - goto out; > - > - BUG_ON(save_parent.dentry != dir); > - path_put(&nd->path); > - nd->path = save_parent; > - nd->inode = dir->d_inode; > - save_parent.mnt = NULL; > - save_parent.dentry = NULL; > - if (got_write) { > - mnt_drop_write(nd->path.mnt); > - got_write = false; > - } > - retried = true; > - goto retry_lookup; > } > > static int do_tmpfile(struct nameidata *nd, unsigned flags, -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/namei.c b/fs/namei.c index 4c4f95a..3d9511e 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -3166,9 +3166,7 @@ static int do_last(struct nameidata *nd, int acc_mode = op->acc_mode; unsigned seq; struct inode *inode; - struct path save_parent = { .dentry = NULL, .mnt = NULL }; struct path path; - bool retried = false; int error; nd->flags &= ~LOOKUP_PARENT; @@ -3211,7 +3209,6 @@ static int do_last(struct nameidata *nd, return -EISDIR; } -retry_lookup: if (open_flag & (O_CREAT | O_TRUNC | O_WRONLY | O_RDWR)) { error = mnt_want_write(nd->path.mnt); if (!error) @@ -3292,23 +3289,14 @@ finish_lookup: if (unlikely(error)) return error; - if ((nd->flags & LOOKUP_RCU) || nd->path.mnt != path.mnt) { - path_to_nameidata(&path, nd); - } else { - save_parent.dentry = nd->path.dentry; - save_parent.mnt = mntget(path.mnt); - nd->path.dentry = path.dentry; - - } + path_to_nameidata(&path, nd); nd->inode = inode; nd->seq = seq; /* Why this, you ask? _Now_ we might have grown LOOKUP_JUMPED... */ finish_open: error = complete_walk(nd); - if (error) { - path_put(&save_parent); + if (error) return error; - } audit_inode(nd->name, nd->path.dentry, 0); error = -EISDIR; if ((open_flag & O_CREAT) && d_is_dir(nd->path.dentry)) @@ -3331,13 +3319,9 @@ finish_open_created: goto out; BUG_ON(*opened & FILE_OPENED); /* once it's opened, it's opened */ error = vfs_open(&nd->path, file, current_cred()); - if (!error) { - *opened |= FILE_OPENED; - } else { - if (error == -EOPENSTALE) - goto stale_open; + if (error) goto out; - } + *opened |= FILE_OPENED; opened: error = open_check_o_direct(file); if (!error) @@ -3353,26 +3337,7 @@ out: } if (got_write) mnt_drop_write(nd->path.mnt); - path_put(&save_parent); return error; - -stale_open: - /* If no saved parent or already retried then can't retry */ - if (!save_parent.dentry || retried) - goto out; - - BUG_ON(save_parent.dentry != dir); - path_put(&nd->path); - nd->path = save_parent; - nd->inode = dir->d_inode; - save_parent.mnt = NULL; - save_parent.dentry = NULL; - if (got_write) { - mnt_drop_write(nd->path.mnt); - got_write = false; - } - retried = true; - goto retry_lookup; } static int do_tmpfile(struct nameidata *nd, unsigned flags,