On Wed, 12 Feb 2025, Al Viro wrote:
> On Wed, Feb 12, 2025 at 03:16:08AM +0000, Al Viro wrote:
> > On Fri, Feb 07, 2025 at 02:36:48PM +1100, NeilBrown wrote:
> > > @@ -1690,6 +1692,15 @@ struct dentry *lookup_one_qstr_excl(const struct qstr *name,
> > > dput(dentry);
> > > dentry = old;
> > > }
> > > +found:
> >
> > ... and if ->lookup() returns an error, this will blow up (as bot has just
> > reported).
Yes, I need an early exit if (IS_ERR(dentry)). Thanks.
> >
> > > + if (d_is_negative(dentry) && !(flags & LOOKUP_CREATE)) {
> > > + dput(dentry);
> > > + return ERR_PTR(-ENOENT);
> > > + }
> > > + if (d_is_positive(dentry) && (flags & LOOKUP_EXCL)) {
> > > + dput(dentry);
> > > + return ERR_PTR(-EEXIST);
> > > + }
> >
> >
> > > @@ -4077,27 +4084,13 @@ static struct dentry *filename_create(int dfd, struct filename *name,
> > > * '/', and a directory wasn't requested.
> > > */
> > > if (last.name[last.len] && !want_dir)
> > > - create_flags = 0;
> > > + create_flags &= ~LOOKUP_CREATE;
> >
> > See the patch I've posted in earlier thread; the entire "strip LOOKUP_CREATE"
> > thing is wrong.
>
> On top of mainline that's
>
> filename_create(): don't force handling trailing slashes into the common path
>
> Only mkdir accepts pathnames that end with / - anything like mknod() (symlink(),
> etc.) always fails on those. Don't try to force that the common codepath -
> all we are doing is a lookup and check for existence to determine which
> error should it be. Do that before bothering with mnt_want_write(), etc.;
> as far as underlying filesystem is concerned it's just a lookup. Simplifies
> the normal codepath and kills the lookup intent dependency on more than
> the call site.
>
> Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> ---
> diff --git a/fs/namei.c b/fs/namei.c
> index 3ab9440c5b93..6189e54f767a 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -4054,13 +4054,13 @@ static struct dentry *filename_create(int dfd, struct filename *name,
> struct dentry *dentry = ERR_PTR(-EEXIST);
> struct qstr last;
> bool want_dir = lookup_flags & LOOKUP_DIRECTORY;
> - unsigned int reval_flag = lookup_flags & LOOKUP_REVAL;
> - unsigned int create_flags = LOOKUP_CREATE | LOOKUP_EXCL;
> int type;
> int err2;
> int error;
>
> - error = filename_parentat(dfd, name, reval_flag, path, &last, &type);
> + lookup_flags &= LOOKUP_REVAL;
> +
> + error = filename_parentat(dfd, name, lookup_flags, path, &last, &type);
> if (error)
> return ERR_PTR(error);
>
> @@ -4070,18 +4070,28 @@ static struct dentry *filename_create(int dfd, struct filename *name,
> */
> if (unlikely(type != LAST_NORM))
> goto out;
> + /*
> + * mkdir foo/bar/ is OK, but for anything else a slash in the end
> + * is always an error; the only question is which one.
> + */
> + if (unlikely(last.name[last.len] && !want_dir)) {
> + dentry = lookup_dcache(&last, path->dentry, lookup_flags);
> + if (!dentry)
> + dentry = lookup_slow(&last, path->dentry, lookup_flags);
I do see some value in the simplicity of this approach, though maybe not
as much value as you see. But the above uses inode_lock_share(), rather
than the nested version, so lockdep will complain.
If you open-code a nested lock, or write a new helper, you get very
close to the sequence for calling lookup_one_qstr_excl() below. So
it isn't clear to me that the benefit is worth the cost.
This current code in filename_create isn't actually wrong is it?
Thanks,
NeilBrown
> + if (!IS_ERR(dentry)) {
> + error = d_is_positive(dentry) ? -EEXIST : -ENOENT;
> + dput(dentry);
> + dentry = ERR_PTR(error);
> + }
> + goto out;
> + }
>
> /* don't fail immediately if it's r/o, at least try to report other errors */
> err2 = mnt_want_write(path->mnt);
> - /*
> - * Do the final lookup. Suppress 'create' if there is a trailing
> - * '/', and a directory wasn't requested.
> - */
> - if (last.name[last.len] && !want_dir)
> - create_flags = 0;
> + /* do the final lookup */
> inode_lock_nested(path->dentry->d_inode, I_MUTEX_PARENT);
> dentry = lookup_one_qstr_excl(&last, path->dentry,
> - reval_flag | create_flags);
> + lookup_flags | LOOKUP_CREATE | LOOKUP_EXCL);
> if (IS_ERR(dentry))
> goto unlock;
>
> @@ -4089,16 +4099,6 @@ static struct dentry *filename_create(int dfd, struct filename *name,
> if (d_is_positive(dentry))
> goto fail;
>
> - /*
> - * Special case - lookup gave negative, but... we had foo/bar/
> - * From the vfs_mknod() POV we just have a negative dentry -
> - * all is fine. Let's be bastards - you had / on the end, you've
> - * been asking for (non-existent) directory. -ENOENT for you.
> - */
> - if (unlikely(!create_flags)) {
> - error = -ENOENT;
> - goto fail;
> - }
> if (unlikely(err2)) {
> error = err2;
> goto fail;
>
