| Message ID | tencent_4E2FCFC90D97A5910DFA926DDD945D9B1907@qq.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ovl: fix BUG: Dentry still in use in unmount | expand |
Hi Edward, Thanks for the quick fix, but it is incorrect. On Sun, Dec 17, 2023 at 10:11 AM Edward Adam Davis <eadavis@qq.com> wrote: > > workdir and destdir could be the same when copying up to indexdir. This is not the reason for the bug, the reason is: syzbot exercised the forbidden practice of moving the workdir under lowerdir while overlayfs is mounted and tripped a dentry reference leak. > > Fixes: c63e56a4a652 ("ovl: do not open/llseek lower file with upper sb_writers held") > Reported-and-tested-by: syzbot+8608bb4553edb8c78f41@syzkaller.appspotmail.com > Signed-off-by: Edward Adam Davis <eadavis@qq.com> > --- > fs/overlayfs/copy_up.c | 20 +++++++++++++------- > 1 file changed, 13 insertions(+), 7 deletions(-) > > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c > index 4382881b0709..ae5eb442025d 100644 > --- a/fs/overlayfs/copy_up.c > +++ b/fs/overlayfs/copy_up.c > @@ -731,10 +731,14 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c) > .rdev = c->stat.rdev, > .link = c->link > }; > + err = -EIO; > + /* workdir and destdir could be the same when copying up to indexdir */ > + if (lock_rename(c->workdir, c->destdir) != NULL) > + goto unlock; You can't do that. See comment below ovl_copy_up_data(). > > err = ovl_prep_cu_creds(c->dentry, &cc); > if (err) > - return err; > + goto unlock; > > ovl_start_write(c->dentry); > inode_lock(wdir); > @@ -743,8 +747,9 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c) > ovl_end_write(c->dentry); > ovl_revert_cu_creds(&cc); > > + err = PTR_ERR(temp); > if (IS_ERR(temp)) > - return PTR_ERR(temp); > + goto unlock; > > /* > * Copy up data first and then xattrs. Writing data after > @@ -760,10 +765,9 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c) > * If temp was moved, abort without the cleanup. > */ > ovl_start_write(c->dentry); > - if (lock_rename(c->workdir, c->destdir) != NULL || > - temp->d_parent != c->workdir) { > + if (temp->d_parent != c->workdir) { dput(temp); here is all that should be needed to fix the leak. > err = -EIO; > - goto unlock; > + goto unlockcd; > } else if (err) { > goto cleanup; > } See my suggested fix at https://github.com/amir73il/linux/commits/ovl-fixes Al, Heads up. This fix will have a minor conflict with a8b0026847b8 ("rename(): avoid a deadlock in the case of parents having no common ancestor") on your work.rename branch. I plan to push my fix to linux-next soon, but I see that work.rename is not in linux-next yet. Thanks, Amir.
diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c index 4382881b0709..ae5eb442025d 100644 --- a/fs/overlayfs/copy_up.c +++ b/fs/overlayfs/copy_up.c @@ -731,10 +731,14 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c) .rdev = c->stat.rdev, .link = c->link }; + err = -EIO; + /* workdir and destdir could be the same when copying up to indexdir */ + if (lock_rename(c->workdir, c->destdir) != NULL) + goto unlock; err = ovl_prep_cu_creds(c->dentry, &cc); if (err) - return err; + goto unlock; ovl_start_write(c->dentry); inode_lock(wdir); @@ -743,8 +747,9 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c) ovl_end_write(c->dentry); ovl_revert_cu_creds(&cc); + err = PTR_ERR(temp); if (IS_ERR(temp)) - return PTR_ERR(temp); + goto unlock; /* * Copy up data first and then xattrs. Writing data after @@ -760,10 +765,9 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c) * If temp was moved, abort without the cleanup. */ ovl_start_write(c->dentry); - if (lock_rename(c->workdir, c->destdir) != NULL || - temp->d_parent != c->workdir) { + if (temp->d_parent != c->workdir) { err = -EIO; - goto unlock; + goto unlockcd; } else if (err) { goto cleanup; } @@ -801,16 +805,18 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c) ovl_inode_update(inode, temp); if (S_ISDIR(inode->i_mode)) ovl_set_flag(OVL_WHITEOUTS, inode); + +unlockcd: + ovl_end_write(c->dentry); unlock: unlock_rename(c->workdir, c->destdir); - ovl_end_write(c->dentry); return err; cleanup: ovl_cleanup(ofs, wdir, temp); dput(temp); - goto unlock; + goto unlockcd; } /* Copyup using O_TMPFILE which does not require cross dir locking */
workdir and destdir could be the same when copying up to indexdir. Fixes: c63e56a4a652 ("ovl: do not open/llseek lower file with upper sb_writers held") Reported-and-tested-by: syzbot+8608bb4553edb8c78f41@syzkaller.appspotmail.com Signed-off-by: Edward Adam Davis <eadavis@qq.com> --- fs/overlayfs/copy_up.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)