On Thu, Feb 24, 2022 at 04:08:36PM +1100, NeilBrown wrote:
> On Wed, 23 Feb 2022, J. Bruce Fields wrote:
> > For what it's worth, I applied this to recent upstream (038101e6b2cd)
> > and fed it through my usual scripts--tests all passed, but I did see
> > this lockdep warning.
> >
> > I'm not actually sure what was running at the time--probably just cthon.
> >
> > --b.
> >
> > [ 142.679891] ======================================================
> > [ 142.680883] WARNING: possible circular locking dependency detected
> > [ 142.681999] 5.17.0-rc5-00005-g64e79f877311 #1778 Not tainted
> > [ 142.682970] ------------------------------------------------------
> > [ 142.684059] test1/4557 is trying to acquire lock:
> > [ 142.684881] ffff888023d85398 (DENTRY_PAR_UPDATE){+.+.}-{0:0}, at: d_lock_update_nested+0x5/0x6a0
> > [ 142.686421]
> > but task is already holding lock:
> > [ 142.687171] ffff88801f618bd0 (&type->i_mutex_dir_key#6){++++}-{3:3}, at: path_openat+0x7cb/0x24a0
> > [ 142.689098]
> > which lock already depends on the new lock.
> >
> > [ 142.690045]
> > the existing dependency chain (in reverse order) is:
> > [ 142.691171]
> > -> #1 (&type->i_mutex_dir_key#6){++++}-{3:3}:
> > [ 142.692285] down_write+0x82/0x130
> > [ 142.692844] vfs_rmdir+0xbd/0x560
> > [ 142.693351] do_rmdir+0x33d/0x400
>
> Thanks. I hadn't tested rmdir :-)
OK. I tested with this applied and didn't see any issues.
--b.
>
> "rmdir" and "open(O_CREATE)" take these locks in the opposite order.
>
> I think the simplest fix might be to change the inode_lock(_shared) taken
> on the dir in open_last_Lookups() to use I_MUTEX_PARENT. That is
> consistent with unlink and rmdir etc which use I_MUTEX_PARENT on the
> parent.
>
> open() doesn't currently use I_MUTEX_PARENT because it never needs to
> lock the child. But as it *is* a parent that is being locked, using
> I_MUTEX_PARENT probably make more sense.
>
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3513,9 +3513,9 @@ static const char *open_last_lookups(struct nameidata *nd,
> }
> shared = !!(dir->d_inode->i_flags & S_PAR_UPDATE);
> if ((open_flag & O_CREAT) && !shared)
> - inode_lock(dir->d_inode);
> + inode_lock_nested(dir->d_inode, I_MUTEX_PARENT);
> else
> - inode_lock_shared(dir->d_inode);
> + inode_lock_shared_nested(dir->d_inode, I_MUTEX_PARENT);
> dentry = lookup_open(nd, file, op, got_write);
> if (!IS_ERR(dentry) && (file->f_mode & FMODE_CREATED))
> fsnotify_create(dir->d_inode, dentry);
>
> Thanks,
> NeilBrown
