| Message ID | 20230920173445.3943581-5-bschubert@ddn.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fuse: full atomic open and atomic-open-revalidate | expand |
On Thu, Sep 21, 2023 at 12:48 AM Bernd Schubert <bschubert@ddn.com> wrote: > > This was suggested by Miklos based on review from the previous > patch that introduced atomic open for positive dentries. > In open_last_lookups() the dentry was not used anymore when atomic > revalidate was available, which required to release the dentry, > then code fall through to lookup_open was done, which resulted > in another d_lookup and also d_revalidate. All of that can > be avoided by the new atomic_revalidate_open() function. > > Another included change is the introduction of an enum as > d_revalidate return code. > > Signed-off-by: Bernd Schubert <bschubert@ddn.com> > Cc: Miklos Szeredi <miklos@szeredi.hu> > Cc: Dharmendra Singh <dsingh@ddn.com> > Cc: Christian Brauner <brauner@kernel.org> > Cc: Al Viro <viro@zeniv.linux.org.uk> > Cc: linux-fsdevel@vger.kernel.org > --- > fs/namei.c | 45 +++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 43 insertions(+), 2 deletions(-) > > diff --git a/fs/namei.c b/fs/namei.c > index f01b278ac0ef..8ad7a0dfa596 100644 > --- a/fs/namei.c > +++ b/fs/namei.c > @@ -3388,6 +3388,44 @@ static struct dentry *atomic_open(struct nameidata *nd, struct dentry *dentry, > return dentry; > } > > +static struct dentry *atomic_revalidate_open(struct dentry *dentry, > + struct nameidata *nd, > + struct file *file, > + const struct open_flags *op, > + bool *got_write) > +{ > + struct mnt_idmap *idmap; > + struct dentry *dir = nd->path.dentry; > + struct inode *dir_inode = dir->d_inode; > + int open_flag = op->open_flag; > + umode_t mode = op->mode; > + > + if (unlikely(IS_DEADDIR(dir_inode))) > + return ERR_PTR(-ENOENT); > + > + file->f_mode &= ~FMODE_CREATED; > + > + if (unlikely(open_flag & O_CREAT)) { > + WARN_ON(1); if (WARN_ON_ONCE(open_flag & O_CREAT)) { is more compact and produces a nicer print > + return ERR_PTR(-EINVAL); > + } > + > + if (open_flag & (O_TRUNC | O_WRONLY | O_RDWR)) > + *got_write = !mnt_want_write(nd->path.mnt); > + else > + *got_write = false; > + > + if (!got_write) > + open_flag &= ~O_TRUNC; > + > + inode_lock_shared(dir->d_inode); > + dentry = atomic_open(nd, dentry, file, open_flag, mode); > + inode_unlock_shared(dir->d_inode); > + > + return dentry; > + > +} > + > /* > * Look up and maybe create and open the last component. > * > @@ -3541,8 +3579,10 @@ static const char *open_last_lookups(struct nameidata *nd, > if (IS_ERR(dentry)) > return ERR_CAST(dentry); > if (dentry && unlikely(atomic_revalidate)) { > - dput(dentry); > - dentry = NULL; > + BUG_ON(nd->flags & LOOKUP_RCU); The assertion means that someone wrote bad code it does not mean that the kernel internal state is hopelessly corrupted. Please avoid BUG_ON and use WARN_ON_ONCE and if possible (seems to be the case here) also take the graceful error path. It's better to fail an open than to crash the kernel. Thanks, Amir. > + dentry = atomic_revalidate_open(dentry, nd, file, op, > + &got_write); > + goto drop_write; > } > if (likely(dentry)) > goto finish_lookup; > @@ -3580,6 +3620,7 @@ static const char *open_last_lookups(struct nameidata *nd, > else > inode_unlock_shared(dir->d_inode); > > +drop_write: > if (got_write) > mnt_drop_write(nd->path.mnt); > > -- > 2.39.2 >
On 9/21/23 08:16, Amir Goldstein wrote: > On Thu, Sep 21, 2023 at 12:48 AM Bernd Schubert <bschubert@ddn.com> wrote: >> >> This was suggested by Miklos based on review from the previous >> patch that introduced atomic open for positive dentries. >> In open_last_lookups() the dentry was not used anymore when atomic >> revalidate was available, which required to release the dentry, >> then code fall through to lookup_open was done, which resulted >> in another d_lookup and also d_revalidate. All of that can >> be avoided by the new atomic_revalidate_open() function. >> >> Another included change is the introduction of an enum as >> d_revalidate return code. >> >> Signed-off-by: Bernd Schubert <bschubert@ddn.com> >> Cc: Miklos Szeredi <miklos@szeredi.hu> >> Cc: Dharmendra Singh <dsingh@ddn.com> >> Cc: Christian Brauner <brauner@kernel.org> >> Cc: Al Viro <viro@zeniv.linux.org.uk> >> Cc: linux-fsdevel@vger.kernel.org >> --- >> fs/namei.c | 45 +++++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 43 insertions(+), 2 deletions(-) >> >> diff --git a/fs/namei.c b/fs/namei.c >> index f01b278ac0ef..8ad7a0dfa596 100644 >> --- a/fs/namei.c >> +++ b/fs/namei.c >> @@ -3388,6 +3388,44 @@ static struct dentry *atomic_open(struct nameidata *nd, struct dentry *dentry, >> return dentry; >> } >> >> +static struct dentry *atomic_revalidate_open(struct dentry *dentry, >> + struct nameidata *nd, >> + struct file *file, >> + const struct open_flags *op, >> + bool *got_write) >> +{ >> + struct mnt_idmap *idmap; >> + struct dentry *dir = nd->path.dentry; >> + struct inode *dir_inode = dir->d_inode; >> + int open_flag = op->open_flag; >> + umode_t mode = op->mode; >> + >> + if (unlikely(IS_DEADDIR(dir_inode))) >> + return ERR_PTR(-ENOENT); >> + >> + file->f_mode &= ~FMODE_CREATED; >> + >> + if (unlikely(open_flag & O_CREAT)) { >> + WARN_ON(1); > > if (WARN_ON_ONCE(open_flag & O_CREAT)) { > > is more compact and produces a nicer print Thanks a lot for your review Amir! Nice, I hadn't noticed that these macros actually return a value. Also updated the related fuse patch, which had a similar construct. > >> + return ERR_PTR(-EINVAL); >> + } >> + >> + if (open_flag & (O_TRUNC | O_WRONLY | O_RDWR)) >> + *got_write = !mnt_want_write(nd->path.mnt); >> + else >> + *got_write = false; >> + >> + if (!got_write) >> + open_flag &= ~O_TRUNC; >> + >> + inode_lock_shared(dir->d_inode); >> + dentry = atomic_open(nd, dentry, file, open_flag, mode); >> + inode_unlock_shared(dir->d_inode); >> + >> + return dentry; >> + >> +} >> + >> /* >> * Look up and maybe create and open the last component. >> * >> @@ -3541,8 +3579,10 @@ static const char *open_last_lookups(struct nameidata *nd, >> if (IS_ERR(dentry)) >> return ERR_CAST(dentry); >> if (dentry && unlikely(atomic_revalidate)) { >> - dput(dentry); >> - dentry = NULL; >> + BUG_ON(nd->flags & LOOKUP_RCU); > > The assertion means that someone wrote bad code > it does not mean that the kernel internal state is hopelessly corrupted. > Please avoid BUG_ON and use WARN_ON_ONCE and if possible > (seems to be the case here) also take the graceful error path. > It's better to fail an open than to crash the kernel. Thanks, updated. I had used BUG_ON because there is a similar BUG_ON a few lines below. Added another RFC patch to covert that one as well. I'm going to wait a few days for possible other reviews and will then send out the new version. The updated v10 branch is here https://github.com/bsbernd/linux/tree/atomic-open-for-6.5-v10 Thanks, Bernd
On Thu, Sep 21, 2023 at 11:09 AM Bernd Schubert <bernd.schubert@fastmail.fm> wrote: > > > > On 9/21/23 08:16, Amir Goldstein wrote: > > On Thu, Sep 21, 2023 at 12:48 AM Bernd Schubert <bschubert@ddn.com> wrote: > >> > >> This was suggested by Miklos based on review from the previous > >> patch that introduced atomic open for positive dentries. > >> In open_last_lookups() the dentry was not used anymore when atomic > >> revalidate was available, which required to release the dentry, > >> then code fall through to lookup_open was done, which resulted > >> in another d_lookup and also d_revalidate. All of that can > >> be avoided by the new atomic_revalidate_open() function. > >> > >> Another included change is the introduction of an enum as > >> d_revalidate return code. > >> > >> Signed-off-by: Bernd Schubert <bschubert@ddn.com> > >> Cc: Miklos Szeredi <miklos@szeredi.hu> > >> Cc: Dharmendra Singh <dsingh@ddn.com> > >> Cc: Christian Brauner <brauner@kernel.org> > >> Cc: Al Viro <viro@zeniv.linux.org.uk> > >> Cc: linux-fsdevel@vger.kernel.org > >> --- > >> fs/namei.c | 45 +++++++++++++++++++++++++++++++++++++++++++-- > >> 1 file changed, 43 insertions(+), 2 deletions(-) > >> > >> diff --git a/fs/namei.c b/fs/namei.c > >> index f01b278ac0ef..8ad7a0dfa596 100644 > >> --- a/fs/namei.c > >> +++ b/fs/namei.c > >> @@ -3388,6 +3388,44 @@ static struct dentry *atomic_open(struct nameidata *nd, struct dentry *dentry, > >> return dentry; > >> } > >> > >> +static struct dentry *atomic_revalidate_open(struct dentry *dentry, > >> + struct nameidata *nd, > >> + struct file *file, > >> + const struct open_flags *op, > >> + bool *got_write) > >> +{ > >> + struct mnt_idmap *idmap; > >> + struct dentry *dir = nd->path.dentry; > >> + struct inode *dir_inode = dir->d_inode; > >> + int open_flag = op->open_flag; > >> + umode_t mode = op->mode; > >> + > >> + if (unlikely(IS_DEADDIR(dir_inode))) > >> + return ERR_PTR(-ENOENT); > >> + > >> + file->f_mode &= ~FMODE_CREATED; > >> + > >> + if (unlikely(open_flag & O_CREAT)) { > >> + WARN_ON(1); > > > > if (WARN_ON_ONCE(open_flag & O_CREAT)) { > > > > is more compact and produces a nicer print > > Thanks a lot for your review Amir! Nice, I hadn't noticed that > these macros actually return a value. Also updated the related > fuse patch, which had a similar construct. > > > > >> + return ERR_PTR(-EINVAL); > >> + } > >> + > >> + if (open_flag & (O_TRUNC | O_WRONLY | O_RDWR)) > >> + *got_write = !mnt_want_write(nd->path.mnt); > >> + else > >> + *got_write = false; > >> + > >> + if (!got_write) > >> + open_flag &= ~O_TRUNC; > >> + > >> + inode_lock_shared(dir->d_inode); > >> + dentry = atomic_open(nd, dentry, file, open_flag, mode); > >> + inode_unlock_shared(dir->d_inode); > >> + > >> + return dentry; > >> + > >> +} > >> + > >> /* > >> * Look up and maybe create and open the last component. > >> * > >> @@ -3541,8 +3579,10 @@ static const char *open_last_lookups(struct nameidata *nd, > >> if (IS_ERR(dentry)) > >> return ERR_CAST(dentry); > >> if (dentry && unlikely(atomic_revalidate)) { > >> - dput(dentry); > >> - dentry = NULL; > >> + BUG_ON(nd->flags & LOOKUP_RCU); > > > > The assertion means that someone wrote bad code > > it does not mean that the kernel internal state is hopelessly corrupted. > > Please avoid BUG_ON and use WARN_ON_ONCE and if possible > > (seems to be the case here) also take the graceful error path. > > It's better to fail an open than to crash the kernel. > > Thanks, updated. I had used BUG_ON because there is a similar BUG_ON a > few lines below. checkpatch strictly forbids new BUG_ON: "Do not crash the kernel unless it is absolutely unavoidable-- use WARN_ON_ONCE() plus recovery code (if feasible) instead of BUG() or variants" But it's true that vfs code has several of those. > Added another RFC patch to covert that one as well. > I'm going to wait a few days for possible other reviews and will then send > out the new version. The updated v10 branch is here > > https://github.com/bsbernd/linux/tree/atomic-open-for-6.5-v10 > IIUC, patches 3,4 are independent vfs optimization. Is that correct? Since you are going to need review of vfs maintainers and since Christian would probably want to merge them via the vfs tree, I think it would be better for you to post them separately from your series if possible, so Miklos would be able to pick up the fuse patches when they are reviewed. Thanks, Amir.
On 9/21/23 10:29, Amir Goldstein wrote: > On Thu, Sep 21, 2023 at 11:09 AM Bernd Schubert > <bernd.schubert@fastmail.fm> wrote: >> >> >> >> On 9/21/23 08:16, Amir Goldstein wrote: >>> On Thu, Sep 21, 2023 at 12:48 AM Bernd Schubert <bschubert@ddn.com> wrote: >>>> >>>> This was suggested by Miklos based on review from the previous >>>> patch that introduced atomic open for positive dentries. >>>> In open_last_lookups() the dentry was not used anymore when atomic >>>> revalidate was available, which required to release the dentry, >>>> then code fall through to lookup_open was done, which resulted >>>> in another d_lookup and also d_revalidate. All of that can >>>> be avoided by the new atomic_revalidate_open() function. >>>> >>>> Another included change is the introduction of an enum as >>>> d_revalidate return code. >>>> >>>> Signed-off-by: Bernd Schubert <bschubert@ddn.com> >>>> Cc: Miklos Szeredi <miklos@szeredi.hu> >>>> Cc: Dharmendra Singh <dsingh@ddn.com> >>>> Cc: Christian Brauner <brauner@kernel.org> >>>> Cc: Al Viro <viro@zeniv.linux.org.uk> >>>> Cc: linux-fsdevel@vger.kernel.org >>>> --- >>>> fs/namei.c | 45 +++++++++++++++++++++++++++++++++++++++++++-- >>>> 1 file changed, 43 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/fs/namei.c b/fs/namei.c >>>> index f01b278ac0ef..8ad7a0dfa596 100644 >>>> --- a/fs/namei.c >>>> +++ b/fs/namei.c >>>> @@ -3388,6 +3388,44 @@ static struct dentry *atomic_open(struct nameidata *nd, struct dentry *dentry, >>>> return dentry; >>>> } >>>> >>>> +static struct dentry *atomic_revalidate_open(struct dentry *dentry, >>>> + struct nameidata *nd, >>>> + struct file *file, >>>> + const struct open_flags *op, >>>> + bool *got_write) >>>> +{ >>>> + struct mnt_idmap *idmap; >>>> + struct dentry *dir = nd->path.dentry; >>>> + struct inode *dir_inode = dir->d_inode; >>>> + int open_flag = op->open_flag; >>>> + umode_t mode = op->mode; >>>> + >>>> + if (unlikely(IS_DEADDIR(dir_inode))) >>>> + return ERR_PTR(-ENOENT); >>>> + >>>> + file->f_mode &= ~FMODE_CREATED; >>>> + >>>> + if (unlikely(open_flag & O_CREAT)) { >>>> + WARN_ON(1); >>> >>> if (WARN_ON_ONCE(open_flag & O_CREAT)) { >>> >>> is more compact and produces a nicer print >> >> Thanks a lot for your review Amir! Nice, I hadn't noticed that >> these macros actually return a value. Also updated the related >> fuse patch, which had a similar construct. >> >>> >>>> + return ERR_PTR(-EINVAL); >>>> + } >>>> + >>>> + if (open_flag & (O_TRUNC | O_WRONLY | O_RDWR)) >>>> + *got_write = !mnt_want_write(nd->path.mnt); >>>> + else >>>> + *got_write = false; >>>> + >>>> + if (!got_write) >>>> + open_flag &= ~O_TRUNC; >>>> + >>>> + inode_lock_shared(dir->d_inode); >>>> + dentry = atomic_open(nd, dentry, file, open_flag, mode); >>>> + inode_unlock_shared(dir->d_inode); >>>> + >>>> + return dentry; >>>> + >>>> +} >>>> + >>>> /* >>>> * Look up and maybe create and open the last component. >>>> * >>>> @@ -3541,8 +3579,10 @@ static const char *open_last_lookups(struct nameidata *nd, >>>> if (IS_ERR(dentry)) >>>> return ERR_CAST(dentry); >>>> if (dentry && unlikely(atomic_revalidate)) { >>>> - dput(dentry); >>>> - dentry = NULL; >>>> + BUG_ON(nd->flags & LOOKUP_RCU); >>> >>> The assertion means that someone wrote bad code >>> it does not mean that the kernel internal state is hopelessly corrupted. >>> Please avoid BUG_ON and use WARN_ON_ONCE and if possible >>> (seems to be the case here) also take the graceful error path. >>> It's better to fail an open than to crash the kernel. >> >> Thanks, updated. I had used BUG_ON because there is a similar BUG_ON a >> few lines below. > > checkpatch strictly forbids new BUG_ON: > "Do not crash the kernel unless it is absolutely unavoidable-- use > WARN_ON_ONCE() plus recovery code (if feasible) instead of BUG() or variants" > > But it's true that vfs code has several of those. Yeah sorry, I had seen the warning, but ignored it, in favor of code consistency. > >> Added another RFC patch to covert that one as well. >> I'm going to wait a few days for possible other reviews and will then send >> out the new version. The updated v10 branch is here >> >> https://github.com/bsbernd/linux/tree/atomic-open-for-6.5-v10 >> > > IIUC, patches 3,4 are independent vfs optimization. > Is that correct? Hmm, not exactly. Patches 3 is a dependency for patch 5-7. So far atomic open didn't handle positive dentries / revalidate. Patch 3 adds support for that, the file system then needs to signal in its ->d_revalidate return code that it wants atomic_open to do the revalidation (which adds a bit complexity to the atomic_open method). Patch 3 has then two cases, O_CREATE and plain open without O_CREATE. Patch 4 is an optimization for patch 3, for plain open. I actually start to wonder if I shouldn't remove plain open from patch 3 and to add that in patch 4. Probably easier to read. Thanks for making me think about that :) > > Since you are going to need review of vfs maintainers > and since Christian would probably want to merge them > via the vfs tree, I think it would be better for you to post > them separately from your series if possible, so Miklos > would be able to pick up the fuse patches when they are > reviewed. Only the new patch 8 is independent, I'm going to send that out separately today. Thanks, Bernd
Hi Bernd,
kernel test robot noticed the following build warnings:
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Bernd-Schubert/fuse-rename-fuse_create_open/20230921-013805
base: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping.git for-next
patch link: https://lore.kernel.org/r/20230920173445.3943581-5-bschubert%40ddn.com
patch subject: [PATCH v9 4/7] vfs: Optimize atomic_open() on positive dentry
config: i386-randconfig-141-20230927 (https://download.01.org/0day-ci/archive/20231011/202310111259.WGjXat6p-lkp@intel.com/config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce: (https://download.01.org/0day-ci/archive/20231011/202310111259.WGjXat6p-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202310111259.WGjXat6p-lkp@intel.com/
New smatch warnings:
fs/namei.c:3418 atomic_revalidate_open() warn: variable dereferenced before check 'got_write' (see line 3414)
Old smatch warnings:
fs/namei.c:1573 lookup_dcache() warn: passing zero to 'ERR_PTR'
fs/namei.c:1658 lookup_fast() warn: passing zero to 'ERR_PTR'
fs/namei.c:2189 hash_name() error: uninitialized symbol 'bdata'.
fs/namei.c:2600 __kern_path_locked() warn: inconsistent returns '&path->dentry->d_inode->i_rwsem'.
fs/namei.c:3480 lookup_open() error: uninitialized symbol 'error'.
vim +/got_write +3418 fs/namei.c
0bb53ce89df211 Bernd Schubert 2023-09-20 3391 static struct dentry *atomic_revalidate_open(struct dentry *dentry,
0bb53ce89df211 Bernd Schubert 2023-09-20 3392 struct nameidata *nd,
0bb53ce89df211 Bernd Schubert 2023-09-20 3393 struct file *file,
0bb53ce89df211 Bernd Schubert 2023-09-20 3394 const struct open_flags *op,
0bb53ce89df211 Bernd Schubert 2023-09-20 3395 bool *got_write)
0bb53ce89df211 Bernd Schubert 2023-09-20 3396 {
0bb53ce89df211 Bernd Schubert 2023-09-20 3397 struct mnt_idmap *idmap;
0bb53ce89df211 Bernd Schubert 2023-09-20 3398 struct dentry *dir = nd->path.dentry;
0bb53ce89df211 Bernd Schubert 2023-09-20 3399 struct inode *dir_inode = dir->d_inode;
0bb53ce89df211 Bernd Schubert 2023-09-20 3400 int open_flag = op->open_flag;
0bb53ce89df211 Bernd Schubert 2023-09-20 3401 umode_t mode = op->mode;
0bb53ce89df211 Bernd Schubert 2023-09-20 3402
0bb53ce89df211 Bernd Schubert 2023-09-20 3403 if (unlikely(IS_DEADDIR(dir_inode)))
0bb53ce89df211 Bernd Schubert 2023-09-20 3404 return ERR_PTR(-ENOENT);
0bb53ce89df211 Bernd Schubert 2023-09-20 3405
0bb53ce89df211 Bernd Schubert 2023-09-20 3406 file->f_mode &= ~FMODE_CREATED;
0bb53ce89df211 Bernd Schubert 2023-09-20 3407
0bb53ce89df211 Bernd Schubert 2023-09-20 3408 if (unlikely(open_flag & O_CREAT)) {
0bb53ce89df211 Bernd Schubert 2023-09-20 3409 WARN_ON(1);
0bb53ce89df211 Bernd Schubert 2023-09-20 3410 return ERR_PTR(-EINVAL);
0bb53ce89df211 Bernd Schubert 2023-09-20 3411 }
0bb53ce89df211 Bernd Schubert 2023-09-20 3412
0bb53ce89df211 Bernd Schubert 2023-09-20 3413 if (open_flag & (O_TRUNC | O_WRONLY | O_RDWR))
0bb53ce89df211 Bernd Schubert 2023-09-20 @3414 *got_write = !mnt_want_write(nd->path.mnt);
Dereferenced
0bb53ce89df211 Bernd Schubert 2023-09-20 3415 else
0bb53ce89df211 Bernd Schubert 2023-09-20 3416 *got_write = false;
Here too.
0bb53ce89df211 Bernd Schubert 2023-09-20 3417
0bb53ce89df211 Bernd Schubert 2023-09-20 @3418 if (!got_write)
Checked too late. But I think maybe it was supposed to check
if (!*got_write)?
0bb53ce89df211 Bernd Schubert 2023-09-20 3419 open_flag &= ~O_TRUNC;
0bb53ce89df211 Bernd Schubert 2023-09-20 3420
0bb53ce89df211 Bernd Schubert 2023-09-20 3421 inode_lock_shared(dir->d_inode);
0bb53ce89df211 Bernd Schubert 2023-09-20 3422 dentry = atomic_open(nd, dentry, file, open_flag, mode);
On 10/11/23 09:17, Dan Carpenter wrote: > Hi Bernd, > > kernel test robot noticed the following build warnings: > > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > url: https://github.com/intel-lab-lkp/linux/commits/Bernd-Schubert/fuse-rename-fuse_create_open/20230921-013805 > base: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping.git for-next > patch link: https://lore.kernel.org/r/20230920173445.3943581-5-bschubert%40ddn.com > patch subject: [PATCH v9 4/7] vfs: Optimize atomic_open() on positive dentry > config: i386-randconfig-141-20230927 (https://download.01.org/0day-ci/archive/20231011/202310111259.WGjXat6p-lkp@intel.com/config) > compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0 > reproduce: (https://download.01.org/0day-ci/archive/20231011/202310111259.WGjXat6p-lkp@intel.com/reproduce) > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > the same patch/commit), kindly add following tags > | Reported-by: kernel test robot <lkp@intel.com> > | Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > | Closes: https://lore.kernel.org/r/202310111259.WGjXat6p-lkp@intel.com/ > > New smatch warnings: > fs/namei.c:3418 atomic_revalidate_open() warn: variable dereferenced before check 'got_write' (see line 3414) > > Old smatch warnings: > fs/namei.c:1573 lookup_dcache() warn: passing zero to 'ERR_PTR' > fs/namei.c:1658 lookup_fast() warn: passing zero to 'ERR_PTR' > fs/namei.c:2189 hash_name() error: uninitialized symbol 'bdata'. > fs/namei.c:2600 __kern_path_locked() warn: inconsistent returns '&path->dentry->d_inode->i_rwsem'. > fs/namei.c:3480 lookup_open() error: uninitialized symbol 'error'. > > vim +/got_write +3418 fs/namei.c Thanks, fixed in my v10 branch,.
diff --git a/fs/namei.c b/fs/namei.c index f01b278ac0ef..8ad7a0dfa596 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -3388,6 +3388,44 @@ static struct dentry *atomic_open(struct nameidata *nd, struct dentry *dentry, return dentry; } +static struct dentry *atomic_revalidate_open(struct dentry *dentry, + struct nameidata *nd, + struct file *file, + const struct open_flags *op, + bool *got_write) +{ + struct mnt_idmap *idmap; + struct dentry *dir = nd->path.dentry; + struct inode *dir_inode = dir->d_inode; + int open_flag = op->open_flag; + umode_t mode = op->mode; + + if (unlikely(IS_DEADDIR(dir_inode))) + return ERR_PTR(-ENOENT); + + file->f_mode &= ~FMODE_CREATED; + + if (unlikely(open_flag & O_CREAT)) { + WARN_ON(1); + return ERR_PTR(-EINVAL); + } + + if (open_flag & (O_TRUNC | O_WRONLY | O_RDWR)) + *got_write = !mnt_want_write(nd->path.mnt); + else + *got_write = false; + + if (!got_write) + open_flag &= ~O_TRUNC; + + inode_lock_shared(dir->d_inode); + dentry = atomic_open(nd, dentry, file, open_flag, mode); + inode_unlock_shared(dir->d_inode); + + return dentry; + +} + /* * Look up and maybe create and open the last component. * @@ -3541,8 +3579,10 @@ static const char *open_last_lookups(struct nameidata *nd, if (IS_ERR(dentry)) return ERR_CAST(dentry); if (dentry && unlikely(atomic_revalidate)) { - dput(dentry); - dentry = NULL; + BUG_ON(nd->flags & LOOKUP_RCU); + dentry = atomic_revalidate_open(dentry, nd, file, op, + &got_write); + goto drop_write; } if (likely(dentry)) goto finish_lookup; @@ -3580,6 +3620,7 @@ static const char *open_last_lookups(struct nameidata *nd, else inode_unlock_shared(dir->d_inode); +drop_write: if (got_write) mnt_drop_write(nd->path.mnt);
This was suggested by Miklos based on review from the previous patch that introduced atomic open for positive dentries. In open_last_lookups() the dentry was not used anymore when atomic revalidate was available, which required to release the dentry, then code fall through to lookup_open was done, which resulted in another d_lookup and also d_revalidate. All of that can be avoided by the new atomic_revalidate_open() function. Another included change is the introduction of an enum as d_revalidate return code. Signed-off-by: Bernd Schubert <bschubert@ddn.com> Cc: Miklos Szeredi <miklos@szeredi.hu> Cc: Dharmendra Singh <dsingh@ddn.com> Cc: Christian Brauner <brauner@kernel.org> Cc: Al Viro <viro@zeniv.linux.org.uk> Cc: linux-fsdevel@vger.kernel.org --- fs/namei.c | 45 +++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 43 insertions(+), 2 deletions(-)