| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-id: <173933190416.22054.5881139463496565922@noble.neil.brown.name>
Date: Wed, 12 Feb 2025 14:45:04 +1100
From: "NeilBrown" <neilb@...e.de>
To: "Al Viro" <viro@...iv.linux.org.uk>
Cc: "Christian Brauner" <brauner@...nel.org>, "Jan Kara" <jack@...e.cz>,
"Greg Kroah-Hartman" <gregkh@...uxfoundation.org>,
"Rafael J. Wysocki" <rafael@...nel.org>, "Danilo Krummrich" <dakr@...nel.org>,
"Kent Overstreet" <kent.overstreet@...ux.dev>,
"Trond Myklebust" <trondmy@...nel.org>, "Anna Schumaker" <anna@...nel.org>,
"Namjae Jeon" <linkinjeon@...nel.org>, "Steve French" <sfrench@...ba.org>,
"Sergey Senozhatsky" <senozhatsky@...omium.org>,
"Tom Talpey" <tom@...pey.com>, "Paul Moore" <paul@...l-moore.com>,
"Eric Paris" <eparis@...hat.com>, linux-kernel@...r.kernel.org,
linux-bcachefs@...r.kernel.org, linux-fsdevel@...r.kernel.org,
linux-nfs@...r.kernel.org, linux-cifs@...r.kernel.org, audit@...r.kernel.org
Subject:
Re: [PATCH 2/2] VFS: add common error checks to lookup_one_qstr_excl()
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@...iv.linux.org.uk>
> ---
> 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;
>
Powered by blists - more mailing lists